diff --git a/browse/src/error-handling.ts b/browse/src/error-handling.ts index 90bfb17f2..6c0247a04 100644 --- a/browse/src/error-handling.ts +++ b/browse/src/error-handling.ts @@ -7,8 +7,6 @@ import * as fs from 'fs'; -const IS_WINDOWS = process.platform === 'win32'; - // ─── Filesystem ──────────────────────────────────────────────── /** Remove a file, ignoring ENOENT (already gone). Rethrows other errors. */ @@ -39,24 +37,32 @@ export function safeKill(pid: number, signal: NodeJS.Signals | number): void { /** * Check if a PID is alive. Pure boolean probe — never throws. * + * Signal 0 on EVERY platform (#1952). Node maps `process.kill(pid, 0)` to an + * OpenProcess existence check on Windows — and on Windows the browse daemon + * runs under Node (dist/server-node.mjs + bun-polyfill, the documented + * fallback for oven-sh/bun#4253) — so the POSIX idiom is portable here. + * + * Windows used to shell out to `tasklist /FI "PID eq "` and + * string-match the CSV. That was wrong in two ways, both hit in production: + * + * 1. FALSE NEGATIVES UNDER LOAD (#2414/#2295): tasklist takes ~700-1700ms + * on an idle box and far longer under memory pressure. A Bun.spawnSync + * that hits its `timeout` still RETURNS, carrying partial stdout — so + * the `.includes()` match came back false and a LIVE process was + * reported dead. Callers that validate liveness before killing + * (killAgentByRecord, the terminal-agent watchdog) then skipped the + * kill and respawned around the survivor — one leaked terminal-agent + * per tick, self-reinforcing (each orphan slows the next tasklist). + * 2. A console window per probe (#1952): the watchdog blinked a conhost + * window into the foreground every 60s for the whole session. + * + * Signal 0 spawns nothing, cannot time out, and is orders of magnitude + * faster (~0.004ms vs ~270ms measured in #2414). + * * EPERM means the process EXISTS but we lack rights to signal it. That is - * alive — returning false there makes callers that validate liveness before - * killing (killAgentByRecord, the terminal-agent watchdog) skip the kill and - * respawn around a survivor, leaking one process per watchdog tick (#2414, - * #2295). + * alive — returning false there would reintroduce failure mode 1. */ export function isProcessAlive(pid: number): boolean { - if (IS_WINDOWS) { - try { - const result = Bun.spawnSync( - ['tasklist', '/FI', `PID eq ${pid}`, '/NH', '/FO', 'CSV'], - { stdout: 'pipe', stderr: 'pipe', timeout: 3000, windowsHide: true } - ); - return result.stdout.toString().includes(`"${pid}"`); - } catch { - return false; - } - } try { process.kill(pid, 0); return true; diff --git a/browse/test/process-liveness-windows.test.ts b/browse/test/process-liveness-windows.test.ts index d474473dc..104f23d71 100644 --- a/browse/test/process-liveness-windows.test.ts +++ b/browse/test/process-liveness-windows.test.ts @@ -54,16 +54,15 @@ describe('process liveness probe (Windows terminal-agent leak)', () => { expect(isProcessAlive(2147483646)).toBe(false); }); - test('3. isProcessAlive spawns NO subprocess on POSIX (signal-0 path)', () => { + test('3. isProcessAlive spawns NO subprocess on ANY platform (signal-0, #1952)', () => { // The heart of the bug: a liveness probe that forks is slow enough to // time out, and a timed-out probe silently answers "dead". Signal 0 - // cannot time out because it never leaves the process. - // - // Merged design note: on win32 the helper DOES keep a single hardened - // tasklist probe (windowsHide, bounded timeout, quoted-CSV PID match) - // because Bun's process.kill(pid, 0) throws ESRCH for live Windows PIDs - // in compiled binaries. The POSIX path stays subprocess-free. - if (process.platform === 'win32') return; + // cannot time out because it never leaves the process. Node maps + // process.kill(pid, 0) to an OpenProcess existence check on Windows — + // and the Windows daemon runs under Node (server-node.mjs + + // bun-polyfill), so the POSIX idiom is portable and the win32 tasklist + // branch is GONE (it caused both the false negatives above and the + // per-tick console flash of #1952). const origSpawn = (Bun as any).spawn; const origSpawnSync = (Bun as any).spawnSync; const spawns: string[] = []; @@ -79,16 +78,14 @@ describe('process liveness probe (Windows terminal-agent leak)', () => { } }); - test('4. no source file probes liveness via tasklist outside the central helper', () => { - // Static tripwire: ad-hoc tasklist existence checks scattered across src/ - // resurrect the false-negative class (each call site re-invents the - // timeout/parse handling and gets it subtly wrong). The ONE sanctioned - // site is error-handling.ts's isProcessAlive win32 branch — centralized, - // windowsHide, bounded timeout, quoted-CSV `"${pid}"` match. Every other - // file must route through the helper. + test('4. no source file probes liveness via tasklist — signal-0 is the only probe (#1952)', () => { + // Static tripwire: a tasklist existence check ANYWHERE in src/ + // resurrects both the false-negative class (#2414: a timed-out spawnSync + // still returns, with partial stdout, so a live process reads as dead) + // and the per-tick console flash (#1952). isProcessAlive uses + // process.kill(pid, 0) on every platform; nothing gets an exemption. const offenders: string[] = []; for (const { file, content } of readAllSourceFiles()) { - if (file === 'error-handling.ts') continue; // the canonical helper const code = stripComments(content); // `PID eq` is the existence-probe form specifically. Other tasklist // uses (e.g. IMAGENAME filters for browser detection) are unaffected. diff --git a/browse/test/windows-spawn-hide.test.ts b/browse/test/windows-spawn-hide.test.ts index 086a28657..58eb59640 100644 --- a/browse/test/windows-spawn-hide.test.ts +++ b/browse/test/windows-spawn-hide.test.ts @@ -39,8 +39,9 @@ describe('windowsHide on Windows-reachable spawns (#1835)', () => { }); test('Windows-only process probes pass windowsHide', () => { - // tasklist in isProcessAlive — runs in polling loops. - expectHideNearEvery(SRC('error-handling.ts'), "'tasklist'"); + // isProcessAlive no longer spawns anything (signal-0 on every platform, + // #1952) — process-liveness-windows.test.ts pins that it stays + // subprocess-free, which is stronger than hiding a window. // powershell DPAPI + tasklist in cookie import. const cookie = SRC('cookie-import-browser.ts'); expectHideNearEvery(cookie, "'powershell'");