mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-18 12:52:25 +03:00
fix(acp): release listeners, timers and sessions on every sendPrompt outcome (#13096)
`acpManager` being a module-level singleton is what turns this from a per-call leak into unbounded growth — the `MaxListenersExceededWarning` at 11 is the visible symptom. Routing every outcome through one `settle()` is the right shape, and deleting the session on the child's own exit fixes the map growth that `getActiveSessions()`'s `alive` filter was hiding. 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-sendprompt-listener-leak.md
Normal file
1
changelog.d/fixes/13095-acp-sendprompt-listener-leak.md
Normal file
@@ -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.
|
||||
@@ -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<typeof setTimeout> | undefined;
|
||||
|
||||
let idleTimer: ReturnType<typeof setTimeout>;
|
||||
// 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);
|
||||
|
||||
101
tests/unit/acp-manager-sendprompt-leak-13095.test.ts
Normal file
101
tests/unit/acp-manager-sendprompt-leak-13095.test.ts
Normal file
@@ -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"
|
||||
);
|
||||
});
|
||||
Reference in New Issue
Block a user