fix(sessions): settle executions after runner or sandbox loss - #6501
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3f25f06 to
80ad140
Compare
mmabrouk
left a comment
There was a problem hiding this comment.
AGENT-GENERATED review, low weight. A read guide and the points to confirm; not an approval.
Read in this order
api/oss/src/utils/env.py+apis/fastapi/sessions/router.py— thedurable_stopflag and the legacy-path fallback (the rollback contract).api/oss/src/core/sessions/commands/service.pysettle— the one-transaction settlement across four DAOs.api/oss/src/dbs/postgres/sessions/executions/dao.py— the terminal compare-and-set andxmaxwinner detection.api/oss/src/tasks/asyncio/sessions/orphan_sweep.py— the watchdog, its second selection, and the abandoned-command sweep.services/runner/src/sessions/turn-settle.ts+engines/sandbox_agent/sandbox-liveness.ts— the runner-side hang and dead-sandbox guards.
Top findings
- Large PR (27 commits): watchdog + atomic settlement + quarantine + liveness probe + hang guard + heartbeat timeout + the approval-decision-then-prompt fix. All serve one invariant, but a human may want the approval change reviewed on its own.
- The rollback flag and all new settings are in
env.py; migrations are additive with downgrades and chain cleanly. - Confirm the shared-transaction settlement rolls back atomically, and that
xmax = 0reliably marks the insert winner underon_conflict_do_update. - No
records_closed_atleftover in the tree.
80ad140 to
43d77f1
Compare
|
Fix: parked-approval Stop on the local provider ( What the matrix found. Cell Cause. A regression in this branch's Fix. Split the settle check in Tests (
Runner unit suite: 2690 passed, 4 failed. The 4 failures are pre-existing in Residual race (follow-up, not a blocker). On an unpatched client, reparking without a confirmed cancel can leave the parked prompt open; stray frames from the rejected prompt could demux into a fast follow-up turn. Every current build ships |
Execution watchdog died on its first failing passThe final QA matrix found the watchdog was not running: the Cause
On the QA stack the first pass ran during the window when migration 026 had not yet added Fix (two commits)
Tests
Operational noteThe task is recreated only by the FastAPI lifespan. Any stack whose API process started while a sweep pass could raise still has a dead watchdog task and needs an API restart to recreate it. With migration 026 applied, a restarted loop settles the backlog and stays up. |
Follow-up: one unmappable command row no longer poisons the whole settleWith the watchdog running again (run 1b settled a lost execution at 141.7 s), every pass then logged Fix (
|
Two more watchdog hardenings1. A runner's claim survives a row it cannot map (
|
Symptom: an isolated re-run showed the full cost of the gap. The sandbox was deleted at 16:26:31, the runner's own socket was told SANDBOX_NOT_FOUND at 16:26:37, and the turn still beat running=true for thirty minutes. What ended it was the 30 minute per-tool-call deadline, and only then did the turn's error and done records persist, 27 minutes after the client had given up. Cause: everything downstream of the turn ending was already correct. The error terminal, the records, the running=false beat that clears the row and the teardown all happen within two seconds of the trip. The only defect was WHEN the turn ended, so the previous commit's unit tests pinned the trigger without showing the terminal it produces. Fix: drive the whole path through the real environment wiring with a fake socket. One test proves a provider answer naming the sandbox ends the turn as a sandbox_gone error terminal and reclaims the sandbox in the teardown. Its control proves an ordinary 502 on the same socket leaves the turn running, so a proxy blip cannot kill a healthy run. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Keep quarantine metrics tolerant of records-worker test doubles, align the atomic settlement fixture with the current interaction kind, and retain Ruff formatting after the conflict resolution. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
755ef53 to
4aecd1f
Compare
📘 Docs preview
This comment updates in place on every push. |
mmabrouk
left a comment
There was a problem hiding this comment.
🤖 The AI agent says:
This PR adds execution settlement, watchdog recovery, and protection against late output.
Focused simplification review using the new simplify skill: one non-blocking suggestion below. This is a static design/code review, not a rerun of the live release gate or a replacement for existing correctness reviews.
Railway Preview Environment
Updated at 2026-09-04T21:57:29.819Z |
mmabrouk
left a comment
There was a problem hiding this comment.
Agent-generated Codex review, low weight.
Verdict: CHANGES NEEDED
I found two additional P1 races in the watchdog's plain session_streams updates; details are inline. Both allow a stale sweep snapshot to overwrite or evict a stream generation that became live while the pass was awaiting other services.
Two earlier review-body findings also remain applicable at this head, so I did not duplicate them as new inline threads: the 30-minute idle/per-tool defaults still abort legitimate quiet tool or subagent work before the supported total deadline, and a settled_turns() lookup exception still lets the caller collapse the row and permanently hide a missing ending. See review #6501 (review).
The rebase otherwise preserves the requested compatibility points: flag-off cancellation still routes through request_cancel_legacy, flag-off late-record quarantine still uses the records fallback, the #6503 admission/replay/Stop fixes remain present, and the #6496 preflight-cancel and scoped-reap files match the base. The sandbox-gone latch is armed only after admission and keeps ordinary HTTP responses non-terminal.
Validation: the API sessions suite passed 684 tests with 9 warnings in 11.05s at nice -n 19. The supplied databases were not empty at their migration ledgers: core was already at OSS revision 027 while this branch ends at 026, and tracing was at 005, so this was a forward-schema run. Current CI also has one failing TypeScript-format gate on packages/agenta-chat/tests/unit/assets/agentTurn.test.ts; other completed checks are green, with acceptance jobs still pending at review time.
Use project, session, turn, and observed heartbeat predicates for each stream write. Only reconcile Redis and watches after a guarded update commits, and leave rows that advanced during the sweep untouched. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Track terminal-record lookup failures as deferred candidates. Keep their stream rows and Redis ownership intact so a later sweep can retry safely, and cover the failure-then-recovery sequence. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Keep the 30-minute idle and per-tool defaults as the watchdog product bounds. Forward both existing runner environment variables through every OSS and EE Compose service and document the defaults beside them. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Teach the Redis sweep fake to return the affected-row count produced by SQLAlchemy updates. This keeps the existing cleanup regressions aligned with the watchdog compare-and-set contract. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
|
Agent-generated, low weight. Regarding the run-limit finding in review 5117444093: the 30-minute idle and per-tool defaults are intentionally retained for this round, and the final default remains a product decision for Mahmoud. The trade-off is that 30 minutes bounds a quiet wedged turn (including the Daytona failure this watchdog addresses), while a legitimately silent long-running tool or subagent must opt into a longer window through AGENTA_RUNNER_RUN_IDLE_TIMEOUT_MS and AGENTA_RUNNER_TOOL_CALL_TIMEOUT_MS. Commit c0a5ab9 forwards both overrides into every OSS and EE runner Compose service and documents the defaults. |
mmabrouk
left a comment
There was a problem hiding this comment.
Agent-generated Codex review, low weight.
Verdict: CHANGES NEEDED
The requested guards, rowcount filtering, post-commit Redis ordering, per-project lookup deferral, and configurable 30-minute idle/tool defaults are present. Two P1 interleavings remain inline.
Move the guarded stream update ahead of lost execution settlement and share its transaction. Release only the swept Redis generation with one atomic compare-and-delete script, and cover both ordering races. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
mmabrouk
left a comment
There was a problem hiding this comment.
Agent-generated Codex review, low weight.
Verdict: CHANGES NEEDED
The primary rowcount ordering and the fully-installed turn-B cleanup case are fixed, but three race/recovery gaps remain inline.
Make established-turn heartbeat writes conditional on the row still naming that turn and on the execution having no terminal outcome. Treat a refused write as a dead turn and cover the sweep-commit-before-heartbeat-write interleaving against Postgres.\n\nClaude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Include the heartbeat turn in Redis owner values while preserving the replica-shaped public API and legacy values. Snapshot the full owner generation for watchdog cleanup, and cover the same-replica refresh gap before the new turn installs its locks.\n\nClaude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Route collapsed legacy rows without a durable turn id through unconditional Redis cleanup. Tombstone the turn ids recovered from stale locks and restore the null-turn regression coverage.\n\nClaude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Keep the nested-session watchdog regression attached to the full owner-value lookup introduced by the affinity generation fence.\n\nClaude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
mmabrouk
left a comment
There was a problem hiding this comment.
Agent-generated Codex review, low weight.
Verdict: CHANGES NEEDED
Two P1 race windows remain inline. The sweep now compares the full generated owner value, legacy bare owner values remain readable/upgradable, and null-turn cleanup plus its tests are restored. I found no separate flag-off regression in this delta.
Require an established-turn heartbeat to observe the stream row as alive and running before it can update the durable mirror. Cover the lock-wait interleaving where the statement snapshot predates the sweep commit and assert the heartbeat update affects no rows. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Carry the full owner value returned by a failed affinity claim into the departed-replica reclaim. Compare and delete that exact generation so a same-replica new-turn refresh cannot be removed in the reclaim gap. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
mmabrouk
left a comment
There was a problem hiding this comment.
Agent-generated Codex review, low weight.
Verdict: SHIP
The heartbeat UPDATE is fenced by the stream row’s own turn and alive/running flags, so PostgreSQL’s post-lock row recheck rejects the collapsed row; the real-Postgres test confirms the UPDATE is blocked on the sweep lock before commit and returns rowcount 0. Departed-replica reclaim compare-deletes the exact owner generation returned by the failed claim without re-reading it. No regression found in 5c5355e..9609237.
Context
When a runner died mid-turn, the session stayed "running" forever. When the runner came back after the watchdog had already ended the turn, its late output landed in the transcript beside the ending, so one turn showed two endings. This PR is the watchdog half of the Stop package in the session-control design (PR #6495), stacked on the durable Stop command in PR #6503.
Changes
The branch carries three groups of commits. Read them in order.
Watchdog and quarantine (the original slice). A sweep every 60 s ends any turn whose runner heartbeat is older than 90 s with one
execution_losterror and onedone, keyed on heartbeat age rather than on the owner lease. Records that arrive after that ending are kept but markedquarantined_atand hidden from every transcript read, so one ending stays effective.rejectis one setting away.Two sweep fixes found on the integrated stack. A stopped row whose runner died before its terminal record got no ending; now the sweep writes it and releases the dead turn's
alivelock. Each sweep pass is bounded, and a pass that times out is logged.Review fixes from 3 September. These came from the staff review of the design and touch the command path as well, so they sit here rather than on #6503:
AGENTA_SESSIONS_DURABLE_STOPand the legacy cancel response moved down into PR feat(sessions): deliver durable Stop directly to the runner #6503 on 2026-09-04, so feat(sessions): deliver durable Stop directly to the runner #6503 is safe alone. This PR keepsAGENTA_SESSIONS_LATE_OUTPUT(quarantinedefault orreject) and everything below. Rebased on the new feat(sessions): deliver durable Stop directly to the runner #6503 head; 26 commits; head43d77f1989.max_deliveries; a Stop whose runner is gone settleslost. Before this, the sweep skipped that case and the session read "stopping" forever.session_executionstakes a compare-and-set from the runner outcome route and from the watchdog; the loser gets a clear "lost the race" result. Records ingest only reads that state, fails open when core Postgres is down, and quarantines only output written after an involuntary ending (lost, orstoppedby the other writer). Ausagethat trails its owndoneis ordinary history.session.promptwith the new text instead of resuming the old prompt. Before this, the next message after an approval Stop answered the tool denial and never saw the new question, on both providers.Before:
POST /sessions/{id}/cancelduring an approval → 202, gate cancelled, next message → "The command was refused. How would you like to proceed?"After: the same Stop → 202, gate cleared and parked warm, next message → the answer to the new question in the same sandbox.
Tests
pytest oss/tests/pytest/unit/sessions -qagainst Postgres: 622 passed on this branch, 671 on the integrated branch.pnpm testinservices/runner: 2,692 passed on this branch, 2,743 integrated.reviews/folder on PR docs(sessions): draft session control and shared live-events RFC #6495 and the night status on PR docs(sessions): collect overnight reviews, spikes, and test evidence #6505.oss000000023_add_session_executionsandoss000000024_add_execution_redis_reconciliationare additive with downgrades. Note: PR feat(sessions): separate session history from tracing retention and gate immutable record writes #6517's migration was renumbered to 25 so the ids no longer collide; whichever PR lands second re-points itsdown_revisionto the new head.What to QA
Live cells run on the integrated stack with the release-gate driver from PR #6518 (
session_control.py). The morning report on PR #6505 lists the results per harness and provider. Watch for: stop-approval on Pi local and Daytona (the fix above), post-stop-row (is_runningfalse within seconds of the Stop), stale-tail (latedonequarantined), restart-after-stop (native session survives).Fixes of 2026-09-04 (found by the new failure cells)
6d3add4b58).8b809fafef).session_executions(bd5b330b89), with a nullableending_written_atmark set by the watchdog and by records ingest, a partial index, descending order, and a stopped-shaped ending for a user Stop (migrationoss000000026,eee7fd7d33).All three were reviewed by the agents that found them. Live re-runs on the merged head follow.
Fixes of 2026-09-04, afternoon (found by the full matrix on the merged head)
Stop during a parked approval: the Stop required a settled harness cancel; on a client without
cancelSessionit threw, tore down the warm sandbox, and wrote no execution row. When no cancel can be sent, the Stop now reparks the sandbox warm and settles as stopped (3e9d5c1651).Watchdog loop dead on the first error: the loop's except branch called
log.exceptiononMultiLogger, which had no such method, so the first failed pass killed the loop for the life of the process with zero log lines. The loop logs withexc_infoandMultiLoggergainedexception(6e0962cd14,d9df2f2cd4).Sweep poisoned by one unknown command row: a row with a kind this build cannot map raised inside the batch and left every abandoned Stop pending. Both the claim and the abandoned paths now skip unmappable rows through one shared mapper and warn once per batch (
757900145c,c663b43b50).Lost settlement left the row running: the watchdog settled the execution lost but
session_streams.is_runningstayed true until the runner's own beat. The same pass now clears the flag, the running lock, and the mirror on the row that still names the dead turn (d40dd7c9d0).Returning runner re-beat a dead turn: after the sweep cleared the row, an unpaused runner's late heartbeat set it running again. The sweep tombstones every swept turn, and the heartbeat path refuses a tombstoned turn (
a8e7920e0f).Hard kill keeps the session affinity: a runner killed with no grace never released
owner:session:<id>, so the restarted runner's first heartbeat for the next turn lost the non-stealing owner claim and refused the message with "already running a turn" for up to 120 s; the earlier fix only covered a turn the sweep declared lost. The heartbeat now reclaims affinity from a departed replica when the running lock is free or its own; the alive lock stays the single arbiter. Not behind the flag, because the key and the lock primitives are shared with the legacy path (15d58add1c).The lost settlement did not clear the running flag (finding 7): the watchdog pass wrote the session row's flags through the ORM after nested sessions had detached the row, so the write never reached the database while a plain UPDATE in the same pass did. Both session_streams writes in the sweep are now plain UPDATEs from captured ids; a Postgres test replays the real pass (
2cbd1d3cdd,2613968468).A deleted Daytona sandbox left a dead turn beating for 30 minutes: the provider's proxy keeps answering "not found" for a deleted sandbox, which the liveness probe counted as alive, and the transport's error died unhandled inside the protocol library. A "not found" answer now ends the turn with one error record within seconds; the detector arms only after the sandbox is acquired so a start-up race cannot trip it (
1ddea5e81e). Follow-ups: the two-minute first-byte timer never fires; the Python SDK adapter's idle timeout (180 s) is shorter than the runner's own (360 s).Live on the integration stack after these fixes (Pi local and Codex local, last-message shape): stop-approval, runner-gone-late, stale-tail, sandbox-gone, concurrent-stops, repeat-stop, stop-during-completion, and records-outage pass. The Codex runner-gone-late re-run on
15d58add1cand the runner-gone read-while-paused cell are still open.Agent-generated, low weight. Not merged.
https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV