diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a4e937e8..c4793d32b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,18 @@ matching `## [X.Y.Z]` section (plus install instructions). Do not maintain parallel copies under `docs/` or `scripts/notes/`. At cut time: rename `## [Unreleased]` to `## [X.Y.Z] - YYYY-MM-DD`, then run the release script. +## [Unreleased] + +### Fixed + +- Retained worker sessions (`spawn_agent`, resumable via `resume_agent`/`followup_task`) now have + their own retention cap, separate from the TUI's finished-session display cap. Previously they + shared that 20-item cap, so `resume_agent` on an early worker failed with a bare `not_found` once + a fan-out of more than 20 workers had finished. A session dropped by the retention cap still + releases its sidecars/reactor/lock entry, always evicts least-recently-used first, and never + evicts a running session. `resume_agent`/`followup_task` against an evicted session now report + its terminal status plus a pointer to `read_agent_trace`, instead of `not_found`. + ## [0.3.0] - 2026-08-24 ### Breaking diff --git a/src/subagent/agent-fleet.test.ts b/src/subagent/agent-fleet.test.ts index 185da3b63..71dad9279 100644 --- a/src/subagent/agent-fleet.test.ts +++ b/src/subagent/agent-fleet.test.ts @@ -196,12 +196,17 @@ 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. 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. + // cap. + // + // CL-7007: this test previously asserted (as CL-7001's fix left it) that + // the store itself had already evicted and released the earliest + // session, because a retained session shared the 20-item display cap + // with every other finished session — exactly the shipped defect this + // ticket fixes (resume_agent/followup_task failed with a bare + // "not_found" past 20 spawned workers, blaming the caller for nothing). + // Open retained sessions now have their own cap (`maxRetained`, default + // 50), so 25 of them all stay resumable; fleetRecords/wait_agents is + // still asserted below as the durable source of truth regardless. const COUNT = 25; const deps = makeDeps(async () => ({ report: "irrelevant", agentRetained: true })); const spawn = createSpawnAgentTool(deps); @@ -220,10 +225,9 @@ describe("spawn_agent + wait_agents", () => { // Let every spawn's run() resolve and complete() land before collecting. await new Promise((resolve) => setTimeout(resolve, 20)); - // 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(); + // 25 open retained sessions is under the default maxRetained (50), so + // the earliest is still present and resumable — not evicted. + expect(deps.sessions.get(ids[0]!)).toBeDefined(); // Every single one is retrievable through wait_agents too. const waited = await callTool(wait, { targets: ids, timeout_ms: 5000 }); diff --git a/src/subagent/lifecycle-tools.ts b/src/subagent/lifecycle-tools.ts index 2d7575323..d37cba9c8 100644 --- a/src/subagent/lifecycle-tools.ts +++ b/src/subagent/lifecycle-tools.ts @@ -136,9 +136,10 @@ export function createResumeAgentTool(deps: LifecycleToolDeps): AgentTool { const target = parsed.target.trim(); const outcome = deps.sessions.resumeOne(target); if (!outcome.ok) { + const hint = outcome.hint !== undefined ? ` ${outcome.hint}` : ""; return lifecycleResult( call.id, - `Error: cannot resume "${target}" (status: ${outcome.status}).`, + `Error: cannot resume "${target}" (status: ${outcome.status}).${hint}`, ); } return lifecycleResult(call.id, JSON.stringify({ agent_id: target, status: "running" })); @@ -235,9 +236,10 @@ export function createFollowupTaskTool(deps: LifecycleToolDeps): AgentTool { } const outcome = await deps.sessions.followupOne(target, message); if (!outcome.ok) { + const hint = outcome.hint !== undefined ? ` ${outcome.hint}` : ""; return lifecycleResult( call.id, - `Error: cannot send followup to "${target}" (status: ${outcome.status}).`, + `Error: cannot send followup to "${target}" (status: ${outcome.status}).${hint}`, ); } return lifecycleResult( diff --git a/src/subagent/retain-salvage.test.ts b/src/subagent/retain-salvage.test.ts index 9db4489a3..663f90a46 100644 --- a/src/subagent/retain-salvage.test.ts +++ b/src/subagent/retain-salvage.test.ts @@ -32,13 +32,23 @@ describe("retained session lifecycle", () => { expect(closed).toBe(true); }); - test("retained completed sessions are exempt from the display cap without bound", () => { - const store = createSubAgentSessionStore({ maxCompleted: 3 }); + // CL-7007: retained completed sessions are no longer bounded by + // `maxCompleted` (the TUI display cap) at all — that was CL-7002's fix, + // and it created a new bug: resume_agent/followup_task started failing + // with a bare "not_found" once more than `maxCompleted` (default 20) + // workers had spawned in a turn, even though every one of them was still + // perfectly reusable. Open retained sessions now get their own explicit + // cap, `maxRetained`, sized for fan-out rather than a sidebar list — this + // test moved from asserting `maxCompleted` bounds them to asserting + // `maxRetained` does (still bounded, still no leak, just the right knob). + test("retained completed sessions are bounded by maxRetained, not the display cap", () => { + const store = createSubAgentSessionStore({ maxCompleted: 3, maxRetained: 3 }); for (let i = 0; i < 50; i++) { const s = store.start({ description: `w${i}`, agentId: "build", brief: "b", retained: true }); + store.registerClose(s.id, async () => {}); store.complete(s.id, "done"); } - console.log("sessions retained despite maxCompleted=3:", store.list().length); + console.log("sessions retained despite maxRetained=3:", store.list().length); expect(store.list().length).toBeLessThanOrEqual(3); }); diff --git a/src/subagent/session-store.test.ts b/src/subagent/session-store.test.ts index 51b34057c..5f93b7935 100644 --- a/src/subagent/session-store.test.ts +++ b/src/subagent/session-store.test.ts @@ -358,13 +358,19 @@ describe("CL-6943 reusable worker sessions", () => { expect(store.resumeOne(session.id)).toEqual({ ok: false, status: "shutdown" }); }); - // 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 }); + // CL-7001 originally folded a retained, still-open session into + // maxCompleted (the TUI display cap) with no separate bound at all, + // fixing the unbounded leak but creating a new bug: resume_agent / + // followup_task fail once more than `maxCompleted` (default 20) workers + // have spawned, even though every one of them is still perfectly + // reusable. CL-7007 gives open retained sessions their own cap + // (`maxRetained`) instead — this test changed from asserting that + // `maxCompleted` evicts a retained session (no longer true: retained + // sessions are excluded from that cap, see isOpenRetained) to asserting + // that `maxRetained` does, with the same "handles still get released" + // guarantee. + test("pruneRetained evicts a retained, still-open session past maxRetained and releases it", () => { + const store = createSubAgentSessionStore({ maxCompleted: 1, maxRetained: 1 }); const retained = store.start({ description: "keep-me", agentId: "a", @@ -378,7 +384,13 @@ describe("CL-6943 reusable worker sessions", () => { store.complete(retained.id, "## Summary\nDone."); for (let i = 0; i < 3; i++) { - const s = store.start({ description: `fill-${i}`, agentId: "a", brief: "b" }); + const s = store.start({ + description: `fill-${i}`, + agentId: "a", + brief: "b", + retained: true, + }); + store.registerClose(s.id, async () => {}); store.complete(s.id, "## Summary\nDone."); } @@ -386,6 +398,100 @@ describe("CL-6943 reusable worker sessions", () => { expect(closed).toBe(true); }); + // CL-7002's fix (retained sessions are no longer exempt from any cap) must + // survive CL-7007: a non-retained finished session still obeys + // maxCompleted exactly as before. + test("maxCompleted still evicts an ordinary (non-retained) finished session", () => { + const store = createSubAgentSessionStore({ maxCompleted: 1 }); + const first = store.start({ description: "first", agentId: "a", brief: "b" }); + store.complete(first.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(first.id)).toBeUndefined(); + }); + + test("resume_agent on a retention-evicted session returns an actionable status, not not_found", () => { + const store = createSubAgentSessionStore({ maxRetained: 1 }); + const retained = store.start({ + description: "keep-me", + agentId: "a", + brief: "b", + retained: true, + }); + store.registerClose(retained.id, async () => {}); + store.complete(retained.id, "## Summary\nDone."); + + for (let i = 0; i < 3; i++) { + const s = store.start({ + description: `fill-${i}`, + agentId: "a", + brief: "b", + retained: true, + }); + store.registerClose(s.id, async () => {}); + store.complete(s.id, "## Summary\nDone."); + } + + const outcome = store.resumeOne(retained.id); + expect(outcome.ok).toBe(false); + if (!outcome.ok) { + expect(outcome.status).toBe("completed"); + expect(outcome.hint).toMatch(/read_agent_trace/); + } + }); + + test("a running session is never evicted by maxRetained even when the cap is exceeded", () => { + const store = createSubAgentSessionStore({ maxRetained: 1 }); + const running = store.start({ + description: "keep-me", + agentId: "a", + brief: "b", + retained: true, + }); + store.markRunning(running.id); + // Resume it back to "running" so it is an open, actively-driven session. + store.registerClose(running.id, async () => {}); + store.complete(running.id, "## Summary\nDone."); + store.resumeOne(running.id); + expect(store.get(running.id)?.lifecycleStatus).toBe("running"); + + for (let i = 0; i < 5; i++) { + const s = store.start({ + description: `fill-${i}`, + agentId: "a", + brief: "b", + retained: true, + }); + store.registerClose(s.id, async () => {}); + store.complete(s.id, "## Summary\nDone."); + } + + expect(store.get(running.id)).toBeDefined(); + expect(store.get(running.id)?.lifecycleStatus).toBe("running"); + }); + + test("maxRetained bounds memory: many spawned-and-completed retained sessions do not grow without limit", () => { + const store = createSubAgentSessionStore({ maxRetained: 5 }); + for (let i = 0; i < 50; i++) { + const s = store.start({ + description: `worker-${i}`, + agentId: "a", + brief: "b", + retained: true, + }); + store.registerClose(s.id, async () => {}); + store.complete(s.id, "## Summary\nDone."); + } + const openRetained = store + .list() + .filter((s) => s.retained === true && s.lifecycleStatus === "completed"); + expect(openRetained.length).toBeLessThanOrEqual(5); + }); + test("once closed, a retained session becomes a normal finished record subject to the cap", async () => { const store = createSubAgentSessionStore({ maxCompleted: 1 }); const retained = store.start({ diff --git a/src/subagent/session-store.ts b/src/subagent/session-store.ts index a60503658..f19bcd111 100644 --- a/src/subagent/session-store.ts +++ b/src/subagent/session-store.ts @@ -92,9 +92,11 @@ export interface SubAgentSession { // "pending_init" until the run wires up markRunning(); see the type doc. lifecycleStatus: AgentLifecycleStatus; // True when this session's agent is meant to survive a clean completion - // (spawn_agent opts in). Only a retained session in lifecycleStatus - // "completed" is exempt from pruneCompleted's cap — once close_agent runs, - // this flips back to false and the cap applies normally. + // (spawn_agent opts in). An open retained session ("completed" or + // "interrupted") is governed by its own retention cap (`maxRetained`), + // separate from `maxCompleted` (the TUI display cap) — see pruneRetained. + // Once close_agent runs, this flips back to false and the session is a + // normal finished record subject to `maxCompleted` like any other. retained?: boolean; } @@ -114,8 +116,17 @@ export interface StartSessionInput { export interface SubAgentSessionStoreOptions { // Cap on completed/failed sessions retained after finish. Running sessions - // are never pruned by this bound. + // are never pruned by this bound. Does NOT govern open retained sessions + // ("completed"/"interrupted" with retained:true) — see maxRetained. maxCompleted?: number; + // CL-7007: cap on open retained sessions (spawn_agent workers a caller may + // still resume_agent/followup_task). Sized for fan-out (dozens of + // concurrent workers), independent of maxCompleted's TUI display cap. + // Least-recently-used is evicted first; a "running" session is never + // evicted regardless of this bound. Non-finite/undefined values (and a + // JSON round-trip that turned a configured Infinity into null) fall back + // to the default rather than silently becoming 0. + maxRetained?: number; // Cap on transcript entries per session (oldest dropped). maxEntries?: number; // Cap on characters per text/thinking/result entry. @@ -164,7 +175,10 @@ export interface SubAgentSessionStore { // "shutdown" session is gone for good (close_agent is permanent), an // "interrupted" one already tore its agent down, and "running"/ // "pending_init"/"not_found" have nothing to resume. - resumeOne(id: string): { ok: true } | { ok: false; status: AgentLifecycleStatus }; + // CL-7007: a session dropped by pruneRetained still reports its terminal + // lifecycleStatus plus `hint` pointing at read_agent_trace — never a bare + // "not_found" that reads like a bad id. + resumeOne(id: string): { ok: true } | { ok: false; status: AgentLifecycleStatus; hint?: string }; // CL-6997: registers the per-session interrupt/followup handles run.ts // hands back via onAgentReady. Distinct maps from registerClose/closeOne // above (interrupt must never route through close's codepath). @@ -182,7 +196,9 @@ export interface SubAgentSessionStore { followupOne( id: string, message: string, - ): Promise<{ ok: true; reply: string } | { ok: false; status: AgentLifecycleStatus }>; + ): Promise< + { ok: true; reply: string } | { ok: false; status: AgentLifecycleStatus; hint?: string } + >; subscribe(listener: () => void): () => void; clear(): void; } @@ -190,9 +206,32 @@ export interface SubAgentSessionStore { export const DEFAULT_CANCEL_REASON = "Cancelled by operator"; const DEFAULT_MAX_COMPLETED = 20; +// CL-7007: sized for fan-out dispatch (dozens of spawn_agent workers), not a +// sidebar list — see maxRetained doc above. +const DEFAULT_MAX_RETAINED = 50; const DEFAULT_MAX_ENTRIES = 400; const DEFAULT_MAX_ENTRY_CHARS = 24_000; +const EVICTED_RETENTION_HINT = + "Session evicted to bound retained-session memory; recover full detail via read_agent_trace(agent_id)."; + +// Bound on tombstones kept for evicted sessions, so an unbounded stream of +// short-lived retained workers cannot grow this map forever either. +const MAX_EVICTED_TOMBSTONES = 500; + +/** Resolves a configured cap, guarding against non-finite values (including + * a JSON round-trip that turned a configured `Infinity` into `null`) so the + * cap can never silently collapse to `0`/`NaN`. */ +function resolveCap(value: number | undefined, fallback: number): number { + return value !== undefined && Number.isFinite(value) ? value : fallback; +} + +/** Terminal record for a session dropped from the store by retention eviction. */ +interface EvictedRecord { + lifecycleStatus: AgentLifecycleStatus; + hint: string; +} + let nextId = 0; function defaultCreateId(): string { nextId += 1; @@ -298,7 +337,8 @@ function stringifyUnknown(value: unknown): string { export function createSubAgentSessionStore( options: SubAgentSessionStoreOptions = {}, ): SubAgentSessionStore { - const maxCompleted = options.maxCompleted ?? DEFAULT_MAX_COMPLETED; + const maxCompleted = resolveCap(options.maxCompleted, DEFAULT_MAX_COMPLETED); + const maxRetained = resolveCap(options.maxRetained, DEFAULT_MAX_RETAINED); const maxEntries = options.maxEntries ?? DEFAULT_MAX_ENTRIES; const maxEntryChars = options.maxEntryChars ?? DEFAULT_MAX_ENTRY_CHARS; const now = options.now ?? (() => Date.now()); @@ -317,6 +357,23 @@ export function createSubAgentSessionStore( const interruptHandles = new Map void>(); const followupHandles = new Map Promise>(); const listeners = new Set<() => void>(); + // CL-7007: tombstones for sessions dropped by pruneRetained, keyed by id, + // insertion-ordered (Map preserves it) so the oldest can be dropped first + // once MAX_EVICTED_TOMBSTONES is exceeded. Lets resume_agent/followup_task + // report an actionable terminal status instead of a bare "not_found" for a + // session evicted purely to bound retention memory. + const evicted = new Map(); + + const recordEviction = (session: SubAgentSession): void => { + evicted.set(session.id, { + lifecycleStatus: session.lifecycleStatus, + hint: EVICTED_RETENTION_HINT, + }); + if (evicted.size > MAX_EVICTED_TOMBSTONES) { + const oldest = evicted.keys().next().value; + if (oldest !== undefined) evicted.delete(oldest); + } + }; // Per-session revision counters, bumped on every mutation. Notify fires on // every streamed child token, so list()/get()/listForStrip() would otherwise @@ -405,16 +462,25 @@ export function createSubAgentSessionStore( 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. + // An open retained session (spawn_agent's reusable-session contract: + // retained:true and still addressable — "completed" or "interrupted") is + // governed by pruneRetained's own cap below, not this one. + const isOpenRetained = (s: SubAgentSession): boolean => + s.retained === true && + (s.lifecycleStatus === "completed" || s.lifecycleStatus === "interrupted"); + + // CL-7001/CL-7007: `maxCompleted` bounds every ordinary finished session — + // one that was never retained, or a retained one already closed via + // close_agent (retained flips back to false there). It is a TUI display + // cap and was never sized to also be the retention policy for reusable + // sessions; open retained sessions are excluded here and bounded instead + // by pruneRetained. 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" && s.lifecycleStatus !== "running") { + if (s.status !== "running" && s.lifecycleStatus !== "running" && !isOpenRetained(s)) { releaseHandles(id); sessions.delete(id); forgetRevision(id); @@ -423,7 +489,9 @@ export function createSubAgentSessionStore( return; } const finished = [...sessions.values()] - .filter((s) => s.status !== "running" && s.lifecycleStatus !== "running") + .filter( + (s) => s.status !== "running" && s.lifecycleStatus !== "running" && !isOpenRetained(s), + ) .sort((a, b) => (a.finishedAt ?? 0) - (b.finishedAt ?? 0)); const excess = finished.length - maxCompleted; if (excess <= 0) return; @@ -437,6 +505,32 @@ export function createSubAgentSessionStore( } }; + // CL-7007: bounds open retained sessions (dozens-of-workers fan-out), the + // resource-safety bound CL-7002 removed by mistake when it folded retained + // sessions into pruneCompleted's TUI cap. Evicts least-recently-used first + // (by lastActivityAt); a session actively running again is never a + // candidate (excluded by isOpenRetained requiring "completed"/ + // "interrupted"). Handles are released exactly like pruneCompleted's + // eviction — sidecars, reactor, and the lock entry are not simply + // forgotten — and a tombstone is kept so resume_agent/followup_task can + // still report an actionable status afterward instead of "not_found". + const pruneRetained = (): void => { + const openRetained = [...sessions.values()] + .filter(isOpenRetained) + .sort((a, b) => a.lastActivityAt - b.lastActivityAt); + const excess = openRetained.length - maxRetained; + if (excess <= 0) return; + for (let i = 0; i < excess; i++) { + const drop = openRetained[i]; + if (drop !== undefined) { + releaseHandles(drop.id); + recordEviction(drop); + 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. @@ -711,6 +805,7 @@ export function createSubAgentSessionStore( cancelHandles.delete(id); if (!agentRetained) closeHandles.delete(id); pruneCompleted(); + pruneRetained(); }); }, @@ -765,7 +860,13 @@ export function createSubAgentSessionStore( async closeOne(id: string, deadlineMs: number): Promise { const session = sessions.get(id); - if (session === undefined) return "not_found"; + if (session === undefined) { + // CL-7007: an id evicted by pruneRetained already had its handles + // released — from close_agent's perspective that is indistinguishable + // from "already shut down", not a bad id. + if (evicted.has(id)) return "shutdown"; + return "not_found"; + } if (session.lifecycleStatus === "shutdown") return "shutdown"; let close = closeHandles.get(id); if (close === undefined) { @@ -831,15 +932,24 @@ export function createSubAgentSessionStore( mutate(id, (s) => { s.lifecycleStatus = "interrupted"; }); + pruneRetained(); return { ok: true }; }, async followupOne( id: string, message: string, - ): Promise<{ ok: true; reply: string } | { ok: false; status: AgentLifecycleStatus }> { + ): Promise< + { ok: true; reply: string } | { ok: false; status: AgentLifecycleStatus; hint?: string } + > { const session = sessions.get(id); - if (session === undefined) return { ok: false, status: "not_found" }; + if (session === undefined) { + const tombstone = evicted.get(id); + if (tombstone !== undefined) { + return { ok: false, status: tombstone.lifecycleStatus, hint: tombstone.hint }; + } + return { ok: false, status: "not_found" }; + } if ( session.retained !== true || (session.lifecycleStatus !== "completed" && session.lifecycleStatus !== "interrupted") @@ -856,12 +966,21 @@ export function createSubAgentSessionStore( s.report = reply; pushEntry(s, { kind: "report", content: capText(reply, maxEntryChars) }); }); + pruneRetained(); return { ok: true, reply }; }, - resumeOne(id: string): { ok: true } | { ok: false; status: AgentLifecycleStatus } { + resumeOne( + id: string, + ): { ok: true } | { ok: false; status: AgentLifecycleStatus; hint?: string } { const session = sessions.get(id); - if (session === undefined) return { ok: false, status: "not_found" }; + if (session === undefined) { + const tombstone = evicted.get(id); + if (tombstone !== undefined) { + return { ok: false, status: tombstone.lifecycleStatus, hint: tombstone.hint }; + } + return { ok: false, status: "not_found" }; + } if (session.lifecycleStatus !== "completed" || session.retained !== true) { return { ok: false, status: session.lifecycleStatus }; } @@ -917,6 +1036,7 @@ export function createSubAgentSessionStore( sessions.clear(); revisions.clear(); snapshotCache.clear(); + evicted.clear(); notify(); }, };