diff --git a/CHANGELOG.md b/CHANGELOG.md index d7e6e5918..e4a3fd00b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/subagent/agent-fleet.test.ts b/src/subagent/agent-fleet.test.ts index bcc588d4c..185da3b63 100644 --- a/src/subagent/agent-fleet.test.ts +++ b/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 }); diff --git a/src/subagent/agent-fleet.ts b/src/subagent/agent-fleet.ts index 7a5d18b41..054a5db71 100644 --- a/src/subagent/agent-fleet.ts +++ b/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; diff --git a/src/subagent/retain-salvage.test.ts b/src/subagent/retain-salvage.test.ts new file mode 100644 index 000000000..9db4489a3 --- /dev/null +++ b/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(); + }); +}); diff --git a/src/subagent/run.ts b/src/subagent/run.ts index 5084042d4..faf41c857 100644 --- a/src/subagent/run.ts +++ b/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((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 { 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", @@ -363,6 +371,10 @@ describe("CL-6943 reusable worker sessions", () => { 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++) { @@ -370,8 +382,8 @@ describe("CL-6943 reusable worker sessions", () => { 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); diff --git a/src/subagent/session-store.ts b/src/subagent/session-store.ts index d83aff8b7..c0af22f3a 100644 --- a/src/subagent/session-store.ts +++ b/src/subagent/session-store.ts @@ -5,6 +5,7 @@ // this store is the dedicated child record the enter-session UI reads. import type { ReactorEmittedEvent } from "@intx/inference"; +import { DEFAULT_CLOSE_DEADLINE_MS } from "./dispose.js"; import { stopReasonFromReport } from "./report.js"; import { toolCallPreview } from "./tool-preview.js"; @@ -130,7 +131,12 @@ export interface SubAgentSessionStore { listForStrip(): readonly SubAgentSession[]; start(input: StartSessionInput): SubAgentSession; appendEvent(id: string, event: ReactorEmittedEvent): void; - complete(id: string, report: string): void; + // `agentRetained` (CL-7001) must mirror run.ts's own turnSucceeded gate — + // true only when the caller actually skipped teardown for this turn. A + // deadline/cancel salvage resolves the same promise agent-fleet routes + // here but always disposes its agent first, so omitting/false-ing this + // keeps a disposed session from ever reporting as resumable. + complete(id: string, report: string, opts?: { agentRetained?: boolean }): void; fail(id: string, error: string): void; // Register the live abort handle for a running session so cancel() can stop // the child reactor (agent.close), not only flip status. @@ -363,10 +369,31 @@ export function createSubAgentSessionStore( } }; + // CL-7001: releases any resources this store still holds for `id` — the + // registered close handle (invoked best-effort, fire-and-forget, so a + // wedged descendant cannot stall the caller that triggered eviction) and + // the cancel handle. Called whenever a session record is dropped, so a + // retained-but-idle session's real agent is never simply forgotten about. + const releaseHandles = (id: string): void => { + const close = closeHandles.get(id); + if (close !== undefined) { + closeHandles.delete(id); + void close(DEFAULT_CLOSE_DEADLINE_MS).catch(() => {}); + } + cancelHandles.delete(id); + }; + + // CL-7001: `maxCompleted` is the one bound this store owns for every + // finished session, retained or not — a retained-but-idle ("completed") + // session used to be exempt here with no separate cap or TTL, which is + // exactly why every spawn_agent worker leaked by default. A session that + // was resumed and is actively running again (lifecycleStatus "running") + // is still excluded: it has a live caller, not an idle leak. const pruneCompleted = (): void => { if (maxCompleted <= 0) { for (const [id, s] of sessions) { - if (s.status !== "running") { + if (s.status !== "running" && s.lifecycleStatus !== "running") { + releaseHandles(id); sessions.delete(id); forgetRevision(id); } @@ -374,30 +401,52 @@ export function createSubAgentSessionStore( return; } const finished = [...sessions.values()] - .filter( - (s) => - s.status !== "running" && - // CL-6943: a retained session that is still open ("completed", or - // "running" again after resume_agent) is reusable and must not be - // evicted by this display cap out from under it — only a shutdown - // (or never-retained) finished session counts toward the limit. - !( - s.retained === true && - (s.lifecycleStatus === "completed" || s.lifecycleStatus === "running") - ), - ) + .filter((s) => s.status !== "running" && s.lifecycleStatus !== "running") .sort((a, b) => (a.finishedAt ?? 0) - (b.finishedAt ?? 0)); const excess = finished.length - maxCompleted; if (excess <= 0) return; for (let i = 0; i < excess; i++) { const drop = finished[i]; if (drop !== undefined) { + releaseHandles(drop.id); sessions.delete(drop.id); forgetRevision(drop.id); } } }; + // CL-7001: resolves once `id` either gets a close handle registered, goes + // shutdown, disappears, or `deadlineMs` elapses (whichever first) — the + // wait closeOne uses for a close_agent call that raced agent setup. + const waitForCloseHandle = ( + id: string, + deadlineMs: number, + ): Promise<((deadlineMs?: number) => Promise) | undefined> => { + return new Promise((resolve) => { + let settled = false; + const listener = (): void => check(); + const finish = (value: ((deadlineMs?: number) => Promise) | undefined): void => { + if (settled) return; + settled = true; + clearTimeout(timer); + listeners.delete(listener); + resolve(value); + }; + const check = (): void => { + const session = sessions.get(id); + if (session === undefined || session.lifecycleStatus === "shutdown") { + finish(undefined); + return; + } + const close = closeHandles.get(id); + if (close !== undefined) finish(close); + }; + listeners.add(listener); + const timer = setTimeout(() => finish(closeHandles.get(id)), deadlineMs); + check(); + }); + }; + const mutate = (id: string, fn: (session: SubAgentSession) => void): void => { const session = sessions.get(id); if (session === undefined) return; @@ -600,18 +649,31 @@ export function createSubAgentSessionStore( }); }, - complete(id: string, report: string): void { + complete(id: string, report: string, opts?: { agentRetained?: boolean }): void { + // CL-7001: run.ts always disposes on a salvage return (deadline/cancel) + // even though it resolves through this same success path — only trust + // "still open, resumable" when the caller says the agent genuinely + // survived this turn. + // Defaults true: complete() historically meant "clean completion," and + // task-tool.ts / tests call it that way with no opts at all. Only + // agent-fleet's spawn_agent path ever has a salvage to report, and it + // always passes this flag explicitly (see its call site). + const agentRetained = opts?.agentRetained ?? true; mutate(id, (session) => { // Cancel wins races: a late complete after operator cancel must not // resurrect the session as done. if (session.status !== "running") return; session.status = "done"; // CL-6943: retained sessions stay "completed" (open, reusable) here — - // only close_agent (closeOne) moves them to "shutdown". A session - // that never opted into retention has no live agent behind it by the - // time this fires either way, so the distinction only matters for - // whether pruneCompleted's cap may evict the record. + // only close_agent (closeOne) moves them to "shutdown". session.lifecycleStatus = "completed"; + // CL-7001: a disposed salvage (deadline/cancel) resolves through + // this same path but run.ts has already torn its agent down — clear + // `retained` so resumeOne's `retained === true` gate can never see + // it as open, without touching the lifecycleStatus invariant every + // other completion (including a never-retained one) already relies + // on. + if (!agentRetained) session.retained = false; session.finishedAt = now(); clearToolCalls(session); session.report = report; @@ -620,7 +682,10 @@ export function createSubAgentSessionStore( const stopped = stopReasonFromReport(report); if (stopped !== null) session.stopReason = stopped; pushEntry(session, { kind: "report", content: capText(report, maxEntryChars) }); + // A disposed salvage has nothing left for its close handle to do — + // release it now rather than leaving a stale reference around. cancelHandles.delete(id); + if (!agentRetained) closeHandles.delete(id); pruneCompleted(); }); }, @@ -632,6 +697,11 @@ export function createSubAgentSessionStore( // A thrown run always tears down its agent in run.ts's finally // (persist only skips teardown on a clean success) — so there is // nothing left to resume here, and retained no longer applies. + // CL-7001: a deadline/cancel salvage does NOT throw — it returns a + // report through the same success path a clean completion uses, so + // it never reaches this function. complete() carries the equivalent + // "agent was actually disposed" check for that case via its + // agentRetained flag; this function only ever needed to cover throws. session.lifecycleStatus = "shutdown"; session.retained = false; session.finishedAt = now(); @@ -662,23 +732,45 @@ export function createSubAgentSessionStore( registerClose(id: string, close: (deadlineMs?: number) => Promise): void { if (!sessions.has(id)) return; closeHandles.set(id, close); + // CL-7001: wake anything blocked in closeOne's waitForCloseHandle below — + // a close_agent call that arrived during the agent-setup window (before + // this registration) is waiting on exactly this notification instead of + // reporting false success over an unreleasable session. + notify(); }, async closeOne(id: string, deadlineMs: number): Promise { const session = sessions.get(id); if (session === undefined) return "not_found"; if (session.lifecycleStatus === "shutdown") return "shutdown"; - const close = closeHandles.get(id); - if (close !== undefined) { - closeHandles.delete(id); - // Bounded here too, defense-in-depth against a caller-registered - // close that does not honor its own deadline argument — a wedged - // descendant must not hang the whole close_agent call. - await Promise.race([ - close(deadlineMs).catch(() => {}), - new Promise((resolve) => setTimeout(resolve, deadlineMs)), - ]); + let close = closeHandles.get(id); + if (close === undefined) { + // CL-7001: close_agent landed in the setup window — the session + // exists but createAgentWithLiveToolDispatch hasn't finished and + // registerClose hasn't fired yet. Wait for it (bounded) instead of + // returning "shutdown" immediately: that used to report false + // success while leaving the eventual agent unreleasable forever + // (the early return above short-circuits every retry once + // lifecycleStatus flips). + close = await waitForCloseHandle(id, deadlineMs); + const stillHere = sessions.get(id); + if (stillHere === undefined) return "not_found"; + if (stillHere.lifecycleStatus === "shutdown") return "shutdown"; + if (close === undefined) { + // Never became closeable within the deadline: report the honest + // in-progress status rather than a false "shutdown" — the caller + // can retry, and this session is still findable to retry against. + return stillHere.lifecycleStatus; + } } + closeHandles.delete(id); + // Bounded here too, defense-in-depth against a caller-registered + // close that does not honor its own deadline argument — a wedged + // descendant must not hang the whole close_agent call. + await Promise.race([ + close(deadlineMs).catch(() => {}), + new Promise((resolve) => setTimeout(resolve, deadlineMs)), + ]); mutate(id, (s) => { s.lifecycleStatus = "shutdown"; s.retained = false; @@ -715,6 +807,20 @@ export function createSubAgentSessionStore( for (const session of running) { if (cancelSession(session.id, reason)) cancelled.push(session.id); } + // CL-7001: a retained session is "done", not "running", so the loop + // above always skipped it — both /clear and session-close route + // through cancelAll, so a retained worker's LSP sidecars, reactor, and + // heldLocks entry outlived the parent turn indefinitely. Release every + // still-open retained session here too, regardless of `status`. + for (const session of sessions.values()) { + if (session.retained === true && session.lifecycleStatus !== "shutdown") { + releaseHandles(session.id); + mutate(session.id, (s) => { + s.lifecycleStatus = "shutdown"; + s.retained = false; + }); + } + } return cancelled; }, @@ -726,8 +832,10 @@ export function createSubAgentSessionStore( }, clear(): void { - // Drop handles without invoking them — callers that need teardown should - // cancelAll first (parent stop / /clear). + // CL-7001: invoke every registered close (best-effort, fire-and-forget) + // before dropping the maps — this used to drop closeHandles without + // calling them, leaking every retained session's agent permanently. + for (const id of closeHandles.keys()) releaseHandles(id); cancelHandles.clear(); closeHandles.clear(); sessions.clear(); diff --git a/src/subagent/types.ts b/src/subagent/types.ts index 5597b629f..2042e4644 100644 --- a/src/subagent/types.ts +++ b/src/subagent/types.ts @@ -172,4 +172,13 @@ export type RunSubAgentParams = { export interface RunSubAgentResult { report: string; stopReason?: ForcedStopReason; + /** + * CL-7001: true only on the clean-completion path when `persist: true` + * actually skipped teardown (mirrors run.ts's own turnSucceeded gate). A + * deadline/cancel salvage returns without throwing but always disposes its + * agent, so this is absent (falsy) there even though the promise resolves + * the same way a clean completion does — the session store uses this to + * keep a disposed salvage from ever looking resumable. + */ + agentRetained?: boolean; }