Repository navigation
Python: fix get_latest silently returning a stale checkpoint on decode failure - #9485
pratik wayase (PratikWayase) wants to merge 3 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
| owner: str | None = None | ||
| if ( | ||
| isinstance(stored, dict) | ||
| and "workflow_name" not in duplicate_keys |
There was a problem hiding this comment.
pratik wayase (@PratikWayase), duplicate_keys collects duplicates from every nested JSON object, so a repeated workflow_name inside state makes this ownership check fail even when the root has exactly one matching workflow_name. A newer checkpoint containing "state": {"workflow_name": "a", "workflow_name": "b"} is therefore quarantined as unowned, and get_latest() returns the older checkpoint instead of surfacing corruption in a file with known matching ownership. Please track root ownership duplicates separately from payload duplicates so this path cannot silently resume stale state.
| ex, | ||
| ) | ||
| continue | ||
| except Exception as e: |
There was a problem hiding this comment.
pratik wayase (@PratikWayase), if a checkpoint is deleted after _checkpoint_paths() discovers it but before the first stat() or open(), this handler wraps FileNotFoundError in a plain WorkflowCheckpointException. The outer loop retries only _CheckpointScanInvalidated, so deleting even an unrelated workflow's checkpoint immediately fails otherwise healthy recovery after one scan. The added deletion test removes the file after parsing, which exercises the later validation handler instead. Please classify disappearance during the initial read as scan invalidation too, allowing the next attempt to return the surviving checkpoint.


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.