Fix/47 persist agent state across restarts - #275
Open
peter-abj wants to merge 10 commits into
Open
Conversation
…tart Add tests demonstrating that ContextManager results are lost when Orchestrator is re-initialized (API restart). Shows gap between SessionStore persistence and in-memory ContextManager cache. - test_context_manager_memory_lost_on_restart: Demonstrates tool re-execution after restart instead of cache hit - test_session_store_persists_results_but_not_context_cache: Shows SessionStore saves some state but not cached tool results Fix type annotations in agent modules for mypy compliance. Fixes: ascherj#47
- Create PLAN.md with detailed solution framework: - Understand: Root cause analysis (in-memory cache lost on restart) - Map: Files and functions involved - Plan: 5 concrete sub-tasks for implementation - Inputs/outputs: Data flow specification - Risks & unknowns: Serialization, cache collisions, Redis failures - Edge cases: Concurrent reviews, TTL expiration, failure handling - Add Week 8 section to JOURNAL.md: - Link reproduction commit (9500b94) - Summarize reproduction findings - Note open questions for Week 9 implementation
The orchestrator's memoized tool results lived only in ContextManager's in-memory dict, so an API restart during a long-running review wiped the cache and forced every tool to re-run from scratch. Persist the cache to Redis alongside session state: - ContextManager.to_dict/from_dict serialize memoized ToolResults into a JSON-safe form, skipping anything unserializable instead of raising. - SessionStore.get_cache/set_cache store the cache under a separate cache: key so it restores independently of session state. - Orchestrator restores the cache at the start of run() and checkpoints after every tool, so a restart mid-review keeps completed work. Closes ascherj#47
- Added scope-fit checklist covering: bounded fix, 3-4 week scope, clear acceptance criteria, user impact, maintainer engagement, specific files - Explained why this is appropriate Tier 3 work for skill level - Clarified learning goals: async orchestration, Redis persistence, state management - Addresses grading feedback on 'Issue fit and selection reasoning' section
Updated feedback section and reflections for Week 10.
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
1 participant
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.
Summary
Long-running reviews lose all progress when the API restarts. The orchestrator
memoizes each tool's result in
ContextManager, but that cache lives only in aplain in-memory dict, so restarting the process during a review throws it away and
every tool has to run again from scratch. This PR persists the cache to Redis
alongside the existing session state and restores it on the next run, so a review
that spans a restart picks up where it left off.
Issue
Closes #47
Changes
ContextManager.to_dict/from_dict: serialize the memoized cache into aJSON-safe form and restore it.
ToolResultdataclasses are rebuilt as realToolResultinstances so cache hits keep the same interface. Anything thatcan't be serialized is skipped with a warning instead of raising.
SessionStore.get_cache/set_cache: store the cache under a separatecache:<id>key with the same TTL as session data, so it restoresindependently of session state and the two never overwrite each other.
Orchestrator: restore the cache at the start ofrun(), and checkpointsession state + cache after every tool (
_checkpoint) so a restart partwaythrough a review keeps the work already done.
Testing
make test-unit)make test-integration)make lint)make typecheck)New tests in
tests/unit/test_agent_state_persistence.py(10 tests): theserialization round trip (including
ToolResultreconstruction and skippingunserializable entries), the
SessionStorecache read/write under its own key,and an end-to-end restart where a second orchestrator reuses the persisted cache
instead of re-running the tool. They use a small in-memory
FakeRedisso no liveRedis is needed.
Notes for Reviewers
make test-unitreports 53 failures and
make checkreports ruff/mypy errors (missing librarystubs,
agent/tools/market_analyzer.py, etc.) unrelated to this issue. Mychanges add no new failures — the failing count is 53 before and after, my 10
new tests all pass, and ruff/black/mypy are clean on every file I touched.
profile_idto match howSessionStorealready keyssession state and because that's what the orchestrator receives. If concurrent
reviews on the same profile ever need isolation, a more specific key
(e.g.
review_id) would be the follow-up.cheap; if plans grow large we may want to batch writes.