mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-16 09:55:29 +02:00
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>
98 lines
4.8 KiB
TypeScript
98 lines
4.8 KiB
TypeScript
import { describe, test, expect } from 'bun:test';
|
|
import * as fs from 'fs';
|
|
import * as path from 'path';
|
|
|
|
// v1.44 terminal-agent watchdog — static-grep invariants.
|
|
//
|
|
// The watchdog respawns terminal-agent when its PID dies. Live process-tree
|
|
// tests would require spawning, killing, and observing across two real Bun
|
|
// processes — slow and flaky in the free tier. These tripwires defend the
|
|
// 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(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', () => {
|
|
const src = fs.readFileSync(CONTROL_TS, 'utf-8');
|
|
expect(src).toMatch(/export function spawnTerminalAgent\(/);
|
|
// Must clean up prior PID before spawning (no zombies).
|
|
expect(src).toContain('readAgentRecord(stateDir)');
|
|
expect(src).toContain('killAgentByRecord(prior');
|
|
expect(src).toContain('clearAgentRecord(stateDir)');
|
|
});
|
|
|
|
test('2. watchdog is gated on ownsTerminalAgent', () => {
|
|
const src = fs.readFileSync(SERVER_TS, 'utf-8');
|
|
// Match the comment + the guard. The guard MUST be a positive check;
|
|
// an inverted check would respawn for embedders and trample their PTY.
|
|
const block = sliceBetween(src, '─── Terminal-Agent Watchdog', 'Factory-scoped validateAuth');
|
|
expect(block).toMatch(/if \(ownsTerminalAgent\)/);
|
|
expect(block).toContain('agentWatchdogInterval = setInterval');
|
|
});
|
|
|
|
test('3. watchdog uses PID liveness, not process name probe', () => {
|
|
const src = fs.readFileSync(SERVER_TS, 'utf-8');
|
|
const block = sliceBetween(src, '─── Terminal-Agent Watchdog', 'Factory-scoped validateAuth');
|
|
// The whole point of the v1.44 watchdog over v1.43- pkill teardown:
|
|
// identity-based liveness. Slow-but-alive agents must NOT trigger
|
|
// respawn (split-brain defense).
|
|
expect(block).toContain('readAgentRecord(stateDir)');
|
|
expect(block).toContain('isProcessAlive(record.pid)');
|
|
// Negative: no executable name-based process lookup. Allow the strings
|
|
// to appear in prose comments (the watchdog doc explains what it
|
|
// replaces), reject only actual invocations.
|
|
expect(block).not.toMatch(/spawnSync\s*\(\s*['"]pkill/);
|
|
expect(block).not.toMatch(/Bun\.spawn\s*\(\s*\[\s*['"]pgrep/);
|
|
});
|
|
|
|
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');
|
|
// 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');
|
|
// Window pruning: old entries must be evicted before counting toward
|
|
// the limit. Otherwise a daemon up for a week with one crash a day
|
|
// would eventually trip the guard.
|
|
expect(block).toMatch(/respawnHistory\.shift\(\)/);
|
|
});
|
|
|
|
test('5. watchdog interval is cleared on shutdown', () => {
|
|
const src = fs.readFileSync(SERVER_TS, 'utf-8');
|
|
expect(src).toContain('if (agentWatchdogInterval) clearInterval(agentWatchdogInterval)');
|
|
});
|
|
|
|
test('6. tick interval is env-overridable for tests', () => {
|
|
const src = fs.readFileSync(SERVER_TS, 'utf-8');
|
|
expect(src).toContain('GSTACK_AGENT_WATCHDOG_TICK_MS');
|
|
});
|
|
|
|
test('7. CLI cold-start path uses the same spawnTerminalAgent helper', () => {
|
|
const cli = fs.readFileSync(
|
|
path.resolve(import.meta.path, '..', '..', 'src', 'cli.ts'),
|
|
'utf-8',
|
|
);
|
|
// Otherwise the CLI and watchdog could drift on spawn env/cwd, and
|
|
// teardown invariants tested against one would silently miss the other.
|
|
expect(cli).toContain('spawnTerminalAgent({');
|
|
expect(cli).toContain("from './terminal-agent-control'");
|
|
});
|
|
});
|
|
|
|
function sliceBetween(source: string, start: string, end: string): string {
|
|
const i = source.indexOf(start);
|
|
if (i === -1) throw new Error(`marker not found: ${start}`);
|
|
const j = source.indexOf(end, i + start.length);
|
|
if (j === -1) throw new Error(`end marker not found: ${end}`);
|
|
return source.slice(i, j);
|
|
}
|