Python: raise instead of silently returning a stale checkpoint from get_latest - #7832
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The implementation introduces an inconsistent flat-directory contract and performs synchronous decoding on the event loop.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates file-backed checkpoint recovery to raise when the newest checkpoint cannot be decoded instead of returning stale state.
Changes:
- Selects the latest checkpoint from JSON metadata before decoding.
- Adds filesystem consistency and corruption checks.
- Expands tests for decoding, metadata, concurrency, and filesystem edge cases.
File summaries
| File | Description |
|---|---|
python/packages/core/agent_framework/_workflows/_checkpoint.py |
Implements metadata-based latest-checkpoint selection and validation. |
python/packages/core/tests/workflow/test_checkpoint.py |
Adds comprehensive get_latest behavior tests. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
/review |
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (3 commit(s)): b03e996cdca3, b5d9ec7498bb, 3574c9b6c57b
Model: gpt-5.6-sol-fast
Overview
The change avoids stale fallback by selecting from validated metadata, decoding only the captured latest payload in a worker thread, and checking paths, JSON ambiguity, file identity, and observed filesystem changes. The expanded tests provide strong coverage for malformed matching metadata, nested IDs, reader allowlists, and TOCTOU cases. Two availability risks remain: an unidentifiable file blocks every workflow sharing the directory until cleanup, and any concurrent directory change can abort production recovery even when an unrelated workflow caused it.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (2 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_workflows/_checkpoint.py
|
Please re-open when comments are addressed & threads resolved, as well as unit tests are passing. |
|
Evan Mattson (@moonbox3) I've pushed commit Could you please reopen this pull request so the automated checks can run on the new commit? My account does not have permission to reopen it. |
Motivation & Context
FileCheckpointStorage.get_latest()previously selected fromlist_checkpoints(), which logs read failures and skips those files. When the newest saved checkpoint could not be decoded, recovery silently returned an older checkpoint orNone.This can happen when a writer saves an application type that a later reader does not allow. Invalid saved timestamps can also hide newer state. Recovery needs to surface these failures while keeping a shared storage directory usable for other workflows.
Description & Review Guide
workflow_namebefore saving, write new JSON as UTF-8, and retain the former platform encoding as a fallback when reading legacy files.WorkflowCheckpointException. Files whose owner cannot be identified are excluded with a warning and retained for inspection. Transient filesystem changes trigger up to three internal scans before an error reaches the caller.Ownership is determined from root JSON fields. Invalid fields for a known different workflow do not block recovery. Duplicate ownership fields naming the requested workflow, redirected IDs, invalid matching timestamps and root pickle envelopes are rejected. The flat format cannot recover ownership after that information is destroyed; unidentified files remain on disk, and the selector does not rename a path that a concurrent writer may have replaced.
The scan includes nested directories and retains valid empty and relative ID aliases. In-root directory aliases and cycles are visited once; paths escaping the storage root and non-regular checkpoint files are rejected. Temporary files are ignored. Older payloads are not decoded to find the latest checkpoint, and listing retains its tolerance of unreadable entries.
File versions are checked around reading and again with directory membership before accepting the snapshot. On Windows,
ctimeis compared only within the same API; identity, size and modification time remain comparable across path and descriptor results. The file handle closes before JSON parsing so a concurrent atomic replacement can proceed during parsing. These optimistic checks depend on filesystem change reporting and atomic writers. Sustained changes can exhaust the three attempts; writes after validation belong to a later snapshot. Equal timestamps retain directory iteration order.Regression coverage includes the original refused-payload failure, matching metadata corruption, nested paths, selected snapshot consistency, shared ownership, concurrent writers, retry exhaustion, legacy encoding and event-loop responsiveness. Six additional scenarios exercise the actual hosting recovery and streaming paths. On merge commit
fb49a1ac, including main0d3ea14f, the native aggregate passed on macOS/Python 3.13.7 with 10859 passed, 34 skipped and 2 xfailed after enabling PowerFx with a separate .NET runtime. The native package coverage gate passed, and the checkpoint module has 100% line coverage in that run. Linux/Python 3.11.14 passed 10591 tests with 302 skipped and 2 xfailed, using four workers and the unchanged 60-second timeout. Linux skips include unavailable PowerFx; other optional-dependency and platform skips remain explicit gaps. Strict core source typing, all five test type checkers for core and hosting, and scoped lint and formatting passed. Native Windows/Ubuntu CI is still pending for this commit.Related Issue
Closes #7831
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.The full build and unit checklist remains unchecked pending native CI. The compatibility item remains for maintainer review of the stricter error policy for matching checkpoints and ownership validation at write time.