Repository navigation
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.
…ecise error Since microsoft#8127 SecretString is an immutable, slots-based non-str wrapper with no __reduce__/__getstate__, so pickle protocols >= 2 dump it fine and fail at restore on the immutability guard. FileCheckpointStorage's save-time round-trip check (microsoft#8214) surfaced that as an opaque decode error at save; Cosmos and Foundry-hosted storage skip that check, so the same state saved cleanly and only failed on resume. Unwrapping to str first, the obvious workaround, writes the credential to durable storage in cleartext. The checkpoint codec now refuses a SecretString at encode time with a WorkflowCheckpointException that says why (unpicklable by design, and a successful pickle would persist the secret in cleartext) and what to do instead (keep credentials out of checkpointed state, re-resolve from settings or configuration on restore). This matches the durable session state path, which already rejects the type with a precise error (microsoft#8874), and because the refusal lives in the shared codec it lands at save time on every storage backend, closing the Cosmos load-time asymmetry. Nested SecretString values inside plain dict/list state are caught by the same recursion. A SecretString buried in a custom object still fails the round-trip check with the generic wrapper; that edge is unchanged.
…icrosoft#8875) * Python: Refuse to save Cosmos DB checkpoints that cannot be restored CosmosCheckpointStorage.save only encoded the checkpoint, while load decodes it under the storage's allowed_checkpoint_types. A checkpoint holding an unregistered application type was written successfully and only failed when a workflow tried to resume from it. FileCheckpointStorage has refused such checkpoints at save time since microsoft#8214. Validate the encoded checkpoint the same way before writing, wrap encoding failures in WorkflowCheckpointException, and skip creating the database or container when the save is refused. Register ComputeTask in the Cosmos checkpointing sample, whose checkpoints could not be restored. * Python: Share save-time checkpoint validation between file and Cosmos storage Move the encode-and-restore check into a private core helper, _encode_checkpoint_for_storage(checkpoint, allowed_types), which returns the validated encoded payload. FileCheckpointStorage.save and CosmosCheckpointStorage.save both call it and keep their own I/O, so the two stores can no longer drift apart. Neither store's behavior changes. Add a FileCheckpointStorage test for the wrapped encoding failure, a path that only the Cosmos tests reached before.
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.