diff --git a/changelog.d/fixes/13095-acp-buffer-cap.md b/changelog.d/fixes/13095-acp-buffer-cap.md new file mode 100644 index 0000000000..61473dca6d --- /dev/null +++ b/changelog.d/fixes/13095-acp-buffer-cap.md @@ -0,0 +1 @@ +- **fix(acp):** bound the ACP session output buffers — `stdoutBuffer` and `stderrBuffer` now cap at 1 MiB keeping the most recent output behind a visible `[...output truncated...]` marker, and `stderrBuffer` is reset per prompt instead of accumulating for the lifetime of the session. diff --git a/src/lib/acp/manager.ts b/src/lib/acp/manager.ts index 85725fd5fc..099239e9b1 100644 --- a/src/lib/acp/manager.ts +++ b/src/lib/acp/manager.ts @@ -30,6 +30,34 @@ export interface AcpSession { createdAt: Date; } +/** + * Upper bound for each per-session output buffer. + * + * Both buffers grow on every chunk a CLI agent writes and are only reset when + * the next prompt starts, so a chatty or looping agent can grow them without + * limit while the session stays alive. 1 MiB is far above a realistic agent + * response while keeping a stuck session's footprint bounded. + */ +const MAX_BUFFER_CHARS = 1_048_576; + +const TRUNCATION_NOTICE = "\n[...output truncated...]\n"; + +/** + * Append to a buffer, keeping the most recent output when the cap is exceeded. + * + * The tail is what callers care about: `sendPrompt` resolves with the stdout + * collected since the prompt was written, and stderr is read for diagnostics + * after a failure. Dropping from the front keeps both useful. + */ +function appendCapped(buffer: string, chunk: string): string { + const combined = buffer + chunk; + if (combined.length <= MAX_BUFFER_CHARS) return combined; + + const keep = MAX_BUFFER_CHARS - TRUNCATION_NOTICE.length; + if (keep <= 0) return combined.slice(-MAX_BUFFER_CHARS); + return TRUNCATION_NOTICE + combined.slice(-keep); +} + /** * ACP Session Manager * @@ -79,12 +107,12 @@ export class AcpManager extends EventEmitter { }; child.stdout?.on("data", (chunk: Buffer) => { - session.stdoutBuffer += chunk.toString(); + session.stdoutBuffer = appendCapped(session.stdoutBuffer, chunk.toString()); this.emit("stdout", { sessionId, data: chunk.toString() }); }); child.stderr?.on("data", (chunk: Buffer) => { - session.stderrBuffer += chunk.toString(); + session.stderrBuffer = appendCapped(session.stderrBuffer, chunk.toString()); this.emit("stderr", { sessionId, data: chunk.toString() }); }); @@ -125,8 +153,11 @@ export class AcpManager extends EventEmitter { const session = this.sessions.get(sessionId); if (!session?.alive) throw new Error(`Session ${sessionId} is not alive`); - // Clear buffer before sending + // Clear buffers before sending. stderr is reset too: it was previously only + // ever appended to, so diagnostics for one prompt carried stale output from + // every earlier prompt in the session. session.stdoutBuffer = ""; + session.stderrBuffer = ""; // Send prompt this.sendInput(sessionId, prompt + "\n"); diff --git a/tests/unit/acp-manager-buffer-cap-13095.test.ts b/tests/unit/acp-manager-buffer-cap-13095.test.ts new file mode 100644 index 0000000000..31ac053625 --- /dev/null +++ b/tests/unit/acp-manager-buffer-cap-13095.test.ts @@ -0,0 +1,143 @@ +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"); + +const AGENT_ID = "buffer-cap-probe"; +const CAP = 1_048_576; + +/** + * Spawn a node process that writes `bytes` of stdout (or stderr) and stays alive, + * so the buffers can be inspected while the session is still running. + */ +function makeAgent(stream: "stdout" | "stderr", bytes: number) { + setCustomAgents([ + { + id: AGENT_ID, + name: "Buffer cap probe", + binary: process.execPath, + acpSpawnable: true, + }, + ]); + const script = ` + const chunk = "x".repeat(64 * 1024); + let written = 0; + const target = ${bytes}; + while (written < target) { + process.${stream}.write(chunk); + written += chunk.length; + } + setInterval(() => {}, 1000); + `; + return ["-e", script]; +} + +async function waitForOutput(session: { stdoutBuffer: string; stderrBuffer: string }) { + // Give the child time to flush everything it intends to write. + for (let i = 0; i < 60; i++) { + await new Promise((r) => setTimeout(r, 50)); + if (session.stdoutBuffer.length > CAP / 2 || session.stderrBuffer.length > CAP / 2) break; + } + await new Promise((r) => setTimeout(r, 300)); +} + +test("stdout buffer stays bounded when an agent floods it (#13095)", async () => { + const mgr = new AcpManager(); + const session = mgr.spawn(AGENT_ID, process.execPath, makeAgent("stdout", 4 * CAP)); + try { + await waitForOutput(session); + assert.ok( + session.stdoutBuffer.length > 0, + "precondition: the probe agent must have written something" + ); + assert.ok( + session.stdoutBuffer.length <= CAP, + `stdoutBuffer grew to ${session.stdoutBuffer.length} chars, above the ${CAP} cap` + ); + } finally { + mgr.kill(session.id); + } +}); + +test("stderr buffer stays bounded when an agent floods it (#13095)", async () => { + const mgr = new AcpManager(); + const session = mgr.spawn(AGENT_ID, process.execPath, makeAgent("stderr", 4 * CAP)); + try { + await waitForOutput(session); + assert.ok( + session.stderrBuffer.length > 0, + "precondition: the probe agent must have written something" + ); + assert.ok( + session.stderrBuffer.length <= CAP, + `stderrBuffer grew to ${session.stderrBuffer.length} chars, above the ${CAP} cap` + ); + } finally { + mgr.kill(session.id); + } +}); + +test("truncation keeps the most recent output, not the oldest (#13095)", async () => { + setCustomAgents([ + { + id: AGENT_ID, + name: "Buffer cap probe", + binary: process.execPath, + acpSpawnable: true, + }, + ]); + const script = ` + const chunk = "x".repeat(64 * 1024); + let written = 0; + while (written < ${2 * CAP}) { process.stdout.write(chunk); written += chunk.length; } + process.stdout.write("FINAL-MARKER"); + setInterval(() => {}, 1000); + `; + const mgr = new AcpManager(); + const session = mgr.spawn(AGENT_ID, process.execPath, ["-e", script]); + try { + await waitForOutput(session); + // The tail is the part callers use: sendPrompt resolves with stdout, and + // stderr is read for diagnostics after a failure. + assert.ok( + session.stdoutBuffer.endsWith("FINAL-MARKER"), + "the newest output must survive truncation" + ); + assert.ok(session.stdoutBuffer.length <= CAP, "buffer must still respect the cap"); + } finally { + mgr.kill(session.id); + } +}); + +test("stderr is reset between prompts so diagnostics are per-prompt (#13095)", async () => { + setCustomAgents([ + { + id: AGENT_ID, + name: "Buffer cap probe", + binary: process.execPath, + acpSpawnable: true, + }, + ]); + // Echoes stdin back on stdout, and writes a fixed line to stderr per prompt. + const script = ` + process.stdin.on("data", (d) => { + process.stderr.write("warn:" + d.toString().trim() + "\\n"); + process.stdout.write("ok\\n"); + }); + setInterval(() => {}, 1000); + `; + const mgr = new AcpManager(); + const session = mgr.spawn(AGENT_ID, process.execPath, ["-e", script]); + try { + await mgr.sendPrompt(session.id, "first", 6000); + await mgr.sendPrompt(session.id, "second", 6000); + assert.ok( + !session.stderrBuffer.includes("warn:first"), + `stderr from an earlier prompt leaked into the next one: ${JSON.stringify(session.stderrBuffer)}` + ); + assert.ok(session.stderrBuffer.includes("warn:second"), "current prompt's stderr must be kept"); + } finally { + mgr.kill(session.id); + } +});