mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-17 20:32:25 +03:00
fix(acp): bound session output buffers and reset stderr per prompt (#13100)
Keeping the tail is the right direction — `sendPrompt` resolves with the stdout collected since the prompt was written and stderr is read for diagnostics after a failure, so the newest output is what callers actually use. The `[...output truncated...]` marker keeps it from being silent. Resetting `stderrBuffer` alongside `stdoutBuffer` fixes the subtler half: diagnostics for one prompt were carrying stale output from every earlier one. Verified on the tree that actually ships — your branch merged onto the current tip, which already carries #13096: `appendCapped()` and `settle()` coexist cleanly and all 7 assertions across both ACP test files pass together. I reformatted the changelog fragment to the `changelog.d` convention (`- **fix(scope):** …`) before merging — `check:changelog-integrity` rejects a fragment that does not start with a markdown bullet, which is the same gate your #13158 was about. Wording is yours, unchanged in substance. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit.
This commit is contained in:
1
changelog.d/fixes/13095-acp-buffer-cap.md
Normal file
1
changelog.d/fixes/13095-acp-buffer-cap.md
Normal file
@@ -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.
|
||||
@@ -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");
|
||||
|
||||
143
tests/unit/acp-manager-buffer-cap-13095.test.ts
Normal file
143
tests/unit/acp-manager-buffer-cap-13095.test.ts
Normal file
@@ -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);
|
||||
}
|
||||
});
|
||||
Reference in New Issue
Block a user