fix(coding-agent): detach hard-aborted refs so ensureLive can't route into a dead session

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
This commit is contained in:
roboomp
2026-08-01 09:42:41 +00:00
parent d350dd8f84
commit 57732e9dcd
4 changed files with 32 additions and 18 deletions
@@ -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://<id>).
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;
}
+9 -5
View File
@@ -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);
};
+8 -1
View File
@@ -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;
}
@@ -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