Python: FileCheckpointStorage save/load symmetry (#8181) - #8214
Evan Mattson (moonbox3) merged 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. |
|
Friendly ping: this PR is mergeable and waiting on review (FileCheckpointStorage save/load symmetry for #8181). Happy to address any feedback. |
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
Looks good—thanks!
|
Eduard van Valkenburg (@eavanvalkenburg) Thanks for the re-approval — CI looks green / mergeable on this head whenever you're ready to merge. |
microsoft#8214 landed and rewrote `save()`, which this branch also rewrites. Resolved in favour of this branch's machinery for the conflicting hunk -- their `_write_atomic` is the pre-rewrite version -- while deliberately carrying their two changes forward rather than letting "keep ours" discard them: * `encoding="utf-8"` on the write. Their fix landed inside the conflicting hunk, so resolving in our favour dropped it; it is restored on this branch's `os.fdopen`. This is a real bug, not a style change: `json.dump(..., ensure_ascii=False)` writes non-ASCII raw, and the platform default is cp1252 on Windows, so a checkpoint containing CJK text or emoji raised `UnicodeEncodeError`. Both read sites came through the automatic merge already UTF-8; fixing only one side would have traded a loud write-time error for silent read-time corruption. Verified by round-tripping CJK, accented and emoji codepoints. * Their save-time encode/decode validation, which the automatic merge kept. It raises before `_enqueue_write`, so a rejected checkpoint never takes a destination ticket, and it sits outside the critical section so it does not extend how long a destination is held. Checked rather than assumed: no test was lost on either side (71 here + main's, 74 after), and all 25 tests this branch adds still fail against the new `main`, so the description's teeth claim still holds. The two newly uncovered lines in the file are microsoft#8214's own exception branch, not this branch's.
Two conflicts, both in imports, both resolved by keeping what this branch needs: * `_checkpoint.py` -- `main` dropped `import time`; this branch still needs it for `_replace_with_retry`'s backoff, which `main` does not have. Verified `time.sleep` is still referenced rather than assuming. * `test_checkpoint.py` -- `main` added a module-level `import threading` while this branch added `import time` and the `ConcurrentFuture` alias. All three kept, and the function-local `import threading` added earlier today is now redundant so it is removed in favour of the module-level one. Checked: no test lost on either side (74 here + main's, 75 after), and both this branch's machinery and microsoft#8214's `encoding="utf-8"` and save-time validation are still present.
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.