fix(runtime-host): detach sandbox graph wake reconciliation - #4626
fix(runtime-host): detach sandbox graph wake reconciliation#4626testikun wants to merge 1 commit into
Conversation
Generated-by: OpenAI Codex
476377e to
b659ff4
Compare
jackwener
left a comment
There was a problem hiding this comment.
Approved at exact head b659ff4e0ae2d21397c17d630db6950154b62c2e. I found no blocking or non-blocking issues.
The previous path held Session admission while notifyPermissionResponse() waited for the activity lease owned by the wake-started turn that was itself parked on the answered sandbox boundary. The new detached callback lets the durable answer and captured continuation complete, releases Session admission, and still routes callback failures into the Host fail-stop path.
I rebuilt the affected Runtime and Runtime Host packages and passed 64 interaction, sandbox-boundary, graph-wake, and restart-recovery tests. Restoring the previous awaited callback made the new regression fail deterministically because the interaction answer remained unsettled. A synthetic merge with current main was clean; that merge result also built and passed the same 64 tests. The exact-head hosted test check is complete and successful.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
The deadlock is real and I traced the full cycle rather than taking it from the description: #answer holds the session admission lease for the whole of #answerSandboxBoundary (interaction-coordinator.ts:735), SessionAdmissionGate serializes per session, and the awaited reconciliation reaches #settlePermissionResponse, whose first act is activityRegistry.acquire(rootSessionId) (agent-graph-supervisor-wake.ts:721), which waits for the session to go idle and is non-reentrant. The parked turn cannot reach a terminal state without re-entering admission, and admission is held by the answer. Closed loop.
Detaching is the right shape, and worth saying why: notifyPermissionResponse is already a fire-and-forget contract, it returns Promise<void> | undefined, tracks its own work in #runTracked and converts its own errors through onError. Awaiting it on the answer path was the anomaly. So this restores the intended shape rather than routing around something deeper, and it adds no new state or authority.
On whether this PR is needed: the fault is provable in code, but #3328 does not establish that users hit it. That report is a vague "long tasks keep erroring" on 0.1.11 with one request_sandbox_boundary sitting at 4m54s, and no lease-contention evidence. The PR body is honest about calling it the first durable fault behind that issue rather than the confirmed cause. I would still fix it, a provable deadlock on a normal path does not need a witness. But Fixes #3328 would be overclaiming.
One thing to fix before merge.
[P2] The detach also moved two throwing reads out of the lease, and they land on #poison
The new call is
void Promise.resolve()
.then(() => this.#onSandboxBoundarySettled(request.sessionId))
.catch((error: unknown) => { this.#poison(error); });but #onSandboxBoundarySettled is notifySandboxBoundaryGraphWake (execution-composition.ts:673), which does two reads before it notifies, and both can throw:
sandbox-boundary-graph-wake.ts:49 calls readHeaderSnapshot, which throws SessionNotFoundError once the session is gone, with no isSessionNotFoundError guard here (compare execution-composition.ts:1284, which has one). And :33-38 throws Graph operator Session ... does not match root Session ... when listGraphIds falls back to agentGraphIdForRootSession after the root's epoch rows are cleared.
Before this change both reads sat inside the answer's lease, and retirement runs through this.#admission.runMany(...) (session-retirement-coordinator.ts:441), so they could not interleave. After it, they can.
Where it lands matters: .catch goes to #poison, and composition's onPoison is retainUntilProcessExit(); beginDrain(); requestDrain();. So a graph subagent answering a boundary and then having its root retired shortly after takes the whole Host's interaction authority into fail-stop. The window is narrow and wakes are durable so recover() converges on restart, which is why this is P2 and not P1, but the failure mode is Host-wide.
The smaller fix is also the smaller diff: resolve the lineage inside the lease and detach only notifyPermissionResponse(rootSessionId), by splitting onSandboxBoundarySettled into resolve and notify. That keeps the old serialization and the new detach at once.
Two smaller notes. A client receiving answered no longer implies the reconciliation ran in this process: notifyPermissionResponse returns early when #closed, and close() does not track this detached task, so a drain between the two leaves the wake parked until the next recover(). Worth a line in the behavior-change section, which currently says only that it no longer waits. And the new .catch → #poison branch has no test; the fake onSandboxBoundarySettled never rejects, so the escalation path above has no regression cover.
Next step
Fix the lineage-read scope, then this is good to go. The 88-line test does go through the real interaction.answer handler and fails deterministically without the change, which is the right shape.
Worth flagging on process: the only approval here is from an automated review agent whose own comment says it is not an independent human review. So this has not had a human look at it yet, and if it merges as-is someone should make an explicit call on the P2 path.
Manual acceptance: a real long Agent Graph run in Desktop, park on a request_sandbox_boundary prompt, answer it, confirm the Session is not frozen and subsequent Session operations respond immediately. Then answer a boundary on a graph operator and delete the root session right after, confirming the Host does not enter fail-stop.
Evidence boundary: static read at b659ff4e0, no build, no tests, and I did not reproduce either the deadlock or the P2 race. How wide the retirement window actually is comes from reading admission usage in session-retirement-coordinator.ts, not from walking every precondition of a removal plan, so it may be narrower than I describe, or wider if there is an automatic removal trigger I did not find.
AI-assisted review: drafted with Maka.
Summary
Fixes #3328
A long Agent Graph run could freeze a Desktop Session after the user answered a
request_sandbox_boundaryprompt. The durable answer path held Session admission while awaiting graph-wake reconciliation. A wake-started turn can still be parked on that prompt while holding the graph activity lease; reconciliation then waits for the same lease, so the answer and every later Session operation queue forever.This is the first durable fault behind the long-task freeze described in #3328 and the related reproduction in #3866. The reported React error #185 is a separate historical 0.1.11 inline-completion crash: that seam was already removed and fenced by #3292/#3755/#4208, and current main does not reintroduce it.
The fix detaches reconciliation after the durable answer is applied and routes detached failures through the existing fail-stop path. It does not change provider retry policy, sandbox authority rules, or local-model support.
Verification
@maka/runtime-hostsucceeded.git diff --checkpassed.The repository npm scripts could not be used with the system Node 20 in this environment because the current tree requires
stripTypeScriptTypesandnode:sqlite; the same checks were run with the bundled Node 24 runtime.AI use
Tool(s) and scope: OpenAI Codex analyzed the issue and related reproductions, edited the Runtime Host fix and regression test, and ran verification. A human contributor remains responsible for review, accuracy, and submission.
Checklist
Does this PR entail a change in behavior?