diff --git a/changelog.d/fixes/13095-acp-sendprompt-listener-leak.md b/changelog.d/fixes/13095-acp-sendprompt-listener-leak.md new file mode 100644 index 0000000000..56473cc8c2 --- /dev/null +++ b/changelog.d/fixes/13095-acp-sendprompt-listener-leak.md @@ -0,0 +1 @@ +- **fix(acp):** release the `stdout`/`exit` listeners and the idle timer that a `sendPrompt` timeout used to leave attached to the `acpManager` singleton, and drop sessions that exited on their own from the session map instead of keeping them forever. diff --git a/src/lib/acp/manager.ts b/src/lib/acp/manager.ts index 85bc05e720..85725fd5fc 100644 --- a/src/lib/acp/manager.ts +++ b/src/lib/acp/manager.ts @@ -90,6 +90,10 @@ export class AcpManager extends EventEmitter { child.on("exit", (code, signal) => { session.alive = false; + // Only kill() used to remove entries, so any agent that exited on its own + // stayed in the map forever. getActiveSessions() filters on `alive`, which + // hid the growth from callers. + this.sessions.delete(sessionId); this.emit("exit", { sessionId, code, signal }); }); @@ -129,31 +133,35 @@ export class AcpManager extends EventEmitter { // Wait for response (collect until process goes idle or timeout) return new Promise((resolve, reject) => { - const timer = setTimeout(() => { - reject(new Error(`ACP timeout after ${timeoutMs}ms`)); - }, timeoutMs); + let idleTimer: ReturnType | undefined; - let idleTimer: ReturnType; + // Every outcome -- idle, exit, or timeout -- has to release the same + // resources. `acpManager` is a module-level singleton, so a branch that + // skips this leaks a listener per call for the lifetime of the process. + const settle = (finish: () => void) => { + clearTimeout(timer); + clearTimeout(idleTimer); + this.removeListener("stdout", onData); + this.removeListener("exit", onExit); + finish(); + }; + + const timer = setTimeout(() => { + settle(() => reject(new Error(`ACP timeout after ${timeoutMs}ms`))); + }, timeoutMs); const onData = ({ sessionId: sid }: { sessionId: string }) => { if (sid !== sessionId) return; // Reset idle timer on new data clearTimeout(idleTimer); idleTimer = setTimeout(() => { - clearTimeout(timer); - this.removeListener("stdout", onData); - this.removeListener("exit", onExit); - resolve(session.stdoutBuffer); + settle(() => resolve(session.stdoutBuffer)); }, 2000); // 2s idle = response complete }; const onExit = ({ sessionId: sid }: { sessionId: string }) => { if (sid !== sessionId) return; - clearTimeout(timer); - clearTimeout(idleTimer); - this.removeListener("stdout", onData); - this.removeListener("exit", onExit); - resolve(session.stdoutBuffer); + settle(() => resolve(session.stdoutBuffer)); }; this.on("stdout", onData); diff --git a/tests/unit/acp-manager-sendprompt-leak-13095.test.ts b/tests/unit/acp-manager-sendprompt-leak-13095.test.ts new file mode 100644 index 0000000000..d3b5dce291 --- /dev/null +++ b/tests/unit/acp-manager-sendprompt-leak-13095.test.ts @@ -0,0 +1,101 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { AcpManager } = await import("../../src/lib/acp/manager.ts"); +const { setCustomAgents } = await import("../../src/lib/acp/registry.ts"); + +// A registered agent whose binary is just node running a script that stays quiet, +// so sendPrompt() reliably hits its timeout instead of resolving on data/exit. +const AGENT_ID = "acp-leak-probe"; +setCustomAgents([ + { + id: AGENT_ID, + name: "ACP leak probe", + binary: process.execPath, + description: "test-only agent", + }, +]); + +function spawnIdleSession(manager) { + // Keeps stdin open and never writes to stdout: the prompt can only time out. + return manager.spawn(AGENT_ID, process.execPath, [ + "-e", + "process.stdin.resume(); setTimeout(() => {}, 60_000);", + ]); +} + +test("sendPrompt timeout does not leak listeners on the manager (#13095)", async () => { + const manager = new AcpManager(); + const session = spawnIdleSession(manager); + + try { + const before = { + stdout: manager.listenerCount("stdout"), + exit: manager.listenerCount("exit"), + }; + + // Each of these must reject on the timeout path. + for (let i = 0; i < 12; i++) { + await assert.rejects( + () => manager.sendPrompt(session.id, "ping", 15), + /ACP timeout after 15ms/, + `attempt ${i + 1} should time out` + ); + } + + // The timeout branch has to tear down both listeners it registered. Before the + // fix these grew by one per timed-out prompt and were never released, which + // matters because `acpManager` is a module-level singleton. + assert.equal( + manager.listenerCount("stdout"), + before.stdout, + "stdout listeners must return to the pre-prompt count" + ); + assert.equal( + manager.listenerCount("exit"), + before.exit, + "exit listeners must return to the pre-prompt count" + ); + } finally { + manager.killAll(); + } +}); + +test("sendPrompt timeout clears its idle timer so the process can settle (#13095)", async () => { + const manager = new AcpManager(); + const session = spawnIdleSession(manager); + + try { + await assert.rejects( + () => manager.sendPrompt(session.id, "ping", 15), + /ACP timeout after 15ms/ + ); + + // A leaked idle timer keeps a 2s handle (and the captured session) alive after + // the promise already rejected. Nothing should be pending on the manager. + assert.equal(manager.listenerCount("stdout"), 0); + assert.equal(manager.listenerCount("exit"), 0); + } finally { + manager.killAll(); + } +}); + +test("exited sessions are removed from the session map (#13095)", async () => { + const manager = new AcpManager(); + // Exits immediately on its own; nothing calls kill() for it. + const session = manager.spawn(AGENT_ID, process.execPath, ["-e", "process.exit(0)"]); + + await new Promise((resolve) => { + manager.on("exit", ({ sessionId }) => { + if (sessionId === session.id) resolve(); + }); + }); + // Let the exit handler finish its bookkeeping. + await new Promise((resolve) => setTimeout(resolve, 50)); + + assert.equal( + manager.getSession(session.id), + undefined, + "a session that exited on its own must not stay in the map" + ); +});