mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-27 21:19:18 +00:00
* fix: zombie process accumulation + health endpoint timeout Three fixes for cascading failure mode in long-running deployments: 1. cli.ts: Install SIGCHLD handler to reap zombie children. Bun (like Node) only auto-reaps when a handler is registered. Without this, child processes spawned by the worker (embed batches, shell jobs, sub-agents) become zombies when they exit, accumulating in the PID table. 2. serve-http.ts: Add 5s timeout to /health endpoint's getStats() call. When the DB connection pool is saturated (e.g., from zombie processes holding phantom connections), getStats() hangs indefinitely, making the server appear dead to health checks even though it's running. 3. worker.ts: Call engine.disconnect() in the finally block after draining in-flight jobs. Releases PgBouncer connection slots immediately on shutdown rather than waiting for TCP keepalive expiry. 4. supervisor.ts + autopilot.ts: Auto-detect tini on PATH and wrap the spawned worker with it. Belt-and-suspenders with the SIGCHLD handler — tini catches children spawned by native addons that bypass the JS event loop. Zero-config: works when tini is installed, silently skips when not. * refactor(zombie-reap): extract idempotent SIGCHLD installer module Extract the inline SIGCHLD handler from cli.ts into a small dedicated module so it's testable directly without importing cli.ts (which invokes main() at module load — incompatible with bun:test imports). The new installSigchldHandler() uses a named module-level handler + includes() check to dedupe across hot-import scenarios. EventEmitter does NOT dedupe listeners by reference, so without this guard a re-import of zombie-reap.ts would accumulate handlers. _uninstallSigchldHandlerForTests() is the test-only escape hatch so test/zombie-reap.test.ts's afterAll can prevent cross-file listener accumulation in the parallel shard process — codex review #6 noted that mutating global process signal listeners in parallel pools is a leak class the isolation lint doesn't protect against. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(spawn-helpers): extract detectTini + buildSpawnInvocation; DRY-consolidate supervisor + autopilot Pulls the duplicated tini detection + (cmd, args) composition out of src/core/minions/supervisor.ts and src/commands/autopilot.ts into a single src/core/minions/spawn-helpers.ts module that both consume. Side effects: - Autopilot now resolves tini ONCE at startup instead of shelling out via execSync('which tini') on every worker respawn (every restart-after-crash path lost ~1ms + a fork to /usr/bin/which). - detectTini() passes env: process.env explicitly to execFileSync. Bun snapshots env at startup; without this, runtime PATH mutations (in tests via withEnv, or in any prod code that ever changes PATH) are invisible to `which`. Tiny correctness fix that also makes the test work. - MinionSupervisor gains an `isTiniDetected` read-only accessor so test/supervisor-tini.test.ts can assert the constructor wired tini correctly without exposing the resolved path or needing to spawn the full lifecycle. The existing worker_spawned event payload still carries {tini: true} for runtime observability (per codex review #5). Test coverage: - test/spawn-helpers.test.ts: pure function tests for both helpers (with-tini / without-tini / empty-args / detectTini smoke) - test/supervisor-tini.test.ts: constructor wiring with PATH stripped vs. PATH containing a fake-tini script in a tmpdir Both files are *.test.ts (parallel-safe) and pass scripts/check-test-isolation.sh without new allow-list entries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(serve-http): extract probeHealth() + drop /health timeout 5s -> 3s Three changes folded into one commit because they touch the same route handler and would conflict if split: 1. Extract probeHealth(engine, engineName, version, timeoutMs) as a pure exported function. Route handler becomes one branchless line: res.status(result.status).json(result.body) This makes the timeout / db-error / happy paths unit-testable directly without an Express test client and without a hardcoded 5000 literal inside the route closure. 2. Export HEALTH_TIMEOUT_MS = 3000 (was inline 5000). Fly.io default health-check timeout is 5s; at 5s exact, the orchestrator may record a request as a timeout instead of getting the 503 (race). 3s gives 2s of headroom for TCP, response framing, and clock skew. The DB-pool-saturation signal still surfaces; we just stop racing the orchestrator deadline. 3. The route handler shape change (4 try/catch lines -> 1 wrapper line) keeps response semantics identical for all three paths. Test coverage: - test/serve-http-health.test.ts: 4 cases (happy / timeout / db-error / exported constant). Calls probeHealth directly with mock engines whose getStats() resolves / rejects / hangs forever. Wall-clock per test bounded by passing timeoutMs: 100. - Existing test/e2e/serve-http-oauth.test.ts /health happy-path case still covers the Express wiring (one-line route handler is identical Express plumbing for 200 and 503). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(worker): log engine.disconnect errors during shutdown instead of swallowing Replace bare \`try { await this.engine.disconnect(); } catch {}\` with \`catch (e) { console.error('[worker] disconnect failed during shutdown:', e); }\`. Why: shutdown is best-effort, but the original silent catch was exactly the bug class the v0.26.9 D14 direction (isUndefinedColumnError swap-in on oauth-provider.ts) was created to surface. If a future regression breaks pool teardown so disconnect rejects, we'll never know without an audit log line. Two-character diff to the catch, no behavior change for the happy path. Test coverage in test/worker-shutdown-disconnect.test.ts: - Happy path: disconnect spy called once during shutdown (intercept-only, not call-through, so the shared engine stays connected for the next test in the file). - Error path: disconnect throws, error is logged with the \`[worker] disconnect failed during shutdown:\` prefix and the bare Error as second arg, and start() still resolves (no rethrow). Spy via spyOn() on the engine instance — object-level, not module-level, so R2 of scripts/check-test-isolation.sh (which forbids module-level mocks in non-serial unit tests) is satisfied. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(e2e): real-binary zombie reaping reproduction (DATABASE_URL-gated) Spawns the gbrain CLI as \`bun run src/cli.ts jobs work --concurrency 1\` against a real Postgres with GBRAIN_ALLOW_SHELL_JOBS=1, submits a shell job from the CLI side (remote: false, bypasses the v0.26.9 RCE gate), captures the worker's shell child PID from the job result, sleeps 300ms, then \`ps -o stat= -p <pid>\` to assert the process is NOT lingering as a zombie (Z state). Why this shape: - \`gbrain serve --http\` was the original plan but doesn't start a worker (only the MCP server) AND submit_job over MCP carries remote: true, which rejects shell at operations.ts:1391 (the v0.26.9 RCE-fix gate). jobs work + CLI-side submit is the only architecture that boots through cli.ts (so installSigchldHandler() actually runs) and lets a shell job execute. - \`shell\` requires absolute cwd (shell.ts:53). Payload includes cwd: '/tmp'. - ps check is run while the worker is STILL ALIVE (no PID-recycle race — worker holds the process tree, so the captured PID is meaningful). Negative control (manual, NOT in CI, documented in test header): Comment out installSigchldHandler() in src/cli.ts -> rebuild -> re-run -> expect stat=Z. Re-enable -> expect stat empty (process gone, reaped). Demonstrates the test catches the regression class without paying CI cost for a separate broken-build target. Skips: - DATABASE_URL not set (matches existing E2E pattern in helpers.ts) - Windows (POSIX-only; tini and SIGCHLD don't exist there) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(postgres-engine): make disconnect() idempotent so it doesn't clobber the module-level singleton PostgresEngine.disconnect() was non-idempotent: after the first call ended \`_sql\` and set it to null, a second call fell through to the \`else\` branch that calls db.disconnect() — which clears the GLOBAL module-level connection used by helpers.ts, the CLI main path, and every test that hadn't opted into a private pool. This bit minions-shell.test.ts and the entire downstream E2E suite when commit671ef099(in this branch) added engine.disconnect() to MinionWorker.start()'s finally block. Tests that did: await worker.start(); // worker disconnects (was the new behavior) await engine.disconnect(); // test cleanup; pre-fix fell through // to db.disconnect() and killed // the global connection …would silently kill the helpers.ts singleton, and the next test in the file would fail in its beforeEach with "No database connection". Fix: track \`_connectionStyle\` ('instance' | 'module' | null) on the engine and only call db.disconnect() when this engine actually owns the global. After ending an instance-pool, _connectionStyle stays 'instance' so a second disconnect() is a no-op rather than a side-effect. Test coverage: test/e2e/postgres-engine-disconnect-idempotency.test.ts pins both contracts: - instance-pool engine: second disconnect MUST NOT clobber the module singleton (the bug above). - module-singleton engine: second disconnect is a no-op (resolves cleanly, no throw). Required for: minions-shell.test.ts to keep passing alongside the worker changes on this branch. Discovered during E2E sweep after the unit-test green light. Commit 7 in this branch then walks back the worker-side disconnect entirely (engine ownership belongs to the CLI handler) but this idempotency fix stays in place as a defense-in-depth guard against any future code calling disconnect twice on the same engine. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: move engine.disconnect() from worker.start() to gbrain jobs work CLI handler (engine ownership) Commit671ef099(the original fix in this branch) put \`await this.engine.disconnect()\` inside MinionWorker.start()'s finally block to free PgBouncer pool slots immediately on shutdown. That was the right intent on the wrong layer: the worker doesn't own the engine, the CLI handler that creates the engine does. The mismatched ownership broke every test that shares a single engine across multiple worker.start() / worker.stop() cycles: - test/e2e/minions-shell-pglite.test.ts → shared PGLite engine, second test failed with "PGLite not connected" - test/e2e/worker-abort-recovery.test.ts → 3 tests, same shape - test/e2e/minions-shell.test.ts → 3 Postgres tests broken by the second-disconnect-clobbers-global-singleton symptom (commit 6 of this branch fixed the underlying engine non-idempotency, but the worker-disconnect call was still wrong on its own) Fix: - worker.ts: remove the engine.disconnect() call. Add a comment documenting WHY the worker doesn't disconnect (ownership invariant) so a future contributor doesn't put it back. - src/commands/jobs.ts case 'work': wrap worker.start() in a try/finally that calls engine.disconnect() on shutdown. The CLI created the engine (line 631 area), so the CLI disposes of it. Disconnect failure logs to stderr with the "[gbrain jobs work] engine disconnect failed during shutdown:" prefix rather than the bare \`catch {}\` of earlier waves — matches the v0.26.9 D14 direction of preferring loud-but-best-effort over silent. Test: - test/worker-shutdown-disconnect.test.ts now pins the inverse invariant: worker.start() MUST NOT call engine.disconnect(), and the engine MUST remain queryable after start() returns. Two tests, instance-level spy, parallel-safe (no module mocking). End state: gbrain jobs work in production still frees pool slots immediately on shutdown (intent of671ef099preserved), tests that share an engine don't break (regression class fixed), and the engine ownership invariant is now codified in code AND in the test suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: clearTimeout in probeHealth race + platform guard SIGCHLD on Windows Two adversarial-review auto-fixes from /ship's pre-landing review pass. Both reviewers (Claude adversarial subagent + Codex adversarial) flagged the timer leak independently; Codex additionally caught the Windows crash risk. 1. probeHealth race timer leak (serve-http.ts): `Promise.race([getStats(), setTimeout(...)])` doesn't cancel the loser. Without `clearTimeout`, every fast /health request leaves a 3s pending timer in the event loop until it fires. Under sustained probe rates (Fly.io polls every ~10s, orchestrator load balancers can be much tighter), this builds a rolling backlog of timers and avoidable event loop wakeups in the hottest endpoint. Capture the timer handle, clear it in a `finally` block. No-op when the timer already fired. 2. SIGCHLD platform guard (zombie-reap.ts): SIGCHLD is POSIX-only. On Windows, `process.on('SIGCHLD', ...)` throws ENOTSUP because Windows doesn't have signals. Bun behaves the same. Without this guard, any future Windows port of a gbrain CLI tool would crash at boot before main() even runs. The zombie-reaping fix is itself POSIX-only (tini, ps, /proc), so the guard is consistent with the platform's capability set. NOT in this commit (intentionally out of scope): - Cancelling engine.getStats() when /health times out. Both reviewers noted this would need AbortController support in the engine layer which doesn't exist yet. The 503 timeout already improves on master's hang behavior; full cancellation is a follow-up. - Switching /health to a lighter probe (SELECT 1 instead of count(*) across 6 tables). Pre-existing behavior; refactoring the probe shape is wider blast radius than this branch's zombie-reaping scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: bump version and changelog (v0.28.1) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: update CLAUDE.md for v0.28.1 zombie reaping + health + engine ownership Add v0.28.1 file annotations covering: - src/core/zombie-reap.ts (new) — Layer 1 SIGCHLD reaper module - src/core/minions/spawn-helpers.ts (new) — pure detectTini + buildSpawnInvocation helpers - src/core/minions/worker.ts — engine-ownership invariant (no engine.disconnect) - src/core/minions/supervisor.ts — consumes spawn-helpers, exposes isTiniDetected - src/commands/serve-http.ts — probeHealth() + HEALTH_TIMEOUT_MS = 3000 - src/commands/jobs.ts — case 'work' owns engine lifecycle via try/finally - src/commands/autopilot.ts — resolves tini once at startup - src/core/postgres-engine.ts — disconnect() is idempotent via _connectionStyle Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Wintermute <wintermute@garrytan.com> Co-authored-by: Garry Tan <garrytan@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
49 lines
1.8 KiB
TypeScript
49 lines
1.8 KiB
TypeScript
/**
|
|
* Tests for src/core/minions/spawn-helpers.ts — pure helpers that build the
|
|
* (cmd, args) tuple for spawning the gbrain worker, optionally wrapped in
|
|
* tini for zombie reaping.
|
|
*
|
|
* `buildSpawnInvocation` is a pure function — directly testable without any
|
|
* mocking. `detectTini` shells out to `which tini`; the test asserts only
|
|
* that it returns a string (presence depends on the test machine).
|
|
*/
|
|
|
|
import { describe, test, expect } from 'bun:test';
|
|
import { buildSpawnInvocation, detectTini } from '../src/core/minions/spawn-helpers.ts';
|
|
|
|
describe('buildSpawnInvocation', () => {
|
|
test('without tini: returns cliPath + raw args', () => {
|
|
const result = buildSpawnInvocation('', '/bin/gbrain', ['jobs', 'work']);
|
|
expect(result).toEqual({ cmd: '/bin/gbrain', args: ['jobs', 'work'] });
|
|
});
|
|
|
|
test('with tini: wraps cliPath with tini and "--" separator', () => {
|
|
const result = buildSpawnInvocation('/usr/bin/tini', '/bin/gbrain', ['jobs', 'work']);
|
|
expect(result).toEqual({
|
|
cmd: '/usr/bin/tini',
|
|
args: ['--', '/bin/gbrain', 'jobs', 'work'],
|
|
});
|
|
});
|
|
|
|
test('empty args list is preserved on both branches', () => {
|
|
expect(buildSpawnInvocation('', '/bin/gbrain', [])).toEqual({
|
|
cmd: '/bin/gbrain',
|
|
args: [],
|
|
});
|
|
expect(buildSpawnInvocation('/usr/bin/tini', '/bin/gbrain', [])).toEqual({
|
|
cmd: '/usr/bin/tini',
|
|
args: ['--', '/bin/gbrain'],
|
|
});
|
|
});
|
|
});
|
|
|
|
describe('detectTini', () => {
|
|
test('returns a string (smoke test only — actual presence depends on machine)', () => {
|
|
const result = detectTini();
|
|
expect(typeof result).toBe('string');
|
|
// Do NOT assert truthiness: tini may or may not be installed on the
|
|
// test host. We only verify the function doesn't throw and returns
|
|
// a defined string ('' when absent, path when present).
|
|
});
|
|
});
|