test(sessions): integrate and verify the session-control slices - #6506
Draft
mmabrouk wants to merge 198 commits into
Draft
test(sessions): integrate and verify the session-control slices#6506mmabrouk wants to merge 198 commits into
mmabrouk wants to merge 198 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
A second user message on a session with a turn in flight killed both turns and left the session locked until the 30-minute lease expired (#6417, #5539, #5538). The platform's arbiter was always correct; the runner acted before reading its answer. The runner starts a turn's alive watchdog before it touches any sandbox, and that watchdog's first heartbeat is an atomic `nx` acquire of the session's `alive` lock in the API. When a second turn lost that acquire the API already answered `is_current_turn: false`. The runner read it only as "abort later", then walked into the keepalive pool, found the first turn's environment busy, and destroyed it. Turn one lost its sandbox mid-answer and turn two aborted on its own watchdog signal. Read the answer before acting: - `startAliveWatchdog` now reports `admitted`, the first beat's answer only. A later `is_current_turn: false` stays a cancel and keeps travelling the `onInterrupted` -> abort path. A network or HTTP failure still fails open. - `server.ts` stops a refused turn at the edge, before the interaction sweep, before the persisting emitter, and before `run()`. Nothing is persisted, so the refused message never enters the session's history and the client can keep the user's text. The refusal streams as an `error` event carrying the new `session_turn_in_use` code plus a failed terminal result. - The keepalive coordinator no longer evicts a `busy` entry. That branch was the destruction half of the bug. It now refuses, which is the backstop for the window admission leaves open when the API is unreachable. A `destroyed` entry still evicts and cold-starts, because nothing is in flight on it. Queue and steer are out of scope: both need a durable pending-input store, while refusing needs none. This is the `on_busy: reject` policy only. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
… pending gates CANCEL tombstoned whichever turn held alive/running at that instant, so a Stop applied after its turn ended killed the next turn, and the tombstone lives an hour with a refresh on every read (#6417, review H-3). It also left pending interactions alone, unlike kill, so a stopped session kept an approval card whose buttons answered a turn that no longer existed (#6315). Adds the optional `expected_execution_id` to the cancel request (the RFC's public name for what the coordination plane calls a turn id, per D-010). With it, cancel touches that turn or returns 409 and writes nothing. Without it, cancel refuses a turn whose recorded start is later than the request's arrival. That comparison needs a turn's start, which nothing recorded and which cannot be derived: session_turns.start_time is written by the runner after the fact, and a browser turn's id is a runner-minted uuid4. Adds one API-side Redis key in the shape of the existing tombstone key, written once when a turn takes alive. The route now reads the turns the cancel ended and cancels their pending gates with the same helper kill uses, scoped by turn. The arrival-time check is a backstop, not the fix. Measured against a live stack, it refuses 0 of 14 real Stop-then-Send races: the Stop genuinely reaches the API after the next turn starts. First-party clients sending the id is what closes the race, and no client can today. See docs/design/session-control-and-live-events/slice-stop-guard.md. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Replay already rendered a cancelled interaction as closed: settleApprovalPart maps a `cancelled` row to `output-denied`. The live path did not. The in-memory pending list was not gated on `stopped`, unlike the elicitation and connection docks beside it, so after a Stop the card stayed up with working buttons and hot keyboard shortcuts until a reload (#6315). Stop now cancels those gates server-side, so pressing approve answers a turn that is gone. Puts the rule in getLivePendingApprovals so the desktop and the mobile chat cannot disagree about it. `stopped` clears on the next send, so a new turn's gates appear normally. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
The records stream worker added every decoded Redis message id to its acknowledged list during deserialization, before it attempted the Postgres write. A failed `append_many` logged an error and continued, and the shared consumer loop then acknowledged and deleted those messages from the stream. Every Postgres failure was therefore permanent, silent record loss, and the worker reported success while doing it (#5496). `append_many` is one statement in one transaction, so one record Postgres rejected also took its whole batch with it, losing up to fifty unrelated records per rejection (#5594). Three changes: - `process_batch` returns a message id only once its rows are committed, or once the worker has decided to drop it on purpose (undecodable, or over quota). A failed entitlements check now defers instead of dropping, because an unreachable meter is transient. - A failed group is rewritten one record at a time, so a rejected record no longer discards the rest of its batch. - `StreamConsumer` gains an opt-in reclaim pass. `read_batch` only ever asks for `>`, so without it an unacknowledged entry is invisible to every later read and "leave it pending" would still lose the record. The pass claims the group's pending entries, and drops one after `max_deliveries` failures with an error log naming the lost record. The drop applies only while other records are committing. The delivery counter cannot tell a rejected record apart from a database that is down, so dropping on the count alone would delete every record in flight once an outage outlasts the budget. A live run against a real Redis found that hole; the guard closes it. The reclaim pass is off for the tracing and events workers, so their behaviour is unchanged. Verified against a real Redis 8: five records published during a twenty second write outage stayed pending, then all landed on recovery with no duplicates and an empty stream; a permanently rejected record let its batch mates through and was dropped loudly once traffic resumed. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
`RecordsRetentionDAO.delete_records_before_cutoff` selected and deleted on `RecordDBE.id`. That attribute does not exist. The records key is `(project_id, record_id)`, so every call to the retention flush raised before it deleted anything and records have never been aged out. Scope added on purpose: this defect is clear, obvious and one line, it sits in the records durability area this branch already touches, and Spike D found it while auditing the same pipeline. It is kept in its own commit so it can be reverted or landed alone. Verified: `hasattr(RecordDBE, "id")` is False, the primary key constraint at `dbes.py:18` is `(project_id, record_id)`, and the corrected statement compiles against the Postgres dialect. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
… turn The runner now refuses a message sent while another turn is already running on the same session (#6417, #5539, #5538). A naive refusal is worse than the bug for the person typing: the composer clears synchronously on submit, so the text they wrote is gone and there is no way to get it back. - `useAgentChatQueue` remembers the message it handed to `sendQueued`, both on the immediate path and on a queue release, and hands it back once through `takeLastSent`. A queued message never needed this; it is already in the queue and rendered by the dock. An immediately-sent one had nowhere to live. It is deliberately NOT re-queued: the queue releases on a settled "error" status, which for a refusal would re-send and be refused again in a loop. - `AgentConversation` puts that text back into the composer when the stream error is the refusal. The rAF mirrors the edit-stash restore beside it, because the editor clears itself after `onSubmit` returns. - The bubble says "Message not sent" instead of "The agent run failed", and offers no retry: nothing failed, and the text is already back in the box. - `parseAgentRunError` carries the stable class for the refusal, so the code reaches the bubble whether it arrives on the message part or the error. The refusal message is the contract with the runner. It is produced once, in `services/runner/src/sessions/admission.ts`, and reaches the browser verbatim: the SDK keeps a clean one-line runner error unchanged and the Vercel egress passes it through as `errorText`. Both constants must stay byte-identical. Mobile shares the queue hook and the error model but has its own composer and error effect, so it gets the refusal class without the text restore. 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's edge now acts on the first heartbeat's `is_current_turn`, so the three answers that decide a session's behaviour need their own coverage. The API code is unchanged; these lock the contract the runner reads. - A second turn arriving while a DIFFERENT turn holds `running` is refused, and the running turn's alive lock is untouched. - A refused turn's end beat (its watchdog release) cannot clear the live turn's `running`, because the release is owner-scoped. - An approval resume IS admitted while the previous turn is parked. `alive` alone cannot tell a park from a live turn; the absent `running` owner is what distinguishes them, and getting this wrong would stop every approval in the product from resuming. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Scan terminal session executions for missing transcript endings even after the session stream advances to a newer turn. Keep Redis cleanup scoped to the execution still named by the current stream row. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
PR #6517 already uses oss000000025 on this chain. This file becomes oss000000026 on top of 024. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Re-point oss000000025 onto oss000000026 so the core chain is 024 -> 026 -> 025. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
This branch is the integration of every Stop-package branch, for deployment and live testing only. It is not the source for review; review the package PRs. Head
589e376330merges PR #6496 (fa8c9f5cfc), PR #6503 (e4cbd64ac9), PR #6501 (eee7fd7d33), PR #6504 (017ab21a53), and PR #6517 (8c7af6954e) on top of #6500 and #6502, with #6517's migration re-pointed onto 026 in the merge so the core chain has one head (024, 026, 025).Changes
Merge commits only, plus conflict resolutions in
services/runner/src/server.ts,services/runner/src/sessions/alive.ts, the owner-release test,orphan_sweep.py(identical sweep fixes on both sides),env.py(union of settings), and mobileLiveConversation.tsx(the stop guard adapted tocancelSessionExecution). The merge record with every resolution is in the night status on PR #6505.Tests
pnpm lint-fixand ruff clean.AGENTA_SESSIONS_DURABLE_STOP=trueand migrations applied. The live cell results per harness and provider are being added to this description as they land; see the QA table below.QA on this head
Driver:
session_control.pyfrom PR #6518. Stackagenta-ee-dev-session-integration,AGENTA_SESSIONS_DURABLE_STOP=true. Run folders under~/agenta-qa-evidence/.20260903-230015-3207644-session-control20260903-233439-3632265-session-control20260903-234628-3777662-session-controlis_runningfalse 0.18 s after Stop), codex-child (reaped in 1.0 s), stale-tail (late record quarantined)20260903-235010-3823500-session-control20260904-003056-143305-session-controlBefore the approval fix in PR #6501, stop-approval on Pi failed 5 of 5 on local and Daytona.
Agent-generated, low weight. Not merged.
https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Note: an earlier Codex hook run failed four cells; the cause was the runner going unhealthy (cgroup freeze) after the restart cell plus one driver KeyError, both resolved. The Codex reap fix from PR #6496 (
cce2b21bc3) is not yet in this branch; re-merge #6496 before the next integration QA run.