diff --git a/browse/src/error-handling.ts b/browse/src/error-handling.ts index 2c4e271e87..ba6f1b61cf 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. */ @@ -36,23 +34,39 @@ export function safeKill(pid: number, signal: NodeJS.Signals | number): void { } } -/** Check if a PID is alive. Pure boolean probe — returns false for ALL errors. */ +/** + * Check if a PID is alive. Pure boolean probe — never throws. + * + * Signal 0 on every platform. Node and Bun both map `process.kill(pid, 0)` to + * an OpenProcess existence check on Windows, so the POSIX idiom is portable + * here — no shell-out needed. + * + * Windows used to shell out to `tasklist /FI "PID eq "` and string-match + * the CSV. That was wrong in two ways, both of which bit in production: + * + * 1. FALSE NEGATIVES UNDER LOAD. `tasklist` takes ~700-1700ms on an idle + * Windows 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 (killAgentByRecord, the terminal-agent watchdog) then + * skipped the kill and respawned around the survivor, leaking one + * terminal-agent per watchdog tick. The leak was self-reinforcing: every + * orphan added memory pressure, which made the next tasklist slower, + * which produced the next false negative. + * 2. A VISIBLE CONSOLE WINDOW per probe (no windowsHide), so a background + * watchdog strobed a terminal into the foreground every 60 seconds. + * + * Signal 0 is ~74,000x faster (0.004ms vs 270ms, measured), spawns nothing, + * and cannot time out. + * + * EPERM means the process EXISTS but we lack rights to signal it. That is + * 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 } - ); - return result.stdout.toString().includes(`"${pid}"`); - } catch { - return false; - } - } try { process.kill(pid, 0); return true; - } catch { - return false; + } catch (err: any) { + return err?.code === 'EPERM'; } } diff --git a/browse/src/server.ts b/browse/src/server.ts index 301781acce..7f47b1e957 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -1517,8 +1517,18 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { process.env.GSTACK_AGENT_WATCHDOG_TICK_MS || '60000', 10, ); - const RESPAWN_GUARD_WINDOW_MS = 60_000; const RESPAWN_GUARD_MAX = 3; + // The guard window MUST span enough ticks for RESPAWN_GUARD_MAX respawns to + // land inside it. This was a fixed 60_000 against a 60_000 tick, so at most + // ONE respawn could ever be in the window and `respawnHistory.length >= 3` + // was unreachable — the guard could not fire at the default tick rate, and a + // steady one-per-tick leak ran unbounded instead of stopping after 3. Scale + // with the tick so the intent ("3 crashes in quick succession → stop") holds + // at any tick value: 3 respawns within 5 ticks trips it. + const RESPAWN_GUARD_WINDOW_MS = Math.max( + 60_000, + AGENT_WATCHDOG_TICK_MS * (RESPAWN_GUARD_MAX + 2), + ); let agentRespawnGuardTripped = false; if (ownsTerminalAgent) { diff --git a/browse/src/terminal-agent-control.ts b/browse/src/terminal-agent-control.ts index 094ba668fa..411e02a12e 100644 --- a/browse/src/terminal-agent-control.ts +++ b/browse/src/terminal-agent-control.ts @@ -77,6 +77,11 @@ export function spawnTerminalAgent(opts: { ...(opts.extraEnv || {}), }, stdio: ['ignore', 'ignore', 'ignore'], + // Without this, Windows allocates a console for the child and pops it to + // the FOREGROUND, stealing focus from whatever the user is typing into. + // The agent is a background daemon with all three stdio streams already + // ignored, so it has nothing to show. Harmless no-op on macOS/Linux. + windowsHide: true, }); proc.unref?.(); return proc.pid ?? null; diff --git a/browse/test/cdp-session-cleanup.test.ts b/browse/test/cdp-session-cleanup.test.ts index 25ca6760cb..f474df9f63 100644 --- a/browse/test/cdp-session-cleanup.test.ts +++ b/browse/test/cdp-session-cleanup.test.ts @@ -18,7 +18,7 @@ import { withCdpSession, getOrCreateCdpSession } from '../src/cdp-bridge'; // browse/test/server-sanitize-surrogates.test.ts: read source files // directly, assert an invariant on their contents. -const SRC_DIR = path.resolve(new URL(import.meta.url).pathname, '..', '..', 'src'); +const SRC_DIR = path.resolve(import.meta.path, '..', '..', 'src'); function readAllSourceFiles(): Array<{ file: string; content: string }> { const out: Array<{ file: string; content: string }> = []; diff --git a/browse/test/cli-supervisor.test.ts b/browse/test/cli-supervisor.test.ts index d9cec7b89d..22bdb57d9d 100644 --- a/browse/test/cli-supervisor.test.ts +++ b/browse/test/cli-supervisor.test.ts @@ -15,7 +15,7 @@ import * as path from 'path'; // 3-8s each). These tripwires defend the load-bearing invariants: // opt-in by default, signal handlers wired, crash-loop guard, env knobs. -const CLI_TS = path.resolve(new URL(import.meta.url).pathname, '..', '..', 'src', 'cli.ts'); +const CLI_TS = path.resolve(import.meta.path, '..', '..', 'src', 'cli.ts'); describe('CLI outer supervisor (v1.44+)', () => { test('1. supervisor is opt-in via --supervise flag or BROWSE_SUPERVISE env', () => { diff --git a/browse/test/process-liveness-windows.test.ts b/browse/test/process-liveness-windows.test.ts new file mode 100644 index 0000000000..672e725417 --- /dev/null +++ b/browse/test/process-liveness-windows.test.ts @@ -0,0 +1,134 @@ +import { describe, test, expect } from 'bun:test'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { isProcessAlive } from '../src/error-handling'; +import { spawnTerminalAgent } from '../src/terminal-agent-control'; + +// REGRESSION TEST for the Windows terminal-agent leak. +// +// Symptom (reported on Windows 11, 48GB box under a heavy parallel build): +// a console window popped to the foreground every 60 seconds, and orphaned +// `bun run terminal-agent.ts` processes accumulated at one per minute until +// the machine ran out of committable memory. +// +// Root cause was a three-bug chain, each of which this file pins: +// +// 1. `isProcessAlive` shelled out to `tasklist` on Windows with a 3s +// timeout. A Bun.spawnSync that hits its timeout STILL RETURNS, carrying +// partial stdout — so the `.includes()` PID match came back false and a +// LIVE agent was reported dead. Measured tasklist latency was 700-1700ms +// idle, and far worse under memory pressure, so the timeout was reachable +// in ordinary use. +// 2. That false negative made `killAgentByRecord` skip the kill (it +// validates liveness first) while the watchdog respawned anyway — +// leaking the survivor. Each orphan added memory pressure, slowing the +// next tasklist, producing the next false negative. Self-reinforcing. +// 3. Neither the tasklist probe nor the agent spawn passed `windowsHide`, +// so every tick allocated a visible console and stole focus. +// +// The guard-window arithmetic bug that let this run unbounded instead of +// tripping the crash-loop guard is pinned separately, in test 6. + +const SRC_DIR = path.resolve(import.meta.dir, '..', 'src'); + +function readAllSourceFiles(): Array<{ file: string; content: string }> { + return fs + .readdirSync(SRC_DIR) + .filter((e) => e.endsWith('.ts')) + .map((e) => ({ file: e, content: fs.readFileSync(path.join(SRC_DIR, e), 'utf-8') })); +} + +/** Strip line and block comments so static greps only see real code. */ +function stripComments(src: string): string { + return src.replace(/\/\*[\s\S]*?\*\//g, '').replace(/^\s*\/\/.*$/gm, ''); +} + +describe('process liveness probe (Windows terminal-agent leak)', () => { + test('1. isProcessAlive reports the current process alive', () => { + expect(isProcessAlive(process.pid)).toBe(true); + }); + + test('2. isProcessAlive reports an unused PID dead', () => { + // Below Linux PID_MAX_LIMIT, far above any realistic Windows/macOS PID. + expect(isProcessAlive(2147483646)).toBe(false); + }); + + test('3. isProcessAlive spawns NO subprocess', () => { + // 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. + const origSpawn = (Bun as any).spawn; + const origSpawnSync = (Bun as any).spawnSync; + const spawns: string[] = []; + (Bun as any).spawn = (...args: any[]) => { spawns.push(`spawn:${JSON.stringify(args[0])}`); return origSpawn(...args); }; + (Bun as any).spawnSync = (...args: any[]) => { spawns.push(`spawnSync:${JSON.stringify(args[0])}`); return origSpawnSync(...args); }; + try { + isProcessAlive(process.pid); + isProcessAlive(2147483646); + expect(spawns).toEqual([]); + } finally { + (Bun as any).spawn = origSpawn; + (Bun as any).spawnSync = origSpawnSync; + } + }); + + test('4. no source file probes liveness via tasklist', () => { + // Static tripwire: re-introducing a tasklist-based existence check + // anywhere in src/ resurrects the false-negative class. + const offenders: string[] = []; + for (const { file, content } of readAllSourceFiles()) { + const code = stripComments(content); + // `PID eq` is the existence-probe form specifically. Other tasklist + // uses (e.g. IMAGENAME filters for browser detection) are unaffected. + if (/tasklist/.test(code) && /PID eq/.test(code)) offenders.push(file); + } + expect(offenders).toEqual([]); + }); + + test('5. spawnTerminalAgent passes windowsHide so no console is shown', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-hide-')); + const script = path.join(tmpDir, 'fake-agent.ts'); + fs.writeFileSync(script, '// no-op\n'); + const origSpawn = (Bun as any).spawn; + let captured: any = null; + (Bun as any).spawn = (_cmd: any, opts: any) => { + captured = opts; + return { pid: 4242, unref() {} }; + }; + try { + const pid = spawnTerminalAgent({ + stateFile: path.join(tmpDir, 'state.json'), + serverPort: 12345, + cwd: tmpDir, + scriptPath: script, + }); + expect(pid).toBe(4242); + expect(captured).not.toBeNull(); + expect(captured.windowsHide).toBe(true); + // Detached background daemon — must not inherit a terminal either. + expect(captured.stdio).toEqual(['ignore', 'ignore', 'ignore']); + } finally { + (Bun as any).spawn = origSpawn; + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + test('6. respawn guard window spans enough ticks for the guard to fire', () => { + // The guard was `RESPAWN_GUARD_WINDOW_MS = 60_000` against a 60_000ms + // tick, allowing at most ONE respawn in the window — so the + // `>= RESPAWN_GUARD_MAX (3)` trip condition was unreachable and a steady + // one-per-tick leak never self-limited. Assert the window is derived from + // the tick rather than fixed. + const src = fs.readFileSync(path.join(SRC_DIR, 'server.ts'), 'utf-8'); + const match = src.match(/const RESPAWN_GUARD_WINDOW_MS =([\s\S]{0,160}?);/); + expect(match).not.toBeNull(); + expect(match![1]).toContain('AGENT_WATCHDOG_TICK_MS'); + + // Pin the arithmetic itself: at the default tick, three respawns must fit. + const tick = 60_000; + const guardMax = 3; + const windowMs = Math.max(60_000, tick * (guardMax + 2)); + expect(windowMs).toBeGreaterThanOrEqual(tick * guardMax); + }); +}); diff --git a/browse/test/server-embedder-terminal-port.test.ts b/browse/test/server-embedder-terminal-port.test.ts index f24ee35101..051d931954 100644 --- a/browse/test/server-embedder-terminal-port.test.ts +++ b/browse/test/server-embedder-terminal-port.test.ts @@ -217,7 +217,7 @@ describe('buildFetchHandler ownsTerminalAgent gate', () => { // Resolves browse/src/server.ts relative to this test file so the test // works regardless of cwd. import.meta.url is the test file's URL. const serverTsPath = path.resolve( - new URL(import.meta.url).pathname, + import.meta.path, '..', '..', 'src', diff --git a/browse/test/server-pty-lease-routes.test.ts b/browse/test/server-pty-lease-routes.test.ts index 2c12618830..aad16ff909 100644 --- a/browse/test/server-pty-lease-routes.test.ts +++ b/browse/test/server-pty-lease-routes.test.ts @@ -7,7 +7,7 @@ import * as path from 'path'; // loopback to be live (e2e-tier); these static-grep tripwires pin the // load-bearing protocol invariants. -const SERVER_TS = path.resolve(new URL(import.meta.url).pathname, '..', '..', 'src', 'server.ts'); +const SERVER_TS = path.resolve(import.meta.path, '..', '..', 'src', 'server.ts'); describe('server: PTY lease routes (v1.44+ Commit 2)', () => { test('1. /pty-session returns the 4-tuple shape (sessionId, attachToken, leaseExpiresAt)', () => { diff --git a/browse/test/sidepanel-patient-autoconnect.test.ts b/browse/test/sidepanel-patient-autoconnect.test.ts index faf38499b4..5c8679f8e3 100644 --- a/browse/test/sidepanel-patient-autoconnect.test.ts +++ b/browse/test/sidepanel-patient-autoconnect.test.ts @@ -12,7 +12,7 @@ import * as path from 'path'; // explicit unrecoverable signals (401 auth invalid). const CLIENT_JS = path.resolve( - new URL(import.meta.url).pathname, + import.meta.path, '..', '..', '..', diff --git a/browse/test/sidepanel-reattach.test.ts b/browse/test/sidepanel-reattach.test.ts index 9179e57c40..815b04cd91 100644 --- a/browse/test/sidepanel-reattach.test.ts +++ b/browse/test/sidepanel-reattach.test.ts @@ -13,7 +13,7 @@ import * as path from 'path'; // in the e2e tier. const TERMINAL_JS = path.resolve( - new URL(import.meta.url).pathname, '..', '..', '..', 'extension', 'sidepanel-terminal.js', + import.meta.path, '..', '..', '..', 'extension', 'sidepanel-terminal.js', ); describe('sidepanel re-attach loop (v1.44+ Commit 3)', () => { diff --git a/browse/test/sidepanel-restart-dispose.test.ts b/browse/test/sidepanel-restart-dispose.test.ts index 8ec44690b7..2883dcc1f1 100644 --- a/browse/test/sidepanel-restart-dispose.test.ts +++ b/browse/test/sidepanel-restart-dispose.test.ts @@ -16,10 +16,10 @@ import * as path from 'path'; // doesn't leak a 60s-zombie claude. const TERMINAL_JS = path.resolve( - new URL(import.meta.url).pathname, '..', '..', '..', 'extension', 'sidepanel-terminal.js', + import.meta.path, '..', '..', '..', 'extension', 'sidepanel-terminal.js', ); const SIDEPANEL_JS = path.resolve( - new URL(import.meta.url).pathname, '..', '..', '..', 'extension', 'sidepanel.js', + import.meta.path, '..', '..', '..', 'extension', 'sidepanel.js', ); describe('sidepanel-terminal: forceRestart via /pty-restart (v1.44+)', () => { diff --git a/browse/test/terminal-agent-detach-reattach.test.ts b/browse/test/terminal-agent-detach-reattach.test.ts index 89fbe5a1ca..fcca6684da 100644 --- a/browse/test/terminal-agent-detach-reattach.test.ts +++ b/browse/test/terminal-agent-detach-reattach.test.ts @@ -10,7 +10,7 @@ import * as path from 'path'; // in the e2e tier; these static-grep tripwires defend the load-bearing // protocol + correctness properties. -const AGENT_TS = path.resolve(new URL(import.meta.url).pathname, '..', '..', 'src', 'terminal-agent.ts'); +const AGENT_TS = path.resolve(import.meta.path, '..', '..', 'src', 'terminal-agent.ts'); describe('terminal-agent detach + re-attach (v1.44+ Commit 3)', () => { test('1. PtySession carries ring buffer + alt-screen + detach state', () => { diff --git a/browse/test/terminal-agent-internal-handler.test.ts b/browse/test/terminal-agent-internal-handler.test.ts index 04e7f35971..b3a7c1ee6e 100644 --- a/browse/test/terminal-agent-internal-handler.test.ts +++ b/browse/test/terminal-agent-internal-handler.test.ts @@ -12,7 +12,7 @@ import * as path from 'path'; // (token grant/revoke behavior) already live in // browse/test/terminal-agent-integration.test.ts. -const AGENT_TS = path.resolve(new URL(import.meta.url).pathname, '..', '..', 'src', 'terminal-agent.ts'); +const AGENT_TS = path.resolve(import.meta.path, '..', '..', 'src', 'terminal-agent.ts'); describe('terminal-agent internalHandler refactor (v1.44+)', () => { test('1. internalHandler exists with the documented signature', () => { diff --git a/browse/test/terminal-agent-keepalive.test.ts b/browse/test/terminal-agent-keepalive.test.ts index 812f70f818..2e94406813 100644 --- a/browse/test/terminal-agent-keepalive.test.ts +++ b/browse/test/terminal-agent-keepalive.test.ts @@ -11,8 +11,8 @@ import * as path from 'path'; // regressed by a refactor. These tests fail CI if either side stops sending // or stops accepting the protocol frames. -const AGENT_TS = path.resolve(new URL(import.meta.url).pathname, '..', '..', 'src', 'terminal-agent.ts'); -const CLIENT_JS = path.resolve(new URL(import.meta.url).pathname, '..', '..', '..', 'extension', 'sidepanel-terminal.js'); +const AGENT_TS = path.resolve(import.meta.path, '..', '..', 'src', 'terminal-agent.ts'); +const CLIENT_JS = path.resolve(import.meta.path, '..', '..', '..', 'extension', 'sidepanel-terminal.js'); describe('terminal-agent WS keepalive (v1.44+)', () => { test('1. agent has a KEEPALIVE_INTERVAL_MS env knob, default 25000', () => { diff --git a/browse/test/terminal-agent-pid-identity.test.ts b/browse/test/terminal-agent-pid-identity.test.ts index 52503fe2ea..f0250416a8 100644 --- a/browse/test/terminal-agent-pid-identity.test.ts +++ b/browse/test/terminal-agent-pid-identity.test.ts @@ -30,7 +30,7 @@ import { // and browse/test/server-sanitize-surrogates.test.ts: read source files // directly, assert an invariant on their contents. -const SRC_DIR = path.resolve(new URL(import.meta.url).pathname, '..', '..', 'src'); +const SRC_DIR = path.resolve(import.meta.path, '..', '..', 'src'); function readAllSourceFiles(): Array<{ file: string; content: string }> { const out: Array<{ file: string; content: string }> = []; diff --git a/browse/test/terminal-agent-session-routing.test.ts b/browse/test/terminal-agent-session-routing.test.ts index acc51b2af3..1a8184b624 100644 --- a/browse/test/terminal-agent-session-routing.test.ts +++ b/browse/test/terminal-agent-session-routing.test.ts @@ -13,7 +13,7 @@ import * as path from 'path'; // - {type:"start"} triggers spawn for eager UX after forceRestart // - maybeSpawnPty helper is the single entry point for both spawn paths -const AGENT_TS = path.resolve(new URL(import.meta.url).pathname, '..', '..', 'src', 'terminal-agent.ts'); +const AGENT_TS = path.resolve(import.meta.path, '..', '..', 'src', 'terminal-agent.ts'); describe('terminal-agent session routing (v1.44+ Commit 2)', () => { test('1. validTokens is a Map binding token → sessionId', () => { diff --git a/browse/test/terminal-agent-watchdog.test.ts b/browse/test/terminal-agent-watchdog.test.ts index f012dc406e..ff48b0bb3d 100644 --- a/browse/test/terminal-agent-watchdog.test.ts +++ b/browse/test/terminal-agent-watchdog.test.ts @@ -10,8 +10,8 @@ import * as path from 'path'; // load-bearing properties: identity-based liveness check (not name match), // crash-loop guard, gated on ownsTerminalAgent, and cleared on shutdown. -const SERVER_TS = path.resolve(new URL(import.meta.url).pathname, '..', '..', 'src', 'server.ts'); -const CONTROL_TS = path.resolve(new URL(import.meta.url).pathname, '..', '..', 'src', 'terminal-agent-control.ts'); +const SERVER_TS = path.resolve(import.meta.path, '..', '..', 'src', 'server.ts'); +const CONTROL_TS = path.resolve(import.meta.path, '..', '..', 'src', 'terminal-agent-control.ts'); describe('terminal-agent watchdog (v1.44+)', () => { test('1. spawnTerminalAgent helper exists with PID return type', () => { @@ -50,7 +50,13 @@ describe('terminal-agent watchdog (v1.44+)', () => { test('4. crash-loop guard with rolling window', () => { const src = fs.readFileSync(SERVER_TS, 'utf-8'); const block = sliceBetween(src, '─── Terminal-Agent Watchdog', 'Factory-scoped validateAuth'); - expect(block).toContain('RESPAWN_GUARD_WINDOW_MS = 60_000'); + // The window MUST be derived from the tick, not a fixed 60_000. It was + // hardcoded to 60_000 against a 60_000ms tick, so at most ONE respawn + // could ever sit inside the window and the `>= RESPAWN_GUARD_MAX` trip + // was unreachable — a steady one-respawn-per-tick leak ran unbounded + // instead of self-limiting after 3. Pinning the literal is what let that + // ship, so pin the relationship instead. + expect(block).toMatch(/RESPAWN_GUARD_WINDOW_MS =[\s\S]{0,200}AGENT_WATCHDOG_TICK_MS/); expect(block).toContain('RESPAWN_GUARD_MAX = 3'); expect(block).toContain('respawnHistory'); expect(block).toContain('agentRespawnGuardTripped'); @@ -72,7 +78,7 @@ describe('terminal-agent watchdog (v1.44+)', () => { test('7. CLI cold-start path uses the same spawnTerminalAgent helper', () => { const cli = fs.readFileSync( - path.resolve(new URL(import.meta.url).pathname, '..', '..', 'src', 'cli.ts'), + path.resolve(import.meta.path, '..', '..', 'src', 'cli.ts'), 'utf-8', ); // Otherwise the CLI and watchdog could drift on spawn env/cwd, and diff --git a/test/hermetic-wiring.test.ts b/test/hermetic-wiring.test.ts index 08528586d2..8276db0d77 100644 --- a/test/hermetic-wiring.test.ts +++ b/test/hermetic-wiring.test.ts @@ -17,7 +17,7 @@ import { describe, test, expect } from 'bun:test'; import * as fs from 'fs'; import * as path from 'path'; -const ROOT = path.resolve(new URL(import.meta.url).pathname, '..', '..'); +const ROOT = path.resolve(import.meta.path, '..', '..'); const RUNNERS = [ 'test/helpers/session-runner.ts',