feat: State persistence recovery fix - #113
Conversation
|
Hi @saharannaveen There are some minor issues have given inline review comments kindly check /address also the CI is failing with some issues, may need to be resolved. LGTM otherwise |
d836d18 to
fa37166
Compare
WalkthroughAdds optional lifecycle persistence for compiled graphs. Redis tracks inflight runs and checkpoints. PostgreSQL stores leases, interruption metadata, and recovery status. Shutdown persists active runs, while startup initializes Aegra and resumes eligible runs. New settings control persistence and recovery. Tests cover encryption, Redis tracking, lease recovery, startup behavior, and pod coordination. The mock MCP server now uses FastMCP Streamable HTTP. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
🚀 Post-Merge Actions
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
af71484 to
500468f
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deep_agent/aegra/lifecycle.py`:
- Around line 445-530: Refactor persist_inflight_runs to expire all collected
run_ids with one database UPDATE using a run_id = ANY(...) filter, rather than
calling _expire_run_lease once per run. Preserve the existing running-status
condition, lease metadata updates, logging, deregistration, and persisted-count
behavior; update _expire_run_lease or replace its usage so only a single
synchronous Postgres connection is opened during shutdown.
- Around line 343-369: Update get_inflight_run_ids to batch the per-key hash
reads through a Redis pipeline rather than calling hgetall inside the loop.
Preserve filtering for the current POD_ID, collection of non-empty run_id
values, and the existing unavailable/error fallback behavior.
In `@deep_agent/aegra/startup.py`:
- Around line 160-172: Cap the interrupted-run recovery query in the startup
recovery flow by adding the same 20-row limit used by the stale-run diagnostic
query. Keep the recovered_run_ids collection and subsequent UPDATE behavior
unchanged, while ensuring the SELECT does not load more than 20 rows.
In `@scripts/inspect-db.sh`:
- Around line 55-68: Replace direct $THREAD_ID interpolation in the thread and
forensics SQL commands with psql variable binding, passing the identifier
through the psql invocation and referencing the bound variable in each query.
Apply the same protection to the additional affected query range while
preserving the existing query behavior and output.
In `@scripts/kind-pod-kill-test.sh`:
- Line 33: Replace the hardcoded username and password in the DB connection
string with values sourced from environment variables or the project’s existing
secret configuration, while preserving the current host, port, and database
name.
In `@scripts/pod-kill-and-recover.sh`:
- Line 104: Update the background Aegra command in the pod recovery script to
write logs to a securely created temporary file using mktemp, rather than the
predictable /tmp/agent.log path. Reuse the generated temporary filename for
output redirection and preserve the existing background startup behavior.
In `@tests/integration/test_lifecycle_integration.py`:
- Around line 30-75: Update the _FakeRedis test double to implement scan_iter
with the same prefix-pattern behavior as keys, yielding matching keys after
cleaning expired entries. Ensure get_inflight_run_ids can scan this mock
successfully so lifecycle assertions validate actual returned run IDs rather
than the module’s exception fallback.
In `@tests/unit/test_lifecycle.py`:
- Around line 465-476: Add pytest asyncio markers to test_no_interrupted_runs
and every other async test in TestResumeInterruptedRuns in
tests/unit/test_lifecycle.py (anchor site, lines 465-476), and to
test_resume_after_persist and test_stale_lease_reclaimed in
tests/integration/test_lifecycle_integration.py (sibling site, lines 177-206).
Alternatively, confirm the project config enables asyncio_mode = auto; do not
leave these async tests unmarked when that setting is unavailable.
- Around line 87-97: Replace the mocked `_get_encryption_secret` call in
`test_encryption_secret_uses_sso_client_secret` with an invocation of the real
`lifecycle._get_encryption_secret` function while retaining the patched settings
containing `SSO_CLIENT_SECRET`; assert that it returns the configured secret so
the test exercises the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f1d52010-19bc-42a7-99ba-13a4b01ae37a
📒 Files selected for processing (14)
deep_agent/aegra/graph.pydeep_agent/aegra/lifecycle.pydeep_agent/aegra/shutdown.pydeep_agent/aegra/startup.pydeep_agent/src/infrastructure/subagents.pydeep_agent/src/settings.pyscripts/inspect-db.shscripts/kind-pod-kill-test.shscripts/pod-kill-and-recover.shtests/integration/conftest.pytests/integration/test_lifecycle_integration.pytests/mocks/mock_mcp_server.pytests/unit/aegra/test_graph.pytests/unit/test_lifecycle.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
redhat-data-and-ai/template-mcp(manual)redhat-data-and-ai/template-ui(manual)
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/inspect-db.sh (1)
102-108: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCompare the claimant with the original worker before labeling recovery, because a non-null, non-dash
claimed_byonly proves that a run is claimed and mislabels ordinary single-worker runs as recovered by a different pod.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/inspect-db.sh` around lines 102 - 108, Update the “WORKER POD HISTORY” query in the runs case to compare claimed_by with the run’s original worker identity, and only include runs where those values differ; retain the existing non-null/non-dash filters and recovered-by-pod output.scripts/kind-pod-kill-test.sh (1)
172-192: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail when
claimed_bydoes not match a running pod instead of killing the first pod, because the fallback can terminate an unrelated replica and leave the target worker alive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/kind-pod-kill-test.sh` around lines 172 - 192, Update the KILL selection logic around HANDLING_POD and POD_TO_KILL so that no fallback pod is selected when no running pod matches the claimed handler. Fail the script with an error and stop before kubectl delete, ensuring only the explicitly matched pod can be terminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deep_agent/aegra/lifecycle.py`:
- Around line 361-364: Update the pipeline flow around client.pipeline() and
pipeline.execute() to process keys in bounded batches instead of queuing every
scanned key at once. Execute each batch, collect or handle its results
incrementally, and ensure no unbounded list of keys or hashes is materialized
while preserving the existing hgetall behavior.
In `@scripts/inspect-db.sh`:
- Around line 177-182: Update the checkpoint aggregation query in the $PG
invocation to cast metadata::jsonb->>'step' to a numeric type before applying
MIN and MAX, so from_step and to_step use numeric ordering while preserving the
existing run and count aggregation.
---
Outside diff comments:
In `@scripts/inspect-db.sh`:
- Around line 102-108: Update the “WORKER POD HISTORY” query in the runs case to
compare claimed_by with the run’s original worker identity, and only include
runs where those values differ; retain the existing non-null/non-dash filters
and recovered-by-pod output.
In `@scripts/kind-pod-kill-test.sh`:
- Around line 172-192: Update the KILL selection logic around HANDLING_POD and
POD_TO_KILL so that no fallback pod is selected when no running pod matches the
claimed handler. Fail the script with an error and stop before kubectl delete,
ensuring only the explicitly matched pod can be terminated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d1722fd1-eec6-4c63-9bbe-4d12e1bb8cb0
📒 Files selected for processing (7)
deep_agent/aegra/lifecycle.pydeep_agent/aegra/startup.pyscripts/inspect-db.shscripts/kind-pod-kill-test.shscripts/pod-kill-and-recover.shtests/integration/test_lifecycle_integration.pytests/unit/test_lifecycle.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
redhat-data-and-ai/template-mcp(manual)redhat-data-and-ai/template-ui(manual)
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
deep_agent/aegra/lifecycle.py (1)
361-364: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAvoid unbounded key materialization
As per path instructions, the 50-key pipelines still rely on
keys = list(client.scan_iter(...)), which retains every matching key in memory; consume the iterator incrementally into bounded batches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deep_agent/aegra/lifecycle.py` around lines 361 - 364, Update the key-processing flow around the batch_size loop to consume client.scan_iter(...) incrementally instead of materializing all keys with list(...). Accumulate at most 50 keys per batch, process each batch before collecting the next, and preserve the existing run_ids behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@deep_agent/aegra/lifecycle.py`:
- Around line 361-364: Update the key-processing flow around the batch_size loop
to consume client.scan_iter(...) incrementally instead of materializing all keys
with list(...). Accumulate at most 50 keys per batch, process each batch before
collecting the next, and preserve the existing run_ids behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b740cdaf-7b2e-4f0e-a058-07f7d51e0605
📒 Files selected for processing (3)
deep_agent/aegra/lifecycle.pyscripts/inspect-db.shscripts/kind-pod-kill-test.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
redhat-data-and-ai/template-mcp(manual)redhat-data-and-ai/template-ui(manual)
9e23cae to
5e2d6d7
Compare
…(RHITAIF-206) Persist in-flight conversation state on SIGTERM so new pods can resume interrupted runs from LangGraph checkpoints. - Add lifecycle.py: Redis inflight tracking, Postgres run status updates, Fernet-encrypted token persistence, FOR UPDATE SKIP LOCKED resume - Add shutdown step: persist_inflight_runs() marks active runs as interrupted - Add startup step: resume_interrupted_runs() scans and reclaims orphaned runs - Add graph.py import guard for lifecycle availability - Add 4 lifecycle settings (feature-flagged, enabled by default) - Add 22 unit tests + 6 integration tests (28 total, all passing) Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
…(RHITAIF-206) Persist in-flight conversation state on SIGTERM so new pods can resume interrupted runs from LangGraph checkpoints. Components: - lifecycle.py: Redis inflight tracking with status=active/interrupted, Fernet-encrypted SSO token persistence, FOR UPDATE SKIP LOCKED resume with lease-based claiming (claimed_by + lease_expires_at) - shutdown.py: persist_inflight_runs() reads active_runs dict, marks runs interrupted in both Redis and Postgres - startup.py: resume_interrupted_runs() scans for orphaned runs, claims with lease, loads checkpoint, rebuilds graph context - graph.py: hooks register_inflight/deregister_inflight and attaches execution context to compiled graph for run boundary tracking - settings.py: 4 feature-flagged lifecycle settings (default enabled) Tests: 31 unit + 7 integration = 38 total, all passing Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
- Remove non-existent checkpoint_id column from runs query - Select assistant_id and execution_params instead - Update resume_fn signature to (run_id, thread_id) - Fix integration tests to match new signatures Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
Direct ainvoke(None) cancels pending tool calls. Instead, use
POST /threads/{thread_id}/runs/wait with checkpoint config to
resume through the same path as a normal user request. This
ensures proper graph execution with checkpointer injection.
Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
Per LangGraph docs, graph.ainvoke(None, config={thread_id}) resumes
from the latest checkpoint — nodes before the checkpoint are skipped,
nodes after re-execute including LLM calls and tool invocations.
Attach checkpointer and store from db_manager explicitly since we're
outside Aegra's normal request path.
Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
…n-pod' Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
On shutdown, expire leases so the built-in LeaseReaper re-enqueues interrupted runs through the standard Aegra worker path. No custom graph building or resume code needed. Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
…sting Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
- startup.py: add _init_aegra_db() to initialize Postgres checkpointer when running under raw uvicorn (production Containerfile) - scripts/kind-pod-kill-test.sh: automated pod kill & recovery demo for Kind cluster testing Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
Kill only the agent (port 5002) instead of all services. Use configurable PG_CONTAINER env var, clear MCP circuit breaker on restart, and remove UI restart logic since it's managed separately. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
- Fix SQL parameterization bug: INTERVAL '%s seconds' → %s * INTERVAL '1 second' - Replace Redis KEYS with SCAN for production safety - Convert resume_interrupted_runs to async psycopg.AsyncConnection - Add warning log when refresh token encryption is skipped - Update test mocks for async connection pattern Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
Aegra's run_executor sets aegra:run:done:<id> even for interrupted runs. On startup, clear done/counter/cache keys for interrupted runs being re-enqueued so LeaseReaper can pick them up. Also wire up the LIFECYCLE_RESUME_ON_STARTUP config flag and convert startup DB calls to async psycopg. Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
Replace plain FastAPI REST server with FastMCP using Streamable HTTP transport so the agent's MCP client can connect via the standard MCP protocol at /mcp. Same 4 tools, same logic. Signed-off-by: Naveen Saharan <nsaharan@redhat.com> Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
The graph factory tests pass MagicMock objects for model_name and assistant_id, which fail Pydantic validation in build_execution_context. Patch LIFECYCLE_PERSISTENCE_ENABLED to False since these tests don't exercise lifecycle behavior. Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
- Batch Redis hgetall calls via pipeline in get_inflight_run_ids (N+1 fix) - Batch lease expiry into single UPDATE ... WHERE run_id = ANY() query - Add LIMIT to unbounded interrupted runs SELECT in startup.py - Use psql variable binding in inspect-db.sh to prevent SQL injection - Source DB credentials from env vars in kind-pod-kill-test.sh - Use mktemp for secure temp file in pod-kill-and-recover.sh - Add scan_iter and pipeline hgetall to _FakeRedis in integration tests - Fix tautological test to call real _get_encryption_secret() - Fix integration tests to mock AsyncConnection.connect (not sync) Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
- Batch Redis pipeline hgetall in groups of 50 to bound memory - Fix worker pod history query to compare original vs recovery pod - Fail explicitly when handler pod can't be matched instead of killing wrong replica Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
0ee1d83 to
5f64817
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deep_agent/aegra/graph.py`:
- Around line 385-407: The cached graph must not retain per-request lifecycle
state such as user identity or the encrypted refresh token. Update the flow
around build_execution_context and the _lifecycle_* setattr calls so this
context is supplied per request rather than stored on the shared compiled
object, or extend cache_key to include the user-specific identity before
_graph_cache lookup/storage; preserve shared graph reuse without cross-user
context leakage.
In `@deep_agent/aegra/lifecycle.py`:
- Around line 577-622: Change the resume batch flow around the SELECT/UPDATE
logic to claim all eligible rows and commit the transaction before invoking
resume_fn. Store the claimed run_id and thread_id values, then process them
afterward so resume_fn and its success/error updates do not run while the FOR
UPDATE locks are held; preserve per-run result recording, logging, and
deregister_inflight behavior.
- Around line 221-243: The register_inflight flow in lifecycle.py performs
synchronous psycopg.connect and UPDATE work on the async agent request path;
make this database write non-blocking by using an async PostgreSQL connection or
offloading the entire existing connection, cursor, commit, and update sequence
via asyncio.to_thread. Preserve the current started_by_pod update and debug
logging behavior while ensuring register_inflight does not block the event loop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 16ad62de-44ff-447c-a103-baf6277fa831
📒 Files selected for processing (11)
deep_agent/aegra/graph.pydeep_agent/aegra/lifecycle.pydeep_agent/aegra/shutdown.pydeep_agent/aegra/startup.pydeep_agent/src/infrastructure/subagents.pydeep_agent/src/settings.pytests/integration/conftest.pytests/integration/test_lifecycle_integration.pytests/mocks/mock_mcp_server.pytests/unit/aegra/test_graph.pytests/unit/test_lifecycle.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
redhat-data-and-ai/template-mcp(manual)redhat-data-and-ai/template-ui(manual)
5f64817 to
dc57005
Compare
Remove inspect-db.sh, pod-kill-and-recover.sh, and kind-pod-kill-test.sh from the repo. These are local development/testing utilities. Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
When a pod is killed and the LeaseReaper re-enqueues a run, the stored input_data and command cause LangGraph to re-process them on top of the checkpoint state. This triggers PatchToolCallsMiddleware to mark in-flight tool calls as cancelled, causing the orchestrator to re-delegate and re-trigger HITL approval. Fix: at startup, clear input_data and command from execution_params for all recoverable runs before the LeaseReaper re-enqueues them. This makes _resolve_input() return None so the graph resumes purely from checkpoint. Also removes orphaned tools (queue_task, check_task_status, get_pending_results) from PROMPT.md. Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
cryptography 49.0.0 has a Bleichenbacher-style timing oracle in pkcs7_decrypt_* functions (CVSS 8.2, fixed in 50.0.0). Not exploitable in template-agent — PKCS7 decrypt is never used; cryptography is a transitive dep for TLS only. Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
The lifecycle:inflight:* Redis keys were never created in production. register_inflight() was stashed via setattr on the compiled graph, but Aegra's graph.copy() drops setattr attributes — the hook never fired. All recovery operates through Postgres (LeaseReaper + checkpoints). Removed: register_inflight, update_inflight_checkpoint, _mark_inflight_interrupted, deregister_inflight, get_inflight_run_ids, get_redis_client wrapper, REDIS_INFLIGHT_PREFIX, REDIS_INFLIGHT_TTL. Simplified persist_inflight_runs (Postgres-only) and resume_interrupted_runs (removed no-op deregister call). Fixed startup.py importing get_redis_client from lifecycle instead of redis module (was causing warnings on every restart). Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
Signed-off-by: Naveen Saharan <nsaharan@redhat.com>
2e8ae38 to
a24a237
Compare
Summary
Descrition
When an agent pod is killed mid-conversation (rolling update, OOM, node eviction), the in-flight run is lost — the user sees
a frozen chat with no response, and the conversation cannot be resumed. There is no mechanism to track which runs were
active, persist their state, or recover them on a replacement pod.
Changes
template-agent (14 commits, 2680 lines added)
New module: deep_agent/aegra/lifecycle.py — Lifecycle state persistence engine that tracks inflight runs in Redis and
recovers them after pod restarts:
TTL-based lease
Redis and Postgres
duplicate processing across replicas), claims them via lease, and re-enqueues through the Aegra worker executor
Modified: deep_agent/aegra/startup.py — Calls resume_interrupted_runs on pod startup to recover any runs left behind by a
killed predecessor
Modified: deep_agent/aegra/shutdown.py — Calls persist_inflight_runs during graceful shutdown to mark active runs as
interrupted before the pod terminates
Modified: deep_agent/aegra/graph.py — Integrates lifecycle tracking into the agent graph: registers inflight at run start,
updates checkpoint on node completion, deregisters on completion/failure
Testing: 641-line unit test suite (test_lifecycle.py) + 318-line integration tests + 3 shell scripts for Kind cluster
pod-kill testing (kind-pod-kill-test.sh, pod-kill-and-recover.sh, inspect-db.sh)
User Flow
checkpoint_id.
resumes the graph from the last completed node.
nothing happened.