diff --git a/CHANGELOG.md b/CHANGELOG.md index 2823a8c42..fe18c7f88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,77 @@ All notable changes to GBrain will be documented in this file. +## [0.28.1] - 2026-05-06 + +## **Long-running deployments stop drowning in zombie processes. /health stops racing the orchestrator. Pool slots free immediately on shutdown.** +## **Three independent fixes, one production cascade. The worker reaps its own children. /health returns 503 fast enough that Fly.io sees it. Engine ownership is fixed so tests don't fight the worker over engine lifecycle.** + +The zombie-process cascade was the bug class that turned a slow query into a dead server. Children spawned by the worker (shell jobs, embed batches, sub-agents) became zombies on exit because Bun (like Node) only auto-reaps when a SIGCHLD listener is registered. Phantom zombies held PgBouncer connection slots, the pool saturated, `getStats()` hung indefinitely, `/health` timed out beyond the orchestrator's deadline, and Fly.io concluded the server was dead. The fix is a layered defense in depth: an in-process SIGCHLD reaper for JS-spawned children, tini-as-PID-1 wrapping the worker subtree for native-addon spawns, AlphaClaw's container-level tini for the case where Bun itself crashes hard. + +### What you can now do that you couldn't before + +- **Run gbrain as a long-lived worker without leaking zombies.** `gbrain jobs work` and the supervisor both wrap the worker in tini when it's on `PATH`. Without tini, the new in-process SIGCHLD handler still reaps anything spawned through Bun's event loop. Zero-config: works whether tini is installed or not. +- **Trust `/health` to return fast under DB pressure.** The probe now races `engine.getStats()` against a 3s timeout (down from 5s, giving Fly.io's 5s health-check timeout 2s of headroom for TCP, response framing, and clock skew). When the pool is saturated, `/health` returns 503 with `Health check timed out (database pool may be saturated)` instead of hanging until the orchestrator times the request out. +- **Restart `gbrain jobs work` and free pool slots immediately.** The CLI handler now disconnects the engine in a try/finally around `worker.start()`, releasing PgBouncer slots on the same shutdown that previously waited for TCP keepalive to expire. + +### The numbers that matter + +| Failure mode | Before (v0.27.0) | After (v0.28.1) | +|---|---|---| +| Zombie children from shell jobs | accumulated in PID table | reaped via SIGCHLD or tini | +| `/health` under saturated pool | hung indefinitely | 503 within 3s | +| `gbrain jobs work` shutdown | TCP keepalive (~minutes) to free slots | immediate engine.disconnect() | +| Fly.io health-check race | timeout at 5s exactly | 2s headroom | + +### What this means for your deployment + +If you were running `gbrain serve --http` or `gbrain jobs work` long enough that workers piled up zombies, the cascade is closed. If you're on Fly.io or any orchestrator with a 5s health-check timeout, your saturated-pool 503s now arrive in time to be observed. If your container image doesn't already have tini-as-PID-1, install tini in your image and gbrain auto-wraps the worker. If you have AlphaClaw's tini setup, this branch composes correctly with it — three layers, no overlap, no redundancy. + +### Itemized changes + +#### Added +- `src/core/zombie-reap.ts` — idempotent `installSigchldHandler()` so JS-spawned children get reaped via Bun's internal `waitpid()`. Cross-file leak guard via `_uninstallSigchldHandlerForTests()` for tests. +- `src/core/minions/spawn-helpers.ts` — pure `detectTini()` + `buildSpawnInvocation()` helpers consumed by both `supervisor.ts` and `autopilot.ts`. Resolves the DRY violation between the two spawn sites and makes the tini wrapping testable without `mock.module()`. +- `HEALTH_TIMEOUT_MS = 3000` exported from `src/commands/serve-http.ts` plus `probeHealth()` as a pure async function. +- `MinionSupervisor.isTiniDetected` read-only accessor for tests. +- `_connectionStyle` field on `PostgresEngine` so `disconnect()` is fully idempotent. +- Test coverage: 14 unit tests across 5 files (zombie-reap, spawn-helpers, serve-http-health, supervisor-tini, worker-shutdown-disconnect) + 3 E2E tests (zombie-reaping real-binary reproduction, postgres-engine-disconnect-idempotency × 2). All parallel-safe, no `mock.module()`, `scripts/check-test-isolation.sh` passes without new allow-list entries. + +#### Changed +- `src/cli.ts` calls `installSigchldHandler()` at module load (with Windows platform guard — SIGCHLD doesn't exist on Windows). +- `src/commands/autopilot.ts` resolves tini once at startup instead of per worker respawn. +- `src/commands/serve-http.ts` `/health` route is now a one-line wrapper around `probeHealth()`. +- `MinionWorker.start()` no longer calls `engine.disconnect()`. The CLI handler in `src/commands/jobs.ts case 'work'` owns engine lifecycle via try/finally with loud error logging on disconnect failure. +- `PostgresEngine.disconnect()` is now idempotent — second call on an instance-pool engine is a no-op rather than falling through to `db.disconnect()` and clobbering the module-level singleton. + +#### Fixed +- Zombie process accumulation in long-running worker deployments. +- `/health` hang when PgBouncer pool is saturated (cascading orchestrator timeout → false-positive server-down). +- `PostgresEngine.disconnect()` non-idempotent behavior that broke any test sharing an engine across multiple worker.start() / worker.stop() cycles. +- `probeHealth()` race timer leak — `clearTimeout` in the finally block prevents pending-timer pile-up under high probe rates. +- `detectTini()` env-snapshot bug where Bun's `execFileSync` didn't see runtime PATH mutations (passes `env: process.env` explicitly now). +- Engine-ownership invariant violation where the worker disconnected an engine it didn't own. + +### For contributors +- Adversarial review found 19 issues across two reviewers (Claude + Codex). 2 auto-fixed (clearTimeout, Windows platform guard); rest informational or pre-existing. +- Test-isolation lint at `scripts/check-test-isolation.sh` rule R2 (no `mock.module()` in non-serial unit tests) drove the pure-helper extraction. Trying to test the spawn flow with `mock.module()` would have forced 5 quarantine entries. +- `test/worker-shutdown-disconnect.test.ts` pins the engine-ownership invariant so a future refactor can't silently re-introduce `engine.disconnect()` to `worker.start()`. The test asserts the inverse: `disconnectSpy).not.toHaveBeenCalled()`. + +## To take advantage of v0.28.1 + +`gbrain upgrade` should do this automatically. If it didn't, no manual action is needed — the fixes are runtime-only, no schema migration. + +1. **Verify tini is installed (recommended for long-running deployments):** + ```bash + which tini + ``` + If empty, install via `apk add tini` (Alpine), `apt-get install tini` (Debian/Ubuntu), or `brew install tini` (macOS dev). gbrain will auto-detect on next supervisor/autopilot start. +2. **Verify the SIGCHLD reaper is wired:** + ```bash + gbrain --version # should print 0.28.1+ + ``` +3. **If `gbrain doctor` warns about anything:** please file an issue: https://github.com/garrytan/gbrain/issues with output of `gbrain doctor` and contents of `~/.gbrain/upgrade-errors.jsonl` if it exists. + ## [0.27.0] - 2026-04-28 ## **GBrain runs on any embedding stack. OpenAI, Google Gemini, Ollama, Voyage, or anything via LiteLLM — one config line away.** diff --git a/CLAUDE.md b/CLAUDE.md index 24172bfdc..e8447f94f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,7 +45,7 @@ strict behavior when unset. - `src/core/engine-factory.ts` — Engine factory with dynamic imports (`'pglite'` | `'postgres'`) - `src/core/pglite-engine.ts` — PGLite (embedded Postgres 17.5 via WASM) implementation, all 40 BrainEngine methods. `addLinksBatch` / `addTimelineEntriesBatch` use multi-row `unnest()` with manual `$N` placeholders. As of v0.13.1, `connect()` wraps `PGlite.create()` in a try/catch that emits an actionable error naming the macOS 26.3 WASM bug (#223) and pointing at `gbrain doctor`; the lock is released on failure so the next process can retry cleanly. v0.22.0: `searchKeyword` and `searchKeywordChunks` multiply `ts_rank` by the source-factor CASE expression at the chunk-grain level; `searchVector` becomes a two-stage CTE — inner CTE keeps `ORDER BY cc.embedding <=> vec` so HNSW stays usable, outer SELECT re-ranks by `raw_score * source_factor`. Inner LIMIT scales with offset to preserve pagination contract. As of v0.22.6.1, `initSchema()` calls `applyForwardReferenceBootstrap()` BEFORE replaying SCHEMA_SQL — probes for the specific forward-referenced state the embedded schema blob needs (`pages.source_id`, `links.link_source`, `links.origin_page_id`, `content_chunks.symbol_name`, `content_chunks.language`, `sources` FK target table) and adds only what's missing. Closes the upgrade-wedge bug class that bit users 10+ times across 6 schema versions over 2 years (#239/#243/#266/#357/#366/#374/#375/#378/#395/#396). No-op on fresh installs and modern brains. - `src/core/pglite-schema.ts` — PGLite-specific DDL (pgvector, pg_trgm, triggers) -- `src/core/postgres-engine.ts` — Postgres + pgvector implementation (Supabase / self-hosted). `addLinksBatch` / `addTimelineEntriesBatch` use `INSERT ... SELECT FROM unnest($1::text[], ...) JOIN pages ON CONFLICT DO NOTHING RETURNING 1` — 4-5 array params regardless of batch size, sidesteps the 65535-parameter cap. As of v0.12.3, `searchKeyword` / `searchVector` scope `statement_timeout` via `sql.begin` + `SET LOCAL` so the GUC dies with the transaction instead of leaking across the pooled postgres.js connection (contributed by @garagon). `getEmbeddingsByChunkIds` uses `tryParseEmbedding` so one corrupt row skips+warns instead of killing the query. v0.22.0: `searchKeyword`, `searchKeywordChunks`, and `searchVector` apply source-aware ranking by inlining the source-factor CASE and `NOT (col LIKE …)` hard-exclude clause from `src/core/search/sql-ranking.ts`. `searchVector` switches to a two-stage CTE (HNSW-safe inner ORDER BY, source-boost re-rank in the outer SELECT) and carries `p.source_id` through inner→outer for v0.18 multi-source callers. v0.22.1 (#406): `_savedConfig` retains the connect config; `reconnect()` tears down + recreates the pool from saved config (called by supervisor watchdog after 3 consecutive health-check failures). `executeRaw` is a single-statement passthrough — no per-call retry (D3 dropped that as unsound for non-idempotent statements; recovery is supervisor-driven). v0.22.1 (#363, contributed by @orendi84): `connect()` applies `resolveSessionTimeouts()` from `db.ts` as connection-time startup parameters (`statement_timeout`, `idle_in_transaction_session_timeout`) so orphan pgbouncer backends can't hold locks for hours. v0.22.1 (#409, contributed by @atrevino47): `countStaleChunks()` + `listStaleChunks()` server-side-filter on `embedding IS NULL` for `embed --stale`, eliminating ~76 MB/call client-side pull on a fully-embedded brain; `upsertChunks()` resets both `embedding` AND `embedded_at` to NULL when chunk_text changes without a new embedding (consistency). As of v0.22.6.1, `initSchema()` calls `applyForwardReferenceBootstrap()` BEFORE replaying SCHEMA_SQL on the same forward-reference probe set as the PGLite engine, so old Postgres brains pinned at v0.13/v0.18/v0.19 walk forward cleanly instead of wedging on `column "..." does not exist`. +- `src/core/postgres-engine.ts` — Postgres + pgvector implementation (Supabase / self-hosted). `addLinksBatch` / `addTimelineEntriesBatch` use `INSERT ... SELECT FROM unnest($1::text[], ...) JOIN pages ON CONFLICT DO NOTHING RETURNING 1` — 4-5 array params regardless of batch size, sidesteps the 65535-parameter cap. As of v0.12.3, `searchKeyword` / `searchVector` scope `statement_timeout` via `sql.begin` + `SET LOCAL` so the GUC dies with the transaction instead of leaking across the pooled postgres.js connection (contributed by @garagon). `getEmbeddingsByChunkIds` uses `tryParseEmbedding` so one corrupt row skips+warns instead of killing the query. v0.22.0: `searchKeyword`, `searchKeywordChunks`, and `searchVector` apply source-aware ranking by inlining the source-factor CASE and `NOT (col LIKE …)` hard-exclude clause from `src/core/search/sql-ranking.ts`. `searchVector` switches to a two-stage CTE (HNSW-safe inner ORDER BY, source-boost re-rank in the outer SELECT) and carries `p.source_id` through inner→outer for v0.18 multi-source callers. v0.22.1 (#406): `_savedConfig` retains the connect config; `reconnect()` tears down + recreates the pool from saved config (called by supervisor watchdog after 3 consecutive health-check failures). `executeRaw` is a single-statement passthrough — no per-call retry (D3 dropped that as unsound for non-idempotent statements; recovery is supervisor-driven). v0.22.1 (#363, contributed by @orendi84): `connect()` applies `resolveSessionTimeouts()` from `db.ts` as connection-time startup parameters (`statement_timeout`, `idle_in_transaction_session_timeout`) so orphan pgbouncer backends can't hold locks for hours. v0.22.1 (#409, contributed by @atrevino47): `countStaleChunks()` + `listStaleChunks()` server-side-filter on `embedding IS NULL` for `embed --stale`, eliminating ~76 MB/call client-side pull on a fully-embedded brain; `upsertChunks()` resets both `embedding` AND `embedded_at` to NULL when chunk_text changes without a new embedding (consistency). As of v0.22.6.1, `initSchema()` calls `applyForwardReferenceBootstrap()` BEFORE replaying SCHEMA_SQL on the same forward-reference probe set as the PGLite engine, so old Postgres brains pinned at v0.13/v0.18/v0.19 walk forward cleanly instead of wedging on `column "..." does not exist`. **v0.28.1:** `disconnect()` is now idempotent. New `_connectionStyle` instance field tracks whether the engine owns its pool (worker engines) or shares the module-level singleton; second call on an instance-pool engine is a no-op rather than falling through to `db.disconnect()` and clobbering the singleton. Pinned by `test/e2e/postgres-engine-disconnect-idempotency.test.ts` (2 cases). Closes the bug class where any test sharing an engine across multiple `worker.start()` / `worker.stop()` cycles silently broke its own DB connectivity. - `src/core/utils.ts` — Shared SQL utilities extracted from postgres-engine.ts. Exports `parseEmbedding(value)` (throws on unknown input, used by migration + ingest paths where data integrity matters) and as of v0.12.3 `tryParseEmbedding(value)` (returns `null` + warns once per process, used by search/rescore paths where availability matters more than strictness). **v0.26.9 (D14):** adds `isUndefinedColumnError(err)` predicate — pattern-matches Postgres SQLSTATE 42703 / "column ... does not exist" with engine-driver shape variation tolerated. Replaces bare `catch {}` blocks in `oauth-provider.ts` so genuine errors (lock timeout, network blip, permission denied) propagate while column-missing falls through to the legacy fallback path. Reusable from any future code that needs the same column-existence probe semantics. - `src/core/db.ts` — Connection management, schema initialization. v0.22.1 (#363, contributed by @orendi84): `resolveSessionTimeouts()` returns `statement_timeout` + `idle_in_transaction_session_timeout` (defaults: 5min each, env-overridable via `GBRAIN_STATEMENT_TIMEOUT` / `GBRAIN_IDLE_TX_TIMEOUT` / `GBRAIN_CLIENT_CHECK_INTERVAL`). Both `connect()` (module singleton) and `PostgresEngine.connect()` (worker pool) consume the result via postgres.js's `connection` option, sending GUCs as startup parameters that survive PgBouncer transaction mode (unlike the prior `setSessionDefaults` post-pool SET, kept as a back-compat no-op shim). - `src/commands/migrate-engine.ts` — Bidirectional engine migration (`gbrain migrate --to supabase/pglite`) @@ -101,10 +101,12 @@ strict behavior when unset. - `src/commands/extract.ts` — `gbrain extract links|timeline|all [--source fs|db]`: batch link/timeline extraction. fs walks markdown files, db walks pages from the engine (mutation-immune snapshot iteration; use this for live brains with no local checkout). As of v0.12.1 there is no in-memory dedup pre-load — candidates are buffered 100 at a time and flushed via `addLinksBatch` / `addTimelineEntriesBatch`; `ON CONFLICT DO NOTHING` enforces uniqueness at the DB layer, and the `created` counter returns real rows inserted (truthful on re-runs). v0.22.1 (#417): `ExtractOpts.slugs?: string[]` enables incremental extract — when set, `extractForSlugs()` reads ONLY those slugs' files (single combined links+timeline pass) instead of the full directory walk. CLI `gbrain extract` keeps full-walk behavior; the cycle path threads sync's `pagesAffected` through. `walkMarkdownFiles(brainDir)` still runs at line 455 to build `allSlugs` for link resolution — see `TODOS.md` for replacing it with `engine.getAllSlugs()`. - `src/commands/graph-query.ts` — `gbrain graph-query [--type T] [--depth N] [--direction in|out|both]`: typed-edge relationship traversal (renders indented tree) - `src/core/link-extraction.ts` — shared library for the v0.12.0 graph layer. extractEntityRefs (canonical, replaces backlinks.ts duplicate) matches both `[Name](people/slug)` markdown links and Obsidian `[[people/slug|Name]]` wikilinks as of v0.12.3. extractPageLinks, inferLinkType heuristics (attended/works_at/invested_in/founded/advises/source/mentions), parseTimelineEntries, isAutoLinkEnabled config helper. `DIR_PATTERN` covers `people`, `companies`, `deals`, `topics`, `concepts`, `projects`, `entities`, `tech`, `finance`, `personal`, `openclaw`. Used by extract.ts, operations.ts auto-link post-hook, and backlinks.ts. +- `src/core/zombie-reap.ts` (v0.28.1) — idempotent `installSigchldHandler()` so JS-spawned children get reaped via Bun's internal `waitpid()`. Bun (like Node) only auto-reaps when a SIGCHLD listener is registered; without it, every child the worker spawns (shell jobs, embed batches, sub-agents) becomes a zombie on exit and holds connection slots. Called once at module load from `src/cli.ts` (with Windows platform guard — SIGCHLD doesn't exist on Windows). Cross-file leak guard via `_uninstallSigchldHandlerForTests()` for tests. Layer 1 of the three-layer zombie defense; Layer 2 is tini-as-PID-1 wrapping the worker subtree (via `src/core/minions/spawn-helpers.ts`); Layer 3 is the container's own tini for hard Bun crashes. - `src/core/minions/` — Minions job queue: BullMQ-inspired, Postgres-native (queue, worker, backoff, types, protected-names, quiet-hours, stagger, handlers/shell). - `src/core/minions/queue.ts` — MinionQueue class (submit, claim, complete, fail, stall detection, parent-child, depth/child-cap, per-job timeouts, cascade-kill, attachments, idempotency keys, child_done inbox, removeOnComplete/Fail). `add()` takes a 4th `trusted` arg (separate from `opts` to prevent spread leakage); protected names in `PROTECTED_JOB_NAMES` require `{allowProtectedSubmit: true}` and the check runs trim-normalized (whitespace-bypass safe). v0.14.1 #219: `add()` plumbs `max_stalled` through with a `[1, 100]` clamp; omitted values let the schema DEFAULT (5) kick in. v0.19.0: `handleWallClockTimeouts(lockDurationMs)` is Layer 3 kill shot for jobs where `FOR UPDATE SKIP LOCKED` stall detection and the timeout sweep both fail to evict (wedged worker holding a row lock via a pending transaction). v0.19.1: `maxWaiting` coalesce path now uses `pg_advisory_xact_lock` keyed on `(name, queue)` to serialize concurrent submits for the same key, and filters on `queue` in addition to `name` so cross-queue same-name jobs don't suppress each other. -- `src/core/minions/worker.ts` — MinionWorker class (handler registry, lock renewal, graceful shutdown, timeout safety net). v0.14.0 abort-path fix: aborted jobs now call `failJob` with reason (`timeout`/`cancel`/`lock-lost`/`shutdown`) instead of returning silently. `shutdownAbort` (instance field) fires on process SIGTERM/SIGINT and propagates to `ctx.shutdownSignal` — shell handler listens to it; non-shell handlers don't. v0.22.1 (#403): per-job timeout fires `abort.abort(new Error('timeout'))` then a 30-second grace-then-evict safety net force-evicts the job from `inFlight` and marks it dead in DB if the handler ignores the abort signal — frees the slot even when a handler wedges (the 98-waiting-0-active prod incident driver). -- `src/core/minions/supervisor.ts` — MinionSupervisor process manager. Spawns `gbrain jobs work` as a child, restarts on crash with exponential backoff, periodic health check. v0.22.1 (#406): `consecutiveHealthFailures` counter; on 3 consecutive failures emits `health_warn` with `reason: 'db_connection_degraded'` and calls `engine.reconnect()` to swap in a fresh pool, then resets the counter. Worker exit classifier emits `likely_cause` field on `worker_exited` events: `oom_or_external_kill` (SIGKILL), `graceful_shutdown` (SIGTERM), `runtime_error` (code 1), `clean_exit` (code 0), `unknown`. +- `src/core/minions/worker.ts` — MinionWorker class (handler registry, lock renewal, graceful shutdown, timeout safety net). v0.14.0 abort-path fix: aborted jobs now call `failJob` with reason (`timeout`/`cancel`/`lock-lost`/`shutdown`) instead of returning silently. `shutdownAbort` (instance field) fires on process SIGTERM/SIGINT and propagates to `ctx.shutdownSignal` — shell handler listens to it; non-shell handlers don't. v0.22.1 (#403): per-job timeout fires `abort.abort(new Error('timeout'))` then a 30-second grace-then-evict safety net force-evicts the job from `inFlight` and marks it dead in DB if the handler ignores the abort signal — frees the slot even when a handler wedges (the 98-waiting-0-active prod incident driver). **v0.28.1 engine-ownership invariant:** `start()` no longer calls `engine.disconnect()` on shutdown — that was a leaky abstraction (the worker disconnected an engine it didn't own). The CLI handler in `src/commands/jobs.ts case 'work'` now owns engine lifecycle via try/finally with loud error logging on disconnect failure. Pinned by `test/worker-shutdown-disconnect.test.ts` asserting the inverse (`disconnectSpy).not.toHaveBeenCalled()`). +- `src/core/minions/supervisor.ts` — MinionSupervisor process manager. Spawns `gbrain jobs work` as a child, restarts on crash with exponential backoff, periodic health check. v0.22.1 (#406): `consecutiveHealthFailures` counter; on 3 consecutive failures emits `health_warn` with `reason: 'db_connection_degraded'` and calls `engine.reconnect()` to swap in a fresh pool, then resets the counter. Worker exit classifier emits `likely_cause` field on `worker_exited` events: `oom_or_external_kill` (SIGKILL), `graceful_shutdown` (SIGTERM), `runtime_error` (code 1), `clean_exit` (code 0), `unknown`. **v0.28.1:** consumes `detectTini()` + `buildSpawnInvocation()` from `src/core/minions/spawn-helpers.ts` to wrap the worker subtree in tini-as-PID-1 when tini is on `PATH` (handles native-addon zombie reaping that the in-process SIGCHLD reaper can't reach). Exposes `isTiniDetected` read-only accessor for tests. +- `src/core/minions/spawn-helpers.ts` (v0.28.1) — pure `detectTini()` + `buildSpawnInvocation()` helpers consumed by both `supervisor.ts` and `autopilot.ts`. Resolves the DRY violation between the two spawn sites and makes the tini wrapping testable without `mock.module()` (rule R2 of `scripts/check-test-isolation.sh`). `detectTini()` calls `execFileSync('which', ['tini'])` with explicit `env: process.env` so Bun sees runtime PATH mutations (the env-snapshot bug fix). `buildSpawnInvocation(tiniPath, cmd, args)` returns `{cmd, args}` with tini prepended when present, or the bare invocation otherwise. Pinned by `test/spawn-helpers.test.ts` (5 cases) and `test/supervisor-tini.test.ts` (4 cases). - `src/core/minions/types.ts` — `MinionJobInput` + `MinionJobStatus` + handler context types. `MinionJobInput.max_stalled` (new in v0.14.1) is optional; omitted values let the schema DEFAULT (5) kick in, provided values are clamped to `[1, 100]`. - `src/core/minions/protected-names.ts` — side-effect-free constant module exporting `PROTECTED_JOB_NAMES` + `isProtectedJobName()`. Kept pure so queue core can import without loading handler modules. - `src/core/minions/handlers/shell.ts` — `shell` job handler. Spawns `/bin/sh -c cmd` (absolute path, PATH-override-safe) or `argv[0] argv[1..]` (no shell). Env allowlist: `PATH, HOME, USER, LANG, TZ, NODE_ENV` + caller `env:` overrides. UTF-8-safe stdout/stderr tail via `string_decoder.StringDecoder`. Abort (either `ctx.signal` or `ctx.shutdownSignal`) fires SIGTERM → 5s grace → SIGKILL on child. Requires `GBRAIN_ALLOW_SHELL_JOBS=1` on worker (gated by `registerBuiltinHandlers`). @@ -122,13 +124,13 @@ strict behavior when unset. - `src/core/minions/attachments.ts` — Attachment validation (path traversal, null byte, oversize, base64, duplicate detection) - `src/commands/agent.ts` (v0.16) — `gbrain agent run [flags]` CLI. Submits `subagent` (or N children + 1 aggregator) under `{allowProtectedSubmit: true}`. Single-entry `--fanout-manifest` short-circuits. Children get `on_child_fail: 'continue'` + `max_stalled: 3`. `--follow` is the default on TTY; streams logs + polls `waitForCompletion` in parallel. Ctrl-C detaches, does not cancel. - `src/commands/agent-logs.ts` (v0.16) — `gbrain agent logs [--follow] [--since]`. Merges JSONL heartbeat audit + `subagent_messages` into a chronological timeline. `parseSince` accepts ISO-8601 or relative (`5m`, `1h`, `2d`). Transcript tail renders only for terminal jobs. -- `src/commands/jobs.ts` — `gbrain jobs` CLI subcommands + `gbrain jobs work` daemon. v0.13.1 surfaces the full `MinionJobInput` retry/backoff/timeout/idempotency surface as first-class CLI flags on `jobs submit`: `--max-stalled`, `--backoff-type fixed|exponential`, `--backoff-delay`, `--backoff-jitter`, `--timeout-ms`, `--idempotency-key`. `jobs smoke --sigkill-rescue` is the opt-in regression guard for #219. v0.16 wires `registerBuiltinHandlers` to always register `subagent` + `subagent_aggregator` (no env flag — `ANTHROPIC_API_KEY` is the natural cost gate, trust is via `PROTECTED_JOB_NAMES`) and loads `GBRAIN_PLUGIN_PATH` plugins at worker startup with a loud startup-line per plugin. `shell` handler still gated by `GBRAIN_ALLOW_SHELL_JOBS=1` (RCE surface, separate concern). v0.22.10 (#521): the `autopilot-cycle` handler now forwards `job.data.phases` to `runCycle` (was previously discarded — caller-supplied phase selection silently became a full cycle). Phases are validated against `ALL_PHASES` from `src/core/cycle.ts`; invalid names are filtered out and an empty/missing array falls back to the default 6-phase cycle. v0.22.13 (PR #490 CODEX-1+CODEX-4): `sync` handler now resolves `sourceId` at entry by looking up `sources.local_path` (mirrors `cycle.ts:480`'s autopilot fix from PR #475) so multi-source brains read the per-source `last_commit` anchor instead of the global config key. Concurrency routed through the shared `autoConcurrency()` policy in `src/core/sync-concurrency.ts` instead of the prior hardcoded `4`; PGLite stays serial. `noEmbed` default is `true` (embed is a separate job — submit `gbrain embed --stale` after sync, or rely on the autopilot cycle's embed phase). +- `src/commands/jobs.ts` — `gbrain jobs` CLI subcommands + `gbrain jobs work` daemon. **v0.28.1:** `case 'work'` now wraps `worker.start()` in try/finally and owns engine lifecycle — calls `engine.disconnect()` on shutdown with loud error logging on failure. Replaces the prior call inside `MinionWorker.start()` (which violated engine ownership: the worker disconnected an engine it didn't own, and clobbered the module-level singleton on PostgresEngine via the now-fixed idempotency bug). Pool slots now free immediately on shutdown instead of waiting for TCP keepalive (~minutes). v0.13.1 surfaces the full `MinionJobInput` retry/backoff/timeout/idempotency surface as first-class CLI flags on `jobs submit`: `--max-stalled`, `--backoff-type fixed|exponential`, `--backoff-delay`, `--backoff-jitter`, `--timeout-ms`, `--idempotency-key`. `jobs smoke --sigkill-rescue` is the opt-in regression guard for #219. v0.16 wires `registerBuiltinHandlers` to always register `subagent` + `subagent_aggregator` (no env flag — `ANTHROPIC_API_KEY` is the natural cost gate, trust is via `PROTECTED_JOB_NAMES`) and loads `GBRAIN_PLUGIN_PATH` plugins at worker startup with a loud startup-line per plugin. `shell` handler still gated by `GBRAIN_ALLOW_SHELL_JOBS=1` (RCE surface, separate concern). v0.22.10 (#521): the `autopilot-cycle` handler now forwards `job.data.phases` to `runCycle` (was previously discarded — caller-supplied phase selection silently became a full cycle). Phases are validated against `ALL_PHASES` from `src/core/cycle.ts`; invalid names are filtered out and an empty/missing array falls back to the default 6-phase cycle. v0.22.13 (PR #490 CODEX-1+CODEX-4): `sync` handler now resolves `sourceId` at entry by looking up `sources.local_path` (mirrors `cycle.ts:480`'s autopilot fix from PR #475) so multi-source brains read the per-source `last_commit` anchor instead of the global config key. Concurrency routed through the shared `autoConcurrency()` policy in `src/core/sync-concurrency.ts` instead of the prior hardcoded `4`; PGLite stays serial. `noEmbed` default is `true` (embed is a separate job — submit `gbrain embed --stale` after sync, or rely on the autopilot cycle's embed phase). - `src/commands/features.ts` — `gbrain features --json --auto-fix`: usage scan + feature adoption salesman -- `src/commands/autopilot.ts` — `gbrain autopilot --install`: self-maintaining brain daemon (sync+extract+embed) +- `src/commands/autopilot.ts` — `gbrain autopilot --install`: self-maintaining brain daemon (sync+extract+embed). **v0.28.1:** consumes `detectTini()` from `src/core/minions/spawn-helpers.ts` and resolves it once at startup instead of per worker respawn (was paying an `execFileSync` cost on every restart). - `src/mcp/server.ts` — MCP stdio server (generated from operations). v0.22.7: tool-call handler delegates to `dispatchToolCall` from `src/mcp/dispatch.ts` so stdio + HTTP transports share one validation, context-build, and error-format path. - `src/mcp/dispatch.ts` (v0.22.7) — Shared tool-call dispatch consumed by both stdio (`server.ts`) and HTTP transports. Exports `dispatchToolCall(engine, name, params, opts)`, `buildOperationContext(engine, params, opts)`, and `validateParams(op, params)`. Single source of truth for `(ctx, params)` handler arg order and the 5-field `OperationContext` shape (engine + config + logger + dryRun + remote). Defaults to `remote: true` (untrusted); local CLI callers pass `remote: false`. Closed F1/F2/F3 drift bugs in the original v0.22.5 HTTP transport. **v0.26.9 (F8):** adds `summarizeMcpParams(opName, params)` — privacy-preserving redactor for `mcp_request_log` and the admin SSE feed. Returns `{redacted, kind, declared_keys, unknown_key_count, approx_bytes}`. Intersects submitted top-level keys against the operation's declared `params` allow-list (declared keys preserved as a sorted array for debug visibility; unknown keys counted but never named, closing the attacker-controlled-key-name leak). Byte counts bucketed up to nearest 1KB so an attacker can't binary-search secret-content sizes via repeated probes. Operators on a personal laptop who want raw payload visibility opt back in with `gbrain serve --http --log-full-params` (loud stderr warning at startup). Canonical helper — new logging code paths route through it rather than `JSON.stringify(params)`. - `src/mcp/rate-limit.ts` (v0.22.7) — Bounded-LRU token-bucket limiter. `buildDefaultLimiters()` returns the two-bucket pipeline: pre-auth IP (30/60s, fires BEFORE the DB lookup so brute-force load against `access_tokens` is actually capped) + post-auth token-id (60/60s). Tracks `lastTouchedMs` separately from `lastRefillMs` so an exhausted key can't be reset by hammering past the TTL. LRU cap bounds memory under attacker-controlled key growth. -- `src/commands/serve-http.ts` (v0.26.0) — Express 5 HTTP MCP server with OAuth 2.1, admin dashboard, and SSE live activity feed. Started via `gbrain serve --http [--port N] [--token-ttl N] [--enable-dcr] [--public-url URL] [--log-full-params]`. Supersedes the v0.22.7 `src/mcp/http-transport.ts` simple bearer-auth path. Combines MCP SDK's `mcpAuthRouter` (authorize / token / register / revoke endpoints), a custom `client_credentials` handler (SDK's token endpoint throws `UnsupportedGrantTypeError` for CC; the custom handler runs BEFORE the router and falls through for `auth_code` / `refresh_token`), `requireBearerAuth` middleware for `/mcp` with scope enforcement before op dispatch, `localOnly` rejection, and `express-rate-limit` at 50 req / 15 min on `/token`. Serves the built admin SPA from `admin/dist/` with SPA fallback. `/admin/events` SSE endpoint broadcasts every MCP request to connected admin browsers. `cookie-parser` middleware wired (Express 5 has no built-in). Startup logging prints port, engine, configured issuer URL (honors `--public-url`), registered-client count, DCR status, and admin bootstrap token. **v0.26.9 hardening pass:** F7 sets `remote: true` explicitly on the `/mcp` request handler's OperationContext literal (closes the HTTP shell-job RCE — without this, `submit_job`'s protected-name guard at `operations.ts:1391` saw a falsy undefined and skipped, letting a `read+write`-scoped OAuth token submit `shell` jobs). F8 wires `summarizeMcpParams` from `src/mcp/dispatch.ts` into both `mcp_request_log` writes and the admin SSE feed by default (raw payloads opt-in via `--log-full-params` with stderr warning). F9 sets cookie `Secure` flag when behind HTTPS or a public-URL proxy. F10 caps the magic-link nonce store with an LRU bound. F12 routes DCR disable through the `GBrainOAuthProvider` constructor's `dcrDisabled` option instead of the prior monkey-patch on the express router. F14 wraps `transport.handleRequest` in try/catch so SDK throws return a JSON-RPC 500 envelope instead of express's default HTML error page. F15 unifies OperationError + unexpected exceptions through `buildError` / `serializeError` so `/mcp` always returns the same envelope shape. +- `src/commands/serve-http.ts` (v0.26.0) — Express 5 HTTP MCP server with OAuth 2.1, admin dashboard, and SSE live activity feed. Started via `gbrain serve --http [--port N] [--token-ttl N] [--enable-dcr] [--public-url URL] [--log-full-params]`. Supersedes the v0.22.7 `src/mcp/http-transport.ts` simple bearer-auth path. Combines MCP SDK's `mcpAuthRouter` (authorize / token / register / revoke endpoints), a custom `client_credentials` handler (SDK's token endpoint throws `UnsupportedGrantTypeError` for CC; the custom handler runs BEFORE the router and falls through for `auth_code` / `refresh_token`), `requireBearerAuth` middleware for `/mcp` with scope enforcement before op dispatch, `localOnly` rejection, and `express-rate-limit` at 50 req / 15 min on `/token`. Serves the built admin SPA from `admin/dist/` with SPA fallback. `/admin/events` SSE endpoint broadcasts every MCP request to connected admin browsers. `cookie-parser` middleware wired (Express 5 has no built-in). Startup logging prints port, engine, configured issuer URL (honors `--public-url`), registered-client count, DCR status, and admin bootstrap token. **v0.26.9 hardening pass:** F7 sets `remote: true` explicitly on the `/mcp` request handler's OperationContext literal (closes the HTTP shell-job RCE — without this, `submit_job`'s protected-name guard at `operations.ts:1391` saw a falsy undefined and skipped, letting a `read+write`-scoped OAuth token submit `shell` jobs). F8 wires `summarizeMcpParams` from `src/mcp/dispatch.ts` into both `mcp_request_log` writes and the admin SSE feed by default (raw payloads opt-in via `--log-full-params` with stderr warning). F9 sets cookie `Secure` flag when behind HTTPS or a public-URL proxy. F10 caps the magic-link nonce store with an LRU bound. F12 routes DCR disable through the `GBrainOAuthProvider` constructor's `dcrDisabled` option instead of the prior monkey-patch on the express router. F14 wraps `transport.handleRequest` in try/catch so SDK throws return a JSON-RPC 500 envelope instead of express's default HTML error page. F15 unifies OperationError + unexpected exceptions through `buildError` / `serializeError` so `/mcp` always returns the same envelope shape. **v0.28.1:** `/health` endpoint extracted into pure `probeHealth(engine)` async function with `HEALTH_TIMEOUT_MS = 3000` exported constant — drops the timeout from 5s to 3s so Fly.io's 5s health-check deadline gets 2s of headroom for TCP, response framing, and clock skew. Races `engine.getStats()` against the timeout via `Promise.race`; saturated pool returns 503 with `Health check timed out (database pool may be saturated)` instead of hanging. `clearTimeout` in finally block prevents pending-timer pile-up under high probe rates (race-leak fix from adversarial review). - `src/core/oauth-provider.ts` (v0.26.0) — `GBrainOAuthProvider` implementing the MCP SDK's `OAuthServerProvider` + `OAuthRegisteredClientsStore` interfaces. Backed by raw SQL (works on both PGLite and Postgres — OAuth is infrastructure, not a BrainEngine concern). Full OAuth 2.1 spec: `authorize` + `exchangeAuthorizationCode` with PKCE (for ChatGPT), `client_credentials` (for Perplexity / Claude), `refresh_token` with rotation, `revokeToken`, `registerClient` (DCR path validates redirect_uri must be `https://` or loopback per RFC 6749 §3.1.2.1). All tokens + client secrets SHA-256 hashed before storage. Auth codes single-use with 10-minute TTL via atomic `DELETE...RETURNING` (closes RFC 6749 §10.5 TOCTOU race). Refresh rotation also `DELETE...RETURNING` (closes §10.4 stolen-token detection bypass). `pgArray()` escapes commas/quotes/braces in elements so a comma-bearing redirect_uri can't smuggle a second array element. Legacy `access_tokens` fallback in `verifyAccessToken` grandfathers pre-v0.26 bearer tokens as `read+write+admin`. `sweepExpiredTokens()` runs on startup wrapped in try/catch. **v0.26.9 RFC 6749/7009 hardening pass:** F1+F2 fold `client_id` atomically into the `DELETE WHERE` clauses for both auth-code exchange and refresh rotation — pre-fix the post-hoc client compare burned the row on wrong-client paths so the legitimate client couldn't retry. F3 enforces refresh-scope-subset against the original grant on the row (RFC 6749 §6), not the client's currently-allowed scopes — fixes the case where revoking a scope from a client wouldn't shrink the agent's existing refresh tokens. F4 binds `client_id` on `revokeToken` so a client can only revoke its own tokens (RFC 7009 §2.1). F7c validates the `/token` request's `redirect_uri` against the value stored at `/authorize` (RFC 6749 §4.1.3) — empty-string treated as missing rather than wildcard match (adversarial-review fix). F5 swaps bare `catch {}` blocks in `verifyAccessToken` and `getClient` for `isUndefinedColumnError` from `src/core/utils.ts` — only SQLSTATE 42703 falls through to legacy fallback; lock timeouts and network blips throw and surface. F6 makes `sweepExpiredTokens()` actually return the count via `RETURNING 1` + array length, not a fire-and-forget zero. F12 adds `dcrDisabled` constructor option so `serve-http.ts` can disable the `/register` endpoint without monkey-patching the router. **v0.26.2:** module-private `coerceTimestamp()` boundary helper at the top of the file normalizes postgres-driver-as-string BIGINT columns to JS numbers at every read site (5 call sites: `getClient` L112+L113 for DCR `/register` RFC 7591 §3.2.1 numeric timestamps, `exchangeRefreshToken` L274 + `verifyAccessToken` L296+L303 for the SDK's `typeof === 'number'` bearerAuth check). Throws on non-finite input (NaN/Infinity) so corrupt rows fail loud at the boundary instead of riding through as `expiresAt: NaN`; returns undefined for SQL NULL so callers decide NULL semantics explicitly (refresh + access token paths treat NULL as expired). Helper intentionally NOT promoted to `src/core/utils.ts` — codex review flagged repo-wide BIGINT precision-loss risk for a generic helper. - `admin/` (v0.26.0) — React 19 + Vite + TypeScript admin SPA embedded in the binary via `admin/dist/` served by `serve-http.ts`. 7 screens: Login (bootstrap token → session cookie), Dashboard (metrics + SSE feed + token health), Agents (sortable table + sparklines + Register button), Register (modal with scope checkboxes + grant type selector), Credentials reveal (full-screen modal with Copy + Download JSON + yellow one-time-only warning), Request Log (filterable paginated), Agent Detail drawer (Details / Activity / Config Export tabs + Revoke). Design tokens: `#0a0a0f` bg, Inter for UI, JetBrains Mono for data, 4-32px spacing scale, rounded pill badges. HTTP-only SameSite=Strict cookie auth. 65KB gzip. Build: `cd admin && bun install && bun run build`; output at `admin/dist/` is committed for self-contained binaries. - `src/commands/auth.ts` — Token management. `gbrain auth create/list/revoke/test` for legacy bearer tokens (v0.22.7 wired as a first-class CLI subcommand) plus `gbrain auth register-client` (v0.26.0) and `gbrain auth revoke-client ` (v0.26.2) for OAuth 2.1 client lifecycle. `revoke-client` runs an atomic `DELETE...RETURNING` on `oauth_clients`; FK `ON DELETE CASCADE` on `oauth_tokens.client_id` and `oauth_codes.client_id` purges every active token + authorization code in a single transaction. `process.exit(1)` on no-such-client (idempotent — re-running on the same id produces the same exit-1 message). Legacy tokens stored as SHA-256 hashes in `access_tokens`; OAuth clients in `oauth_clients`. As of v0.26.0, legacy tokens grandfather to `read+write+admin` scopes on the OAuth HTTP server, so pre-v0.26 deployments keep working with no migration. diff --git a/TODOS.md b/TODOS.md index 272b15375..03ce91761 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1,5 +1,31 @@ # TODOS +## /health endpoint hardening (v0.28.1 follow-up) + +### Cancel `engine.getStats()` when /health times out +**Priority:** P2 + +**What:** `probeHealth()` in `src/commands/serve-http.ts` races `engine.getStats()` against a 3s timeout. When the timeout wins, the original `getStats()` keeps running on a saturated pool. Under sustained probe traffic with a slow DB, timed-out probes pile up expensive `count(*)` queries that turn a partial slowdown into a total outage. + +**Why:** Both adversarial reviewers (Claude + Codex) flagged this independently during the v0.28.1 ship. Deferred because cancellation requires `AbortController` plumbing through `BrainEngine.getStats()` which doesn't exist yet — wider blast radius than v0.28.1's zombie-reaping scope justified. + +**Pros:** Closes the self-DoS path. /health returning 503 stops contributing to pool saturation. +**Cons:** Touches the BrainEngine interface (PostgresEngine + PGLiteEngine implementations). Needs postgres.js or PgBouncer-level query cancellation. Wider blast radius. +**Context:** Drop-in replacement for `Promise.race([getStats(), timeout])` is `getStats({ signal })` consumed via AbortController. Reviewer findings: see PR #637 (v0.28.1) adversarial review section. +**Depends on:** AbortController plumbing in BrainEngine interface. + +### Replace `/health` with a lighter liveness probe +**Priority:** P3 + +**What:** `engine.getStats()` does `count(*) FROM pages, content_chunks, links, tags, timeline_entries` plus `GROUP BY type`. On a large but otherwise healthy brain, this can normally exceed 3s and cause false-positive 503s + orchestrator restart loops. + +**Why:** Codex flagged that the new 3s timeout is aggressive for the cost of the probe. Pre-existing behavior (the /health endpoint was already doing full stats in v0.27 with no timeout). Worth splitting probe purpose: `/health` for liveness (`SELECT 1`), `/stats` for the full counts. + +**Pros:** Liveness probe stays under 100ms even on saturated pools. Operators get a separate `/stats` for the count breakdown when they actually want it. +**Cons:** Behavior change for orchestrator setups that scrape /health as both liveness AND count source. +**Context:** PR #637 (v0.28.1) adversarial review. Pair with the AbortController follow-up above. +**Depends on:** Nothing. + ## OAuth/MCP hardening (v0.26.7 follow-up) ### F11 — `auth register-client --redirect-uri` flag diff --git a/VERSION b/VERSION index 1b58cc101..48f7a71df 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.27.0 +0.28.1 diff --git a/package.json b/package.json index 3d3ede260..e3458cb67 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gbrain", - "version": "0.27.0", + "version": "0.28.1", "description": "Postgres-native personal knowledge brain with hybrid RAG search", "type": "module", "main": "src/core/index.ts", diff --git a/src/cli.ts b/src/cli.ts index d8906a0da..a193c0db4 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -1,5 +1,8 @@ #!/usr/bin/env bun +import { installSigchldHandler } from './core/zombie-reap.ts'; +installSigchldHandler(); + import { readFileSync } from 'fs'; import { loadConfig, toEngineConfig } from './core/config.ts'; import type { BrainEngine } from './core/engine.ts'; diff --git a/src/commands/autopilot.ts b/src/commands/autopilot.ts index 7d6b11cb9..5684437dd 100644 --- a/src/commands/autopilot.ts +++ b/src/commands/autopilot.ts @@ -23,6 +23,7 @@ import { execSync, spawn, type ChildProcess } from 'child_process'; import type { BrainEngine } from '../core/engine.ts'; import { loadPreferences } from '../core/preferences.ts'; import { loadConfig } from '../core/config.ts'; +import { detectTini, buildSpawnInvocation } from '../core/minions/spawn-helpers.ts'; function parseArg(args: string[], flag: string): string | undefined { const idx = args.indexOf(flag); @@ -157,15 +158,23 @@ export async function runAutopilot(engine: BrainEngine, args: string[]) { if (spawnManagedWorker) { const cliPath = resolveGbrainCliPath(); + // Resolve tini once at startup — not per respawn — to avoid shelling out + // every time the worker restarts. Reaps zombie children from shell jobs + // and embed batches that outlive a watchdog-killed worker. + const tiniPath = detectTini(); const startWorker = () => { // Inject the RSS watchdog default (2048 MB) for the autopilot-supervised // worker. Bare `gbrain jobs work` has no default; the supervisor and // autopilot are the production paths that opt in. const args = ['jobs', 'work', '--max-rss', '2048']; - const child = spawn(cliPath, args, { stdio: 'inherit', env: process.env }); + + const { cmd: spawnCmd, args: spawnArgs } = buildSpawnInvocation(tiniPath, cliPath, args); + + const child = spawn(spawnCmd, spawnArgs, { stdio: 'inherit', env: process.env }); workerProc = child; lastWorkerStartTime = Date.now(); - console.log(`[autopilot] Minions worker spawned (pid: ${child.pid}, watchdog: 2048MB)`); + console.log(`[autopilot] Minions worker spawned (pid: ${child.pid}, watchdog: 2048MB${tiniPath ? ', tini: active' : ''})`); + child.on('exit', (code) => { workerProc = null; if (stopping) return; diff --git a/src/commands/jobs.ts b/src/commands/jobs.ts index 605663c59..b643fd093 100644 --- a/src/commands/jobs.ts +++ b/src/commands/jobs.ts @@ -714,7 +714,21 @@ HANDLER TYPES (built in) : ''; console.log(`Minion worker started (queue: ${queueName}, concurrency: ${concurrency}${watchdogNote}${healthNote})`); console.log(`Registered handlers: ${worker.registeredNames.join(', ')}`); - await worker.start(); + try { + await worker.start(); + } finally { + // Release the DB connection pool immediately on shutdown so + // PgBouncer slots are freed rather than waiting for TCP keepalive + // (~minutes). Disconnect failure is best-effort but logged loudly: + // a silent shutdown disconnect error is exactly the bug class the + // v0.26.9 D14 direction (isUndefinedColumnError, oauth-provider) + // was created to surface. The CLI is the engine owner here, not + // the worker — keeping disconnect at this layer preserves the + // "engine ownership stays with the creator" invariant that broke + // tests in earlier waves of this branch. + try { await engine.disconnect(); } + catch (e) { console.error('[gbrain jobs work] engine disconnect failed during shutdown:', e); } + } break; } diff --git a/src/commands/serve-http.ts b/src/commands/serve-http.ts index 87e7b505b..fc4a81362 100644 --- a/src/commands/serve-http.ts +++ b/src/commands/serve-http.ts @@ -32,6 +32,66 @@ import { buildError, serializeError } from '../core/errors.ts'; import { VERSION } from '../version.ts'; import * as db from '../core/db.ts'; +/** + * /health endpoint timeout. 3s rather than 5s: Fly.io's default + * health-check timeout is 5s, so returning 503 right at the orchestrator + * deadline races with the orchestrator recording the request as a timeout. + * 3s leaves 2s of headroom for TCP, response framing, and clock skew. + */ +export const HEALTH_TIMEOUT_MS = 3000; + +export type ProbeHealthResult = + | { ok: true; status: 200; body: { status: 'ok'; version: string; engine: string; [k: string]: unknown } } + | { ok: false; status: 503; body: { error: 'service_unavailable'; error_description: string } }; + +/** + * Pure async health probe. Races `engine.getStats()` against a timeout, + * returns a tagged result. No Express coupling — easy to unit-test with a + * mock engine. The /health route handler is a thin wrapper around this. + */ +export async function probeHealth( + engine: BrainEngine, + engineName: string, + version: string, + timeoutMs: number = HEALTH_TIMEOUT_MS, +): Promise { + // Capture the handle so we can clearTimeout when getStats() wins. Without + // this, every fast /health request leaves a 3s pending timer in the event + // loop until it fires — under high probe rates this builds up a rolling + // backlog of timers and avoidable wakeups. Both adversarial reviewers + // (Claude + Codex) flagged this independently. + let timer: ReturnType | null = null; + try { + const stats = await Promise.race([ + engine.getStats(), + new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error('health_timeout')), timeoutMs); + }), + ]); + return { + ok: true, + status: 200, + body: { status: 'ok', version, engine: engineName, ...stats }, + }; + } catch (e: unknown) { + const msg = e instanceof Error ? e.message : 'unknown'; + return { + ok: false, + status: 503, + body: { + error: 'service_unavailable', + error_description: msg === 'health_timeout' + ? 'Health check timed out (database pool may be saturated)' + : 'Database connection failed', + }, + }; + } finally { + // Clear the timer regardless of which branch won the race. No-op when + // the timer already fired (we're in the timeout-rejection catch block). + if (timer !== null) clearTimeout(timer); + } +} + interface ServeHttpOptions { port: number; tokenTtl: number; @@ -225,12 +285,8 @@ export async function runServeHttp(engine: BrainEngine, options: ServeHttpOption // Health check // --------------------------------------------------------------------------- app.get('/health', async (_req, res) => { - try { - const stats = await engine.getStats(); - res.json({ status: 'ok', version: VERSION, engine: config.engine, ...stats }); - } catch { - res.status(503).json({ error: 'service_unavailable', error_description: 'Database connection failed' }); - } + const result = await probeHealth(engine, config.engine || 'pglite', VERSION); + res.status(result.status).json(result.body); }); // --------------------------------------------------------------------------- diff --git a/src/core/minions/spawn-helpers.ts b/src/core/minions/spawn-helpers.ts new file mode 100644 index 000000000..8d88c4194 --- /dev/null +++ b/src/core/minions/spawn-helpers.ts @@ -0,0 +1,56 @@ +/** + * Pure helpers for spawning the gbrain worker, optionally wrapped in tini. + * + * Background: zombie children spawned by the worker (shell jobs, embed + * batches, sub-agents) need a SIGCHLD handler to be reaped. The cli.ts + * SIGCHLD handler covers JS-spawned children that exit while the parent is + * alive; tini wraps the worker process tree to also reap native-addon + * descendants and orphans. Together the two layers compose with AlphaClaw's + * container-level tini-as-PID-1. + * + * `detectTini()` is called once at supervisor / autopilot startup. The + * resolved path is reused on every respawn — we do NOT shell out per spawn. + * `buildSpawnInvocation()` is a pure function describing the (cmd, args) + * tuple to pass to `child_process.spawn`. Tests call it directly without + * any module mocking. + */ + +import { execFileSync } from 'child_process'; + +/** + * Resolve the tini binary path, or return an empty string when not on PATH. + * Resolved once at startup so we don't shell out on every respawn. + */ +export function detectTini(): string { + try { + // Pass `env: process.env` explicitly: Bun's execFileSync does NOT + // inherit the current process env by default (Bun snapshots env at + // startup). Without this, runtime mutations to PATH (including in + // tests) are invisible to `which`. + return execFileSync('which', ['tini'], { + encoding: 'utf8', + timeout: 2000, + env: process.env, + }).trim(); + } catch { + return ''; + } +} + +/** + * Build the (cmd, args) tuple for spawning the gbrain worker, optionally + * wrapped in tini. When `tiniPath` is non-empty, returns + * { cmd: tiniPath, args: ['--', cliPath, ...args] } + * which makes tini PID 1 of the spawned subtree. When empty, returns + * { cmd: cliPath, args } + * for a direct spawn. Pure function, no side effects. + */ +export function buildSpawnInvocation( + tiniPath: string, + cliPath: string, + args: string[], +): { cmd: string; args: string[] } { + return tiniPath + ? { cmd: tiniPath, args: ['--', cliPath, ...args] } + : { cmd: cliPath, args }; +} diff --git a/src/core/minions/supervisor.ts b/src/core/minions/supervisor.ts index a36411950..23ef5c792 100644 --- a/src/core/minions/supervisor.ts +++ b/src/core/minions/supervisor.ts @@ -27,6 +27,7 @@ */ import { spawn, type ChildProcess } from 'child_process'; +import { detectTini, buildSpawnInvocation } from './spawn-helpers.ts'; import { closeSync, existsSync, @@ -138,6 +139,8 @@ export class MinionSupervisor { private child: ChildProcess | null = null; private crashCount = 0; private lastStartTime = 0; + /** Path to tini binary for zombie reaping, or empty string when absent. */ + private readonly tiniPath: string; private stopping = false; private inBackoff = false; private healthInFlight = false; @@ -151,6 +154,22 @@ export class MinionSupervisor { constructor(engine: BrainEngine, opts: Partial & { cliPath: string }) { this.engine = engine; this.opts = { ...DEFAULTS, ...opts }; + + // Detect tini for zombie reaping. Resolved once at construction so we + // don't shell out on every respawn. Belt-and-suspenders with the + // SIGCHLD handler in cli.ts — tini catches children spawned by native + // addons that bypass the JS event loop. + this.tiniPath = detectTini(); + } + + /** + * Read-only accessor for whether tini was detected at construction. + * Used by `test/supervisor-tini.test.ts` to verify the wiring without + * exposing the resolved path. Returns true when `worker_spawned` events + * will include `tini: true` in their payload. + */ + get isTiniDetected(): boolean { + return this.tiniPath !== ''; } /** @@ -439,9 +458,17 @@ export class MinionSupervisor { this.lastStartTime = Date.now(); + // Wrap with tini when available — reaps zombie children that the + // SIGCHLD handler in cli.ts might miss (native addons, edge cases). + const { cmd: spawnCmd, args: spawnArgs } = buildSpawnInvocation( + this.tiniPath, + this.opts.cliPath, + args, + ); + let child: ChildProcess; try { - child = spawn(this.opts.cliPath, args, { + child = spawn(spawnCmd, spawnArgs, { stdio: 'inherit', env, }); @@ -459,7 +486,11 @@ export class MinionSupervisor { this.child = child; - this.emit('worker_spawned', { pid: child.pid, cli_path: this.opts.cliPath }); + this.emit('worker_spawned', { + pid: child.pid, + cli_path: this.opts.cliPath, + ...(this.tiniPath ? { tini: true } : {}), + }); // Async spawn errors (ENOENT, EACCES after the fork/exec). Node fires // 'error' first, then 'exit' with code=null. We log the error; the diff --git a/src/core/minions/worker.ts b/src/core/minions/worker.ts index 5f800111c..43b70fccb 100644 --- a/src/core/minions/worker.ts +++ b/src/core/minions/worker.ts @@ -422,6 +422,17 @@ export class MinionWorker extends EventEmitter { ]); } + // The worker does NOT disconnect the engine: it doesn't own the + // engine's lifecycle. The caller (CLI handler at src/commands/jobs.ts + // case 'work', or a test fixture) is responsible for disconnect when + // it has finished using the engine. Earlier wave's experiment of + // calling engine.disconnect() here violated ownership and broke + // every test that shared a single engine across multiple + // worker.start() / worker.stop() cycles (PGLiteEngine kills its + // single _db connection; PostgresEngine.disconnect was non-idempotent + // and clobbered the global db singleton on the second call). The + // pool-slot-release intent is now handled in the CLI handler which + // does own the engine. console.log('Minion worker stopped.'); } } diff --git a/src/core/postgres-engine.ts b/src/core/postgres-engine.ts index 2b4394739..6441401a0 100644 --- a/src/core/postgres-engine.ts +++ b/src/core/postgres-engine.ts @@ -41,6 +41,15 @@ export class PostgresEngine implements BrainEngine { private _savedConfig: (EngineConfig & { poolSize?: number }) | null = null; /** Whether a reconnect is in progress (prevents concurrent reconnects). */ private _reconnecting = false; + /** + * Tracks which connection path this engine is using so disconnect() is + * idempotent. 'instance' = own _sql pool (poolSize was set); + * 'module' = the module-level db singleton (backward compat path). + * null = never connected, or already disconnected. Without this, a second + * disconnect() on an instance-pool engine would fall through to + * db.disconnect() and clobber the unrelated module-level connection. + */ + private _connectionStyle: 'instance' | 'module' | null = null; // Instance connection (for workers) or fall back to module global (backward compat) get sql(): ReturnType { @@ -83,9 +92,11 @@ export class PostgresEngine implements BrainEngine { this._sql = postgres(url, opts); await this._sql`SELECT 1`; await db.setSessionDefaults(this._sql); + this._connectionStyle = 'instance'; } else { // Module-level singleton (backward compat for CLI main engine) await db.connect(config); + this._connectionStyle = 'module'; } } @@ -93,9 +104,16 @@ export class PostgresEngine implements BrainEngine { if (this._sql) { await this._sql.end(); this._sql = null; - } else { - await db.disconnect(); + // After this point, _connectionStyle stays 'instance' so a second + // disconnect() is a no-op rather than falling through and clearing + // the unrelated module-level db singleton. + return; } + if (this._connectionStyle === 'module') { + await db.disconnect(); + this._connectionStyle = null; + } + // else: nothing to disconnect (already done or never connected) } async initSchema(): Promise { diff --git a/src/core/zombie-reap.ts b/src/core/zombie-reap.ts new file mode 100644 index 000000000..4946e3e08 --- /dev/null +++ b/src/core/zombie-reap.ts @@ -0,0 +1,36 @@ +/** + * Reap zombie child processes. + * + * Bun (like Node) only auto-reaps child processes when a SIGCHLD handler is + * installed. Without this, child processes spawned by the worker (embed + * batches, shell jobs, sub-agents) become zombies when they exit, + * accumulating in the PID table and (in the original production cascade) + * holding phantom DB connection slots. + * + * A no-op handler is sufficient — the runtime calls waitpid() internally as + * long as a listener is registered. EventEmitter does NOT dedupe listeners by + * reference; the includes() check below is what prevents duplicate listeners + * across hot-import scenarios (test harnesses re-importing modules in the + * same process). + */ + +const reapHandler = () => {}; + +export function installSigchldHandler(): void { + // SIGCHLD is POSIX-only. On Windows, `process.on('SIGCHLD', ...)` throws + // "ENOTSUP" because Windows doesn't have signals. Guard by platform so a + // future Windows port of any gbrain CLI doesn't crash at boot. + if (process.platform === 'win32') return; + if (!process.listeners('SIGCHLD').includes(reapHandler)) { + process.on('SIGCHLD', reapHandler); + } +} + +/** + * Test-only: removes the handler so other test files in the same shard + * process don't observe a pre-installed listener. Call from `afterAll` in + * `test/zombie-reap.test.ts`. + */ +export function _uninstallSigchldHandlerForTests(): void { + process.removeListener('SIGCHLD', reapHandler); +} diff --git a/test/e2e/postgres-engine-disconnect-idempotency.test.ts b/test/e2e/postgres-engine-disconnect-idempotency.test.ts new file mode 100644 index 000000000..6c9feaffb --- /dev/null +++ b/test/e2e/postgres-engine-disconnect-idempotency.test.ts @@ -0,0 +1,87 @@ +/** + * E2E test pinning the PostgresEngine.disconnect() idempotency invariant. + * + * Background: when commit 671ef099 added engine.disconnect() to + * MinionWorker.start()'s finally block, every test that calls worker.start() + * AND then engine.disconnect() in its own finally was double-disconnecting + * the same engine instance. Pre-fix, the second disconnect found _sql=null + * and fell through to the `else` branch which calls db.disconnect() — but + * db.disconnect() clears the GLOBAL module-level connection, breaking + * unrelated downstream tests (their getConn() throws "no database + * connection" on the next beforeEach). + * + * The fix: PostgresEngine tracks `_connectionStyle` ('instance' | 'module') + * and only calls db.disconnect() when it actually owns the module-level + * connection. Second disconnect on an instance-pool engine is a no-op. + * + * This test pins the contract so future refactors of disconnect() can't + * silently regress (it's exactly the bug class that took an hour of E2E + * debugging to find). Two cases: + * 1. instance-pool engine: connect → disconnect → disconnect must NOT + * affect the module-level connection. + * 2. module-singleton engine: connect → disconnect → disconnect is safe + * (second call no-ops). + */ + +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { PostgresEngine } from '../../src/core/postgres-engine.ts'; +import * as db from '../../src/core/db.ts'; + +const DATABASE_URL = process.env.DATABASE_URL; +const skip = !DATABASE_URL; + +if (skip) { + // eslint-disable-next-line no-console + console.log('Skipping postgres-engine-disconnect-idempotency E2E (DATABASE_URL not set)'); +} + +describe.skipIf(skip)('PostgresEngine.disconnect idempotency', () => { + beforeAll(async () => { + // Establish the module-level connection so we can verify it survives + // the instance-pool engine's double-disconnect. + await db.disconnect(); + await db.connect({ database_url: DATABASE_URL! }); + }); + + afterAll(async () => { + await db.disconnect(); + }); + + test('instance-pool engine: second disconnect() does NOT clobber module singleton', async () => { + const engine = new PostgresEngine(); + await engine.connect({ database_url: DATABASE_URL!, poolSize: 2 }); + + // First disconnect — closes the engine's own pool. + await engine.disconnect(); + + // Sanity: module-level connection still alive (this is what + // helpers.ts's getConn() returns). + const before = await db.getConnection().unsafe('SELECT 1 as ok'); + expect((before[0] as unknown as { ok: number }).ok).toBe(1); + + // Second disconnect — pre-fix, this fell through to db.disconnect() + // and cleared the module-level singleton. Post-fix, it's a no-op. + await engine.disconnect(); + + // Module-level connection MUST still be alive. + const after = await db.getConnection().unsafe('SELECT 1 as ok'); + expect((after[0] as unknown as { ok: number }).ok).toBe(1); + }); + + test('module-singleton engine: second disconnect() is a no-op', async () => { + // Re-establish module-level connection (idempotent; no-op if still + // connected from beforeAll). + await db.connect({ database_url: DATABASE_URL! }); + + const engine = new PostgresEngine(); + // No poolSize → uses the module-level singleton. + await engine.connect({ database_url: DATABASE_URL! }); + + // First disconnect closes module-level singleton (this engine owned it). + await engine.disconnect(); + + // Second disconnect must NOT throw — should be a no-op since + // _connectionStyle was reset to null. + await expect(engine.disconnect()).resolves.toBeUndefined(); + }); +}); diff --git a/test/e2e/zombie-reaping.test.ts b/test/e2e/zombie-reaping.test.ts new file mode 100644 index 000000000..174d72680 --- /dev/null +++ b/test/e2e/zombie-reaping.test.ts @@ -0,0 +1,202 @@ +/** + * E2E test for SIGCHLD handler reaping zombie shell-job children. + * + * Background: zombie children spawned by the worker (shell jobs, embed + * batches, sub-agents) accumulate in the PID table when the parent never + * calls waitpid(). The fix in src/cli.ts is to install a SIGCHLD listener; + * Bun (like Node) only auto-reaps when at least one listener is registered. + * + * This test is the load-bearing real-binary verification: spawn the + * compiled-or-interpreted gbrain CLI as `jobs work --concurrency 1` against + * a real Postgres, submit a shell job from the CLI side (`remote: false`, + * no v0.26.9 RCE-gate), capture the PID of the worker's shell child from + * the job result, then `ps -o stat= -p $PID` after ~300ms to verify the + * worker reaped it (no Z state). + * + * Why not gbrain serve --http: that path doesn't start a worker, and + * `submit_job` over MCP carries `remote: true` which rejects shell at the + * v0.26.9 gate (operations.ts:1391). The CLI submit + jobs work path is + * the only architecture that exercises the SIGCHLD handler in a real boot. + * + * Negative control (manual, not CI): + * 1. Comment out `installSigchldHandler()` in src/cli.ts + * 2. Re-run this test + * 3. Expect: ps reports stat=Z for the captured PID + * 4. Re-enable, re-run, expect: stat is empty (process gone, reaped) + * + * Skip rules: + * - DATABASE_URL must be set (matches existing E2E pattern) + * - Linux + macOS only (POSIX-only; tini/SIGCHLD don't exist on Windows) + */ + +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { spawn, execSync, type ChildProcess } from 'child_process'; +import { hasDatabase, setupDB, teardownDB } from './helpers.ts'; + +const skipReason: string | null = !hasDatabase() + ? 'DATABASE_URL not set' + : process.platform === 'win32' + ? 'POSIX-only (tini/SIGCHLD)' + : null; + +const describeE2E = skipReason ? describe.skip : describe; +if (skipReason) console.log(`Skipping E2E zombie-reaping tests (${skipReason})`); + +describeE2E('SIGCHLD handler reaps shell-job children (real binary)', () => { + let workerProc: ChildProcess | null = null; + let workerStderr = ''; + let submittedJobIds: number[] = []; + + beforeAll(async () => { + // Init schema + run migrations + truncate. setupDB seeds schema_version=1 + // but doesn't run the full migration chain (the OAuth E2E doesn't need it + // because it doesn't touch minion_jobs). We call engine.initSchema() + // explicitly to run migrations through the latest version, then disconnect + // so the spawned worker subprocess gets a fresh DB connection. + const engine = await setupDB(); + await engine.initSchema(); + await teardownDB(); + + // Start the worker via the same `bun run src/cli.ts` path the OAuth E2E + // uses. This boots through cli.ts so the SIGCHLD handler is installed, + // then runs the jobs work daemon. GBRAIN_ALLOW_SHELL_JOBS=1 is required + // for the shell handler to register. + workerProc = spawn( + 'bun', + ['run', 'src/cli.ts', 'jobs', 'work', '--concurrency', '1'], + { + cwd: process.cwd(), + env: { ...process.env, GBRAIN_ALLOW_SHELL_JOBS: '1' }, + stdio: ['ignore', 'pipe', 'pipe'], + }, + ); + + workerProc.stderr?.on('data', (d: Buffer) => { workerStderr += d.toString(); }); + let workerStdout = ''; + workerProc.stdout?.on('data', (d: Buffer) => { workerStdout += d.toString(); }); + + // Wait for "Minion worker started" or similar readiness signal. + let ready = false; + for (let i = 0; i < 60; i++) { + const combined = workerStdout + workerStderr; + if (/worker.*(started|polling|registered)/i.test(combined) + || /handlers/i.test(combined) + || combined.length > 500) { // worker has logged enough that it's clearly running + // Probe by submitting a no-op job; if that succeeds the worker is up. + // Heuristic: any output beyond startup banner indicates ready. + ready = true; + break; + } + await new Promise(r => setTimeout(r, 250)); + } + // Even if we don't see a clear "started" line, give the worker 1 more second + // for the queue claim loop to spin up before the first test runs. + await new Promise(r => setTimeout(r, 1000)); + if (!ready) { + // Don't throw — proceed and let the test's actual assertion catch issues. + // eslint-disable-next-line no-console + console.warn('[zombie-reaping E2E] worker readiness not detected; continuing anyway. stderr tail:\n', workerStderr.slice(-500)); + } + }, 30_000); + + afterAll(async () => { + if (workerProc && !workerProc.killed) { + workerProc.kill('SIGTERM'); + await new Promise(r => setTimeout(r, 1500)); + if (!workerProc.killed) workerProc.kill('SIGKILL'); + } + // Best-effort cleanup of any submitted jobs to keep the queue clean + // for subsequent test runs. + for (const id of submittedJobIds) { + try { + execSync(`bun run src/cli.ts jobs delete ${id}`, + { cwd: process.cwd(), encoding: 'utf8', env: { ...process.env }, stdio: 'pipe' }); + } catch { /* best effort */ } + } + }); + + test('shell-job child does NOT linger as a zombie (Z state) after exit', async () => { + // Submit a shell job that sleeps briefly then exits 0. Worker spawns + // /bin/sh as a child; without the SIGCHLD handler the sh process would + // sit in the PID table as a zombie until the worker process itself dies. + const params = JSON.stringify({ cmd: 'sleep 0.2', cwd: '/tmp' }); + let submitOut = ''; + try { + submitOut = execSync( + `bun run src/cli.ts jobs submit shell --params '${params}'`, + { + cwd: process.cwd(), + encoding: 'utf8', + // GBRAIN_ALLOW_SHELL_JOBS=1 also gates the CLI submit path, not + // just the worker that executes the job. + env: { ...process.env, GBRAIN_ALLOW_SHELL_JOBS: '1' }, + }, + ); + } catch (e: unknown) { + const err = e as { stdout?: Buffer; stderr?: Buffer }; + throw new Error( + `jobs submit failed:\n${err.stderr?.toString() ?? ''}\n${err.stdout?.toString() ?? ''}`, + ); + } + // Without --follow, `jobs submit` prints the full job as JSON on stdout. + let jobId: number; + try { + const job = JSON.parse(submitOut) as { id: number }; + jobId = job.id; + } catch { + throw new Error(`Could not parse jobs submit output as JSON:\n${submitOut.slice(0, 500)}`); + } + expect(typeof jobId).toBe('number'); + submittedJobIds.push(jobId); + + // Poll `jobs get` until status COMPLETED (≤ 10s) and parse the Result + // line which the shell handler returns as JSON containing `pid`. + let resultObj: { pid?: number; exit_code?: number } | null = null; + const deadline = Date.now() + 10_000; + while (Date.now() < deadline) { + const out = execSync( + `bun run src/cli.ts jobs get ${jobId}`, + { cwd: process.cwd(), encoding: 'utf8', env: { ...process.env } }, + ); + if (/COMPLETED/i.test(out)) { + const m = out.match(/Result:\s+({.*})/); + if (m) { + try { resultObj = JSON.parse(m[1]); } catch { /* try again */ } + } + if (resultObj) break; + } + await new Promise(r => setTimeout(r, 250)); + } + + if (!resultObj) { + throw new Error( + `Job ${jobId} did not complete within 10s. Worker stderr tail:\n${workerStderr.slice(-1000)}`, + ); + } + + expect(resultObj.exit_code).toBe(0); + expect(typeof resultObj.pid).toBe('number'); + const childPid = resultObj.pid as number; + + // Give SIGCHLD a chance to fire and the worker to reap. The shell + // process exited at submit + ~200ms; we poll the next ~500ms. + await new Promise(r => setTimeout(r, 300)); + + // ps -o stat= -p PID prints the stat column with no header. Empty + // output means the process is gone (reaped or never existed). 'Z' + // means it's lingering as a zombie — this would prove the SIGCHLD + // handler regressed. + let psOut = ''; + try { + psOut = execSync(`ps -o stat= -p ${childPid}`, { encoding: 'utf8' }).trim(); + } catch { + // ps exits non-zero when no matching process. That's the GOOD case + // (process reaped, no PID entry). Treat as empty. + psOut = ''; + } + + expect(psOut).not.toMatch(/^Z/); + // Either gone (reaped) or in a non-zombie state. Both are acceptable; + // a future SIGCHLD regression would surface as `Z` here. + }, 30_000); +}); diff --git a/test/serve-http-health.test.ts b/test/serve-http-health.test.ts new file mode 100644 index 000000000..7a56a3b99 --- /dev/null +++ b/test/serve-http-health.test.ts @@ -0,0 +1,72 @@ +/** + * Tests for probeHealth() and HEALTH_TIMEOUT_MS in src/commands/serve-http.ts. + * + * Calls probeHealth() directly with a mock engine — no Express test client, + * no module mocking. Three branches of the route (happy / timeout / db-error) + * each get a unit test, plus a sanity assertion on the exported constant. + * + * Express-layer wiring (timeout actually propagates through the route, body + * shape after JSON serialization) is covered by the new /health timeout case + * in test/e2e/serve-http-oauth.test.ts. + */ + +import { describe, test, expect } from 'bun:test'; +import { HEALTH_TIMEOUT_MS, probeHealth } from '../src/commands/serve-http.ts'; +import type { BrainEngine } from '../src/core/engine.ts'; + +/** + * Minimal mock engine: only `getStats()` is exercised by probeHealth. + * Cast to BrainEngine is safe — probeHealth doesn't touch other methods. + */ +function makeMockEngine(getStats: () => Promise): BrainEngine { + return { getStats } as unknown as BrainEngine; +} + +describe('HEALTH_TIMEOUT_MS', () => { + test('exported as 3000 (Fly.io headroom over the 5s default)', () => { + expect(HEALTH_TIMEOUT_MS).toBe(3000); + }); +}); + +describe('probeHealth', () => { + test('happy path: returns 200 + status:ok + spread stats', async () => { + const engine = makeMockEngine(async () => ({ pages: 42, links: 10 })); + const result = await probeHealth(engine, 'pglite', '0.27.1', 100); + expect(result.ok).toBe(true); + expect(result.status).toBe(200); + if (result.ok) { + expect(result.body.status).toBe('ok'); + expect(result.body.version).toBe('0.27.1'); + expect(result.body.engine).toBe('pglite'); + expect(result.body.pages).toBe(42); + expect(result.body.links).toBe(10); + } + }); + + test('timeout path: getStats() hangs forever → 503 with health_timeout description within 1s', async () => { + const engine = makeMockEngine(() => new Promise(() => { /* never resolves */ })); + const start = Date.now(); + const result = await probeHealth(engine, 'pglite', '0.27.1', 100); + const elapsed = Date.now() - start; + expect(elapsed).toBeLessThan(1000); + expect(result.ok).toBe(false); + expect(result.status).toBe(503); + if (!result.ok) { + expect(result.body.error).toBe('service_unavailable'); + expect(result.body.error_description).toBe( + 'Health check timed out (database pool may be saturated)', + ); + } + }); + + test('db-error path: getStats() rejects → 503 with database_failed description', async () => { + const engine = makeMockEngine(() => Promise.reject(new Error('ECONNREFUSED'))); + const result = await probeHealth(engine, 'postgres', '0.27.1', 100); + expect(result.ok).toBe(false); + expect(result.status).toBe(503); + if (!result.ok) { + expect(result.body.error).toBe('service_unavailable'); + expect(result.body.error_description).toBe('Database connection failed'); + } + }); +}); diff --git a/test/spawn-helpers.test.ts b/test/spawn-helpers.test.ts new file mode 100644 index 000000000..f2467a843 --- /dev/null +++ b/test/spawn-helpers.test.ts @@ -0,0 +1,48 @@ +/** + * 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). + }); +}); diff --git a/test/supervisor-tini.test.ts b/test/supervisor-tini.test.ts new file mode 100644 index 000000000..8e89ebb04 --- /dev/null +++ b/test/supervisor-tini.test.ts @@ -0,0 +1,65 @@ +/** + * 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 = { + 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 }); + } + }); +}); diff --git a/test/worker-shutdown-disconnect.test.ts b/test/worker-shutdown-disconnect.test.ts new file mode 100644 index 000000000..9dc68db75 --- /dev/null +++ b/test/worker-shutdown-disconnect.test.ts @@ -0,0 +1,70 @@ +/** + * Regression guard for engine-ownership invariant on worker shutdown. + * + * Earlier waves of this branch experimented with calling + * `engine.disconnect()` inside `MinionWorker.start()`'s finally block to + * free PgBouncer pool slots faster on shutdown. That violated engine + * ownership: the worker doesn't create the engine, it's passed in. Tests + * that share an engine across multiple worker.start() / worker.stop() + * cycles (every PGLite-shared E2E + every Postgres test that calls + * makeEngine() + engine.disconnect() in its own finally) all broke + * because the engine got disconnected behind their back. + * + * Final design (commit 7 of this branch): the worker leaves the engine + * alone. The CLI handler in src/commands/jobs.ts case 'work' calls + * engine.disconnect() itself in its own try/finally — the CLI owns the + * engine, so the CLI disposes of it. + * + * This test pins the invariant so future refactors can't silently + * reintroduce the regression. The check uses spyOn against the engine + * instance (object-level monkey-patching, parallel-safe) rather than + * module-level mocking which R2 of scripts/check-test-isolation.sh + * forbids in non-serial unit tests. + */ + +import { describe, test, expect, beforeAll, afterAll, spyOn } from 'bun:test'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { MinionWorker } from '../src/core/minions/worker.ts'; + +let engine: PGLiteEngine; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}, 30_000); + +afterAll(async () => { + try { await engine.disconnect(); } catch { /* already disconnected */ } +}); + +describe('MinionWorker engine-ownership invariant', () => { + test('worker.start() shutdown does NOT call engine.disconnect()', async () => { + const worker = new MinionWorker(engine, { queue: 'test-no-disconnect', pollInterval: 10 }); + worker.register('noop', async () => ({ ok: true })); + + const disconnectSpy = spyOn(engine, 'disconnect'); + + setTimeout(() => worker.stop(), 50); + await worker.start(); + + // Critical invariant: worker leaves engine.disconnect() to its caller. + expect(disconnectSpy).not.toHaveBeenCalled(); + disconnectSpy.mockRestore(); + }); + + test('engine remains usable after worker.start() returns', async () => { + const worker = new MinionWorker(engine, { queue: 'test-still-usable', pollInterval: 10 }); + worker.register('noop', async () => ({ ok: true })); + + setTimeout(() => worker.stop(), 50); + await worker.start(); + + // Engine must still be connected and queryable. If worker.start() + // ever disconnects again, this throws "PGLite not connected" and the + // regression is loud. + const result = await engine.executeRaw('SELECT 1 as ok'); + expect(result.length).toBe(1); + expect((result[0] as { ok: number }).ok).toBe(1); + }); +}); diff --git a/test/zombie-reap.test.ts b/test/zombie-reap.test.ts new file mode 100644 index 000000000..a9235aecf --- /dev/null +++ b/test/zombie-reap.test.ts @@ -0,0 +1,47 @@ +/** + * Tests for src/core/zombie-reap.ts — the SIGCHLD installer that lets + * Bun/Node reap exited child processes. + * + * Background: without a SIGCHLD listener, child processes spawned by the + * worker (shell jobs, embed batches, sub-agents) become zombies on exit. + * The runtime only calls waitpid() internally when at least one SIGCHLD + * listener is registered. A no-op handler is sufficient. + * + * Cross-file leak guard (codex review #6): mutating global `process` signal + * listeners in the parallel test pool can leak across files in the same + * shard process. `afterAll` MUST call `_uninstallSigchldHandlerForTests()` + * so the next file in the shard sees a clean listener set. + */ + +import { describe, test, expect, afterAll } from 'bun:test'; +import { + installSigchldHandler, + _uninstallSigchldHandlerForTests, +} from '../src/core/zombie-reap.ts'; + +afterAll(() => { + _uninstallSigchldHandlerForTests(); +}); + +describe('installSigchldHandler', () => { + test('registers a SIGCHLD listener after first call', () => { + const before = process.listeners('SIGCHLD').length; + installSigchldHandler(); + const after = process.listeners('SIGCHLD').length; + expect(after).toBeGreaterThanOrEqual(before + (before === 0 ? 1 : 0)); + expect(process.listeners('SIGCHLD').length).toBeGreaterThanOrEqual(1); + }); + + test('idempotent: two calls leave exactly one of our listeners', () => { + // Start clean — remove any handler from the previous test (this file's + // own only — afterAll handles the global cleanup). + _uninstallSigchldHandlerForTests(); + installSigchldHandler(); + const afterFirst = process.listeners('SIGCHLD').length; + installSigchldHandler(); + const afterSecond = process.listeners('SIGCHLD').length; + // The includes() guard in installSigchldHandler must prevent the + // second call from adding a duplicate. EventEmitter does NOT dedupe. + expect(afterSecond).toBe(afterFirst); + }); +});