docs(design): session keep-alive, TTL live sessions in the runner#5153
docs(design): session keep-alive, TTL live sessions in the runner#5153mmabrouk wants to merge 13 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
mmabrouk
changed the title
|
Folded the Codex xhigh review findings into the plan (commit 7963dbd). What changed, per finding: Blockers
High Medium A single Codex xhigh confirmation pass over the amended plan is running; anything material will be folded in one more commit. Implementation of slice 1 (feat/session-keepalive-pool) starts now per the amended plan. |
|
Codex xhigh confirmation pass on the amended plan came back: 5 of the 7 folds confirmed correct outright (gate scope table, listener demux, shutdown cleanup, resume execution context, partial-acquire cleanup, pruned-array fingerprint all verified against the code). Two material corrections, now folded in commit 995fc45:
Also corrected one stale sentence in architecture-notes.md that told the implementer to keep a paused client-tool loopback call in flight, which conflicted with the v1 scope (client-tool pauses never park). That was the single confirmation loop; the plan is now frozen for implementation. Slice 1 is being built to the corrected spec. |
|
Implementation is up, per the amended plan:
Both flag-gated off by default, 704 runner tests green, flag-off byte-identical. Each slice went through a Codex xhigh review plus an independent Claude review with a fix round before commit. The live-deployment loop for slice 2 is the recorded next step (deferred by Mahmoud). |
|
|
||
| ### Turn-flow refactor | ||
| Split `runSandboxAgent` into: | ||
| - `acquireEnvironment(request)`: mount signing through `createSession` + MCP wiring (today lines 328-699). Session-scoped: sandbox handle, session handle, internal tool-MCP server (its URL is baked into sessionInit, so it must live as long as the session), relay dir, mounted cwd, workspace, skills dirs. Finalizers register incrementally as each resource is acquired; a mid-acquire failure runs the finalizers registered so far (reverse order) through the same `destroy()` the pool uses. A half-built environment cannot leak. |
There was a problem hiding this comment.
Please rewrite this part focusing on simplicity. Use the skill that you have to improve the clarity of thoughts and explaining exactly what these things do, because right now the explanation is very short and very complicated. It's not clear how it worked before and how it is working now. Really explain it well and clearly with simple words. Show the before and after, show the flow before and the flow after, and provide examples. It's a very important part. And obviously show what happens if there is a session open and if not.
There was a problem hiding this comment.
Rewritten per the style skill. The before/after flows with worked examples (live session exists vs does not) are architecture-notes.md Part 2; the mechanism is Decisions 4 and 5. Simpler, longer, with examples.
|
|
||
| On approval: `respondPermission(parkedId, "once" | "reject")` -> the original prompt promise continues -> the new turn's emitter streams the remaining events -> the tool executes with its original byte-exact arguments. This removes the argument-drift and task-restart failure classes at the root. The exact-args decision-map machinery stays untouched as the cold fallback (approval TTL expired, restart, etc.). | ||
|
|
||
| Required changes in park mode (v1: Claude ACP permission gates only; corrected in the confirmation pass): the pause controller must not fire the destroy callback (inject a park callback); the environment's `mcpAbort` is simply not fired on a park because the environment stays alive (client-tool pauses themselves NEVER park in v1, see the scope note below, so no paused loopback call is ever held across a park); do not settle the paused call with `TOOL_NOT_EXECUTED_PAUSED` (it will actually run); add `resume()` clearing `pausedToolCallIds` so post-resume `tool_call_update` frames stream again (`shouldSuppressPausedToolCallUpdate`, `sandbox_agent.ts:180`). The "a pause sends NO harness reply" contract (`acp-interactions.ts:68-72`) is unchanged; the reply just arrives later on the same session. The interactions plane needs `resolveInteraction` for the parked token on resume (hook exists at `sandbox_agent.ts:850`), ordered against `cancelStaleInteractions` (`server.ts:275`). |
There was a problem hiding this comment.
I mean, here you're focused a lot on the cloud ACL permission gate. I think whatever the solution is should include cloud and pi. Again, I really think the document does not describe the solution well. It does not describe the context, the flows, what exists yet right now, how it works right now, and how it will be changed. What are the trade-offs? What are the different solutions? Which solution is which and why, and how it would work with cloud and how it would work with pi? We need clarity. We need rewriting.
There was a problem hiding this comment.
Rewritten to cover both harnesses. The four gates are in Part 1, the parkable-vs-not reasoning and the 'what it would take to make Pi parkable' path is Decision 6, and the gate table is plan.md Q7.
| Execution context on resume (amended after review): the ORIGINAL turn's baked environment executes the tool (its signed mount credentials, resolved secrets, callback auth, harness env, sandbox); the NEW turn's context owns streaming and tracing (otel run, emitter, interaction resolve). The credential epoch bounds staleness: expired or rotated credentials evict instead of resuming. | ||
|
|
||
| ### Lifecycle and safety | ||
| - Flags: `AGENTA_RUNNER_SESSION_KEEPALIVE` (default off), `AGENTA_RUNNER_SESSION_TTL_MS` (60000), `AGENTA_RUNNER_SESSION_APPROVAL_TTL_MS` (600000), `AGENTA_RUNNER_SESSION_POOL_MAX` (~8). |
There was a problem hiding this comment.
How much memory does each kind of session take when running locally and when running on a daytona? I think this is an important design decision, especially later when we do this for daytona. A session pool of eight is too small. What happens when that session pool is used? Do we go cold?
There was a problem hiding this comment.
Answered with measured numbers in architecture-notes.md Decision 9: about 336 MB RSS per parked Claude session local, the Daytona cost in Decision 10, pool sizing, and exactly what happens when the pool is full (LRU-evict idle, new session runs cold, never blocked).
| | Continuation throws mid-turn | try/catch in runTurn | destroy; retry once cold | | ||
| | Client disconnects | session-owned runs already survive (`server.ts:237-246`) | on abort: destroy, don't park | | ||
| | Approval reply after approval-TTL | pool miss | cold replay + existing decision-map path | | ||
| | Multi-replica miss (future) | pool miss | cold replay; later route via `owner:session` affinity | |
There was a problem hiding this comment.
Why did we go with project session, by the way, and not just the session ID?
There was a problem hiding this comment.
Answered in architecture-notes.md Decision 1: the session id is caller-supplied, so a request from another project could hit a parked session it does not own; the project scope comes from the server-verified mount-sign response, and no mount means no park.
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Thank you for this first draft. Please address all the comments. I think it requires a lot of rewriting in a way that is clear, in a way that there's context provided, in a way that thinks about the reader, about providing the context to the reader, about providing the trade-offs to the reader, about providing the reasoning why a decision has been made and what were the other options that have, in addition to the updates to the text. Make sure that the answers are clear and written with that context, etc.
If the text has been changed so that the comments will not be shown any more on GitHub in the text, make sure to add new inline comments to explain, like "ok, this is the answer to this question", etc.
…eepalive plan Slice 2 scoped to Claude ACP permission gates only (per-gate-type table); session-lifetime listeners demuxing into the current-turn sink; pool-owned complete idempotent destroy() drained on shutdown; project-scoped pool key plus credential epoch; resumed approvals execute with the original turn's baked environment; incremental acquire finalizers; pruned-history fingerprint contract pinned with tests. Claude-Session: https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv
…eep-alive plan Pool-key project scope comes from the mount-sign response (no project id rides the wire); a session with no mount credentials never parks. The credential epoch hashes resolved secret values process-locally (no version identity exists on the wire), never logged or persisted. The stale park-mode note about client-tool loopback calls now matches the v1 gate scope (Claude ACP permission gates only). Claude-Session: https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv
… drift footgun Slice 1 measured on the dev box: cold 25.61 s, hit-continue 3.12 s, TTL expiry and cold fallback correct. Also records that a stale dev DB whose oss000000006 mounts migration predates the meta column makes mount signing 500, which keep-alive masks by design (no mount scope means never park); the migration fix belongs to that migration's owner, in a new migration. Claude-Session: https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv
Rewrites architecture-notes Decisions 1-3, 5, 6, 9, 10 for clarity, context, and trade-offs (project-scope home, in-place reconfig, warm/cold observability, stop-button and supersede, the backend-lock question, the listener demux, the relay-timeout and gate-parkability facts, the Daytona default-sandbox spec). Adds two follow-up designs: followups/parkable-gates (make the Pi and client-tool gates parkable) and followups/in-place-reconfiguration (change a live session's config without a respawn). Updates README and status. Claude-Session: https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv
The distributed Redis alive-lock already exists and the runner cooperates with it over the heartbeat; keep-alive's busy flag is not a reimplementation, it guards an in-process pool of live warmed sandbox handles a Redis key cannot represent. Addresses the second-review lock question. Claude-Session: https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv
…ld in the resume experiments Claude-Session: https://claude.ai/code/session_01CSTSEXSe4DDhoXCFjZpZ5W
| that the runner holds as a promise, exactly like Claude's ACP gate, and Gate 2 answers it with | ||
| no relay involved. | ||
|
|
||
| - Trade-off: this is the cleanest end state. It removes the whole "pause expressed as a missing |
There was a problem hiding this comment.
that is not accurate. Pi is extensible through extensions. We can build a permission plane similar to Claude using the hooks that Pi provides. The thing that is missing is the ACP bridge not Pi. Let's check who maintains that and what needs to be changed to allow that as a thought experiment
There was a problem hiding this comment.
You were right on both counts, and the doc is corrected. Pi has the hooks: every extension event handler gets ctx.ui.confirm (dialog-capable in RPC mode, the mode the bridge runs Pi in). And the bridge already translates that dialog into a real ACP session/request_permission and feeds the answer back to the blocked confirm (pi-acp dist/index.js:1106-1128). So the chain exists today; the earlier 'upstream work in Pi' claim was wrong. Who maintains what: Pi is Mario Zechner's; pi-acp is a separate third-party MIT adapter by Sergii Kozak (svkozak/pi-acp, 0.0.29 bundled). What is genuinely missing: payload fidelity (the bridge forwards title/message, not the gated call's id and arguments, so we either encode them in the confirm message or land a small upstream change / pnpm patch) and one unproven hop (nobody has driven ctx.ui.confirm through sandbox-agent to the runner's permission listener; the Daytona trust prompt rides the same channel). Option C is rewritten as 'route Pi approvals over the ACP permission plane the bridge already has', and the recommendation changed: spike C's hop first, ship C if the payload survives, else B.
|
|
||
| ## Gate 1: the Pi custom-tool relay gate | ||
|
|
||
| ### How it pauses today |
There was a problem hiding this comment.
Ok, I am not sure I understand why the current design does not work. IT seems to do its job well. If we are within the TTL for the alive of the session (while it's warm, so this means the relay_timeout==session_warm), then the request is paused until we get a response (same behavior as ACP_. In case the session goes COLD (And we need to restart the session) then we do the trick of continue and we need anyways to send back the tool.
What is the problem here? I am not sure I understand? Why are we saying the custom tool relay for pi does not work today with warm sessions
There was a problem hiding this comment.
Your model is right as the end state, and the doc now adopts it. What was missing is why today's code cannot do it yet, and I had that mechanism wrong; verified in code and corrected. Today, the instant an ask is recorded, the pause controller destroys the Pi session (sandbox_agent.ts:1135-1157, wired at 1272-1293); the keep-alive exemption only applies when parkedApproval was recorded, and only the Claude ACP hook records it (1321-1338). So the in-sandbox poll dies of teardown moments after the ask, never of its 60-second deadline, and relay_timeout == warm-TTL alone changes nothing. Even if the session were parked, the approval resume's only live verb is respondPermission; no code writes a response file on approval. The section is now 'Why keep-alive does not park it today' and names the three Claude-shaped pieces: the park decision, the resume verb, and the poll deadline. Option B is reframed as exactly those three deltas, which is your hold-while-warm, write-the-file-on-approval model made precise.
|
|
||
| Same reason as Gate 1, one level in. The wait is a synchronous block inside the Pi process, on | ||
| the same in-sandbox file poll. The runner holds nothing. A synchronous in-process block cannot by | ||
| itself survive a turn boundary. |
There was a problem hiding this comment.
again not sure I understand why. Why does it need to be a new turn. Why don't we just on approval update that file and then unblock pi. In case we reach timeout, then we send a new turn.
There was a problem hiding this comment.
It does not need a new turn, and that is exactly the mechanism; the doc now says it plainly. On the warm path nothing new is ever prompted into Pi: on approval the runner writes the response file (or, under Option C, resolves the held dialog), the blocked hook or execute returns, and the tool runs inside the original still-open prompt(). The 'new turn' is only the transport of the human's decision: the frontend delivers it as a new /run request, and that request's stream adopts the resumed events, the same way the Claude resume already works. A genuinely new agent turn happens only when the park is gone (timeout, session died), which is the cold path. Added as the turn-versus-prompt paragraphs in Gate 1 Option B and in Gate 2's options section.
| (`acp-fetch.ts`). Here the held socket is *Claude's* MCP client connecting into the runner. The | ||
| runner owns the server side, not Claude's client-side request timeout. If Claude's MCP client | ||
| reaps a request that produces no headers for long enough, holding the socket open survives only | ||
| until that timeout, not for an arbitrary park. Whether that timeout is long enough for the idle |
There was a problem hiding this comment.
the part I am not sure I understand here is how the cold path would look like. Basically for all these cases we need a cold path (After timeout) and a warm path. Here you describe the warm path, now if we go cold, how do we sovle this
There was a problem hiding this comment.
Added a dedicated section for this: 'The cold path: every gate, when the answer comes after the park is gone', right after Gate 3. It makes the warm/cold pairing explicit per gate in one table: what is durable at pause time (the session_interactions row plus the decision the frontend folds into the next request), what the cold resume does today (cold replay; the re-issued call is matched by the decision map; for client tools the browser-fulfilled output answers the re-issued call), what changes once session resume lands (rubric B from the experiments: full structured context, but a re-issued call, never the original id), and what the user experiences. The two rules it states: every warm park degrades to this path and nothing here needs building, and the park must never consume the durable record, so whoever answers first wins and an overnight answer always lands.
…ause, pi-acp permission plane, per-gate cold paths) Claude-Session: https://claude.ai/code/session_01CSTSEXSe4DDhoXCFjZpZ5W
d19b7a4 to
b271dea
Compare
|
The Option C spike ran against a real Pi session under the real pi-acp bridge, and it is decisive: Option C is proven and now the recommendation.
The design is updated: Option C is recommended, shipping with the envelope; the two runner work items are envelope parsing into the real gate identity and switching the in-sandbox extension to raise the dialog at the gate. Option B stays documented as the not-chosen fallback. Recorded residual: one live sandbox-agent daemon run as a confidence check (that leg is source-verified). Evidence is in the diff at docs/design/agent-workflows/projects/session-keepalive/followups/parkable-gates/spike-option-c/ (protocol, report, spike extension, ACP client, raw wire transcripts). |
|
Added the implementation-ready plan-feature workspace for Pi approval parking (the proven Option C): docs/design/agent-workflows/projects/pi-approval-parking/. It stands alone for the implementing agent: context (the invariant, goals, non-goals, the JP coordination constraint), research (the current-code mechanics with file:line anchors, the spike and experiment facts distilled), a plan with five slices (0: live daemon confidence run as the gate, 1: envelope parsing + gate classification, 2: extension switch to ctx.ui.confirm, 3: park + resume, 4: live QA), the removed/kept/added delta table, the full warm/cold behavior matrix including flag-off and Daytona rows, rollout/compat (the extension ships inside the runner per run, so no image skew), and six open questions with working defaults. Two implementation findings surfaced during planning and are in the plan: the reply-option mismatch (decisionToReply falls back to once/reject; the Pi dialog offers yes/no, fixed by kind-based option selection) and the double-gate (a dialog-allowed custom tool still hits the relay watcher's permission check; bridged by writing the consumed allow into the turn's stored decisions, keeping the relay as defense-in-depth). |
…rs, 4 must-fixes) Claude-Session: https://claude.ai/code/session_01CSTSEXSe4DDhoXCFjZpZ5W
|
Folded the Codex xhigh plan review into the pi-approval-parking workspace (architecture approved; 3 blockers and 4 must-fixes, all code-verified, now written into the plan). The big ones: the draft's reply-option mapping is deleted (it was backwards: the sandbox-agent daemon already maps once/always/reject to the dialog option by kind, and a literal yes would have selected the reject option, turning approvals into denials; slice 0's live daemon run now doubles as the check on this). Slice 1 must normalize the tool-call id to the envelope's real id at the top of handleRequest, or warm resume silently never fires and the FE approval card gets clobbered. Slice 3 must widen the ParkedApproval.gateType union AND the server-side gate-type guard together. Plus: permission metadata is recovered runner-side (never trusted from the sandbox envelope), the double-gate bridge got a concrete mechanism (a FIFO append API on the shared ConversationDecisions, consume-1-append-1, and a warm-resume seeding verification), the dialog gate is scoped to non-client tools, and a malformed envelope now fails closed (the draft's fall-through default was an unapproved-execution hole under a default-allow plan). Also corrected: flag plumbing lives in buildPiExtensionEnv (pi-assets.ts), multi-gate documents the existing harness-agnostic safe degrade instead of asserting a count, and slice 1 is reframed as dark until slice 2 turns the extension on. Provenance in the workspace status.md; three open questions retired, the live daemon run and the deny-UX product call stay open. |
…on plane removed The dialog gate shipped as the unconditional Pi behavior (Mahmoud, 2026-07-10): no AGENTA_RUNNER_PI_DIALOG_GATE / AGENTA_AGENT_PI_DIALOG_GATE pair, gateType pi-acp-permission (symmetric with claude-acp-permission), the relay permission plumbing and the planned double-gate FIFO bridge deleted, builtin-only runs start no relay. plan.md carries the final mechanism and the reduced warm/cold matrix (keep-alive is the only remaining switch); research.md marks its §1/§7 relay permission mechanics as the replaced code; open-questions #4 (flag coupling) resolved by there being no flag; status.md records slice-0 evidence, the review rounds, and the scope change. Implementation: PR #5185. Claude-Session: https://claude.ai/code/session_01CSTSEXSe4DDhoXCFjZpZ5W
[#5153] feat(runner): park Pi approval gates over the ACP permission plane
|
Closing — superseded by the cluster A session-rebuild stack (#5486 and related). |
1 participant
Every turn today cold-restarts the harness with a flattened text transcript. The agent loses its native memory between messages, and approvals must survive a full session teardown. This caused two production failures on 2026-07-07 (argument drift and a task restart after an approval click). Full analysis: docs/design/agent-workflows/projects/approval-boundary/cold-replay-failure-report.md.
What this PR contains
Design only, no code. A plan-feature workspace at docs/design/agent-workflows/projects/session-keepalive/ with six files: README, context, plan (including the Q&A section and the "could a minimal version work in an hour?" answer), status, open questions, and the code-grounded architecture notes.
The idea: when a turn ends, the runner keeps the session alive for a TTL instead of destroying it. The next message inside that window continues the same live session with only the new user text. Anything that does not match falls back to today's cold replay, so nothing can get worse than today. Flag-gated, local-only first, runner-only. No wire, SDK, or frontend change.
Slices
The approval change
Today the runner destroys the session on pause, so after the human clicks, a fresh model must re-issue the tool call and hope its regenerated arguments match the stored decision key. With keep-alive, the runner answers the still-waiting permission request on the parked session, and the original call runs with its original arguments.
Relation to session resume
This is option 2 from the failure report. The companion design (docs/design/agent-workflows/projects/harness-session-resume/plan.md, option 3) restores memory across restarts and long gaps via the harness's own session files. They compose as a three-tier fallback on the same session_id: live pool hit, then session/load, then cold replay. Recommended order: keep-alive first, then resume.
Open questions for Mahmoud
None block slice 1; each has a working default. Idle TTL (60s?), approval TTL (10min?), pool cap (8?), the history fingerprint over the pruned message array, supersede vs reject on a racing second turn, and the config fingerprint field list.
https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv