mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-27 22:15:33 +00:00
fix(serve): enable parent-death watchdog on Windows via signal-0 liveness probe (#2049)
The stdio parent-death watchdog was hard-wired to spawnSync('ps'), which
does not exist on Windows. The startup probe therefore failed on every
Windows host, the watchdog was permanently disabled ("watchdog disabled:
ps unavailable"), and an orphaned `gbrain serve` held the PGLite write
lock until reboot. Orphans are especially easy to produce on Windows
because MCP hosts launch the server through a cmd.exe wrapper (.bat) and
killing the wrapper does not kill the bun child.
Windows never re-parents orphans, so the cached process.ppid stays
correct for the process lifetime and the watchdog question inverts from
"did the live PPID change?" to "is the original parent still alive?" --
answered in-process with a signal-0 existence probe (process.kill(ppid, 0),
OpenProcess under the hood). No external binary needed. EPERM counts as
alive. Parent dead reports PID 0, which differs from initialParentPid and
fires the existing shutdown path.
- readLiveParentPid / probeWatchdogAvailable: platform split (ps on
POSIX, signal-0 on win32), exported with a platform test seam so CI on
any OS exercises both branches.
- isPidAlive: shared exported helper.
- Watchdog install guard tightened from `!== 1` to `> 1` so a PID-0
"parent already gone" report cannot install a phantom interval that
compares 0 to 0 forever.
- Disabled-mode log generalized (no longer claims ps is the only
mechanism); existing probe-fail test updated to match.
- New unit suite for the platform defaults (6 tests).
Verified live on Windows 11: serve spawned via a .bat wrapper, wrapper
killed without closing stdin -> bun child exits within ~10s and the
PGLite lock dir is released. Before this change the child survived
indefinitely and every subsequent gbrain invocation timed out waiting
for the lock.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
e78f8a1590
commit
42f3960ba5
+81
-22
@@ -35,9 +35,12 @@ export interface ServeOptions {
|
||||
// Defaults to the real implementation when omitted.
|
||||
startMcpServer?: (engine: BrainEngine) => Promise<void>;
|
||||
// Test seam for the parent-process watchdog. The default
|
||||
// (`readLiveParentPid`) reads the live kernel PPID via `ps` because
|
||||
// `process.ppid` is captured at process creation and does not refresh
|
||||
// on re-parent (Node/Bun parity). Tests inject a stub so they can
|
||||
// (`readLiveParentPid`) reads the live kernel PPID via `ps` on POSIX
|
||||
// because `process.ppid` is captured at process creation and does not
|
||||
// refresh on re-parent (Node/Bun parity). On Windows — where the
|
||||
// kernel never re-parents, so the cached ppid stays correct — it
|
||||
// probes the original parent's liveness with signal-0 instead and
|
||||
// reports 0 once the parent is gone. Tests inject a stub so they can
|
||||
// simulate the parent dying without spawning ps or re-parenting any
|
||||
// real process.
|
||||
getParentPid?: () => number;
|
||||
@@ -47,8 +50,9 @@ export interface ServeOptions {
|
||||
setInterval?: (fn: () => void, ms: number) => unknown;
|
||||
clearInterval?: (handle: unknown) => void;
|
||||
// Test seam for the one-shot watchdog readiness probe. The default
|
||||
// runs `spawnSync('ps', ['-o','ppid=','-p',PID])` and returns true on
|
||||
// success. Tests inject a stub to simulate ps unavailability (e.g.
|
||||
// runs `spawnSync('ps', ['-o','ppid=','-p',PID])` on POSIX (signal-0
|
||||
// against our own PID on Windows) and returns true on success. Tests
|
||||
// inject a stub to simulate ps unavailability (e.g.
|
||||
// stripped containers, busybox without procps) without modifying PATH.
|
||||
// When the probe returns false, `installStdioLifecycle` skips the
|
||||
// watchdog interval entirely and emits a loud stderr line. Without
|
||||
@@ -254,7 +258,13 @@ function installStdioLifecycle(
|
||||
// tmux, or a parent shell with PR_SET_CHILD_SUBREAPER). Polling is the
|
||||
// only portable way to notice; see `readLiveParentPid` for why we
|
||||
// cannot rely on `process.ppid` (cached at process creation and never
|
||||
// refreshed on re-parent in Node or Bun).
|
||||
// refreshed on re-parent in Node or Bun). On Windows the same class of
|
||||
// orphan is WORSE in practice: MCP hosts typically launch the server
|
||||
// through a `cmd.exe` wrapper (.bat/.cmd), and killing the wrapper
|
||||
// does not kill the child — so without a watchdog the orphan holds the
|
||||
// PGLite write lock until the machine reboots. `readLiveParentPid`
|
||||
// handles the platform split internally (PPID-change on POSIX,
|
||||
// parent-liveness on Windows); the comparison below works for both.
|
||||
//
|
||||
// We capture the initial parent PID once at install time and fire on
|
||||
// ANY change, not just reparent-to-PID-1. The PR-#676 author's original
|
||||
@@ -272,11 +282,16 @@ function installStdioLifecycle(
|
||||
// — the watchdog claims to be installed but never fires. When the probe
|
||||
// fails, we skip installing the interval entirely and log loudly so the
|
||||
// operator sees the degraded mode instead of a phantom watchdog.
|
||||
// `> 1` (was `!== 1`): PID 1 is the documented legitimate-init-child
|
||||
// skip; PID 0 is the new "parent already gone at install time" report
|
||||
// from the Windows liveness reader — installing an interval that
|
||||
// compares 0 to 0 forever would be a phantom watchdog, and stdin
|
||||
// 'close' already covers a parent that died before we booted.
|
||||
const initialParentPid = deps.getParentPid();
|
||||
if (initialParentPid !== 1) {
|
||||
if (initialParentPid > 1) {
|
||||
if (!deps.probeWatchdog()) {
|
||||
deps.log(
|
||||
'[gbrain serve] watchdog disabled: ps unavailable, parent-death detection unavailable — child will rely on stdin EOF / signals only',
|
||||
'[gbrain serve] watchdog disabled: no parent-liveness mechanism (ps / signal-0 probe failed) — child will rely on stdin EOF / signals only',
|
||||
);
|
||||
} else {
|
||||
parentWatchdog = deps.setInterval(() => {
|
||||
@@ -315,6 +330,23 @@ function installStdioLifecycle(
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Signal-0 process-liveness probe (`process.kill(pid, 0)` — existence
|
||||
* check only, no signal delivered; OpenProcess under the hood on
|
||||
* Windows). EPERM means the PID exists but we lack rights to signal it
|
||||
* — that is still "alive" for watchdog purposes. Exported for direct
|
||||
* unit testing of the Windows watchdog path.
|
||||
*/
|
||||
export function isPidAlive(pid: number): boolean {
|
||||
if (!Number.isInteger(pid) || pid <= 0) return false;
|
||||
try {
|
||||
process.kill(pid, 0);
|
||||
return true;
|
||||
} catch (err) {
|
||||
return (err as NodeJS.ErrnoException).code === 'EPERM';
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve the live parent PID from the kernel (not the cached startup
|
||||
* value). Both Node and Bun expose `process.ppid` as a property captured
|
||||
@@ -325,14 +357,36 @@ function installStdioLifecycle(
|
||||
* indefinitely while `ps -o ppid= -p $$` reports the new parent within
|
||||
* one tick.
|
||||
*
|
||||
* Cost: ~10ms per spawn. Called every 5s (PARENT_WATCHDOG_INTERVAL_MS),
|
||||
* so amortized < 0.5% CPU. Falls back to `process.ppid` if `ps` fails
|
||||
* (best-effort safety net for stripped-down containers, etc.); the
|
||||
* startup probe at watchdog-install time loud-logs and skips the
|
||||
* interval entirely when ps is unavailable, so a per-tick fallback is
|
||||
* a redundant safety net rather than a primary mechanism.
|
||||
* Windows has no `ps` — the original ps-only implementation made the
|
||||
* startup probe fail on every Windows host, so the watchdog was always
|
||||
* disabled and an orphaned serve (e.g. its cmd.exe .bat wrapper killed
|
||||
* by the MCP host without closing stdin) held the PGLite write lock
|
||||
* indefinitely. But Windows also never re-parents orphans, so the
|
||||
* cached `process.ppid` stays correct for the process's lifetime and
|
||||
* the question inverts from "did the live PPID change?" to "is the
|
||||
* original parent still alive?" — answered in-process via signal-0,
|
||||
* no external binary needed. Parent dead → report 0 (kernel PID 0 is
|
||||
* the System Idle Process, never our parent), which differs from
|
||||
* `initialParentPid` and fires the watchdog. Known degraded mode:
|
||||
* Windows recycles PIDs aggressively, so a reused parent PID can mask
|
||||
* a death — stdin EOF / signals remain the primary shutdown channels
|
||||
* and the watchdog stays the backstop, same posture as POSIX.
|
||||
*
|
||||
* Cost: ~10ms per ps spawn on POSIX, effectively free on Windows.
|
||||
* Called every 5s (PARENT_WATCHDOG_INTERVAL_MS), so amortized < 0.5%
|
||||
* CPU. Falls back to `process.ppid` if `ps` fails (best-effort safety
|
||||
* net for stripped-down containers, etc.); the startup probe at
|
||||
* watchdog-install time loud-logs and skips the interval entirely when
|
||||
* no mechanism is available, so a per-tick fallback is a redundant
|
||||
* safety net rather than a primary mechanism.
|
||||
*
|
||||
* `platform` is a test seam (defaults to the real platform) so CI on
|
||||
* any OS can exercise both branches — signal-0 works everywhere.
|
||||
*/
|
||||
function readLiveParentPid(): number {
|
||||
export function readLiveParentPid(platform: NodeJS.Platform = process.platform): number {
|
||||
if (platform === 'win32') {
|
||||
return isPidAlive(process.ppid) ? process.ppid : 0;
|
||||
}
|
||||
try {
|
||||
const r = spawnSync('ps', ['-o', 'ppid=', '-p', String(process.pid)], {
|
||||
encoding: 'utf8',
|
||||
@@ -349,12 +403,14 @@ function readLiveParentPid(): number {
|
||||
}
|
||||
|
||||
/**
|
||||
* One-shot probe at watchdog-install time to confirm ps actually works
|
||||
* on this host. Returns true iff `spawnSync('ps','-o','ppid=','-p',PID)`
|
||||
* exits 0 with a parseable integer. When it returns false, the caller
|
||||
* skips installing the watchdog and emits a loud stderr line — the
|
||||
* operator sees "watchdog disabled" instead of an installed-but-never-
|
||||
* fires phantom.
|
||||
* One-shot probe at watchdog-install time to confirm the platform's
|
||||
* parent-liveness mechanism actually works on this host. POSIX: true
|
||||
* iff `spawnSync('ps','-o','ppid=','-p',PID)` exits 0 with a parseable
|
||||
* integer. Windows: true iff signal-0 succeeds against our own PID
|
||||
* (always alive — verifies the mechanism, not the parent). When it
|
||||
* returns false, the caller skips installing the watchdog and emits a
|
||||
* loud stderr line — the operator sees "watchdog disabled" instead of
|
||||
* an installed-but-never-fires phantom.
|
||||
*
|
||||
* Why a separate probe rather than relying on the per-tick fallback in
|
||||
* `readLiveParentPid`: the per-tick fallback returns the cached
|
||||
@@ -363,7 +419,10 @@ function readLiveParentPid(): number {
|
||||
* while still claiming to be active. The probe surfaces the gap once
|
||||
* at install time and lets the caller short-circuit cleanly.
|
||||
*/
|
||||
function probeWatchdogAvailable(): boolean {
|
||||
export function probeWatchdogAvailable(platform: NodeJS.Platform = process.platform): boolean {
|
||||
if (platform === 'win32') {
|
||||
return isPidAlive(process.pid);
|
||||
}
|
||||
try {
|
||||
const r = spawnSync('ps', ['-o', 'ppid=', '-p', String(process.pid)], {
|
||||
encoding: 'utf8',
|
||||
|
||||
@@ -1,6 +1,13 @@
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import { EventEmitter } from 'events';
|
||||
import { runServe, type ServeOptions } from '../src/commands/serve';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import {
|
||||
runServe,
|
||||
isPidAlive,
|
||||
readLiveParentPid,
|
||||
probeWatchdogAvailable,
|
||||
type ServeOptions,
|
||||
} from '../src/commands/serve';
|
||||
import type { BrainEngine } from '../src/core/engine';
|
||||
|
||||
// These tests cover the stdio lifecycle hooks added to runServe so that the
|
||||
@@ -297,7 +304,7 @@ describe('runServe stdio lifecycle', () => {
|
||||
|
||||
// Watchdog NOT installed — message matches behavior.
|
||||
expect(h.timers.active()).toBe(0);
|
||||
expect(h.logs.some(l => l.includes('[gbrain serve] watchdog disabled: ps unavailable'))).toBe(true);
|
||||
expect(h.logs.some(l => l.includes('[gbrain serve] watchdog disabled: no parent-liveness mechanism'))).toBe(true);
|
||||
|
||||
// Sanity: the other lifecycle paths still work — the shutdown still
|
||||
// funnels through stdin EOF / signals, just not via the watchdog.
|
||||
@@ -501,3 +508,56 @@ describe('runServe stdio lifecycle', () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// Default watchdog implementations — the platform split that decides
|
||||
// whether the watchdog can run at all. The injected-seam tests above
|
||||
// never touch these; before this suite existed, the Windows branch had
|
||||
// zero coverage and the ps-only default silently disabled the watchdog
|
||||
// on every Windows host (orphaned serve → PGLite write lock held until
|
||||
// reboot). Signal-0 works on every platform Node/Bun support, so the
|
||||
// win32 branch is exercised on POSIX CI via the platform test seam.
|
||||
describe('watchdog platform defaults', () => {
|
||||
test('isPidAlive: our own PID is alive', () => {
|
||||
expect(isPidAlive(process.pid)).toBe(true);
|
||||
});
|
||||
|
||||
test('isPidAlive: rejects non-PIDs without probing', () => {
|
||||
expect(isPidAlive(0)).toBe(false);
|
||||
expect(isPidAlive(-1)).toBe(false);
|
||||
expect(isPidAlive(1.5)).toBe(false);
|
||||
expect(isPidAlive(NaN)).toBe(false);
|
||||
});
|
||||
|
||||
test('isPidAlive: an exited child is dead', () => {
|
||||
// Spawn a trivial child and let it exit; its PID must then probe
|
||||
// dead. PID reuse between exit and probe is theoretically possible
|
||||
// but the window is microseconds — acceptable for a unit test of
|
||||
// the same mechanism the production watchdog relies on.
|
||||
const r = spawnSync(process.execPath, ['-e', ''], { timeout: 10_000 });
|
||||
expect(r.pid).toBeGreaterThan(0);
|
||||
expect(isPidAlive(r.pid as number)).toBe(false);
|
||||
});
|
||||
|
||||
test('readLiveParentPid(win32): reports cached ppid while parent is alive', () => {
|
||||
// The test runner's parent (bun's spawner / the shell) is alive, so
|
||||
// the Windows reader must report the cached ppid unchanged — a
|
||||
// healthy tick that must NOT fire the watchdog.
|
||||
expect(readLiveParentPid('win32')).toBe(process.ppid);
|
||||
});
|
||||
|
||||
test('probeWatchdogAvailable(win32): signal-0 mechanism is always available', () => {
|
||||
// No external binary involved — the probe verifies signal-0 against
|
||||
// our own (always-alive) PID. This is the line that un-disables the
|
||||
// watchdog on Windows hosts.
|
||||
expect(probeWatchdogAvailable('win32')).toBe(true);
|
||||
});
|
||||
|
||||
test('readLiveParentPid(default platform): returns a usable integer PID', () => {
|
||||
// POSIX: live kernel PPID via ps (or the cached-ppid fallback).
|
||||
// Windows: liveness-checked cached ppid. Either way the watchdog
|
||||
// install site needs an integer >= 0.
|
||||
const n = readLiveParentPid();
|
||||
expect(Number.isInteger(n)).toBe(true);
|
||||
expect(n).toBeGreaterThanOrEqual(0);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user