diff --git a/browse/src/bun-polyfill.cjs b/browse/src/bun-polyfill.cjs index e0ada11b3..ecfd26efe 100644 --- a/browse/src/bun-polyfill.cjs +++ b/browse/src/bun-polyfill.cjs @@ -75,6 +75,9 @@ globalThis.Bun = { timeout: options.timeout, env: options.env, cwd: options.cwd, + // The browse server runs console-less on Windows; without this every + // subprocess (tasklist, git, icacls) pops a visible console window. + windowsHide: true, }); return { @@ -91,6 +94,10 @@ globalThis.Bun = { stdio, env: options.env, cwd: options.cwd, + detached: options.detached, + // Same as spawnSync: detached daemons (terminal-agent respawns) must + // not flash a console window on Windows. + windowsHide: true, }); return { diff --git a/browse/src/error-handling.ts b/browse/src/error-handling.ts index 2c4e271e8..c824ee094 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,26 @@ 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 implement + * `process.kill(pid, 0)` on Windows via an OpenProcess existence check — + * no child process, no console window. The previous Windows branch spawned + * `tasklist`, which (a) flashed a visible console window from console-less + * daemons on every call (#1952), and (b) silently returned false for LIVE + * processes when tasklist exceeded its 3s timeout — node's spawnSync + * reports timeout via `result.error` without throwing, so the empty-stdout + * path read as "dead". A false "dead" makes the terminal-agent watchdog + * respawn around a live agent and orphan it (split-brain, #2151). + * + * EPERM ⇒ alive: the process exists but can't be signalled by this user. + */ 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 301781acc..547995a09 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -1504,9 +1504,9 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { // (which would create split-brain — two agents writing the port file, // tokens diverging between them, mystery PTY upgrade failures). // - // Crash-loop guard: 3 respawn attempts inside 60s → stop trying and emit - // a one-line error. Manual `forceRestart` from the sidebar clears the - // history (the user is the explicit signal to retry). + // Crash-loop guard: 3 respawn attempts inside 3 ticks → stop trying and + // emit a one-line error. Manual `forceRestart` from the sidebar clears + // the history (the user is the explicit signal to retry). // // Only active when ownsTerminalAgent === true. Embedders that pre-launch // their own PTY server (gbrowser phoenix overlay) must not be auto-respawned @@ -1517,8 +1517,11 @@ 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 must span at least RESPAWN_GUARD_MAX ticks or it can never + // trip: respawn attempts land at most once per tick, so a 60s window + // over a 60s tick sees a single attempt forever (see #2151). + const RESPAWN_GUARD_WINDOW_MS = Math.max(60_000, AGENT_WATCHDOG_TICK_MS * RESPAWN_GUARD_MAX); let agentRespawnGuardTripped = false; if (ownsTerminalAgent) { diff --git a/browse/test/bun-polyfill.test.ts b/browse/test/bun-polyfill.test.ts index 7ca25dfab..f9c3dafe6 100644 --- a/browse/test/bun-polyfill.test.ts +++ b/browse/test/bun-polyfill.test.ts @@ -1,4 +1,5 @@ import { describe, test, expect, afterAll } from 'bun:test'; +import * as fs from 'fs'; import * as path from 'path'; // Load the polyfill into a fresh object (don't clobber globalThis.Bun) @@ -48,6 +49,23 @@ describe('bun-polyfill', () => { expect(lines[2]).toBe('HAS_UNREF'); }); + test('spawn and spawnSync pass windowsHide (no console flash from console-less daemons)', () => { + // Static tripwire (#2151): on Windows the browse server always runs + // under Node via this polyfill, and node's child_process defaults to + // windowsHide: false — so every subprocess the console-less daemon + // spawns (terminal-agent respawns, git, icacls) pops a visible console + // window. Both spawn paths must pass windowsHide: true, and spawn must + // forward `detached` for daemon children. Behavioral assertion isn't + // possible cross-platform (window creation is invisible to the child), + // hence source-level pinning. + const src = fs.readFileSync(polyfillPath, 'utf-8'); + const spawnSyncBlock = src.slice(src.indexOf('spawnSync(cmd'), src.indexOf('spawn(cmd')); + const spawnBlock = src.slice(src.indexOf('spawn(cmd')); + expect(spawnSyncBlock).toContain('windowsHide: true'); + expect(spawnBlock).toContain('windowsHide: true'); + expect(spawnBlock).toContain('detached: options.detached'); + }); + test('Bun.serve creates an HTTP server that responds', async () => { const result = Bun.spawnSync(['node', '-e', ` require('${polyfillPath}'); diff --git a/browse/test/error-handling.test.ts b/browse/test/error-handling.test.ts index b25d98805..b1c31bc26 100644 --- a/browse/test/error-handling.test.ts +++ b/browse/test/error-handling.test.ts @@ -44,4 +44,28 @@ describe('isProcessAlive', () => { test('returns false for non-existent process', () => { expect(isProcessAlive(99999999)).toBe(false); }); + + test('returns true for a live but unsignallable process (EPERM ⇒ alive)', () => { + // PID 4 = System on Windows, PID 1 = init/launchd on POSIX. Both are + // always alive; signalling them as an unprivileged user throws EPERM, + // which must read as "alive" (as root/admin the probe just succeeds). + const protectedPid = process.platform === 'win32' ? 4 : 1; + expect(isProcessAlive(protectedPid)).toBe(true); + }); + + test('never spawns a subprocess (no tasklist on Windows)', () => { + // Static tripwire: the pre-#2151 Windows branch shelled out to + // `tasklist`, which flashed a console window from console-less daemons + // on every watchdog tick (#1952) and silently reported live agents as + // dead when tasklist exceeded its timeout — triggering split-brain + // respawns (#2151). signal-0 is a pure syscall on every platform. + const src = fs.readFileSync( + path.resolve(import.meta.dir, '..', 'src', 'error-handling.ts'), + 'utf-8', + ); + // Reject only actual invocations — the doc comment on isProcessAlive + // may mention tasklist when explaining what the signal-0 probe replaced. + expect(src).not.toMatch(/spawnSync\s*\(\s*\[?\s*['"]tasklist/); + expect(src).not.toMatch(/spawn\s*\(\s*\[?\s*['"]tasklist/); + }); }); diff --git a/browse/test/terminal-agent-watchdog.test.ts b/browse/test/terminal-agent-watchdog.test.ts index f012dc406..c6cdfd6aa 100644 --- a/browse/test/terminal-agent-watchdog.test.ts +++ b/browse/test/terminal-agent-watchdog.test.ts @@ -50,7 +50,12 @@ 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 span at least RESPAWN_GUARD_MAX ticks. Respawn + // attempts land at most once per tick, so a fixed 60s window over a + // 60s tick would see a single attempt forever and never trip (#2151). + expect(block).toContain( + 'RESPAWN_GUARD_WINDOW_MS = Math.max(60_000, AGENT_WATCHDOG_TICK_MS * RESPAWN_GUARD_MAX)', + ); expect(block).toContain('RESPAWN_GUARD_MAX = 3'); expect(block).toContain('respawnHistory'); expect(block).toContain('agentRespawnGuardTripped');