fix(watchdog): signal-0 liveness, tick-scaled respawn guard, windowsHide

Three-bug chain behind the Windows terminal-agent leak (console window
strobing every 60s, one orphaned agent per watchdog tick until the box
ran out of committable memory):

1. isProcessAlive shelled out to `tasklist /FI "PID eq <pid>"` on Windows
   with a 3s timeout. A Bun.spawnSync that hits its timeout still RETURNS
   with partial stdout, so the `.includes()` PID match read a LIVE agent
   as dead — killAgentByRecord skipped the kill, the watchdog respawned
   around the survivor, and every orphan slowed the next tasklist enough
   to produce the next false negative. Now: `process.kill(pid, 0)` on
   every platform (Node and Bun both map signal 0 to an OpenProcess
   existence check on Windows), with EPERM counted as alive. No
   subprocess, no timeout, no console window.

2. The respawn circuit-breaker was mathematically unreachable — verified
   in this tree: RESPAWN_GUARD_WINDOW_MS was a fixed 60_000 against a
   60_000ms default tick, and each tick pushes at most one respawn
   timestamp, so three pushes span ~120s and can never coexist inside a
   60s window (eviction is strict `>`, and setInterval drift plus
   per-tick work always ages the prior entry past the boundary). The
   guard could not fire at the default tick rate and a steady
   one-per-tick leak ran unbounded. The window now scales with the tick:
   max(60_000, tick * (RESPAWN_GUARD_MAX + 2)), so "3 crashes in quick
   succession → stop" holds at any tick value.

3. The tasklist probe popped a visible console per tick (no windowsHide).
   Removing the shell-out kills that site; the agent-spawn site itself
   already passes windowsHide: true (landed with the bun-polyfill
   windowsHide commit — PR #2414's terminal-agent-control.ts hunk is
   reconciled there rather than duplicated).

New browse/test/process-liveness-windows.test.ts pins all three: no
subprocess from the probe, a static tripwire against reintroducing
`tasklist` + `PID eq` liveness checks in src/, the spawnTerminalAgent
windowsHide + stdio contract, and the window-derived-from-tick
arithmetic. terminal-agent-watchdog.test.ts test 4 now pins the
window/tick relationship instead of the fixed literal that let this
ship. Also converts `new URL(import.meta.url).pathname` to
`import.meta.path` across the static-grep tests it touches — the
pathname form yields /C:/... on Windows and breaks path.resolve.

Contributed by @SYKhayyat (PR #2414).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Garry Tan 2026-08-14 19:09:23 -07:00
parent 15c9455a69
commit bbaf5068b0
No known key found for this signature in database
GPG Key ID: C1F69E85C74EFE1D
17 changed files with 200 additions and 36 deletions

View File

@ -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 <pid>"` 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';
}
}

View File

@ -1529,8 +1529,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) {

View File

@ -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 }> = [];

View File

@ -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', () => {

View File

@ -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);
});
});

View File

@ -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',

View File

@ -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)', () => {

View File

@ -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,
'..',
'..',
'..',

View File

@ -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)', () => {

View File

@ -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+)', () => {

View File

@ -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', () => {

View File

@ -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<T> exists with the documented signature', () => {

View File

@ -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', () => {

View File

@ -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 }> = [];

View File

@ -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', () => {

View File

@ -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

View File

@ -19,7 +19,7 @@ import * as path from 'path';
import * as os from 'os';
import { getHermeticDirs, hermeticSkillsConfigDir } from './helpers/hermetic-env';
const ROOT = path.resolve(new URL(import.meta.url).pathname, '..', '..');
const ROOT = path.resolve(import.meta.path, '..', '..');
const RUNNERS = [
'test/helpers/session-runner.ts',