feat(runner): preserve the warm sandbox after Stop - #6496
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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:
📝 SummarySummary by CodeRabbit
WalkthroughThe change adds cooperative Stop handling that can preserve a settled sandbox, abortable environment acquisition with cleanup, native-history verification for continuation, explicit runner ownership release, Codex process reaping, terminal-record safeguards, and design documentation with unit coverage. ChangesSession control lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to Stopped sessions can retain unsafe sandbox state, while resumed sessions can lose conversation context. These paths should be corrected before merge. Sequence Diagram(s)sequenceDiagram
participant Client
participant Runner
participant SandboxAgent
participant Harness
participant SessionStore
Client->>Runner: request Stop
Runner->>SandboxAgent: abort with user-stop reason
SandboxAgent->>Harness: session/cancel
Harness-->>SandboxAgent: settle prompt
SandboxAgent->>SessionStore: persist cancelled terminal record
SandboxAgent-->>Runner: return cancelSettled
Runner->>Runner: park or destroy sandbox
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
This was referenced Sep 2, 2026
mmabrouk
commented
Sep 3, 2026
mmabrouk
left a comment
mmabrouk
left a comment
Member
Author
There was a problem hiding this comment.
Review summary
- Risky:
services/runner/src/engines/sandbox_agent/engine.ts:49- Codex can satisfy this generic settled-cancel predicate while leaving its in-flight shell process alive, so the runner parks a sandbox that is not actually idle. - Missing environments: Claude Code was not tested because no Anthropic key was available. Daytona was not tested. Relevant runner and web test jobs are skipped on the exact head.
User problem
Before this change, Stop aborted the runner's wait but did not cancel the harness prompt. Teardown then deleted the sandbox, causing a cold start and losing the native harness session on the next message. The intended user outcome is a fast Stop followed by a warm continuation in the same sandbox and harness session.
How the change works
The runner labels the heartbeat interruption, sends ACP session/cancel through a small patch to sandbox-agent@0.4.2, waits up to 10 seconds for the open prompt to settle, records stopReason: "cancelled", and parks only when cancellation was sent and the prompt resolved. Unsettled cancellations still destroy the environment. A separate stopped-session time-to-live setting defaults to the existing idle window.
The state test is incomplete for Codex. Prompt settlement proves that the harness answered. It does not prove that subprocesses started by an in-flight tool stopped.
Issue coverage
This PR does not link a GitHub issue, and its timeline has no linked issue. I therefore cannot claim that it fully or partially solves any issue based on repository evidence. PR #6505 references this spike as one part of the overnight work, and integration PR #6506 includes it, but those are pull requests rather than issue coverage.
Dependencies and overlap
This is not independent relative to main. It is stacked on draft PR #6495 at base SHA c72d1f00e5a589a1f0f5e3f60b19862e68dfe315, and the reviewed delta starts at merge base f4a6834ba6af9d9201c225d6cb94020393f03537.
PR #6503 is directly stacked on this branch and adds the durable Stop command and direct-call delivery path. PR #6506 overlaps by integrating #6496, #6503, and the other session-control slices, with additional conflict fixes and combined gate tests. The current heartbeat callback still receives one undifferentiated ownership-loss signal for cancel, steer, and kill. The durable command work in #6503 is therefore relevant to reviewing the final control semantics, but it does not remove the Codex subprocess defect documented here.
Tests and missing environments
At exact head f5b1ae6244885ecfe70191effe9ed5f15fc9e943:
- Local runner typecheck passed.
- Seven focused runner unit files passed, 198 tests total. They cover cancellation helpers, park policy, teardown, pool dispatch, steer mount loss, and lifecycle routing.
- The agenta-chat transcript suite passed, 52 tests.
git diff --checkpassed.- GitHub format, lint, contribution, and secret checks passed.
- GitHub runner, runner acceptance, runner integration, services, and web unit jobs were skipped.
- The PR report records live local Pi and Codex runs. It records no Claude Code run and no Daytona run.
- The recorded Codex probe fails the required idle-state invariant because the shell child remains alive after Stop.
Recommendation
Hold. Do not ship generic warm parking for Codex until cancellation also terminates its in-flight tool process, or gate the warm-park path by harness and keep deleting Codex sandboxes after Stop. After that fix, review this as a stack with #6503 and the integration behavior in #6506.
mmabrouk
changed the title
This was referenced Sep 3, 2026
mmabrouk
added a commit
that referenced
this pull request
Sep 3, 2026
mmabrouk
commented
Sep 4, 2026
mmabrouk
left a comment
mmabrouk
left a comment
Member
Author
There was a problem hiding this comment.
AGENT-GENERATED review, low weight. A read guide and a few places to look; not an approval.
Read in this order
services/runner/src/sessions/stop-signal.ts— the abort label that makes a user Stop distinguishable from every other abort.services/runner/src/engines/sandbox_agent/engine.tsshouldPark— the one place that decides park vs delete.services/runner/src/engines/sandbox_agent/cancel-turn.ts— cancel the harness turn and wait for it to settle.services/runner/src/engines/sandbox_agent/reap-exec.ts— kill the shell child a stopped Codex turn leaves running.services/runner/src/sessions/alive.ts+ streamsservice.pyrelease_owner— hand the affinity key back on shutdown.
Top findings
- One coherent runner-side change; strong tests across all five threads.
- No feature flag, but the fallback is structural: an unpatched client has no
cancelSession, so the cancel is unsettled and the sandbox is destroyed exactly as before. - The park decision rests on
USER_STOP_ABORT_REASONbeing applied at exactly one call site; the reap kills by pid with a small pid-reuse window (bounded, best-effort). stoppedTtlMsis inert and carries an open decision for Mahmoud (move a stopped local session to the 600s approval window).
mmabrouk
marked this pull request as ready for review
September 4, 2026 11:06
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/runner/src/server.ts (1)
654-654: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve persistence failures until the session is marked incomplete.
flush()already consumestakePersistFailures(sessionId). Line 654 then reads zero, sonoteRecordsIncomplete()is skipped after exhausted persistence retries. A later turn can reconstruct from an incomplete transcript. Return the dropped count fromflush()or mark the session incomplete before clearing it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Team
Run ID: a22a0113-5102-4897-bbef-23fc49527469
⛔ Files ignored due to path filters (1)
services/runner/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (53)
api/oss/src/core/sessions/streams/dtos.pyapi/oss/src/core/sessions/streams/service.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_release_owner.pydocs/design/session-control-and-live-events/README.mddocs/design/session-control-and-live-events/context.mddocs/design/session-control-and-live-events/decisions.mddocs/design/session-control-and-live-events/plan.mddocs/design/session-control-and-live-events/records-invariants.mddocs/design/session-control-and-live-events/requirements.mddocs/design/session-control-and-live-events/research.mddocs/design/session-control-and-live-events/rfc.mddocs/design/session-control-and-live-events/spike-a-sandbox-cancel.mddocs/design/session-control-and-live-events/status.mddocs/design/session-control-and-live-events/tonight-handoff.mdservices/runner/patches/sandbox-agent@0.4.2.patchservices/runner/src/engines/sandbox_agent/agent-mount.tsservices/runner/src/engines/sandbox_agent/cancel-turn.tsservices/runner/src/engines/sandbox_agent/credential-preflight.tsservices/runner/src/engines/sandbox_agent/engine.tsservices/runner/src/engines/sandbox_agent/environment-setup.tsservices/runner/src/engines/sandbox_agent/environment.tsservices/runner/src/engines/sandbox_agent/mount.tsservices/runner/src/engines/sandbox_agent/provider.tsservices/runner/src/engines/sandbox_agent/reap-exec.tsservices/runner/src/engines/sandbox_agent/run-turn.tsservices/runner/src/engines/sandbox_agent/runtime-contracts.tsservices/runner/src/engines/sandbox_agent/session-identity.tsservices/runner/src/engines/sandbox_agent/teardown.tsservices/runner/src/environment/abortable-sandbox-provider.tsservices/runner/src/environment/acquire-abort.tsservices/runner/src/environment/harness-session-lifecycle.tsservices/runner/src/environment/mount-lifecycle.tsservices/runner/src/lifecycle/session-coordinator.tsservices/runner/src/protocol.tsservices/runner/src/server.tsservices/runner/src/sessions/alive.tsservices/runner/src/sessions/stop-signal.tsservices/runner/src/tracing/otel.tsservices/runner/tests/unit/acquire-abort.test.tsservices/runner/tests/unit/cancel-continuity.test.tsservices/runner/tests/unit/continuation.test.tsservices/runner/tests/unit/credential-preflight.test.tsservices/runner/tests/unit/environment-units.test.tsservices/runner/tests/unit/harness-cancel-park.test.tsservices/runner/tests/unit/reap-exec.test.tsservices/runner/tests/unit/sandbox-agent-mount.test.tsservices/runner/tests/unit/sandbox-agent-orchestration.test.tsservices/runner/tests/unit/sandbox-lifecycle.test.tsservices/runner/tests/unit/server.test.tsservices/runner/tests/unit/session-ownership-release.test.tsservices/runner/tests/unit/session-pool.test.tsservices/runner/tests/unit/teardown.test.tsweb/packages/agenta-chat/tests/unit/assets/transcriptToMessages.test.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
mmabrouk
added a commit
that referenced
this pull request
Sep 4, 2026
mmabrouk
force-pushed
the
spike/session-cancel-warm
branch
from
September 4, 2026 16:39
fa8c9f5 to
66febb1
Compare
Contributor
📘 Docs preview
This comment updates in place on every push. |
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
mmabrouk
force-pushed
the
spike/session-cancel-warm
branch
from
September 4, 2026 16:42
66febb1 to
5d40633
Compare
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
…parked sandbox A Stop now parks the sandbox instead of deleting it, and the delete is what used to kill whatever the turn had started. Measured on the integration stack, local sandbox provider: Pi and Claude Code kill their shell child inside 0.2 s of the cancel, and Codex leaves it running until the park window closes. Codex differs because its shell is not the ACP adapter's child. The adapter is a JavaScript bridge over a Rust `codex app-server` subprocess, the shell child is a direct child of that Rust process, and the bridge's cancel only sends the `turn/interrupt` request. The interrupt works: the prompt settles cancelled in about 48 ms. The Rust core simply abandons the exec, and that core is a stripped vendored binary we pin rather than build. Reap it from the runner through the sandbox daemon's one-off process API, so the fix ships in the runner image alone and behaves the same on the local and the Daytona provider. A patch to the bridge would have to do the same /proc walk in a bundle installed into the sandbox image, and would ship only through a Daytona snapshot rebuild. Two rules keep the reap off anything a warm session needs: only descendants of the `codex app-server` process are candidates, and only those younger than the turn that was stopped. An stdio MCP server starts with the session, before the prompt, so it is never selected. The app-server itself is never a candidate, so the native harness session survives exactly as before. The reap is best effort and cannot change the park decision. A sandbox that would have been parked is still parked when the reap cannot run, because trading a warm session away for a tidier process table is the wrong trade. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
…d children Every rounding error in the reap must make it kill less, not more. On a COLD first turn Codex clones its plugin repository about a second before the prompt is issued, so a ceiling round put a `git fetch` the SESSION owns one second inside the window meant for the turn's own exec. A child born in the first second of a turn is not physically possible, because the model has to emit a tool call first, so flooring costs nothing and closes the overlap. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
…ed turn writes A user Stop kept the sandbox warm, but only in this process. `hydrateHarnessSessionFromDurable` re-seeds the continuity store from the durable turn ledger only when the latest `session_turns` row carries both `agent_session_id` and `end_time`, and a cancelled turn wrote neither: it took the `invalidateContinuity` branch and never called `complete`. So the row stayed open forever, the hydration branch written for exactly this case never fired, and the first runner restart after a Stop cost the session its native harness session. The next message rebuilt cold and the conversation survived only as the client's replayed transcript. The rule is now the harness's own confirmation, not the park decision. A settled cancel is the same proof that earns the warm park in `shouldPark`: the harness answered the cancelled prompt, so it is idle and its native transcript holds a short but finished turn. That is a faithful resume point, so it advances the in-memory pointer and completes the ledger row on the same path a completed turn uses. A pause, and a cancel the harness never confirmed, still drop the record and fall back to cold replay. An unlabelled abort whose cancel settled now writes the record too, even though the sandbox is deleted. The native session lives on the durable cwd, so the next turn can load it into a fresh sandbox, which is the whole point of the durable mirror. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
… 120 s lease `owner:session:<id>` is claimed by every heartbeat and was released by nothing, and the API's `claim_owner` deliberately never steals from a live owner. So a runner that exited while holding claims locked each of those sessions out of its own replacement for the rest of OWNER_TTL_SECONDS. On the local sandbox provider that is a two-minute outage after every restart: the new replica refuses with "replica X is not the owner of session Y ... Refusing to cold-start on the wrong host", measured at 112 to 123 s against the 120 s lease. The API gains one optional field on the beat it already serves, `release_owner`. It runs first, before the superseded check and before any lock is read or written, and does exactly one thing: `clear_owner`, which is release-if-owner, so a beat from a replica that no longer holds the session is a no-op and can never take affinity from a live one. No turn lock and no stream row is touched, because a departing runner asserts no liveness and no turn. The runner learns which sessions it owns from the beats it already sends: every beat the API answers with this replica's own id records the session and the credential that spoke for it, and a beat this replica lost records nothing. The SIGTERM handler then hands each claim back, after the sandboxes are destroyed so a session whose sandbox is still going does not yet look free, and bounded so it can never hold the process past the grace period. A SIGKILL reaches no handler, so the 120-second lease stays the fallback for that case and for an unreachable API. Nothing else changes: `release_owner` defaults to false, and an ordinary beat still claims. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Anchor app-server discovery to the sandbox-agent server whose exact port matches the current sandbox, so concurrent warm local sessions cannot make the reap ambiguous. Preserve the existing descendant, turn-age, and maximum-candidate safety gates. Claude-Session: https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Propagate mount cancellation, clean mounts that finish late, and treat a non-zero kill exit as a failed Codex reap. Prove the leaked Codex shell child is gone before the warm sandbox parks. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Capture the persist event count before session/load and inspect only events emitted by that attempt. Older session/update records can no longer validate an empty native replay. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Detect both smart and legacy truncation markers before reconstructing conversation history. A partial durable record now fails closed instead of becoming model context. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Record long polling as the selected milestone transport, give each open gate a unique ID, and document the current Codex reap and continuity behavior. Make truncation refusal and pair-level release verification explicit. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Keep the reconstruction invariant in one concise comment while the assertions carry the behavioral detail. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Compose the newer credential differential preflight with the acquire signal around both the sandbox probe and polling delay. Stop now exits promptly without dropping the release branch probe behavior. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Reject only legacy whole-body truncation records that lack an event type. Keep structure-preserving smart-truncated tool results replayable and cover both record shapes in reconstruction tests. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Keep the normal Stop assertion scoped to done events now that admitted session runs also emit a turn correlation event. Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
mmabrouk
force-pushed
the
spike/session-cancel-warm
branch
from
September 4, 2026 17:01
24fa55a to
bbae565
Compare
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.
Agent-generated, low weight. Draft. Do not merge before Mahmoud reviews it.
A user Stop now sends the real harness cancel, waits for the harness to answer, ends the turn as
cancelled(kept in the terminaldonerecord), and parks the sandbox instead of destroying it. The vendored sandbox-agent client is patched (eight lines,cancelSession) to allowsession/cancel; the daemon inside the sandbox needs no change, so Daytona needs no new snapshot. A stopped session parks under its own window,AGENTA_RUNNER_SESSION_STOPPED_TTL_MS, which defaults to the ordinary idle window. On a Stop the runner now also reaps the shell child a Codex turn leaves behind, through the sandbox process API, on both providers. A stopped turn now writes the same continuity record a completed turn writes, so a runner restart hydrates the native harness session from the durable mount. At shutdown the runner releases its Redis owner claims through arelease_ownerfield on the heartbeat, so a restarted runner is admitted at once instead of after the 120 s owner lease.Evidence
11af505e3esleeptool, then continue11af505e3e11af505e3e9110c08000(integration)sleep 300, inspect the parked sandbox9110c080009110c080009e21fba4ee(this branch, port 8980)sleep 300with the reapsandbox_start, new shell command ran9110c08000(integration, PR #6506)| 2026-09-03 | local | Pi |
9110c08000(integration) | Stop,docker restartthe runner, continue | sandbox and harness identity lost, conversation survived only through the client transcript, refused 90 to 150 s | admitted at 123.6 s || 2026-09-03 | local | Pi |
0b8fe254b1(continuity record) | same, server-side reconstruction off, empty client transcript |hydrated ... loaded=true, mode=load, same native session, codeword recalled; sandbox id new on the local provider by design | refused 112 s (owner lease, next row) || 2026-09-03 | local | Pi |
5a10e6b100(owner release) | Stop, hold 30 s, SIGTERM restart, continue | two claims released 154 ms after the signal, admitted on the first attempt, native session loaded | 19.6 s after the restart || 2026-09-03 | local | Pi |
5a10e6b100| same with SIGKILL (control) | no handler runs, refused five times until the lease expired | admitted at 67.5 s |Not verified: Daytona for any harness on this branch (the Daytona rows are on PR #6505 and #6506 when they land), and the reap's
pscall against the Daytona snapshot image.Cause of the Codex leak: the Codex adapter is a thin bridge over a Rust
codex app-serversubprocess whose cancel handler (codex-acp/dist/index.js:30089) only sendsturn/interrupt; the shell child is a child of the Rust process and nobody kills it. Pi kills a detached process group (pi-coding-agent/dist/utils/shell.js:189); Claude Code kills inside its binary.Tests
cd services/runner && pnpm run build:extension && pnpm test: 162 files, 2687 tests pass (head5a10e6b100).pnpm exec vitest run --project unit tests/unit/cancel-continuity.test.ts: 6 pass;tests/unit/session-ownership-release.test.ts: part of the 14 new runner tests.cd api && pytest oss/tests/pytest/unit/sessions/ -q: 491 pass, 41 skipped on a bare host (7 new for the release branch).cd services/runner && pnpm vitest run tests/unit/reap-exec.test.ts: 18 pass.cd services/runner && pnpm typecheck: clean.Branch head
5a10e6b100onspike/session-cancel-warm. Baseagent/session-execution-rfc. Reports:docs/design/session-control-and-live-events/spike-a-sandbox-cancel.mdon this branch, andevidence-2026-09-03/child-process-cleanup.mdpluscancel-continuity.mdon PR #6505.https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV
Reap fix (2026-09-04)
On the local provider every sandbox is a child of the runner in one process table. The Codex reap disabled itself when it saw more than one
codex app-server, so a stopped Codex shell child survived the park window whenever two sessions ran at once. Commitcce2b21bc3scopes the reap to the sandbox's own daemon by an exact--portmatch, keeping the descendant and turn-age checks. Daytona was never affected (one daemon per machine). Tests: reap-exec 22, runner 2,691.Early-stop endings (2026-09-04)
Five Stops within a second showed that a Stop which lands while the sandbox is still being acquired never starts the turn, and nobody wrote the turn's ending; the execution settled with zero records. The runner now persists the stopped ending itself when the run returns without a terminal record (
9bbbf4fb39), and a genuine acquire failure keeps its error before the done instead of reading as a clean finish (44630051a5). Reviewed by the agent that found the defect. Runner tests: 2,694.Continuity and acquire preemption (2026-09-04, afternoon)
The browser pass found that a local session evicted between turns forgot its history: the rebuild reported an empty native session as loaded and the runner then sent only the last message. Three commits:
4bcb5f5a34takes last-message-only only when the native load is verified by observed session/load events and otherwise replays the reconstructed transcript;570fb79aabtracks whether the native history is durable (Daytona always, local once the cwd mount succeeded) and gates verification on it;fa8c9f5cfclets a Stop that lands during a slow sandbox acquire abort the acquire and report within the delivery timeout, destroying a late create and pausing a late reconnect. Reviewed by the agent that found the defect; runner 2,734 tests. Non-blocking follow-up: the Daytona-only~/.pi/agent/sessionsmount is dead weight now that Pi's transcript lives under the durable cwd on both providers.