feat: add snapshot session manager to python - #3283
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Assessment: Comment (a few items to resolve before merge) Well-scoped, well-documented feature with strong test coverage (98%+) and a clear migration story. Feedback is mostly about a process gap around the public-API/deprecation surface and a couple of efficiency/robustness refinements in the save paths — nothing structurally wrong with the design. Review Categories
Nice work on the byte-identical key layout with the TS SDK and the read-only migration path — the design reasoning is easy to follow from the docstrings. |
…g' into opieter-aws/session-checkpointing
|
@strandly-the-agent Review this PR, do API BR and focus on feature parity with Typescript |
strandly-the-agent
left a comment
There was a problem hiding this comment.
🔴 Changes requested — three reproduced blockers plus two persistence/parity defects; this substantial public primitive also needs formal API review before merge.
✅ Reviewed exact head 50bd89c8; Python snapshot/migration tests 43 passed, TS parity-oracle tests 72 passed, and git diff --check passed. Core CI jobs are green; label-size remains failed and some non-core jobs are waiting.
🔴 Reproduced: empty-ID cross-session deletion; message mode restoring pre-compaction state; no viable Graph/Swarm/Bidi replacement for the blanket deprecations.
🟡 Parity: pre-namespaced storage writes session/session/...; same-millisecond IDs violate the documented oldest-first order and differ from TS UUIDv7.
❓ Blocking API decision: please add api/needs-review, complete the substantial-change review/meeting, and only then add api/review-complete. Should Python also expose TS-equivalent manual immutable save and latest restore (snapshot_id=None) before this becomes the recommended parity primitive?
API BR, docs, and verification details
API bar-raising
The new recommended manager and two supporting types are exported directly from strands.session, while three established public managers are deprecated. That meets the substantial-change bar in team/API_BAR_RAISING.md; the PR currently has neither API label. The PR description should also include the complete public signatures/defaults and export inventory required by that process.
TypeScript exposes saveSnapshot({ isLatest: false }) for an explicit immutable checkpoint and restoreSnapshot() without an ID for latest restore. Python currently creates immutable snapshots only through an invocation trigger, and restore_snapshot() requires an immutable ID. Please either close that gap or record the intentionally excluded workflows and rationale in the API decision.
Docs bundle
Please correct these together:
session-management.mdx:181:LocalFileStorage()defaults to./.strands/, not a temporary directory.session-management.mdx:264-277: useprefix="production"(the trailing slash currently producesproduction//) and remove the unsupportedendpoint_urlclaim, or add that parameter to unifiedS3Storage.session-management.mdx:196-225: qualify the layout claim. The adjacent TS examples use legacy{ snapshot: FileStorage/S3Storage }; only unscoped unified TS storage adds the matchingsession/namespace.
Scope
I did not repeat the four earlier inline findings that are now outdated/fixed. The existing redaction-divergence thread remains the right place to record that intentional behavioral difference.
Tests run locally:
Python: 43 passed (snapshot manager + migration + migration e2e)
TypeScript: 72 passed (SessionManager + SnapshotStorageAdapter)
git diff --check: passed
|
@strandly-the-agent review the latest version |
strandly-the-agent
left a comment
There was a problem hiding this comment.
🔴 CHANGES_REQUESTED: the new Graph/Swarm persistence can resume invalid state—causing early/double execution, replay, and terminal restore crashes—and the deprecation still sends Bidi users to a no-op replacement.
✅ Reviewed 2ffcf6ed vs ad5685f7; git diff --check passed. Focused Python suites: snapshot/migration/UUID 73, Graph/Swarm 110, legacy sessions 131 passed. CI Gate, Python matrix, lint, docs, and codecov are green.
✅ Fresh no-Bedrock repros confirmed Graph early/double join, Swarm handoff replay, terminal restore crash, and lossy serialization round-trip.
❓ Blocking API process: this substantial public primitive needs explicit API review/meeting and api/needs-review → api/review-complete before merge.
❓ Blocking parity: should Python expose TS-equivalent manual save/latest restore, multi-agent immutable restore, and configurable node-vs-invocation saves—or is each divergence intentional and approved?
❓ Blocking wire contract: Python and TS use the same keys with incompatible payloads; should they share a canonical schema or use SDK/version-specific namespaces?
Additional verification and docs corrections
The exact latest TypeScript focused suite was not rerun; the earlier 72-pass result predates this multi-agent delta. The PR remains draft; label-size fails, and integration/preview/review jobs are waiting.
Please also correct the bundled documentation examples:
LocalFileStorage()defaults to./.strands/, not a temporary directory.S3Storagehas noendpoint_urlargument, andprefix="production/"currently creates a doubled slash.- The migration example points new storage at
/path/to/sessionswhile the legacyFileSessionManagerreads its unrelated default directory.
No style-only findings are included.
…atest id by default
|
Re-review of ✅ Resolved since last review
🔴 Still blocking
Net: correctness and API surface are in good shape and the two API questions are cleanly resolved — the remaining gates are documentation and reconciling the PR description with what the code actually does. |
|
Re-review of ✅ Looks good
🟡 One API-review consideration (non-blocking)
Still open from prior reviews (unchanged by this commit):
Nice incremental cleanup — the identifier extraction is a good reusability improvement. |
|
Re-review of ✅ Clean, well-scoped removal
This is a nice reduction in surface area — dropping migration removes the most complex/riskiest behavior (read-only delegation into the legacy manager) and leaves a tighter, easier-to-reason-about API. Also resolved by recent commits (thanks!):
🔴 One thing to fix — PR description is now out of date:
Please update the description so reviewers/users aren't led to expect 🟡 Still open (carryover):
|
|
@strandly-the-agent create 3 follow-up issues:
|
|
Filed the three follow-ups:
Two notes:
|
Description
The Python SDK had no way to persist an agent as a whole. The shipped session managers are a message-log design — one file or S3 object per message — which cannot express "restore the agent to how it looked three turns ago." The
Snapshotprimitive (agent.take_snapshot/load_snapshot) already existed and is used by the A2A executor, but nothing persisted snapshots to storage. TypeScript has shipped snapshot-based sessions with checkpointing for a while; this closes that gap.SnapshotSessionManagercaptures the agent as one versioned blob per save. A mutablesnapshot_latestgives crash/restart resume; append-only immutable snapshots keyed by UUIDv7 give time-travel restore. It builds on the unifiedStorageprimitive rather than its own backend, so one storage configuration serves sessions, memory, and anything else layered onStorage. The key layout is byte-identical to TS (session/<id>/scopes/agent/<agent_id>/snapshots/…), so both SDKs share a storage convention.Single agents only. Graph/Swarm and BidiAgent are rejected at their initialization event with
NotImplementedErrorpointing at the message-log managers, rather than attaching and silently persisting nothing. Multi-agent snapshot persistence needs upstream fixes toGraph/Swarmserialize_state/deserialize_state: those were built for interrupt-resume inside a live process and lose state across a real crash-restart boundary. They are pre-existing, being fixed separately, and deliberately out of scope here.Nothing is deprecated. The message-log managers are untouched and remain the only option for orchestrators and Bidi.
Public API Changes
Checkpointing and time travel:
Also exported:
SnapshotTrigger(Protocol) andSaveLatestStrategy.Two intentional deviations from the TS shape, both additive:
save_snapshotreturns the new id so callers need not follow up withlist_snapshot_ids, and the trigger argument is namedagent_datato match TS'sSnapshotTriggerParams.agentData.Related Issues
#3182
Documentation PR
N/A will follow up
Type of Change
New feature
Testing
hatch run prepareExercised the manager end-to-end against a real Bedrock agent across a genuine two-process restart (one process writes and exits; a fresh process over the same directory rehydrates the conversation and state and the model continues coherently), plus checkpointing/time-travel,
save_latest_oncadence, guardrail-redaction flush, stateful-model message discard, image-bytes round-trip. TheS3Storagebackend and the stateful-model (OpenAI Responses) discard run in CI viatests_integ/.Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.