Python: return caller-owned checkpoints from InMemoryCheckpointStorage reads - #7686
Closed
Oleg Solozobov (dev404ai) wants to merge 1 commit into
Closed
Python: return caller-owned checkpoints from InMemoryCheckpointStorage reads#7686Oleg Solozobov (dev404ai) wants to merge 1 commit into
Oleg Solozobov (dev404ai) wants to merge 1 commit into
Conversation
Oleg Solozobov (dev404ai)
temporarily deployed
to
github-app-auth
August 16, 2026 12:50 — with
GitHub Actions
Inactive
Oleg Solozobov (dev404ai)
temporarily deployed
to
github-app-auth
August 16, 2026 12:50 — with
GitHub Actions
Inactive
Oleg Solozobov (dev404ai)
temporarily deployed
to
github-app-auth
August 16, 2026 12:50 — with
GitHub Actions
Inactive
Oleg Solozobov (dev404ai)
temporarily deployed
to
github-app-auth
August 16, 2026 12:51 — with
GitHub Actions
Inactive
Oleg Solozobov (dev404ai)
temporarily deployed
to
github-app-auth
August 16, 2026 12:51 — with
GitHub Actions
Inactive
Contributor
|
Closing in flavor of #7697 as it's a more comprehensive solution. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation & Context
InMemoryCheckpointStoragehanded out the checkpoint objects it stores, so a caller that mutated a loaded checkpoint also mutated the storage. The other two implementations of the sameCheckpointStorageprotocol,FileCheckpointStorageand the Azure Cosmos backend, rebuild every returned checkpoint from its serialized form, so their reads are isolated by construction.That made the in-memory implementation the odd one out on a protocol three backends share, and it is the one tests and samples reach for first, so a workflow that mutates restored state could behave differently depending on which backend was configured.
The protocol itself did not say which behavior was correct, which is how the two were able to drift apart.
Description & Review Guide
What are the major changes?
InMemoryCheckpointStoragenow returnscopy.deepcopyfrom its three read paths,load,list_checkpointsandget_latest, matching the deep copy thatsavealready stored. Isolation on write was deliberate, and only the read side was missing.The
CheckpointStorageprotocol docstring now states the contract the fix restores: reads return objects owned by the caller, backends that serialize satisfy this by construction, and backends holding checkpoints in memory must copy on read.What is the impact of these changes?
All three implementations now agree. Callers may treat a returned checkpoint as their own, which is what the file and Cosmos backends already allowed. No public signature changes, and no behavior changes for callers that do not mutate what they read.
get_latestcopies only the single checkpoint it returns rather than the whole candidate list, so listing cost is unchanged.What do you want reviewers to focus on?
Whether the added protocol sentence states the intended contract. If the intended contract is instead that callers must copy defensively, then the file and Cosmos backends are the ones that diverge and this change should be reversed in favor of documenting that. I read the existing
copy.deepcopyinsaveas evidence that caller isolation was already intended.Related Issue
Fixes #7685
Contribution Checklist
Two tests were added to the workflow checkpoint suite:
test_memory_checkpoint_storage_load_returns_caller_owned_copy— mutating a loaded checkpoint does not change what a later read returns.test_memory_checkpoint_storage_list_and_get_latest_return_caller_owned_copies— the same contract for the other two read paths.Both fail without the fix. With it,
uv run poe testover the workflow suite reports 926 passed, 2 skipped and 2 xfailed, whileruff checkandruff format --checkare clean on both changed files.