Skip to content

Commit 194ec94

Browse files
committed
Fix resource leaks and false success in retained sub-agent sessions
Retained sessions were exempt from any cap/TTL and never released on cancelAll/clear; close_agent during agent setup returned false success over an unreleasable session; a disposed salvage still looked resumable; and the parent-abort listener was torn down on the persist path, stranding a retained session's LSP sidecars, reactor, and @intx/agent lock entry. - session-store.ts: fold retained-completed sessions into the existing maxCompleted bound (no separate cap needed) and release their close handle on eviction; cancelAll and clear() now invoke closeHandles for every still-open retained session; closeOne waits (bounded) for registerClose during the setup window instead of reporting shutdown with nothing to release, and reports the honest in-progress status if the window never closes in time; complete() only leaves a session resumable when the caller confirms the agent actually survived the turn (agentRetained), so a disposed salvage can't look reusable. - run.ts: keep the run controller's parent-abort forwarding alive for a persisted session so a later parent cancel still reaches the registered close handle; the close handle itself fully disposes the controller once the session actually closes. - types.ts / agent-fleet.ts: thread agentRetained through RunSubAgentResult so the store can tell a clean, still-open completion apart from a resolved-but-disposed salvage.
1 parent 7a3efc9 commit 194ec94

8 files changed

Lines changed: 322 additions & 50 deletions

File tree

CHANGELOG.md

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

4251
## [0.2.109] - 2026-08-24
4352

src/subagent/agent-fleet.test.ts

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -196,12 +196,14 @@ describe("spawn_agent + wait_agents", () => {
196196
// DEFAULT_MAX_COMPLETED on SubAgentSessionStore is 20 finished sessions;
197197
// spawn (and complete) enough workers to blow well past it before any of
198198
// them is collected, proving fleetRecords does not depend on the store's
199-
// cap either. CL-6943: a spawn_agent session is now retained (exempt
200-
// from the cap) until close_agent runs, so — unlike the pre-CL-6943
201-
// version of this test — the store also keeps every one of them; that
202-
// is covered by session-store.test.ts's own cap tests.
199+
// cap. CL-7001: a retained session is bounded by this same cap too now
200+
// (it used to be exempt with no separate cap or TTL, which is exactly
201+
// why every spawn_agent worker leaked by default) — so unlike the
202+
// pre-CL-7001 version of this test, the store itself may have already
203+
// evicted (and released) the earliest ones; wait_agents/fleetRecords is
204+
// the durable source of truth this test actually cares about.
203205
const COUNT = 25;
204-
const deps = makeDeps(async () => ({ report: "irrelevant" }));
206+
const deps = makeDeps(async () => ({ report: "irrelevant", agentRetained: true }));
205207
const spawn = createSpawnAgentTool(deps);
206208
const wait = createWaitAgentsTool({ sessions: deps.sessions, fleetRecords: deps.fleetRecords });
207209

@@ -218,8 +220,10 @@ describe("spawn_agent + wait_agents", () => {
218220
// Let every spawn's run() resolve and complete() land before collecting.
219221
await new Promise((resolve) => setTimeout(resolve, 20));
220222

221-
// Retained sessions are exempt from the display cap.
222-
expect(deps.sessions.get(ids[0]!)).toBeDefined();
223+
// The store's own bound may have already evicted (and released) the
224+
// earliest session — fleetRecords below is what wait_agents actually
225+
// depends on, and it is never subject to this cap.
226+
expect(deps.sessions.get(ids[0]!)).toBeUndefined();
223227

224228
// Every single one is retrievable through wait_agents too.
225229
const waited = await callTool(wait, { targets: ids, timeout_ms: 5000 });

src/subagent/agent-fleet.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -485,7 +485,14 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool {
485485
.then((result) => {
486486
if (childCtl.signal.aborted) return;
487487
deps.fleetRecords.resolve(session.id, result.report);
488-
deps.sessions.complete(session.id, result.report);
488+
// CL-7001: result.agentRetained is only true on run.ts's clean-
489+
// completion path when persist actually skipped teardown — a
490+
// deadline/cancel salvage resolves through the same promise but
491+
// always disposed its agent first, so the store must not treat it
492+
// as resumable just because retained:true was requested at spawn.
493+
deps.sessions.complete(session.id, result.report, {
494+
agentRetained: result.agentRetained === true,
495+
});
489496
})
490497
.catch((err) => {
491498
if (childCtl.signal.aborted) return;
Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
import { describe, expect, test } from "bun:test";
2+
import { createSubAgentSessionStore } from "./session-store.js";
3+
4+
describe("retained session lifecycle", () => {
5+
test("a salvaged (deadline/cancel) run lands resumable even though run.ts disposed its agent", () => {
6+
const store = createSubAgentSessionStore({ maxCompleted: 5 });
7+
const s = store.start({ description: "worker", agentId: "build", brief: "b", retained: true });
8+
store.markRunning(s.id);
9+
// run.ts salvage path RETURNS a report (does not throw) with stopReason
10+
// "deadline", but leaves turnSucceeded=false so finally disposes the
11+
// agent. agent-fleet's .then() still routes it to complete() — passing
12+
// agentRetained:false, exactly as its real call site does whenever
13+
// result.agentRetained isn't true.
14+
store.complete(s.id, "Stopped: deadline\n\nPartial work...", { agentRetained: false });
15+
const after = store.get(s.id);
16+
console.log("lifecycleStatus:", after?.lifecycleStatus, "retained:", after?.retained);
17+
const outcome = store.resumeOne(s.id);
18+
console.log("resumeOne outcome:", JSON.stringify(outcome));
19+
expect(outcome.ok).toBe(false);
20+
});
21+
22+
test("cancelAll does not close retained completed sessions", () => {
23+
const store = createSubAgentSessionStore({ maxCompleted: 5 });
24+
const s = store.start({ description: "worker", agentId: "build", brief: "b", retained: true });
25+
let closed = false;
26+
store.registerClose(s.id, async () => {
27+
closed = true;
28+
});
29+
store.complete(s.id, "done");
30+
const cancelled = store.cancelAll("parent stop");
31+
console.log("cancelAll returned:", cancelled, "| close invoked:", closed);
32+
expect(closed).toBe(true);
33+
});
34+
35+
test("retained completed sessions are exempt from the display cap without bound", () => {
36+
const store = createSubAgentSessionStore({ maxCompleted: 3 });
37+
for (let i = 0; i < 50; i++) {
38+
const s = store.start({ description: `w${i}`, agentId: "build", brief: "b", retained: true });
39+
store.complete(s.id, "done");
40+
}
41+
console.log("sessions retained despite maxCompleted=3:", store.list().length);
42+
expect(store.list().length).toBeLessThanOrEqual(3);
43+
});
44+
45+
test("a genuinely retained clean completion IS resumable, and cancelAll releases it", () => {
46+
const store = createSubAgentSessionStore({ maxCompleted: 5 });
47+
const s = store.start({ description: "worker", agentId: "build", brief: "b", retained: true });
48+
store.markRunning(s.id);
49+
let closed = false;
50+
store.registerClose(s.id, async () => {
51+
closed = true;
52+
});
53+
// Mirrors agent-fleet's real call: only a clean turnSucceeded completion
54+
// sets agentRetained.
55+
store.complete(s.id, "done", { agentRetained: true });
56+
expect(store.resumeOne(s.id).ok).toBe(true);
57+
expect(closed).toBe(false);
58+
store.cancelAll("parent stop");
59+
expect(closed).toBe(true);
60+
});
61+
62+
test("clear() releases every retained session's close handle instead of dropping it silently", () => {
63+
const store = createSubAgentSessionStore({ maxCompleted: 5 });
64+
const s = store.start({ description: "worker", agentId: "build", brief: "b", retained: true });
65+
store.markRunning(s.id);
66+
let closed = false;
67+
store.registerClose(s.id, async () => {
68+
closed = true;
69+
});
70+
store.complete(s.id, "done", { agentRetained: true });
71+
store.clear();
72+
expect(closed).toBe(true);
73+
});
74+
75+
test("close_agent during the setup window waits for the handle instead of falsely reporting shutdown", async () => {
76+
const store = createSubAgentSessionStore({ maxCompleted: 5 });
77+
const s = store.start({ description: "worker", agentId: "build", brief: "b", retained: true });
78+
// No registerClose yet — closeOne races the agent-setup window.
79+
const closePromise = store.closeOne(s.id, 200);
80+
let registeredClose = false;
81+
setTimeout(() => {
82+
store.registerClose(s.id, async () => {
83+
registeredClose = true;
84+
});
85+
}, 20);
86+
const status = await closePromise;
87+
expect(status).toBe("shutdown");
88+
expect(registeredClose).toBe(true);
89+
});
90+
91+
test("close_agent gives up honestly (not a false shutdown) if the handle never arrives in time", async () => {
92+
const store = createSubAgentSessionStore({ maxCompleted: 5 });
93+
const s = store.start({ description: "worker", agentId: "build", brief: "b", retained: true });
94+
store.markRunning(s.id);
95+
const status = await store.closeOne(s.id, 30);
96+
expect(status).not.toBe("shutdown");
97+
expect(store.get(s.id)).toBeDefined();
98+
});
99+
});

src/subagent/run.ts

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,14 @@ export interface SubAgentRunController {
197197
deadlineHit: () => boolean;
198198
/** Abort the run from inside, distinct from parent cancel and deadline. */
199199
abort: (reason: Error) => void;
200-
dispose: () => void;
200+
// CL-7001: normally tears down the timer and the parent-abort forwarding
201+
// listener. Pass keepParentListener:true for a run that is persisting
202+
// (retained, clean completion) — otherwise a later parent abort (operator
203+
// cancel/close reaching this run's own params.signal) would stop
204+
// propagating into runController.signal, and closeOnAbort — which is
205+
// registered on runController.signal, not the parent's — would never fire
206+
// for the still-open session.
207+
dispose: (opts?: { keepParentListener?: boolean }) => void;
201208
}
202209

203210
/**
@@ -238,9 +245,11 @@ export function createSubAgentRunController(
238245
abort: (reason: Error): void => {
239246
if (!controller.signal.aborted) controller.abort(reason);
240247
},
241-
dispose: (): void => {
248+
dispose: (opts?: { keepParentListener?: boolean }): void => {
242249
if (timer !== undefined) clearTimeout(timer);
243-
parentSignal?.removeEventListener("abort", onParentAbort);
250+
if (opts?.keepParentListener !== true) {
251+
parentSignal?.removeEventListener("abort", onParentAbort);
252+
}
244253
},
245254
};
246255
}
@@ -812,6 +821,11 @@ export async function runSubAgent(params: RunSubAgentParams): Promise<RunSubAgen
812821
teardown,
813822
new Promise<void>((resolve) => setTimeout(resolve, deadlineMs)),
814823
]);
824+
// CL-7001: the run's finally block kept the parent-abort forwarding
825+
// listener alive for a persisted session (see runController.dispose's
826+
// doc); now that this session is actually closing, tear it down for
827+
// real so the listener does not outlive the session.
828+
runController.dispose();
815829
};
816830
params.onAgentReady(boundedClose);
817831
}
@@ -867,6 +881,10 @@ export async function runSubAgent(params: RunSubAgentParams): Promise<RunSubAgen
867881
return {
868882
report: appendActivitySummary(report, toolNamesUsed),
869883
...(directorForcedStopReason !== undefined ? { stopReason: directorForcedStopReason } : {}),
884+
// CL-7001: only this path skips teardown below when persist is set —
885+
// tell the caller so a salvage below is never mistaken for a still-
886+
// live, resumable agent.
887+
...(params.persist === true ? { agentRetained: true } : {}),
870888
};
871889
} catch (err) {
872890
if (isSubAgentCancelError(err, runController.signal)) {
@@ -913,12 +931,17 @@ export async function runSubAgent(params: RunSubAgentParams): Promise<RunSubAgen
913931
}
914932
} finally {
915933
if (stallWatchdog !== undefined) clearInterval(stallWatchdog);
916-
runController.dispose();
934+
const persisting = params.persist === true && turnSucceeded;
935+
// CL-7001: a persisting run must keep the parent-signal forwarding alive
936+
// (see createSubAgentRunController's dispose doc) — boundedClose (the
937+
// close_agent handle) fully disposes the runController itself once the
938+
// session actually tears down.
939+
runController.dispose({ keepParentListener: persisting });
917940
// CL-6943: a persisted, cleanly-completed session skips teardown here —
918941
// it stays open until close_agent (or a later failed/aborted run) tears
919942
// it down. Everything else (no persist, a thrown error, an
920943
// aborted/salvaged run) disposes exactly as before.
921-
if (!(params.persist === true && turnSucceeded)) {
944+
if (!persisting) {
922945
await disposeSubAgentSession({
923946
signal: runController.signal,
924947
...(closeOnAbort !== undefined ? { closeOnAbort } : {}),

src/subagent/session-store.test.ts

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -350,28 +350,40 @@ describe("CL-6943 reusable worker sessions", () => {
350350
test("resume_agent fails on a session close_agent already shut down (close is permanent)", async () => {
351351
const store = createSubAgentSessionStore();
352352
const session = store.start({ description: "d", agentId: "a", brief: "b", retained: true });
353+
// registerClose always fires in production before onAgentReady's window
354+
// closes (CL-7001) — closeOne otherwise waits for it up to the deadline.
355+
store.registerClose(session.id, async () => {});
353356
store.complete(session.id, "## Summary\nDone.");
354357
await store.closeOne(session.id, 1000);
355358
expect(store.resumeOne(session.id)).toEqual({ ok: false, status: "shutdown" });
356359
});
357360

358-
test("pruneCompleted does not evict a retained, still-open session past maxCompleted", () => {
361+
// CL-7001: a retained, still-open session used to be exempt from this cap
362+
// entirely — no separate cap or TTL — which is exactly why every
363+
// spawn_agent worker leaked by default. maxCompleted is now the one bound
364+
// the store owns for every finished session, retained or not, and
365+
// eviction releases the session's close handle instead of abandoning it.
366+
test("pruneCompleted evicts a retained, still-open session past maxCompleted and releases it", () => {
359367
const store = createSubAgentSessionStore({ maxCompleted: 1 });
360368
const retained = store.start({
361369
description: "keep-me",
362370
agentId: "a",
363371
brief: "b",
364372
retained: true,
365373
});
374+
let closed = false;
375+
store.registerClose(retained.id, async () => {
376+
closed = true;
377+
});
366378
store.complete(retained.id, "## Summary\nDone.");
367379

368380
for (let i = 0; i < 3; i++) {
369381
const s = store.start({ description: `fill-${i}`, agentId: "a", brief: "b" });
370382
store.complete(s.id, "## Summary\nDone.");
371383
}
372384

373-
expect(store.get(retained.id)).toBeDefined();
374-
expect(store.get(retained.id)?.lifecycleStatus).toBe("completed");
385+
expect(store.get(retained.id)).toBeUndefined();
386+
expect(closed).toBe(true);
375387
});
376388

377389
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", () => {
382394
brief: "b",
383395
retained: true,
384396
});
397+
store.registerClose(retained.id, async () => {});
385398
store.complete(retained.id, "## Summary\nDone.");
386399
await store.closeOne(retained.id, 1000);
387400

0 commit comments

Comments
 (0)