mirror of
https://github.com/mksglu/context-mode.git
synced 2026-10-02 04:14:38 +08:00
fix(ctx_insight): cap helper spawnSync + locale-independent Windows netstat parser (#469)
Two follow-ups to #441 landing on `next`: 1. **Hard timeout on every helper-internal spawnSync.** A hung `lsof` / `xdg-open` / `taskkill` would otherwise block the MCP tool indefinitely (e.g. xdg-open waiting on an X11 session, lsof stalling on /proc, taskkill hanging on an unresponsive process). Cap added at 5 s via `HELPER_SPAWN_TIMEOUT_MS`; the helper surfaces a diagnostic instead of stalling the agent. 2. **Locale-independent Windows netstat parser.** The previous parser keyed off `state !== "LISTENING"`, which is locale-translated (Windows-FR shows `À l'écoute`, Windows-DE `ABHÖREN`, Windows-ES `ESCUCHANDO`, etc.). On non-EN Windows, the helper silently matched zero rows and never freed a stuck dashboard port. The remote-address column is NOT translated — a listening TCP socket always has remote `0.0.0.0:0` (IPv4) or `[::]:0` (IPv6). We anchor on that instead of the state string. PID is now read as `tokens[tokens.length - 1]` because translated state names can themselves contain whitespace (`À l'écoute` splits into two tokens). Tests added in `tests/core/server.test.ts`: - 3 cases asserting the timeout option is forwarded to the runner for openBrowserSync, killProcessOnPort (Linux), killProcessOnPort (Windows). - 3 Windows-locale fixtures (FR `À l'écoute`, DE `ABHÖREN`, plus a pure-ESTABLISHED sanity case) confirming the new predicate kills the listening PIDs while still rejecting the ESTABLISHED row whose remote port matches — i.e. the original #441 regression remains fixed. The existing `killProcessOnPort — Windows (netstat)` block (en-US fixture with the must-not-kill remote-port row) and the security-grep guards still pass unchanged. Test plan: - `npm run build` - `npm test` — 2390 pass / 25 skipped, 73 files green - `npm run typecheck` — clean Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ben Younes
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
82690b8b99
commit
1f70bee0ec
+41
-10
@@ -2894,6 +2894,14 @@ export type KillResult = {
|
||||
errors: string[];
|
||||
};
|
||||
|
||||
// Hard upper bound on every helper-internal spawnSync call. Caps tail-latency
|
||||
// when an external binary hangs (xdg-open waiting for an X11 session, lsof
|
||||
// stalling on /proc, taskkill blocking on an unresponsive process, etc.) so
|
||||
// the MCP tool surfaces a diagnostic instead of blocking the agent loop.
|
||||
// 5s is comfortably above the 99th-percentile completion of every command we
|
||||
// invoke; anything past that is hung.
|
||||
const HELPER_SPAWN_TIMEOUT_MS = 5000;
|
||||
|
||||
// Returns the argv attempts for opening `url` on `platform`, in fall-back order.
|
||||
// Pure data — no I/O.
|
||||
export function browserOpenArgv(
|
||||
@@ -2925,7 +2933,7 @@ export function openBrowserSync(
|
||||
const errors: string[] = [];
|
||||
for (const { cmd, args } of attempts) {
|
||||
try {
|
||||
const r = runner(cmd, args, { stdio: "ignore" });
|
||||
const r = runner(cmd, args, { stdio: "ignore", timeout: HELPER_SPAWN_TIMEOUT_MS });
|
||||
// Treat signal-kill (status === null) and any non-zero status as failure
|
||||
// so the next fallback fires.
|
||||
if (!r.error && r.status === 0) return { ok: true, method: cmd };
|
||||
@@ -2942,10 +2950,15 @@ export function openBrowserSync(
|
||||
// the caller can distinguish between (a) port was free, (b) kill succeeded,
|
||||
// (c) kill failed (perms, missing binary, or per-pid failure mid-loop).
|
||||
//
|
||||
// On Windows the netstat parser is anchored on the LOCAL address column and
|
||||
// the LISTENING state — required to avoid cross-matching a remote-port column
|
||||
// and force-killing unrelated processes that happen to have an outbound
|
||||
// connection to the same port number.
|
||||
// On Windows the netstat parser is locale-independent: the STATE column
|
||||
// ("LISTENING" / "ESTABLISHED" / ...) is translated on non-English Windows
|
||||
// (Windows-FR shows "À l'écoute", Windows-DE "ABHÖREN", etc.), but the REMOTE
|
||||
// ADDRESS column is not. A listening TCP socket always has remote
|
||||
// "0.0.0.0:0" (IPv4) or "[::]:0" (IPv6); a connected one has a real
|
||||
// addr:port. We therefore key off the remote column instead of the state
|
||||
// string. This also rules out the pre-fix bug where matching only the local
|
||||
// port number cross-matched a remote :port from an outbound connection and
|
||||
// taskkill'd an unrelated process.
|
||||
export function killProcessOnPort(
|
||||
port: number,
|
||||
platform: NodeJS.Platform = process.platform,
|
||||
@@ -2962,6 +2975,7 @@ export function killProcessOnPort(
|
||||
const r = runner("netstat", ["-ano"], {
|
||||
encoding: "utf-8",
|
||||
stdio: ["ignore", "pipe", "ignore"],
|
||||
timeout: HELPER_SPAWN_TIMEOUT_MS,
|
||||
});
|
||||
if (r.error) {
|
||||
result.errors.push(`netstat: ${r.error.message}`);
|
||||
@@ -2975,19 +2989,32 @@ export function killProcessOnPort(
|
||||
const line = rawLine.trim();
|
||||
if (!line) continue;
|
||||
const tokens = line.split(/\s+/);
|
||||
// netstat -ano LISTENING row: "TCP 0.0.0.0:4747 0.0.0.0:0 LISTENING 1234"
|
||||
// netstat -ano LISTENING row (en-US): "TCP 0.0.0.0:4747 0.0.0.0:0 LISTENING 1234"
|
||||
// The STATE column is locale-translated and may itself contain spaces
|
||||
// (Windows-FR `À l'écoute` splits into two tokens), so we cannot index
|
||||
// STATE by position. PID is always the trailing column; PROTO/LOCAL/
|
||||
// REMOTE are the first three. We anchor on those + a remote-wildcard
|
||||
// check that's locale-independent.
|
||||
if (tokens.length < 5) continue;
|
||||
const [proto, local, , state, pid] = tokens;
|
||||
const proto = tokens[0];
|
||||
const local = tokens[1];
|
||||
const remote = tokens[2];
|
||||
const pid = tokens[tokens.length - 1];
|
||||
if (proto !== "TCP") continue;
|
||||
if (state !== "LISTENING") continue;
|
||||
if (!local.endsWith(portSuffix)) continue;
|
||||
// Listening sockets carry a wildcard remote; anything else is a
|
||||
// connection (and matching it would kill an unrelated process).
|
||||
if (remote !== "0.0.0.0:0" && remote !== "[::]:0") continue;
|
||||
if (!/^\d+$/.test(pid)) continue;
|
||||
pids.add(pid);
|
||||
}
|
||||
for (const pid of pids) {
|
||||
result.attemptedPids.push(pid);
|
||||
try {
|
||||
const k = runner("taskkill", ["/F", "/PID", pid], { stdio: "ignore" });
|
||||
const k = runner("taskkill", ["/F", "/PID", pid], {
|
||||
stdio: "ignore",
|
||||
timeout: HELPER_SPAWN_TIMEOUT_MS,
|
||||
});
|
||||
if (k.error || k.status !== 0) {
|
||||
result.errors.push(
|
||||
`taskkill ${pid}: ${k.error?.message ?? `status=${k.status}`}`,
|
||||
@@ -3003,6 +3030,7 @@ export function killProcessOnPort(
|
||||
const r = runner("lsof", ["-ti", `:${port}`], {
|
||||
encoding: "utf-8",
|
||||
stdio: ["ignore", "pipe", "ignore"],
|
||||
timeout: HELPER_SPAWN_TIMEOUT_MS,
|
||||
});
|
||||
if (r.error) {
|
||||
// ENOENT (lsof not installed) is a real diagnostic; surface it.
|
||||
@@ -3016,7 +3044,10 @@ export function killProcessOnPort(
|
||||
for (const pid of pids) {
|
||||
result.attemptedPids.push(pid);
|
||||
try {
|
||||
const k = runner("kill", [pid], { stdio: "ignore" });
|
||||
const k = runner("kill", [pid], {
|
||||
stdio: "ignore",
|
||||
timeout: HELPER_SPAWN_TIMEOUT_MS,
|
||||
});
|
||||
if (k.error || k.status !== 0) {
|
||||
result.errors.push(
|
||||
`kill ${pid}: ${k.error?.message ?? `status=${k.status}`}`,
|
||||
|
||||
@@ -3881,3 +3881,134 @@ describe("killProcessOnPort — input validation", () => {
|
||||
expect(r.errors.join(" ")).toMatch(/invalid port/);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── ctx_insight helper follow-ups (#441 follow-up) ──────────────────────────
|
||||
//
|
||||
// 1. Hard timeout on every helper-internal spawnSync. A hung lsof/xdg-open/
|
||||
// taskkill would otherwise block the MCP tool indefinitely. We assert the
|
||||
// timeout option is propagated to the runner — the actual cap value is an
|
||||
// implementation detail, but its presence is the regression we lock.
|
||||
//
|
||||
// 2. Windows non-English locale support. The pre-followup parser keyed off
|
||||
// `state !== "LISTENING"` which is locale-translated (Windows-FR shows
|
||||
// `À l'écoute`, Windows-DE `ABHÖREN`, Windows-ES `ESCUCHANDO`, etc.), so
|
||||
// on non-EN Windows the helper would silently match zero rows and never
|
||||
// free a stuck dashboard port. The remote-address column is NOT
|
||||
// locale-translated; we now key off it instead.
|
||||
|
||||
describe("Helper spawnSync timeout (#441 follow-up)", () => {
|
||||
test("openBrowserSync passes a timeout to the runner", () => {
|
||||
const { runner, calls } = makeRunner([{ status: 0 }]);
|
||||
openBrowserSync("http://x", "darwin", runner);
|
||||
|
||||
expect(calls).toHaveLength(1);
|
||||
expect(calls[0].opts).toBeDefined();
|
||||
expect(typeof (calls[0].opts as { timeout?: number }).timeout).toBe("number");
|
||||
expect((calls[0].opts as { timeout: number }).timeout).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
test("killProcessOnPort (Linux) passes a timeout to lsof and kill", () => {
|
||||
const { runner, calls } = makeRunner([
|
||||
{ status: 0, stdout: "1234\n" },
|
||||
{ status: 0 }, // kill 1234
|
||||
]);
|
||||
killProcessOnPort(4747, "linux", runner);
|
||||
|
||||
expect(calls).toHaveLength(2);
|
||||
for (const c of calls) {
|
||||
expect(typeof (c.opts as { timeout?: number }).timeout).toBe("number");
|
||||
expect((c.opts as { timeout: number }).timeout).toBeGreaterThan(0);
|
||||
}
|
||||
});
|
||||
|
||||
test("killProcessOnPort (Windows) passes a timeout to netstat and taskkill", () => {
|
||||
const winFixture = [
|
||||
" Proto Local Address Foreign Address State PID",
|
||||
" TCP 0.0.0.0:4747 0.0.0.0:0 LISTENING 1234",
|
||||
"",
|
||||
].join("\r\n");
|
||||
const { runner, calls } = makeRunner([
|
||||
{ status: 0, stdout: winFixture },
|
||||
{ status: 0 }, // taskkill 1234
|
||||
]);
|
||||
killProcessOnPort(4747, "win32", runner);
|
||||
|
||||
expect(calls.map(c => c.cmd)).toEqual(["netstat", "taskkill"]);
|
||||
for (const c of calls) {
|
||||
expect(typeof (c.opts as { timeout?: number }).timeout).toBe("number");
|
||||
expect((c.opts as { timeout: number }).timeout).toBeGreaterThan(0);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("killProcessOnPort — Windows non-English locale (#441 follow-up)", () => {
|
||||
// Same regression fixture as the en-US Windows test, but with the STATE
|
||||
// column translated. Pre-followup, the helper required `state === "LISTENING"`
|
||||
// and so silently produced zero PIDs on non-EN Windows. The follow-up keys
|
||||
// off the remote-address column (locale-independent) so this fixture must
|
||||
// now match LISTENING rows 1234 + 1235 and skip the ESTABLISHED row 9876
|
||||
// and the UDP row 5555.
|
||||
const netstatFr = [
|
||||
"Connexions actives",
|
||||
"",
|
||||
" Proto Adresse locale Adresse distante État PID",
|
||||
" TCP 0.0.0.0:4747 0.0.0.0:0 À l'écoute 1234",
|
||||
" TCP 192.168.1.5:54321 8.8.8.8:4747 ESTABLI 9876",
|
||||
" UDP 0.0.0.0:4747 *:* 5555",
|
||||
" TCP [::]:4747 [::]:0 À l'écoute 1235",
|
||||
"",
|
||||
].join("\r\n");
|
||||
|
||||
const netstatDe = [
|
||||
"Aktive Verbindungen",
|
||||
"",
|
||||
" Proto Lokale Adresse Remoteadresse Status PID",
|
||||
" TCP 0.0.0.0:4747 0.0.0.0:0 ABHÖREN 1234",
|
||||
" TCP 192.168.1.5:54321 8.8.8.8:4747 HERGESTELLT 9876",
|
||||
" TCP [::]:4747 [::]:0 ABHÖREN 1235",
|
||||
"",
|
||||
].join("\r\n");
|
||||
|
||||
test("French Windows netstat output: kills 1234 and 1235, ignores 9876 + 5555", () => {
|
||||
const { runner } = makeRunner([
|
||||
{ status: 0, stdout: netstatFr },
|
||||
{ status: 0 }, // taskkill 1234
|
||||
{ status: 0 }, // taskkill 1235
|
||||
]);
|
||||
const r = killProcessOnPort(4747, "win32", runner);
|
||||
|
||||
expect(r.attemptedPids).toEqual(expect.arrayContaining(["1234", "1235"]));
|
||||
expect(r.attemptedPids).not.toContain("9876");
|
||||
expect(r.attemptedPids).not.toContain("5555");
|
||||
expect(r.killedPids).toEqual(expect.arrayContaining(["1234", "1235"]));
|
||||
});
|
||||
|
||||
test("German Windows netstat output: kills 1234 and 1235, ignores 9876", () => {
|
||||
const { runner } = makeRunner([
|
||||
{ status: 0, stdout: netstatDe },
|
||||
{ status: 0 }, // taskkill 1234
|
||||
{ status: 0 }, // taskkill 1235
|
||||
]);
|
||||
const r = killProcessOnPort(4747, "win32", runner);
|
||||
|
||||
expect(r.attemptedPids).toEqual(expect.arrayContaining(["1234", "1235"]));
|
||||
expect(r.attemptedPids).not.toContain("9876");
|
||||
expect(r.killedPids).toEqual(expect.arrayContaining(["1234", "1235"]));
|
||||
});
|
||||
|
||||
test("a connected remote :port still does NOT match (remote-column anchor)", () => {
|
||||
// Sanity: the predicate must still reject the ESTABLISHED row whose
|
||||
// REMOTE is 8.8.8.8:4747. This was the original pre-#441 bug; the
|
||||
// follow-up must not regress it while changing the locale strategy.
|
||||
const onlyEstablished = [
|
||||
" Proto Local Address Foreign Address State PID",
|
||||
" TCP 192.168.1.5:54321 8.8.8.8:4747 ESTABLISHED 9876",
|
||||
"",
|
||||
].join("\r\n");
|
||||
const { runner, calls } = makeRunner([{ status: 0, stdout: onlyEstablished }]);
|
||||
const r = killProcessOnPort(4747, "win32", runner);
|
||||
|
||||
expect(r.attemptedPids).toEqual([]);
|
||||
expect(calls).toHaveLength(1); // netstat only — no taskkill issued
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user