Python: FileCheckpointStorage save/load symmetry (#8181) - #8214
Python: FileCheckpointStorage save/load symmetry (#8181)#8214lsmlhi_25 (FOWEPJF255) wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
save() still writes checkpoint JSON without an explicit UTF-8 encoding while load() now forces UTF-8, which can reintroduce save/load asymmetry on non-UTF-8 default locales.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves durability and debuggability of the Python FileCheckpointStorage by enforcing save/load symmetry, standardizing exception behavior for corrupt checkpoint files, and aligning listing APIs so they don’t disagree when decoding fails.
Changes:
- Validate at save-time that an encoded checkpoint can be decoded under the same storage’s
allowed_checkpoint_types, preventing “save succeeds, load fails” scenarios. - Wrap invalid JSON / invalid UTF-8 during
load()intoWorkflowCheckpointException(per theCheckpointStoragecontract). - Make
list_checkpoint_ids()delegate tolist_checkpoints()so both use the same decode/filter path.
File summaries
| File | Description |
|---|---|
python/packages/core/agent_framework/_workflows/_checkpoint.py |
Adds save-time restore validation, wraps JSON/UTF-8 read failures on load, and aligns ID listing with decoded listing. |
python/packages/core/tests/workflow/test_checkpoint.py |
Adds regression tests for invalid JSON/UTF-8 on load and for list ID alignment with decode filtering. |
python/packages/core/tests/workflow/test_checkpoint_unrestricted_pickle.py |
Adds test ensuring file storage refuses to save checkpoints it cannot restore under its own allowed-types policy. |
Review details
Suppressed comments (2)
python/packages/core/agent_framework/_workflows/_checkpoint.py:472
- Since
list_checkpoint_ids()now delegates tolist_checkpoints(), listing IDs inheritslist_checkpoints()file-reading behavior.list_checkpoints()currently opens checkpoint files withoutencoding="utf-8", whileload()explicitly uses UTF-8; this can make listing locale-dependent and causelist_checkpoint_ids()/get_latest()to miss checkpoints thatload()could otherwise read (or vice versa) when non-ASCII is present.
checkpoints = await self.list_checkpoints(workflow_name=workflow_name)
return [checkpoint.checkpoint_id for checkpoint in checkpoints]
python/packages/core/agent_framework/_workflows/_checkpoint.py:382
- This error message is also used for
UnicodeDecodeError(invalid UTF-8), but it only mentions JSON. That can be misleading when the file is valid JSON in another encoding or simply not UTF-8.
raise WorkflowCheckpointException(
f"Checkpoint file for {checkpoint_id} is not valid JSON and cannot be loaded."
) from ex
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
Follow-up: save() and list now open checkpoint JSON with explicit encoding=utf-8 to match load(). |
Include encoding in save-time validation, wrap invalid UTF-8 on load, and delegate list_checkpoint_ids to list_checkpoints.
Match load()'s explicit UTF-8 encoding so save/load stay symmetric on non-UTF-8 default locales.
f2ba45a to
b443bbf
Compare
…icrosoft#8181) Copilot review: UnicodeDecodeError should not claim invalid JSON.
|
Follow-up: |
|
Thanks for the approval. Branch is up to date / CI running; ready to merge when checks are green. |
…8181) Package Checks ruff rule blocking-path-method-in-async-function failed on Path(tmpdir).glob in an async test. Use asyncio.to_thread like other core tests.
Head branch was pushed to by a user without write access
|
lsmlhi_25 (@FOWEPJF255) Already has Eduard van Valkenburg (@eavanvalkenburg) approval and the UTF-8 save/load follow-up. Flagging here in case this just needs a final maintainer merge once required workflows are green. |
Motivation & Context
FileCheckpointStoragecould save checkpoints that the same storage could not restore, surface rawJSONDecodeError/UnicodeDecodeErroron load, and disagree betweenlist_checkpointsandlist_checkpoint_idswhen decoding failed. This breaks durable pause/resume and makes failures harder to handle.Fixes #8181.
Description & Review Guide
allowed_checkpoint_typesbefore writing (including encoding failures), so save refuses payloads that load would reject.loadasWorkflowCheckpointException.list_checkpoint_idsdelegate tolist_checkpointsso both share one decode path.JSONDecodeErrorwrapping on load, andlist_checkpoint_idsdelegation.Related Issue
Fixes #8181
This replaces closed PR #8193 (closed for missing PR template). Same issue; no other open PR for #8181.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.