From 57732e9dcde35a7f0f3f693e33f45e663cf3907f Mon Sep 17 00:00:00 2001 From: roboomp Date: Sat, 1 Aug 2026 09:42:41 +0000 Subject: [PATCH] fix(coding-agent): detach hard-aborted refs so ensureLive can't route into a dead session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Preserving aborted refs on dispose exposed a latent invariant break: the executor's hard-abort path (finalizeSubagentLifecycle) set status `aborted` and disposed the session without detaching it. With the ref now retained, it kept a dangling pointer to the disposed session, and ensureLive returns any non-null ref.session before its revivability check — so hub focus / transcript chat could route into a dead session. - finalizeSubagentLifecycle: detach the session before disposing on the terminal hard-abort path, upholding the AgentRef invariant (session === null when aborted). - release(tombstone): detach before dispose too (capture the live session first), same invariant. - unregisterUnlessParked: preserve `aborted` refs only when already detached; an aborted ref still holding a live session is a bug and is unregistered rather than kept reachable. - Regression test now asserts ensureLive rejects a tombstoned id as terminal. Fixes #7250 --- .../src/registry/agent-lifecycle.ts | 24 +++++++++---------- packages/coding-agent/src/sdk.ts | 14 +++++++---- packages/coding-agent/src/task/executor.ts | 9 ++++++- .../test/registry/agent-lifecycle.test.ts | 3 +++ 4 files changed, 32 insertions(+), 18 deletions(-) diff --git a/packages/coding-agent/src/registry/agent-lifecycle.ts b/packages/coding-agent/src/registry/agent-lifecycle.ts index 5ad7ac1a4..7fb86946a 100644 --- a/packages/coding-agent/src/registry/agent-lifecycle.ts +++ b/packages/coding-agent/src/registry/agent-lifecycle.ts @@ -379,25 +379,25 @@ export class AgentLifecycleManager { } if (options?.tombstone) { - // Mark the tombstone terminal BEFORE disposing. A live session's wrapped - // dispose (createAgentSession) unregisters any non-terminal ref via - // `unregisterUnlessParked`, which would otherwise delete the ref out from - // under the detach/status calls below. Setting `aborted` first makes that - // guard preserve the ref, so a later persisted-subagent rescan skips it. + // Explicit kill: mark the ref terminal `aborted` and detach the session + // BEFORE disposing. aborted refs must satisfy the AgentRef invariant + // (session === null) so ensureLive / hub focus can never route into a + // disposed session; setting the terminal status first also makes + // createAgentSession's dispose wrapper (unregisterUnlessParked) preserve + // the ref instead of removing it, so a later persisted-subagent rescan + // skips it. The transcript file is left intact (history://). this.#registry.setStatus(id, "aborted", ref); } - if (this.#registry.get(id) === ref && ref.session) { + const live = this.#registry.get(id) === ref ? ref.session : null; + if (options?.tombstone) this.#registry.detachSession(id, ref); + if (live) { try { - await ref.session.dispose(); + await live.dispose(); } catch (error) { logger.warn("AgentLifecycleManager.release: session dispose failed", { id, error: String(error) }); } } - if (options?.tombstone) { - this.#registry.detachSession(id, ref); - } else { - this.#registry.unregister(id, ref); - } + if (!options?.tombstone) this.#registry.unregister(id, ref); return true; } diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index 263fec4b0..f37aa9853 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -1633,15 +1633,19 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} const agentKind = (options.taskDepth ?? 0) > 0 || options.parentTaskPrefix ? ("sub" as const) : ("main" as const); let registeredAgentRef: AgentRef | undefined; /** - * Forget the agent ref on teardown — unless the agent is being parked (or is - * already parked/aborted). Parking disposes the session but keeps the ref - * addressable (history://, revive); a hard kill leaves it as a terminal - * `aborted` tombstone. Only process teardown / a plain release unregisters. + * Forget the agent ref on teardown — unless it is a retained terminal ref. + * Parking disposes the session but keeps the ref addressable (history://, + * revive); a hard kill leaves it as a terminal `aborted` tombstone. Both are + * detached (session === null) by the time dispose runs, per the AgentRef + * invariant, so preserving them never keeps a disposed session reachable — an + * aborted ref that still holds a live session is a bug and is unregistered + * rather than handed to ensureLive. Only process teardown / a plain release + * unregisters. */ const unregisterUnlessParked = (): void => { const ref = registeredAgentRef; if (!ref || agentRegistry.get(resolvedAgentId) !== ref) return; - if (ref.status === "parked" || ref.status === "aborted") return; + if (ref.status === "parked" || (ref.status === "aborted" && !ref.session)) return; if (AgentLifecycleManager.global().isParking(resolvedAgentId, ref)) return; agentRegistry.unregister(resolvedAgentId, ref); }; diff --git a/packages/coding-agent/src/task/executor.ts b/packages/coding-agent/src/task/executor.ts index 854d2489d..5538130a0 100644 --- a/packages/coding-agent/src/task/executor.ts +++ b/packages/coding-agent/src/task/executor.ts @@ -2364,7 +2364,14 @@ export async function finalizeSubagentLifecycle(args: { const resumableAbort = args.abortKind === "budget" && args.keepAlive && !args.isolated && args.reviveSession !== null; if (args.aborted && !resumableAbort) { - if (ref && ownsRef) registry.setStatus(args.id, "aborted", ref); + if (ref && ownsRef) { + // Terminal hard kill: mark `aborted` and detach the session before + // disposing so the ref satisfies the AgentRef invariant (session null + // when aborted) — ensureLive/hub focus must treat it as terminal, never + // route into the disposed session. + registry.setStatus(args.id, "aborted", ref); + registry.detachSession(args.id, ref); + } await disposeSession(); return; } diff --git a/packages/coding-agent/test/registry/agent-lifecycle.test.ts b/packages/coding-agent/test/registry/agent-lifecycle.test.ts index 3f85d2fe4..b9923aa02 100644 --- a/packages/coding-agent/test/registry/agent-lifecycle.test.ts +++ b/packages/coding-agent/test/registry/agent-lifecycle.test.ts @@ -553,6 +553,9 @@ describe("AgentLifecycleManager", () => { expect(disposeCalls).toBe(1); expect(registry.get(workerId)?.status).toBe("aborted"); expect(registry.get(workerId)?.session).toBeNull(); + // The tombstone is terminal: ensureLive must not hand back the disposed + // session (the ref carries session === null), it treats it as unrevivable. + await expect(lifecycle.ensureLive(workerId)).rejects.toThrow(/aborted/); // Reopening the Agent Hub rescans on-disk transcripts. The surviving // `.jsonl` must not be re-adopted as a fresh `parked` row, because the