diff --git a/browse/src/error-handling.ts b/browse/src/error-handling.ts index 2c4e271e8..ef1c3dfb0 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,24 @@ 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. + * + * Uses signal-0 on every platform. Node/Bun implement `process.kill(pid, 0)` + * on Windows via `OpenProcess` — a pure existence check that spawns no child + * process and opens no console window. The old Windows branch shelled out to + * `tasklist`, which Windows gives its own console window; on the terminal-agent + * watchdog's per-tick existence check that flashed a `conhost.exe` window every + * 60s for the whole session (#1952). The parent watchdog already uses signal-0 + * directly; this unifies on it. + */ 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) { + // ESRCH → gone (false); EPERM → exists but not signallable → still alive. + // The old Unix branch collapsed every error to false, missing the EPERM edge. + return err?.code === 'EPERM'; } } diff --git a/browse/test/error-handling.test.ts b/browse/test/error-handling.test.ts index b25d98805..776d29449 100644 --- a/browse/test/error-handling.test.ts +++ b/browse/test/error-handling.test.ts @@ -2,6 +2,7 @@ import { describe, test, expect } from 'bun:test'; import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; +import { fileURLToPath } from 'url'; import { safeUnlink, safeKill, isProcessAlive } from '../src/error-handling'; describe('safeUnlink', () => { @@ -44,4 +45,36 @@ describe('isProcessAlive', () => { test('returns false for non-existent process', () => { expect(isProcessAlive(99999999)).toBe(false); }); + + test('treats a live-but-EPERM process as alive (#1952)', () => { + // PID 1 (init/launchd) exists but a non-root user can't signal it → + // process.kill(1, 0) throws EPERM, which means alive, not dead. + // (Skip when running as root, where signalling PID 1 is permitted.) + if (typeof process.getuid === 'function' && process.getuid() === 0) return; + let threwEPERM = false; + try { + process.kill(1, 0); + } catch (err: any) { + threwEPERM = err?.code === 'EPERM'; + } + if (threwEPERM) expect(isProcessAlive(1)).toBe(true); + }); + + // Regression tripwire for #1952: isProcessAlive must never spawn a child + // process to probe a PID. The Windows branch used to shell out to `tasklist`, + // which Windows gives its own console window — flashing a conhost.exe window + // every watchdog tick (default 60s) for the whole session. signal-0 is a pure + // syscall on all platforms (OpenProcess on Windows), so the source must not + // reintroduce tasklist or a spawn in this probe. + test('isProcessAlive source spawns nothing (no Windows console flash, #1952)', () => { + const src = fs.readFileSync( + path.join(path.dirname(fileURLToPath(import.meta.url)), '..', 'src', 'error-handling.ts'), + 'utf-8' + ); + const start = src.indexOf('export function isProcessAlive'); + expect(start).toBeGreaterThan(-1); + const body = src.slice(start, src.indexOf('\n}', start)); + expect(body).not.toMatch(/tasklist/i); + expect(body).not.toMatch(/spawnSync|spawn\(/); + }); });