Repository navigation
Python: fix get_latest silently returning a stale checkpoint on decode failure - #9485
pratik wayase (PratikWayase) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Duplicate fields in unrelated workflows can block recovery, and Windows mutation tests can pass without exercising scan validation.
2 open issues
What changed in this PR
Fixes #7831 by making Python’s file checkpoint recovery raise when the newest checkpoint cannot be decoded, rather than silently returning older state.
Changes:
- Selects checkpoints from metadata, validates the scan, and decodes only the winner.
- Shares decoding logic with
load()while preserving tolerant listing. - Adds recovery, metadata, concurrency, and deserialization-policy tests.
| File | Description |
|---|---|
| python/packages/core/tests/workflow/test_checkpoint.py | Adds checkpoint recovery regression tests. |
| python/packages/core/agent_framework/_workflows/_checkpoint.py | Implements metadata-first selection and snapshot validation. |
Review effort: Balanced
| if "workflow_name" in duplicate_keys: | ||
| raise ValueError("Duplicate checkpoint JSON key: workflow_name") | ||
| if not isinstance(stored, dict) or not isinstance(stored.get("workflow_name"), str): | ||
| raise ValueError("checkpoint workflow_name must be a string") |
There was a problem hiding this comment.
The unconditional failure here means one malformed or legacy JSON file whose workflow owner cannot be identified makes get_latest() fail indefinitely for every workflow sharing this storage root, even when that workflow's checkpoints are healthy. Please isolate or quarantine unowned files so they cannot permanently poison unrelated workflows, while preserving an explicit failure for corrupt checkpoints known to belong to the requested workflow. Please also add shared-root regression coverage.
| ) | ||
| current_paths, current_directories = _checkpoint_paths(root) | ||
| if ( | ||
| set(current_paths) != set(paths) |
There was a problem hiding this comment.
This invalidates the scan for any membership change under the shared root, including an atomic save for a different workflow. Because get_latest() immediately propagates the exception, ordinary concurrent checkpointing can fail unrelated recovery instead of retrying the optimistic read. Please add bounded internal retries for transient scan changes, or otherwise narrow invalidation without missing a newly written matching checkpoint, and cover a concurrent foreign-workflow save.


Motivation & Context
FileCheckpointStorage.get_latest()was silently returning stale state. When the newest checkpoint on disk could not be decoded — for example, because itsstatepayload referenced a type not inallowed_checkpoint_types—get_latestwould fall back to the next-newest checkpoint it could read, and return that as if it were the latest. A workflow then resumed from earlier state, having been told its newer state was saved.The root cause was that
get_latestwas built onlist_checkpoints, which catches per-file decode failures, logs a warning, and returns whatever it could read. The only signal of the refusal was alogger.warningon a code path nobody watches during recovery. Issue #7831 describes the bug and the three behaviors that compose to produce it.Description & Review Guide
What are the major changes?
get_latestno longer goes throughlist_checkpoints. It scans the storage directory itself in three phases:.jsonfile under the storage root (recursively, with symlink/cycle protection), read only the plain-JSON envelope —workflow_name,timestamp,checkpoint_id. No payload decoding happens in this phase. Files whoseworkflow_namedoesn't match the requested workflow are skipped; any file that could belong to the requested workflow but is unreadable raisesWorkflowCheckpointException.Path.stat()consistently) and re-scan the directory tree. If anything changed during the scan — a file's mtime/size/ctime, the set of files, the set of directories, or the storage root itself — raise"changed during the scan; retry get_latest". The caller can retry.load(), which could decode a replacement rather than the validated snapshot. If decode fails, raise; do not fall back to an older checkpoint.list_checkpoints()is intentionally left unchanged. It stays tolerant — one bad file should not break listing. The asymmetry is pinned by a new test (test_file_checkpoint_storage_list_checkpoints_still_skips_undecodable_entries). Issue Python: FileCheckpointStorage saves state it cannot restore: silent save/load asymmetry, get_latest invisibility, listing disagreement, and undocumented JSONDecodeError #8181 raises the broader consistency question of whetherlist_checkpointsshould also surface decode errors; this PR does not take a position on that and leaves the door open.A small
_decode_checkpoint()helper is extracted and shared betweenload()andget_latest()so both apply the sameallowed_checkpoint_typespolicy. Behavior ofload()is unchanged.What is the impact of these changes?
get_lateststops decoding every checkpoint in order to return one — only the winner is decoded.get_latestcall are detected and surfaced as a retryable error rather than silently returning a snapshot that may already be stale.list_checkpoints()behavior is unchanged — nothing that lists checkpoints starts failing.What do you want reviewers to focus on?
Path.stat()andos.fstat(fd)use different Win32 APIs (GetFileAttributesExWvsGetFileInformationByHandle) and can return different values forst_dev,st_ino, and evenst_mtime_ns/st_ctime_nson the same stable file. To make the scan work on Windows, the version tuple was reduced to(st_size, st_mtime_ns, st_ctime_ns)and the two intra-fileos.fstat()comparisons (pre-read and post-read) were removed. The post-loop re-stat usesPath.stat()consistently and still detects concurrent writes. Symlink escapes are caught separately by_validate_file_pathand theis_relative_to(root)check in_checkpoint_paths, so droppingst_devdoes not weaken that guard. Reviewers on Linux/macOS should sanity-check that the reduced tuple is still sufficient there.list_checkpointstolerant. This is intentional and pinned by a test, but if maintainers want symmetry withget_latest(per Python: FileCheckpointStorage saves state it cannot restore: silent save/load asymmetry, get_latest invisibility, listing disagreement, and undocumented JSONDecodeError #8181), that's a follow-up PR — not this one.checkpoint_idvalidation.checkpoint_idmust be a string and must resolve to the exact file being read (_validate_file_path(checkpoint_id) != file_path.resolve()→ raise). This catches redirected-ID attacks where a file's metadata claims to be a different checkpoint. Top-level__pickled__/__type__markers are rejected because they could hide different metadata.Related Issue
Fixes #7831
AI Assistance
AI assistance details: used to help understand the issue and the surrounding code.
Contribution Checklist
get_latestpreviously returned a stale checkpoint on decode failure of the newest; it now raises. Callers that depended on the silent-fallback behavior will see a new exception. Nolist_checkpoints/load/save/deletebehavior changes.