fix(sessions): settle executions after runner or sandbox loss - #6501
fix(sessions): settle executions after runner or sandbox loss#6501mmabrouk wants to merge 70 commits into
Conversation
A user Stop aborted the run signal and nothing else. The turn ended with
stopReason "cancelled", shouldPark answered false for every aborted run, and
the sandbox was deleted, so the next message paid a cold start and lost the
native harness session. The abort never told the harness anything either: it
only made the runner stop waiting, leaving an open prompt and a running tool
that only the teardown ever stopped.
Cancel the harness first, then park. On the cancelled path the turn now sends
the ACP session/cancel notification for the live session and waits a bounded
time for the harness to answer its open prompt. ACP requires the agent to end
that prompt with stopReason "cancelled", so a settled prompt is the harness
reporting it is idle. Only a settled cancel parks; a cancel that cannot be
sent, or that the harness never answers inside the budget, leaves the
environment unknown and still destroys it.
sandbox-agent refuses a manual session/cancel ("Use destroySession(sessionId)
instead"). The guard is in the TypeScript client only, so the pnpm patch adds
cancelSession(id), which sends the same managed cancel destroySession sends
without marking the session record destroyed. The daemon inside the sandbox
proxies ACP and holds no such rule, so no Daytona snapshot rebuild is needed.
The cancel deliberately does not abort env.mcpAbort. That controller belongs to
the environment, not the turn, and a parked environment must keep its tool-MCP
server; the approval-park path already skips it for the same reason.
Client-disconnect behavior is unchanged. The clientGone check moved above the
abort check so a disconnect still destroys whatever the abort says.
Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Answers the six questions the work package asked, with path:line evidence: where the session/cancel guard lives (the vendored client, not the daemon), what the harness reports after a cancel, what happens to a running tool call, why every cancellation path destroyed the sandbox before this change, the eight-line client patch, and why Daytona needs no rebuilt snapshot. Also records the live protocol and its results for Pi and Codex, a negative control that forces the settle budget to 1 ms and shows the destroy path, the recommended settlement timeout for D-016, a release-gate cell, and the three things the spike did not cover. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
The first cut inferred the Stop from `stopReason: "cancelled"`, but the turn sets that value whenever the run signal aborts, whatever aborted it. Any future `controller.abort()` anywhere in the runner would then silently start parking sandboxes nobody had checked, which is the failure the teardown allowlist exists to prevent. The one call site that means a cooperative Stop, the heartbeat interrupt in server.ts, now labels its abort, and shouldPark requires that label alongside the cancelled stop reason and the settled harness cancel. The mechanism is the standard AbortController.abort(reason), so nothing new is threaded through the engine, the coordinator or the turn. Also from the review: - A stopped session parks on its own window, defaulting to the 600 s approval window locally because the user is about to type, and to the ordinary 120 s idle window on Daytona where a parked sandbox is billed compute. One named field, one env var, so the two windows collapse again with one value. - The terminal done record carries stopReason "cancelled" as well as "paused". Without it a stopped turn is indistinguishable from a completed one in Postgres, so neither the frontend nor the release gate can tell a Stop from a finish. Kept as a two-value allowlist so a harness-reported end_turn cannot start appearing there by accident. - Corrects the comment claiming the abort severs the harness fetch. It does not: the signal reaches the client's health wait only, never the ACP transport, which is why the cancelled branch has to send a real session/cancel. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
…windows Answers the reviewers' remaining questions with measurements rather than expectations. The finding that needs a decision: a stopped Codex turn leaves its shell command running inside the parked sandbox, and Pi does not. Measured by cancelling a running sleep and having the next turn list processes; one probe returned two leftovers at once, from two different sessions. Running the same scenario down the destroy path left none, so parking is what makes the child survive rather than something this change merely revealed. The fix belongs in the Codex ACP bridge, which this repo already patches on both image surfaces, and unlike the runner-side cancel it would need a Daytona snapshot rebuild. Also records the current park windows and the new stopped-session window, why the abort now carries an explicit reason, that cancel, steer and kill are indistinguishable to the runner until the durable command plane lands, the terminal-record fix with its Postgres evidence, and two release-gate assertions including one that fails on Codex today on purpose. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
The runner change that carries `stopReason: "cancelled"` through to the terminal done record had live evidence but no test. Three runner assertions now pin it: a Stop carries the reason, a pause still does, and a completed turn plus every harness-reported reason carry nothing. The last one is the point of the two-value allowlist, so it is the one worth having. On the frontend, transcript reconstruction reads only "paused" (transcriptToMessages), so a cancelled done falls through to the ordinary terminator and closes the turn like a completed one. That is the behaviour we want, and "the new value is inert here" is a claim worth a test rather than a comment, so two cases pin it: a stopped turn does not swallow the next turn the way a pause does, and it is not marked paused. Verified: 159 runner files / 2650 tests, and 52 agenta-chat transcript tests. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
… window The first cut shipped a decision that is not mine to make: it defaulted a stopped local session to the 600 s approval window. The field now defaults to the ordinary idle window on both providers, so introducing it changes no timing at all, and it exists only so the value is one named setting with one env var when somebody decides to move it. The recommendation stays, written where the reader who changes it will be standing: make it the approval window on the local provider, because a user who stops is about to type and the 60 s idle window can throw the sandbox away while they are still writing. Daytona would not follow, because a parked Daytona sandbox is billed compute and its 120 s window is already that decision. Try it with AGENTA_RUNNER_SESSION_STOPPED_TTL_MS. A third test covers the env var moving the stopped window without disturbing the ordinary idle one. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
…rk window Adds one table near the top saying what was actually tested and on what, so a reader does not have to infer coverage from the prose: Pi and Codex live on the local sandbox, Claude not tested for want of an Anthropic key, Daytona not tested at all, and per harness what the cancel does to the in-flight tool. Also records that before this change the abort sent no cancel to Claude Code or Codex at all, which is why the difference between the two harnesses was invisible until now. Corrects the park-window section and the matching open question to the value that actually ships. The stopped window defaults to the ordinary idle window and changes no timing; moving it to 600 s locally is a recommendation for Mahmoud, not something this spike decided. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
A user Stop reached the runner only as the absence of a Redis lock, noticed on the next heartbeat up to 30 seconds later. Nothing recorded that a Stop had been asked for, so a Stop against an unreachable runner was simply lost and no execution ever reached a terminal outcome anyone could read. Add session_commands: one row per durable request to change an execution. Two columns that are never merged carry the two questions a caller actually asks. state says where the COMMAND is (pending, claimed, applied, obsolete). outcome says what happened to the EXECUTION (stopped, not_running, superseded_by_newer_turn, failed, lost). A client drawing a Stop button reads the execution; a client retrying safely reads the command id. Every transition is one UPDATE ... WHERE <expected state> RETURNING *, decided by scalar_one_or_none, the same compare-and-set transition_interaction already uses. That is what stops two API replicas both winning a claim or both writing a terminal outcome. Idempotency has two layers: the caller's Idempotency-Key on (project_id, session_id, idempotency_key), and a collapse onto any open command for the same target execution, which is what makes two Stops in a row correct without asking the browser to send a key. session_streams gains two columns. stopping_turn_id names the execution an accepted Stop is waiting on, written in the same transaction as the command insert. turn_started_at records when the row's current turn_id started, because the stale-Stop guard has to compare a Stop's arrival time with the running execution's start time and there was nowhere to read that: updated_at is the heartbeat timestamp and moves every 30 seconds, runner-minted turn ids are uuid4 and carry no time, the Redis lock value is a bare turn id a Lua compare reads whole, and the session_turns append is fire-and-forget so a running turn may have no row. It is stamped only when the id actually changes, so the repeated heartbeats that restamp the same id never move it. Both columns backfill to NULL. A row written before this migration yields no comparison and the guard does not fire, deliberately: a guard that refused every Stop it could not verify would break the common case to protect a rare one. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Control delivery sits behind a port so the transport can be swapped without touching a route, a data shape or a state transition. This adds the first adapter: a direct call to the runner's own POST /cancel over the same authenticated hop that already carries hard kill. No held connection, no poll loop, no per-session Redis channel. Agenta runs one runner, and the parts that carry the correctness are the record and the guards, which are identical whichever transport delivers. The order is not negotiable. The command row is committed BEFORE the runner is called. Calling first and recording afterwards gives back every failure the record exists to close: a crash between the call and the insert leaves an aborted execution with no terminal outcome written anywhere. Where the direct call fails is that env.runner.internal_url is one service address, so with two runner replicas behind a load balancer it reaches the right process only by luck. That failure is quiet, because the wrong process honestly answers "I do not hold that session", which is also what a session that really ended answers. Two things make it loud. Each heartbeat now adds one sorted-set entry naming its replica, and the adapter refuses to deliver at all when more than one replica has beaten inside the census window, so the command stays durable instead of being posted into the dark. And a not_held for a session whose row is alive with a fresh heartbeat is the wrong-replica case and nothing else, so the service settles it lost rather than telling the user the work had already finished. AGENTA_SESSIONS_CONTROL_ADAPTER picks the transport and defaults to direct. The lease, the delivery cap, the sweep interval and the admission deadline join it in one SessionsCommandsConfig block, read through the shared env object. publish_session_ended becomes public on the streams service because a settled Stop publishes the same lifecycle notification an ordinary turn end publishes. There is one ended event, not a Stop-shaped one and a turn-shaped one; a client cannot be asked to tell them apart. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Stop gets its own route instead of being the no-inputs, no-force corner of the four-mode stream command. Cancel means cancel: one route, one meaning. Admission stamps the arrival time first, before any read, then resolves the target execution once from Redis running, falling back to alive so a session parked awaiting an approval is reachable. That parked case is the one with no control channel at all today, because a parked session stops heartbeating. Three guards then decide. A stale expected_execution_id is refused with 409 and nothing is written. With no expectation sent, an execution that started after the request arrived is not the one the user meant, so the command is inserted already settled and targets nothing. And the target is pinned once, so a turn that starts later has a different id and a pinned command cannot reach it. Redis is not written at admission. The stopping execution keeps alive and running while it stops, which is what prevents a second message from starting underneath it. At settlement the API tombstones the stopped turn before releasing running, so a late beat from it cannot re-arm the locks it is about to lose, and it leaves alive to its own time to live exactly as an ordinary turn end does. Force- deleting alive is what makes today's cancel read as a session teardown, and warm resume is the required outcome of Stop. Settlement also cancels that execution's pending interactions, scoped to the one turn so a newer turn's gates survive. An approval card whose execution was stopped is a card whose buttons do nothing. The route is deliberately NOT behind check_runner_concurrency_limit: refusing to STOP work because a project is at its run limit is the exact wrong answer to a busy project. The runner reports what happened on an internal outcome route that authenticates with the shared runner token rather than a project credential, because the runner holds none for a command it was handed. The command id resolves the project, so the exemption widens no tenant boundary: a caller can only settle a command whose id it knows and whose claim it holds. POST /sessions/streams/ keeps its exact current behaviour. It becomes a thin wrapper over this command in a later change, so released clients keep working until the browser and the wrapper flip together. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
The abort controller for a session-owned run was a local variable inside the request handler. Nothing outside that closure could reach it, so the only way to stop a turn was to take the session's Redis lock away and wait up to 30 seconds for the heartbeat to notice. POST /cancel reaches the run directly. A module-level registry maps <projectId>:<sessionId> to the live execution, the same key shape poolKeyFor builds, because two projects may use one session id and the project segment is the tenant boundary. It records startedAt, which is what makes a late Stop safe: the API pins the target at admission, but the runner's comparison against its own memory is exact, so an execution that began after the command was created is never aborted. Registration happens as soon as the abort controller exists, so a Stop during environment acquisition still lands, and it is removed in the same finally that releases the alive watchdog, scoped to the turn id so a finishing turn cannot unregister its successor. Applying a command twice is not harmless: by then the session may be running a newer turn, and a second abort would kill work nobody asked to stop. So the set of applied command ids lives beside the session pool rather than inside any request or loop, and an entry is written when a command is ACCEPTED, not when the cancel finishes, so a duplicate arriving mid-cancel is also a no-op. An already-applied command is a no-op that STILL acknowledges, which repairs a lost acknowledgement without a second abort. /cancel answers 202 when it holds the session and 404 when it does not, and it resolves a parked approval through the keep-alive pool before answering 404 — that session runs no turn, so the registry alone would miss exactly the case that has no control channel today. The response is an acknowledgement, never an outcome: what happened to the execution goes to the API's outcome route, so settlement has one path on every transport. The applier sits above the transport, so a long-poll adapter would call the same applyCommand and change none of these guards. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Stop posted the four-mode stream command with no inputs and no force, which drops the session's Redis lock and leaves the runner to notice on its next heartbeat. It also sent no execution id, so a Stop applied a fraction of a second after its turn ended tombstoned whichever turn had started in between, for an hour. It now posts the cancel route, naming the execution it means. The id is read FRESH from the session row rather than from the project-wide liveness poll, which is up to 15 seconds stale: a stale id is refused with a conflict and the user's Stop would do nothing. When the row names no turn we send no expectation and the API resolves the target, which is what happened before. A conflict is an answer, not a failure — the run this tab was watching had already ended — so the call refreshes the session's own state either way rather than retrying. The call is awaited and the session state refreshed on the response, so the Inspector and the liveness dot stop lying about a run that has been stopped. The client uses raw axios rather than the Fern client, because the route is new and the generated client does not know it yet. Move it onto Fern at the next regeneration. Mobile is untouched. Adds the API tests for the two things a Stop must never get wrong: missing the run the user meant, and killing a run they did not. Fifteen admission cases cover the arrival-time stamp, the stale expectation, the newer-turn guard, the parked session, the collapse of two Stops, and the settlement that releases running while leaving alive alone — that last one is what pins warm resume at the API layer. Fourteen DAO cases run against a real Postgres, because what they test IS the database: the unique constraint, FOR UPDATE SKIP LOCKED under two concurrent claims, and the guarded terminal transition. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Three defects, all found by driving a real Stop against a live stack, and all in the delivery path rather than in the record. THE REGISTRY NEVER HELD THE SESSION, so every Stop got a 404 and settled at once while the turn ran to completion. The entry was keyed by <projectId>:<sessionId>, but the project scope is not known when a run starts: runContext.project.id is empty on the live invoke path, and the scope that forms the pool key comes from the signed mount, which the coordinator resolves after the run is already in flight. Register under the session id at once, and let the coordinator fill the project in through onScopeResolved as soon as it knows it. A lookup matches only when the stored project agrees, so another tenant is refused rather than misrouted; until the project is known the entry matches, because refusing every Stop in the first moments of a run is the bug this replaces. THE OUTCOME REPORT WAS REFUSED WITH A 409, so the command stayed claimed and the session stayed marked stopping forever. The API claimed on the runner's behalf under a placeholder, while the runner reported under its own replica id, and the settle guard compares the two. Read the id out of the runner's acknowledgement and claim under that. THE MULTI-REPLICA CENSUS REFUSED DELIVERY AFTER EVERY RESTART. A runner mints a fresh replica id at boot when AGENTA_RUNNER_REPLICA_ID is unset, so its previous id is still inside the census window and the count reads two. Refusing on that count breaks Stop for the whole window after an ordinary deploy, which is worse than the failure it guards against, and it was observed doing exactly that. The census now logs at error level and delivers anyway. The exact detector was always the other one: a not_held for a session whose row is alive and beating is the wrong-replica failure and nothing else produces it. Also close the collapse race properly. Two Stops in a row already collapsed, but two in the SAME INSTANT did not: admission reads for an open command and then inserts, and neither request can see a row the other has not committed. A unique partial index on (project_id, session_id, kind, target_turn_id) over the open states makes the database decide, and the losing insert reads the winner back. Verified live: two simultaneous requests now return one command id. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Brings the two design documents onto this branch so it stands alone, and adds the slice record: what changed with path:line references, the measured live protocol, the three defects the live run found that no unit test saw, what is left for the long-poll adapter and the stream-route wrapper, and five open questions. The measurement worth keeping: a Stop reaches the running turn in 72ms, the harness confirms at 90ms, the command and the execution settle at 116ms, and the sandbox parks warm just under a second later. The budget was five seconds. The deviation worth flagging: the work package asked the direct adapter to refuse delivery when more than one runner replica has heartbeated in five minutes. It warns instead. A runner mints a fresh replica id at boot, so its own restart puts two ids in the window and refusing there breaks Stop after every deploy, which was observed. The exact detector is the not_held rule, which needs no census. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
…nsus Two corrections after rebasing onto Spike A's final tip and re-reading the revised design. THE ABORT NEEDED THE USER-STOP LABEL. Spike A now parks only an abort the runner can prove was a cooperative Stop: shouldPark requires isUserStopAbort alongside the cancelled stop reason and the settled harness cancel, because inferring the Stop from the stop reason alone would let any future controller.abort() park a sandbox nobody had checked. The execution registry handed the applier a bare controller.abort(), so after the rebase every Stop delivered as a command would have ended the turn cancelled and then DESTROYED the sandbox, which is the exact failure Stop exists to avoid. It now aborts with USER_STOP_ABORT_REASON. A command from the control plane is the clearest user Stop the runner ever sees. Two tests pin it, one on each side of the contract: an abort carrying the label leaves the turn parkable, and an unlabelled one does not. Verified live after the rebase: the runner logs park-cancelled with the stopped-session window, and the next message resumed in the same sandbox. THE REPLICA CENSUS IS GONE. The revised design keeps it as an optional extra and names the exact detector as the one to build. The census cost a Redis write on every heartbeat, and it could not tell two live replicas from one that had restarted, because a runner mints a fresh id at boot. Removing it deletes a per-beat write, two settings, and a whole module. What remains is the detector that is exact: a not_held for a session whose row says alive with a fresh heartbeat means some process is running that session and it is not the one we called. It now also names the owner replica from the Redis owner key, so the log says where the Stop should have gone rather than only that it did not arrive, and it settles the command lost rather than not_running, so the user is told the Stop failed instead of that the work had already finished. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
… removal Re-measures the live protocol against the rebased runner (82ms to the abort, 126ms to settlement, 993ms to the warm park), records the fourth defect the rebase exposed, and rewrites the census entry: it is removed rather than softened, on the revised design's guidance that the not_held detector is the one worth building and the census is optional. That closes the one open question that was a deviation from the brief. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
|
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 |
mmabrouk
changed the title
Settlement released `running` in Redis and stopped there. The row kept `is_running: true`, and the row is the only thing the product's liveness polls read: `query_streams` serves Postgres and never looks at Redis. Nothing else could correct it. Settlement tombstones the stopped execution before it releases `running`, so the runner's own final `is_running=false` heartbeat is refused by the tombstone check and returns before the mirror write at the end of `heartbeat`. The order cannot be swapped: a late beat that found `alive` free would take it straight back under the dead turn's id. The runner reports its outcome as soon as it issues the abort, so the tombstone always wins that race. Measured on the local sandbox with Pi before the fix: the row read `is_running: true` with a pre-Stop `updated_at` for the full 193 s of the sample, while Redis had released `running` within 0.5 s. The tab that pressed Stop therefore showed a "running somewhere else" strip over its own session until the orphan sweep collapsed the row minutes later. After the fix the row reads `is_running: false, is_alive: true` within 0.15 s of the request, and the parked sandbox still resumes warm. `mirror_liveness` re-reads Redis rather than writing a literal `false`, so a newer turn that has already taken `running` is reported and not erased. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Every liveness `refetchInterval` asked "is the alive set non-empty", which is not the question. Stop ends the work and leaves the session alive so the sandbox resumes warm, and an ordinary turn end does the same, so one stopped session held all four polls at 15 s, in every open tab, for the hour that `alive` lock lives. One shared predicate now answers the real question: 15 s while something is RUNNING, 60 s while a session is merely alive, and stop when nothing is alive. The sidebar rail passes a 60 s idle floor instead of stopping, because it must still discover a run it did not start; that baseline was already deliberate and is unchanged. The mobile gates poll keys on running directly, since a running turn is what mints new gates. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
…olves Admission resolves the execution to stop from the Redis `running` key with a fallback to `alive`, so a session parked on an approval is reachable. The `expected_execution_id` guard then compared against `running` alone. A parked approval has released `running` and still holds `alive` under the same turn id, so the guard read `running` as none, refused the request with a conflict naming "current: none", and left the gate pending. The identical Stop sent without an expectation was accepted and cancelled the gate. The browser always sends the id it streamed, so pressing Stop on an approval card was refused in the product while the integration approval cell passed, because its driver sent no expectation. The guard fired on the one case it exists to allow. It now compares against the resolved target. The guard still refuses a stale id: a Stop naming a finished turn on a session parked under a newer one is refused, and the conflict now names the turn that would have been stopped instead of none. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
…aimed Admission inserts the command `pending`, hands it to the runner, and writes `claimed` on the runner's behalf only after the runner answers. A runner that aborts fast reports its outcome inside that window, while the row still says `pending`. The outcome route guarded on `claimed` alone, so that report was refused with a conflict. Observed on a Stop during model output: the runner aborted and parked correctly, logged `[control] outcome HTTP 409`, and the command sat `claimed` for 2 min 17 s until the sweep settled it `obsolete` with outcome `lost`. The user watched "stopping" for the whole sweep window and a Stop that worked was recorded as lost. The guard is now a set, and it is still one statement, so it is evaluated at the moment of the write. Reading the state first and updating after would reopen the same race: the claim can commit in between. The replica guard is widened only where there is nothing to guard. A `pending` row holds no claim, so a null `claimed_by` passes; a claimed row must still be claimed by the reporter, and a report from any other replica is still refused. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
…ndbox A Stop pressed as the answer lands used to tear the sandbox down, so the next message rebuilt cold. Reproduced on the local sandbox with Pi by firing the Stop on the runner's own `prompt stopReason=end_turn` line: [control] aborted command=... turn=c81b783e... [control] outcome reported command=... state=stopped [keepalive] evict key=... reason=no-park:end_turn and the next message took 7.2 s against 1.9 s warm. Two things were wrong, in two places. The execution stays registered through teardown, which writes the transcript, exports the trace and parks the environment, and that takes hundreds of milliseconds. A Stop arriving in that window found a live entry and aborted it. The abort stopped nothing, because the prompt had already settled, but the aborted signal then made `shouldPark` refuse to park a healthy idle environment. The run is now marked settled the instant the harness prompt settles, before teardown begins, and the applier does nothing at all for a settled run. Nothing aborts, so the ordinary park path runs. It reports `obsolete` with `not_running`, because the command stopped nothing. Past that window the runner has dropped the execution and answers `not_held`, and the API judged that on whether the row was beating. A turn that has just ended leaves `alive` set and a fresh beat behind it exactly as a running one does, so every late Stop was settled `lost` and the user was told their Stop failed when the work had simply finished. The discriminator is now `running`: an execution holding it means a process is running this session and it is not the one we called, which is the wrong-replica failure the `lost` outcome exists for. With no `running` owner nothing is executing anywhere, and `not_running` is the honest answer. Verified live on both windows: the command settles `obsolete` / `not_running`, no eviction, and the next message reuses the parked sandbox warm (`hit-continue`, 1.9 s). Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
3f25f06 to
80ad140
Compare
`claim_commands` ended in `[map_command_dbe_to_dto(dbe) for dbe in claimed]`, the same batch map that broke the abandoned-command sweep. A newer API replica can write a command `kind` this older replica's enum does not know, and `map_command_dbe_to_dto` raises ValueError on that row; one such row in a claimed batch threw away the whole claim, including a Stop the runner could act on. Generalize the sweep's defensive mapper into `_map_commands_skipping_unmappable` and route both the claim and the abandoned-command path through it: skip the rows this replica cannot map, warn once per batch with the kinds, count, and which batch (claimed or abandoned), and return the rest. The unknown row is left for a replica that knows its kind. The enum and the write path are unchanged. A unit test claims an unknown-kind row next to a claimable Stop and asserts the Stop is returned, the unknown row is dropped, and the skip is warned once naming the claimed context. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
… lost When the watchdog settled an execution lost, it wrote the terminal records but left the session_streams row reading is_running: true whenever that row was not one the orphan query collapsed. The SEND gate reads that flag, so the next message was refused until the runner returned -- which, for a lost turn, may be never. Observed on the integration stack (run 1c, cell runner-gone): the execution was settled lost at 13:23:56 but the stream row still read is_running true, and the flag only flipped when the paused runner came back and beat. The RFC's rule is that the lost settlement writes the ending, clears is_running, releases alive, and updates the mirror in the same pass. The went-silent collapse already does this for the rows the orphan query matches. This extends the ending-without-collapse branch to every other lost turn: it clears is_running on the row that STILL names the dead turn (keeping is_alive so the session stays resumable), clears the Redis running lock, and publishes the mirror change. Everything is guarded on turn_id, so a row that has advanced to a newer running turn is never disturbed. Two unit tests: a lost execution whose row still names it reads is_running false right after the pass and its running lock is cleared; a lost OLD execution leaves a newer running turn's flag and lock untouched. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Two more watchdog hardenings1. A runner's claim survives a row it cannot map (
|
…is_running After the sweep cleared is_running on a lost turn's row, a runner that returned seconds later beat that same turn_id and re-set is_running true, and the SEND gate refused the next message again (observed live: run 1e, session a70c22d4, turn e49c060b, a beat 3.5 s after the settle). The heartbeat path already refuses a turn the sweep has tombstoned (is_turn_superseded), but the orphan-collapse path tombstoned only the turns that still held the Redis alive/running keys. A turn whose keys a prior Stop settlement had already cleared held nothing to displace, so it was collapsed but never tombstoned, and its returning beat was admitted. Tombstone the collapsed row's own turn_id unconditionally, alongside any displaced key owners. The tombstone carries the 1-hour superseded TTL, so a runner that returns within the hour has its dead-turn beat refused; the ending-without-collapse branch already tombstones its turns the same way. This is the smallest fix on the mechanism the heartbeat path already trusts and adds no read to the beat hot path. A unit test sweeps an orphan whose turn holds no Redis keys and asserts the turn is tombstoned afterwards. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Finding 6: a returning runner re-set is_running on a swept turn (
|
…running turn Symptom. Matrix run 3, cell runner-gone-late, harness codex, provider local. A Stop settled cleanly: the command read applied/stopped, one terminal record was written, and the row read is_running false. The runner was then killed. 2.1 s after it reported healthy, the recovery Send came back with "This session is already running a turn", although no turn was running anywhere. Session 2fdf43f0-c728-42e5-987e-2371501fe748, first turn 97aa3357, recovery turn c8cd62c7. Cause. The cell kills the runner with no grace period, so the shutdown handler was killed while it was tearing down a parked keepalive session and never reached the step that hands back owner:session:<id>. That key kept naming the dead replica for the rest of its 120 s lease. claim_owner never steals from a different owner, so the restarted runner's first heartbeat for the new turn lost the claim and the API answered is_current_turn false. The runner prints INTERRUPTED for any false is_current_turn and then refuses admission, which is why the log and the user both read a live-turn conflict that did not exist. The sweep's owner clear (8b809fa) does not cover this: it fires only for a turn the sweep declares lost, and this turn had ended cleanly 19 ms before the kill. Fix. The owner key says which box is SERVING a session, and only an in-flight turn's heartbeat ever refreshes it, so a claim held by a replica with no running turn protects nothing. The heartbeat now takes it instead of refusing for the rest of the lease. The reclaim runs only from the beat of a real running turn, and only when `running` is unheld or held by the caller's own turn, which is the same discriminator the alive-lock handover already uses. It is clear_owner (release-if-owner) then the ordinary non-stealing claim_owner, so a concurrent claim by a third replica wins and is reported truthfully. The alive lock stays the single arbiter of one execution per session and is untouched. Not behind AGENTA_SESSIONS_DURABLE_STOP. The affinity key and the lock primitives (claim_owner, clear_owner, get_running_owner) are shared with the legacy path, which has the identical defect, so gating the fix would leave that path broken. Scope. A runner killed MID-turn still waits for a sweep tick, not one beat, because `running` is held with a 3600 s TTL. The sweep's clear covers that case; the two are disjoint. Tests. New unit file with 6 tests, no Postgres needed. Three reproduce the live failure and fail without the reclaim. Three are guards that must pass either way: a live turn on another replica is still refused, a turn-end beat never reclaims, and a beat with no turn id never reclaims. api/oss/tests/pytest/unit/sessions reads 577 passed, 70 skipped. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
|
Agent-generated, low weight. Affinity reclaim: the next Send after an ungraceful runner death (15d58ad)SymptomMatrix run 3, cell
No turn was running anywhere. Session CauseThe cell kills the runner with
The alive-lock path would have been fine on its own. The sweep's owner clear (8b809fa) does not cover this. It is gated on the sweep having torn Corroboration in the same log: 0.13 s later the SAME replica beat normally for a different A note for anyone reading the run matrix: the Pi pass of this cell was not a harness FixThe owner key says which box is SERVING a session, and only an in-flight turn's heartbeat ever New The FlagNot behind Scope. A runner killed MID-turn still waits for a sweep tick, not one beat, because TestsNew
Known limitsThree, all rated LOW in review, none blocking.
Follow-ups for other lanes
Matrix cells to re-run
|
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Review summary
I recommend fixing these two correctness risks before merge.
1. Long-running tools are aborted after 30 minutes
Risky: services/runner/src/engines/sandbox_agent/run-limits.ts:36-38
The new defaults treat 30 minutes without an emitted event, or 30 minutes inside one tool call, as proof that the turn is stuck. That is not valid for our supported use cases. A tool can invoke a subagent that works for more than an hour without emitting an ACP event. In that valid flow, the idle timer or per-tool timer aborts the parent turn and records a failure while the tool is still working correctly.
Please do not use a fixed 30-minute silence as proof that a tool is dead. The smallest safe fix for this PR is to disable the idle and per-tool limits by default, or set their defaults at least as high as the supported total turn duration. A better follow-up is a tool-specific policy or a real liveness/progress signal from long-running tools and subagents. Add a test where a tool remains quiet for more than 30 minutes and then completes successfully.
2. A tracing lookup failure can permanently lose the ending
Risky: api/oss/src/tasks/asyncio/sessions/orphan_sweep.py:238-246
When settled_turns() raises, the affected keys enter neither the settled nor unsettled set. The caller still collapses the rows in orphans, so the next watchdog pass cannot select them again. The user can continue, but the previous turn may remain permanently without execution_lost and done.
Please exclude lookup-failed keys from row collapse, Redis cleanup, and tombstoning so a later pass can retry. Healthy projects in the same batch should still proceed. Add a two-pass test where the tracing lookup fails first and succeeds on the next pass.
Reviewed at 15d58add1c88e01e1cd9a8051ab942e6ecb4e6ce.
Symptom: with the runner paused, a watchdog pass settled the execution lost and settled the Stop command, but session_streams.flags kept is_running true on the row that still named the dead turn. The pass logged "settled a session_stream whose runner went silent" for that row. A read 2.65 s later still saw is_running true, and live forensics found no other writer. Cause: the collapse wrote the flags by ORM attribute assignment on the rows loaded at the top of the pass. TransactionsEngine.session is an async_scoped_session keyed by the current asyncio task, so every nested engine.session() in the same pass returns the SAME session and calls session.close() in its finally. That close expunges every ORM row the pass had loaded. The records lookup and the command settlement both open such a nested session before the collapse runs, so by then the rows were detached, no session tracked the mutation, and the final commit emitted no flags UPDATE. A Core UPDATE is not tied to ORM instance state, which is why the command settle's stopping_turn_id write in the same pass persisted while the flags write vanished. Fix: capture the id, project_id, session_id and turn_id of each orphan row as plain values before any nested session runs, then collapse the rows with one Core UPDATE keyed by those ids. The Redis and watch steps that follow read the captured tuples, never the detached rows. Tests: a new Postgres-gated integration test replays the real pass end to end with the real DAOs on a database it creates and drops per run, then reads the row back through a fresh session. Against the pre-fix code it fails, and the SQLAlchemy statement log shows exactly one UPDATE session_streams in the whole pass, the command settle's stopping_turn_id, with no flags UPDATE at all. The three fake-session watchdog suites now apply the Core UPDATE to their in-memory rows. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
|
Agent-generated, low weight. Pushed Symptom. With the runner paused, one watchdog pass settled the execution Cause. The collapse wrote the flags by ORM attribute assignment ( Fix. Capture each orphan row's Tests.
Two notes for whoever rebases.
|
The collapse was moved to a Core UPDATE in 2cbd1d3. The other write to session_streams in the same pass, the is_running clear for a turn settled lost whose row the collapse does not own, still went through ORM attribute assignment. That write was correct only by adjacency: its rows are re-selected after the last nested engine.session(), and no database call runs between that select and the commit. Adjacency is not a property anyone can see. engine.session() is an async_scoped_session keyed by current_task, so any nested call closes the shared session and detaches those rows, and the write is then dropped at commit with no error and no log line. One new database call between the select and the write reintroduces finding 7 on this branch. Capture the row id, project id, session id and new flags as plain values, then perform both session_streams writes from those values just before the commit, each through a Core UPDATE with synchronize_session=False. No ORM attribute write on session_streams remains in the sweep. The write is buffered until the commit either way, so no observable ordering changes. Tests: a second Postgres-gated test seeds a row that beats normally and names a turn already settled lost, which is the branch the clear owns. It patches release_alive to open and close a nested engine.session() between the row load and the write, then reads the row back through a fresh session and asserts is_running false with is_alive kept. Restore the ORM attribute write in this layout and that test fails on is_running still true. The three fake-session watchdog suites now apply a Core UPDATE keyed by a single id as well as by a list. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
|
Agent-generated, low weight. Follow-up to the comment above. Pushed One correction to what I wrote above. That clear never failed in production. It was correct by adjacency: its rows are re-selected after the last nested A second Postgres-gated test pins the property. It seeds a row that beats normally and names a turn already settled |
Prove that a conflicting execution authority rolls back the command transition, and that replaying the winning execution outcome reports only the original insert as the CAS winner. Pin watchdog record lookups to one batch per project and exercise the production flag-off quarantine path with the execution DAO wired. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Show that an abandoned run remains alive to execute its own teardown when it eventually settles. Pin the sandbox health URL derivation for both local and Daytona inspector URL shapes. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Symptom: on Daytona, deleting the sandbox under a running turn did not end the turn. The runner kept sending running=true heartbeats for five minutes, the row kept reading is_running with no stopping_turn_id, and the turn ended only when the runner process took SIGTERM. The same cell passes on every local provider. Cause: two blind spots meet. The liveness probe counts any HTTP answer as alive by design, and Daytona keeps its proxy host up after a sandbox is deleted and answers 404 with x-daytona-error-code: SANDBOX_NOT_FOUND, so the probe never counted one failure. The transport did see the truth, but the ACP client calls failReadable on that 404 and the protocol SDK's read loop never rejects its pending responses, so the session/prompt promise the turn awaits stayed pending and the escaped rejection was only logged as an unhandled rejection. Fix: treat a provider answer that names THIS SANDBOX as gone as a verdict rather than a network symptom. sandbox-gone.ts recognises it, narrowly: the answer must be an HTTP error, and either the provider's own error code names the sandbox or the error body does. A bare 404 and a 401 still count as alive, and the resumable SANDBOX_STOPPED and SANDBOX_ARCHIVED states do not count as death. The liveness probe now ends the turn on the first such answer instead of after three failures, and the turn's own ACP fetch reports the same answer onto a per environment latch the probe fires on at once. The latch is armed only after acquireSandbox resolves, because the SDK's health wait rides the same fetch and tolerates a provider error by design while a sandbox comes up. The probe hands its listener back on dispose, so a warm environment keeps no finished turns. The turn then ends through the run-limit trip path every other limit uses, so it writes one error terminal with code sandbox_gone. A normal Stop, warm parking and native session reuse are untouched. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
|
Agent-generated, low weight. Pushed Symptom. Deleting the Daytona sandbox under a running turn did not end the turn. The runner sent Cause. Two blind spots meet.
Fix. Treat a provider answer that names THIS SANDBOX as gone as a verdict rather than a network symptom.
The turn then ends through the run-limit trip path every other limit already uses, so it writes one error terminal with code Tests. Two follow-ups this does not fix.
|
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
|
Agent-generated, low weight. Follow-up to my previous comment, with one correction to it. Pushed Correction. My first follow-up said the 2 minute time-to-first-byte deadline was dead. That was wrong, and I withdraw it. The isolated re-run's runner log shows what actually ended the turn: The harness had opened a tool call before the sandbox died. What the isolated re-run adds. Everything downstream of the turn ending was already correct. From the trip, the whole chain completes in 2.2 seconds:
So the eviction is not the cause, it is one step of the same teardown. The single defect was WHEN the turn ended, and that is what this branch moves from minute 30 to second 6. The four effects, and where each happens.
That also answers the open question in the re-run's note. Yes, a Send was refused for the whole window, and on the Tests. Standing follow-up, now the only one. |
2251b4d to
cd21450
Compare
1 participant
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