Repository navigation
Python: Resolve checkpoint get_latest timestamp ties through lineage - #9261
Shivam Sharma (ShivamSharma43) wants to merge 3 commits into
Conversation
InMemoryCheckpointStorage.get_latest and FileCheckpointStorage.get_latest picked max() by timestamp alone. Checkpoints taken at the same superstep boundary can share a timestamp, so the result depended on save order and a resume could restore a superseded checkpoint. Among the checkpoints with the newest timestamp, skip any that another one names as previous_checkpoint_id. Fixes microsoft#8513 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review |
1 similar comment
|
/review |
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: No findings
Scope: full PR (1 commit(s)): 740fca80680c
Model: gpt-5.6-sol
Overview
The PR preserves timestamp-primary ordering while using lineage to select the unsuperseded tip when the newest checkpoints tie, and applies that behavior consistently to the two changed core backends. Workflow scoping, empty-store guards, caller-owned copy semantics, and forward/reverse save-order tests across both implementations support the change. The remaining provider-parity concern is pre-existing in unchanged backends, so no publishable PR-introduced finding remains.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
|
Shivam Sharma (@ShivamSharma43) this fixes the in-memory and file stores, but the same stale-resume bug remains in the first-party Cosmos and Foundry checkpoint stores. |
…ores CosmosCheckpointStorage.get_latest asked Cosmos for a single row ordered only by timestamp, and FoundryCheckpointStore.get_latest used max() over timestamps, so a parent/child pair sharing the newest timestamp could still resume from the superseded parent depending on backend/list order. Cosmos now queries MAX(c.timestamp) for the workflow partition, fetches every checkpoint at that timestamp, and resolves the tie through previous_checkpoint_id. Foundry applies the same tie-breaker to its listed checkpoints. The helper mirrors core's _select_latest_checkpoint because both packages support agent-framework-core releases that predate it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Eduard van Valkenburg (@eavanvalkenburg) thanks, good catch. Applied the same lineage tie-breaker to both first-party stores in 83ecc69:
The helper is mirrored in each package rather than imported from core's Tests: added forward/reverse row-order parent→child→grandchild tie tests for both stores, and updated the existing Cosmos |
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
Shivam Sharma (@ShivamSharma43) The parent/child lineage tie-breaker is now applied across the first-party stores, but same-timestamp ties with multiple unsuperseded lineage tips are still backend-order dependent. _select_latest_checkpoint() (and the duplicated Cosmos/Foundry helpers) returns the first candidate not referenced as a parent; two independent leaves therefore produce different results when their input order is reversed. Please apply a stable fallback among remaining leaf candidates, such as checkpoint_id, in all three helpers and add reversed-order coverage for two unsuperseded same-timestamp leaves.
… checkpoint_id Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Eduard van Valkenburg (@eavanvalkenburg) thanks, agreed. Fixed in ff52e60. All three helpers (core Added |
Motivation & Context
InMemoryCheckpointStorage.get_latestandFileCheckpointStorage.get_latestchoosemax()by timestamp alone. Checkpoints taken at the same superstep boundary can carry identical timestamps (theWorkflowCheckpointdocstring notes this), anddatetime.now()resolution can make distinct boundaries collide too. On a tie,max()returns whichever checkpoint is iterated first, so the result depends on save order (or file listing order), and a workflow resuming fromget_latestcan restore a superseded checkpoint without any error.Description & Review Guide
_select_latest_checkpointhelper in_workflows/_checkpoint.py, used by both storages. It keeps the checkpoints with the newest timestamp and, among those, skips any that another one names as itsprevious_checkpoint_id, as theWorkflowCheckpointdocstring describes for lineage ordering.get_latestnow returns the grandchild in any save order, for both storages. Without a tie, the result is the same as before. If tied checkpoints are not related by lineage (for example two separate branches), the first one is returned, as before.CheckpointStorageimplementations (Cosmos, Foundry hosting, the AG-UI wrapper) should get the same tie-breaking. I kept this PR to the two storages named in the issue.test_get_latest_resolves_timestamp_ties_through_lineageintests/workflow/test_checkpoint.pycovers both storages and both save orders. Without the fix, 3 of the 4 cases fail; the in-memory reverse-order case passes by chance.Related Issue
Fixes #8513
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.🤖 Generated with Claude Code