From 00303ebf86cbcb9d5ae100e1aefaf33f34ebdc35 Mon Sep 17 00:00:00 2001 From: Jayesh Betala Date: Fri, 12 Jun 2026 00:22:48 +0530 Subject: [PATCH] fix(browse): stop isProcessAlive from flashing a Windows console every watchdog tick MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows, isProcessAlive() shelled out to `tasklist` via Bun.spawnSync. Windows gives that child its own console window, so the terminal-agent watchdog's per-tick existence check (default 60s) flashed a conhost.exe window for the entire headed session — purely cosmetic but a constant, unexplained blink (#1952). macOS/Linux stayed silent because their branch is a `process.kill(pid, 0)` syscall that spawns nothing. Unify on signal-0 across all platforms — exactly what the parent watchdog two functions away already does. Node/Bun implement `process.kill(pid, 0)` on Windows via OpenProcess: a pure existence check, no child process, no window. This also fixes a latent correctness edge: the old branch collapsed every error to false, so a live-but-unsignalable process (EPERM) read as dead. Now ESRCH -> dead, EPERM -> alive. Tests: existing alive/dead probes still pass; add an EPERM-as-alive case and a static tripwire asserting the probe never reintroduces `tasklist`/spawn (proven to fail on the old body). 9/9 green in error-handling.test.ts. Fixes #1952 Co-Authored-By: Claude Opus 4.8 (1M context) --- browse/src/error-handling.ts | 31 ++++++++++++++-------------- browse/test/error-handling.test.ts | 33 ++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 16 deletions(-) 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\(/); + }); });