Files
gbrain/test/supervisor-tini.test.ts
T
2ea5b71177 v0.28.1 fix: zombie process accumulation + health endpoint timeout (#637)
* 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
commit 671ef099 (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)

Commit 671ef099 (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 of 671ef099 preserved), 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>
2026-05-06 11:54:06 -07:00

66 lines
2.5 KiB
TypeScript

/**
* Tests for MinionSupervisor's tini detection wiring.
*
* Per the eng-review + codex review: the seam is the supervisor's
* `isTiniDetected` accessor (added for testability) and the existing
* `worker_spawned` event payload at supervisor.ts:483-487 which already
* includes `{tini: true}` when tini is in use. Spawning the full lifecycle
* for a tini-presence assertion is overkill; the constructor reads tiniPath
* once via `detectTini()`, and tests control the answer via PATH.
*
* Two cases:
* 1. tini absent (PATH stripped of any directory containing tini) → false
* 2. tini present (PATH points at a tmpdir with a fake `tini` script) → true
*
* Uses withEnv from test/helpers/with-env.ts so PATH mutations restore
* cleanly even if the test throws.
*/
import { describe, test, expect } from 'bun:test';
import { mkdirSync, writeFileSync, chmodSync, rmSync } from 'fs';
import { join } from 'path';
import { tmpdir } from 'os';
import { MinionSupervisor } from '../src/core/minions/supervisor.ts';
import type { BrainEngine } from '../src/core/engine.ts';
import { withEnv } from './helpers/with-env.ts';
const mockEngine: Partial<BrainEngine> = {
kind: 'postgres' as const,
executeRaw: async () => [],
} as unknown as BrainEngine;
describe('MinionSupervisor tini detection', () => {
test('isTiniDetected = false when tini is not on PATH', async () => {
// Empty PATH so `which tini` cannot find anything.
await withEnv({ PATH: '' }, async () => {
const supervisor = new MinionSupervisor(mockEngine as BrainEngine, {
cliPath: '/bin/echo',
});
expect(supervisor.isTiniDetected).toBe(false);
});
});
test('isTiniDetected = true when a tini binary exists on PATH', async () => {
const dir = join(
tmpdir(),
`gbrain-supervisor-tini-test-${process.pid}-${Date.now()}`,
);
mkdirSync(dir, { recursive: true });
const fakeTini = join(dir, 'tini');
writeFileSync(fakeTini, '#!/bin/sh\nexec "$@"\n', 'utf8');
chmodSync(fakeTini, 0o755);
try {
// Prepend our fake-tini directory so `which tini` resolves to it.
// Keep `/usr/bin:/bin` so `which` itself is locatable on macOS/Linux.
await withEnv({ PATH: `${dir}:/usr/bin:/bin` }, async () => {
const supervisor = new MinionSupervisor(mockEngine as BrainEngine, {
cliPath: '/bin/echo',
});
expect(supervisor.isTiniDetected).toBe(true);
});
} finally {
rmSync(dir, { recursive: true, force: true });
}
});
});