Fix retained sub-agent session leaks and close_agent false success

CHANGELOG.md

@@ -35,9 +35,18 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename
   so a wedged descendant cannot hang the call) and `resume_agent(id)` to
   reopen a retained, completed session. Sessions now carry an explicit
   lifecycle status (`pending_init | running | interrupted | completed |
-  shutdown | not_found`) alongside the existing display status; a retained
-  session is exempt from the finished-session display cap until it is
-  actually closed.
+  shutdown | not_found`) alongside the existing display status.
+- Fixed four resource-leak / false-success bugs in the retained-session
+  lifecycle above: a retained session is now released (its close handle
+  invoked, its reactor and LSP sidecars torn down) once it falls out of the
+  same finished-session cap every other session already used, instead of
+  being exempt from any bound; `cancelAll` and session teardown (`/clear`,
+  closing a session) now release every still-open retained session, not
+  only ones still mid-turn; `close_agent` called while a worker's agent is
+  still being constructed now waits for it (bounded by the same close
+  deadline) instead of reporting a false "shutdown" over a session nothing
+  can ever release again; and a session salvaged by a deadline or a cancel
+  no longer reports as resumable once its agent has actually been disposed.
 
 ## [0.2.109] - 2026-08-24
 

src/subagent/agent-fleet.test.ts

@@ -196,12 +196,14 @@ describe("spawn_agent + wait_agents", () => {
     // DEFAULT_MAX_COMPLETED on SubAgentSessionStore is 20 finished sessions;
     // spawn (and complete) enough workers to blow well past it before any of
     // them is collected, proving fleetRecords does not depend on the store's
-    // cap either. CL-6943: a spawn_agent session is now retained (exempt
-    // from the cap) until close_agent runs, so — unlike the pre-CL-6943
-    // version of this test — the store also keeps every one of them; that
-    // is covered by session-store.test.ts's own cap tests.
+    // cap. CL-7001: a retained session is bounded by this same cap too now
+    // (it used to be exempt with no separate cap or TTL, which is exactly
+    // why every spawn_agent worker leaked by default) — so unlike the
+    // pre-CL-7001 version of this test, the store itself may have already
+    // evicted (and released) the earliest ones; wait_agents/fleetRecords is
+    // the durable source of truth this test actually cares about.
     const COUNT = 25;
-    const deps = makeDeps(async () => ({ report: "irrelevant" }));
+    const deps = makeDeps(async () => ({ report: "irrelevant", agentRetained: true }));
     const spawn = createSpawnAgentTool(deps);
     const wait = createWaitAgentsTool({ sessions: deps.sessions, fleetRecords: deps.fleetRecords });
 
@@ -218,8 +220,10 @@ describe("spawn_agent + wait_agents", () => {
     // Let every spawn's run() resolve and complete() land before collecting.
     await new Promise((resolve) => setTimeout(resolve, 20));
 
-    // Retained sessions are exempt from the display cap.
-    expect(deps.sessions.get(ids[0]!)).toBeDefined();
+    // The store's own bound may have already evicted (and released) the
+    // earliest session — fleetRecords below is what wait_agents actually
+    // depends on, and it is never subject to this cap.
+    expect(deps.sessions.get(ids[0]!)).toBeUndefined();
 
     // Every single one is retrievable through wait_agents too.
     const waited = await callTool(wait, { targets: ids, timeout_ms: 5000 });

src/subagent/agent-fleet.ts

@@ -485,7 +485,14 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool {
         .then((result) => {
           if (childCtl.signal.aborted) return;
           deps.fleetRecords.resolve(session.id, result.report);
-          deps.sessions.complete(session.id, result.report);
+          // CL-7001: result.agentRetained is only true on run.ts's clean-
+          // completion path when persist actually skipped teardown — a
+          // deadline/cancel salvage resolves through the same promise but
+          // always disposed its agent first, so the store must not treat it
+          // as resumable just because retained:true was requested at spawn.
+          deps.sessions.complete(session.id, result.report, {
+            agentRetained: result.agentRetained === true,
+          });
         })
         .catch((err) => {
           if (childCtl.signal.aborted) return;

src/subagent/retain-salvage.test.ts

@@ -0,0 +1,99 @@
+import { describe, expect, test } from "bun:test";
+import { createSubAgentSessionStore } from "./session-store.js";
+
+describe("retained session lifecycle", () => {
+  test("a salvaged (deadline/cancel) run lands resumable even though run.ts disposed its agent", () => {
+    const store = createSubAgentSessionStore({ maxCompleted: 5 });
+    const s = store.start({ description: "worker", agentId: "build", brief: "b", retained: true });
+    store.markRunning(s.id);
+    // run.ts salvage path RETURNS a report (does not throw) with stopReason
+    // "deadline", but leaves turnSucceeded=false so finally disposes the
+    // agent. agent-fleet's .then() still routes it to complete() — passing
+    // agentRetained:false, exactly as its real call site does whenever
+    // result.agentRetained isn't true.
+    store.complete(s.id, "Stopped: deadline\n\nPartial work...", { agentRetained: false });
+    const after = store.get(s.id);
+    console.log("lifecycleStatus:", after?.lifecycleStatus, "retained:", after?.retained);
+    const outcome = store.resumeOne(s.id);
+    console.log("resumeOne outcome:", JSON.stringify(outcome));
+    expect(outcome.ok).toBe(false);
+  });
+
+  test("cancelAll does not close retained completed sessions", () => {
+    const store = createSubAgentSessionStore({ maxCompleted: 5 });
+    const s = store.start({ description: "worker", agentId: "build", brief: "b", retained: true });
+    let closed = false;
+    store.registerClose(s.id, async () => {
+      closed = true;
+    });
+    store.complete(s.id, "done");
+    const cancelled = store.cancelAll("parent stop");
+    console.log("cancelAll returned:", cancelled, "| close invoked:", closed);
+    expect(closed).toBe(true);
+  });
+
+  test("retained completed sessions are exempt from the display cap without bound", () => {
+    const store = createSubAgentSessionStore({ maxCompleted: 3 });
+    for (let i = 0; i < 50; i++) {
+      const s = store.start({ description: `w${i}`, agentId: "build", brief: "b", retained: true });
+      store.complete(s.id, "done");
+    }
+    console.log("sessions retained despite maxCompleted=3:", store.list().length);
+    expect(store.list().length).toBeLessThanOrEqual(3);
+  });
+
+  test("a genuinely retained clean completion IS resumable, and cancelAll releases it", () => {
+    const store = createSubAgentSessionStore({ maxCompleted: 5 });
+    const s = store.start({ description: "worker", agentId: "build", brief: "b", retained: true });
+    store.markRunning(s.id);
+    let closed = false;
+    store.registerClose(s.id, async () => {
+      closed = true;
+    });
+    // Mirrors agent-fleet's real call: only a clean turnSucceeded completion
+    // sets agentRetained.
+    store.complete(s.id, "done", { agentRetained: true });
+    expect(store.resumeOne(s.id).ok).toBe(true);
+    expect(closed).toBe(false);
+    store.cancelAll("parent stop");
+    expect(closed).toBe(true);
+  });
+
+  test("clear() releases every retained session's close handle instead of dropping it silently", () => {
+    const store = createSubAgentSessionStore({ maxCompleted: 5 });
+    const s = store.start({ description: "worker", agentId: "build", brief: "b", retained: true });
+    store.markRunning(s.id);
+    let closed = false;
+    store.registerClose(s.id, async () => {
+      closed = true;
+    });
+    store.complete(s.id, "done", { agentRetained: true });
+    store.clear();
+    expect(closed).toBe(true);
+  });
+
+  test("close_agent during the setup window waits for the handle instead of falsely reporting shutdown", async () => {
+    const store = createSubAgentSessionStore({ maxCompleted: 5 });
+    const s = store.start({ description: "worker", agentId: "build", brief: "b", retained: true });
+    // No registerClose yet — closeOne races the agent-setup window.
+    const closePromise = store.closeOne(s.id, 200);
+    let registeredClose = false;
+    setTimeout(() => {
+      store.registerClose(s.id, async () => {
+        registeredClose = true;
+      });
+    }, 20);
+    const status = await closePromise;
+    expect(status).toBe("shutdown");
+    expect(registeredClose).toBe(true);
+  });
+
+  test("close_agent gives up honestly (not a false shutdown) if the handle never arrives in time", async () => {
+    const store = createSubAgentSessionStore({ maxCompleted: 5 });
+    const s = store.start({ description: "worker", agentId: "build", brief: "b", retained: true });
+    store.markRunning(s.id);
+    const status = await store.closeOne(s.id, 30);
+    expect(status).not.toBe("shutdown");
+    expect(store.get(s.id)).toBeDefined();
+  });
+});

src/subagent/run.ts

@@ -197,7 +197,14 @@ export interface SubAgentRunController {
   deadlineHit: () => boolean;
   /** Abort the run from inside, distinct from parent cancel and deadline. */
   abort: (reason: Error) => void;
-  dispose: () => void;
+  // CL-7001: normally tears down the timer and the parent-abort forwarding
+  // listener. Pass keepParentListener:true for a run that is persisting
+  // (retained, clean completion) — otherwise a later parent abort (operator
+  // cancel/close reaching this run's own params.signal) would stop
+  // propagating into runController.signal, and closeOnAbort — which is
+  // registered on runController.signal, not the parent's — would never fire
+  // for the still-open session.
+  dispose: (opts?: { keepParentListener?: boolean }) => void;
 }
 
 /**
@@ -238,9 +245,11 @@ export function createSubAgentRunController(
     abort: (reason: Error): void => {
       if (!controller.signal.aborted) controller.abort(reason);
     },
-    dispose: (): void => {
+    dispose: (opts?: { keepParentListener?: boolean }): void => {
       if (timer !== undefined) clearTimeout(timer);
-      parentSignal?.removeEventListener("abort", onParentAbort);
+      if (opts?.keepParentListener !== true) {
+        parentSignal?.removeEventListener("abort", onParentAbort);
+      }
     },
   };
 }
@@ -812,6 +821,11 @@ export async function runSubAgent(params: RunSubAgentParams): Promise<RunSubAgen
           teardown,
           new Promise<void>((resolve) => setTimeout(resolve, deadlineMs)),
         ]);
+        // CL-7001: the run's finally block kept the parent-abort forwarding
+        // listener alive for a persisted session (see runController.dispose's
+        // doc); now that this session is actually closing, tear it down for
+        // real so the listener does not outlive the session.
+        runController.dispose();
       };
       params.onAgentReady(boundedClose);
     }
@@ -867,6 +881,10 @@ export async function runSubAgent(params: RunSubAgentParams): Promise<RunSubAgen
       return {
         report: appendActivitySummary(report, toolNamesUsed),
         ...(directorForcedStopReason !== undefined ? { stopReason: directorForcedStopReason } : {}),
+        // CL-7001: only this path skips teardown below when persist is set —
+        // tell the caller so a salvage below is never mistaken for a still-
+        // live, resumable agent.
+        ...(params.persist === true ? { agentRetained: true } : {}),
       };
     } catch (err) {
       if (isSubAgentCancelError(err, runController.signal)) {
@@ -913,12 +931,17 @@ export async function runSubAgent(params: RunSubAgentParams): Promise<RunSubAgen
     }
   } finally {
     if (stallWatchdog !== undefined) clearInterval(stallWatchdog);
-    runController.dispose();
+    const persisting = params.persist === true && turnSucceeded;
+    // CL-7001: a persisting run must keep the parent-signal forwarding alive
+    // (see createSubAgentRunController's dispose doc) — boundedClose (the
+    // close_agent handle) fully disposes the runController itself once the
+    // session actually tears down.
+    runController.dispose({ keepParentListener: persisting });
     // CL-6943: a persisted, cleanly-completed session skips teardown here —
     // it stays open until close_agent (or a later failed/aborted run) tears
     // it down. Everything else (no persist, a thrown error, an
     // aborted/salvaged run) disposes exactly as before.
-    if (!(params.persist === true && turnSucceeded)) {
+    if (!persisting) {
       await disposeSubAgentSession({
         signal: runController.signal,
         ...(closeOnAbort !== undefined ? { closeOnAbort } : {}),

src/subagent/session-store.test.ts

@@ -350,28 +350,40 @@ describe("CL-6943 reusable worker sessions", () => {
   test("resume_agent fails on a session close_agent already shut down (close is permanent)", async () => {
     const store = createSubAgentSessionStore();
     const session = store.start({ description: "d", agentId: "a", brief: "b", retained: true });
+    // registerClose always fires in production before onAgentReady's window
+    // closes (CL-7001) — closeOne otherwise waits for it up to the deadline.
+    store.registerClose(session.id, async () => {});
     store.complete(session.id, "## Summary\nDone.");
     await store.closeOne(session.id, 1000);
     expect(store.resumeOne(session.id)).toEqual({ ok: false, status: "shutdown" });
   });
 
-  test("pruneCompleted does not evict a retained, still-open session past maxCompleted", () => {
+  // CL-7001: a retained, still-open session used to be exempt from this cap
+  // entirely — no separate cap or TTL — which is exactly why every
+  // spawn_agent worker leaked by default. maxCompleted is now the one bound
+  // the store owns for every finished session, retained or not, and
+  // eviction releases the session's close handle instead of abandoning it.
+  test("pruneCompleted evicts a retained, still-open session past maxCompleted and releases it", () => {
     const store = createSubAgentSessionStore({ maxCompleted: 1 });
     const retained = store.start({
       description: "keep-me",
       agentId: "a",
       brief: "b",
       retained: true,
     });
+    let closed = false;
+    store.registerClose(retained.id, async () => {
+      closed = true;
+    });
     store.complete(retained.id, "## Summary\nDone.");
 
     for (let i = 0; i < 3; i++) {
       const s = store.start({ description: `fill-${i}`, agentId: "a", brief: "b" });
       store.complete(s.id, "## Summary\nDone.");
     }
 
-    expect(store.get(retained.id)).toBeDefined();
-    expect(store.get(retained.id)?.lifecycleStatus).toBe("completed");
+    expect(store.get(retained.id)).toBeUndefined();
+    expect(closed).toBe(true);
   });
 
   test("once closed, a retained session becomes a normal finished record subject to the cap", async () => {
@@ -382,6 +394,7 @@ describe("CL-6943 reusable worker sessions", () => {
       brief: "b",
       retained: true,
     });
+    store.registerClose(retained.id, async () => {});
     store.complete(retained.id, "## Summary\nDone.");
     await store.closeOne(retained.id, 1000);