mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-27 21:19:18 +00:00
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 commit671ef099(in this branch) added engine.disconnect() to MinionWorker.start()'s finally block. Tests that did: await worker.start(); // worker disconnects (was the new behavior) await engine.disconnect(); // test cleanup; pre-fix fell through // to db.disconnect() and killed // the global connection …would silently kill the helpers.ts singleton, and the next test in the file would fail in its beforeEach with "No database connection". Fix: track \`_connectionStyle\` ('instance' | 'module' | null) on the engine and only call db.disconnect() when this engine actually owns the global. After ending an instance-pool, _connectionStyle stays 'instance' so a second disconnect() is a no-op rather than a side-effect. Test coverage: test/e2e/postgres-engine-disconnect-idempotency.test.ts pins both contracts: - instance-pool engine: second disconnect MUST NOT clobber the module singleton (the bug above). - module-singleton engine: second disconnect is a no-op (resolves cleanly, no throw). Required for: minions-shell.test.ts to keep passing alongside the worker changes on this branch. Discovered during E2E sweep after the unit-test green light. Commit 7 in this branch then walks back the worker-side disconnect entirely (engine ownership belongs to the CLI handler) but this idempotency fix stays in place as a defense-in-depth guard against any future code calling disconnect twice on the same engine. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: move engine.disconnect() from worker.start() to gbrain jobs work CLI handler (engine ownership) Commit671ef099(the original fix in this branch) put \`await this.engine.disconnect()\` inside MinionWorker.start()'s finally block to free PgBouncer pool slots immediately on shutdown. That was the right intent on the wrong layer: the worker doesn't own the engine, the CLI handler that creates the engine does. The mismatched ownership broke every test that shares a single engine across multiple worker.start() / worker.stop() cycles: - test/e2e/minions-shell-pglite.test.ts → shared PGLite engine, second test failed with "PGLite not connected" - test/e2e/worker-abort-recovery.test.ts → 3 tests, same shape - test/e2e/minions-shell.test.ts → 3 Postgres tests broken by the second-disconnect-clobbers-global-singleton symptom (commit 6 of this branch fixed the underlying engine non-idempotency, but the worker-disconnect call was still wrong on its own) Fix: - worker.ts: remove the engine.disconnect() call. Add a comment documenting WHY the worker doesn't disconnect (ownership invariant) so a future contributor doesn't put it back. - src/commands/jobs.ts case 'work': wrap worker.start() in a try/finally that calls engine.disconnect() on shutdown. The CLI created the engine (line 631 area), so the CLI disposes of it. Disconnect failure logs to stderr with the "[gbrain jobs work] engine disconnect failed during shutdown:" prefix rather than the bare \`catch {}\` of earlier waves — matches the v0.26.9 D14 direction of preferring loud-but-best-effort over silent. Test: - test/worker-shutdown-disconnect.test.ts now pins the inverse invariant: worker.start() MUST NOT call engine.disconnect(), and the engine MUST remain queryable after start() returns. Two tests, instance-level spy, parallel-safe (no module mocking). End state: gbrain jobs work in production still frees pool slots immediately on shutdown (intent of671ef099preserved), tests that share an engine don't break (regression class fixed), and the engine ownership invariant is now codified in code AND in the test suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: clearTimeout in probeHealth race + platform guard SIGCHLD on Windows Two adversarial-review auto-fixes from /ship's pre-landing review pass. Both reviewers (Claude adversarial subagent + Codex adversarial) flagged the timer leak independently; Codex additionally caught the Windows crash risk. 1. probeHealth race timer leak (serve-http.ts): `Promise.race([getStats(), setTimeout(...)])` doesn't cancel the loser. Without `clearTimeout`, every fast /health request leaves a 3s pending timer in the event loop until it fires. Under sustained probe rates (Fly.io polls every ~10s, orchestrator load balancers can be much tighter), this builds a rolling backlog of timers and avoidable event loop wakeups in the hottest endpoint. Capture the timer handle, clear it in a `finally` block. No-op when the timer already fired. 2. SIGCHLD platform guard (zombie-reap.ts): SIGCHLD is POSIX-only. On Windows, `process.on('SIGCHLD', ...)` throws ENOTSUP because Windows doesn't have signals. Bun behaves the same. Without this guard, any future Windows port of a gbrain CLI tool would crash at boot before main() even runs. The zombie-reaping fix is itself POSIX-only (tini, ps, /proc), so the guard is consistent with the platform's capability set. NOT in this commit (intentionally out of scope): - Cancelling engine.getStats() when /health times out. Both reviewers noted this would need AbortController support in the engine layer which doesn't exist yet. The 503 timeout already improves on master's hang behavior; full cancellation is a follow-up. - Switching /health to a lighter probe (SELECT 1 instead of count(*) across 6 tables). Pre-existing behavior; refactoring the probe shape is wider blast radius than this branch's zombie-reaping scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: bump version and changelog (v0.28.1) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: update CLAUDE.md for v0.28.1 zombie reaping + health + engine ownership Add v0.28.1 file annotations covering: - src/core/zombie-reap.ts (new) — Layer 1 SIGCHLD reaper module - src/core/minions/spawn-helpers.ts (new) — pure detectTini + buildSpawnInvocation helpers - src/core/minions/worker.ts — engine-ownership invariant (no engine.disconnect) - src/core/minions/supervisor.ts — consumes spawn-helpers, exposes isTiniDetected - src/commands/serve-http.ts — probeHealth() + HEALTH_TIMEOUT_MS = 3000 - src/commands/jobs.ts — case 'work' owns engine lifecycle via try/finally - src/commands/autopilot.ts — resolves tini once at startup - src/core/postgres-engine.ts — disconnect() is idempotent via _connectionStyle Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Wintermute <wintermute@garrytan.com> Co-authored-by: Garry Tan <garrytan@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Wintermute
Garry Tan
Claude Opus 4.7
parent
ee9ceb327a
commit
2ea5b71177
@@ -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.**
|
||||
|
||||
@@ -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 <slug> [--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 <prompt> [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 <job> [--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 <client_id>` (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.
|
||||
|
||||
@@ -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
|
||||
|
||||
+1
-1
@@ -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",
|
||||
|
||||
@@ -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';
|
||||
|
||||
@@ -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;
|
||||
|
||||
+15
-1
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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<ProbeHealthResult> {
|
||||
// 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<typeof setTimeout> | null = null;
|
||||
try {
|
||||
const stats = await Promise.race([
|
||||
engine.getStats(),
|
||||
new Promise<never>((_, 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);
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
@@ -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 };
|
||||
}
|
||||
@@ -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<SupervisorOpts> & { 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
|
||||
|
||||
@@ -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.');
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<typeof postgres> {
|
||||
@@ -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<void> {
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
@@ -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);
|
||||
});
|
||||
@@ -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<unknown>): 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');
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -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).
|
||||
});
|
||||
});
|
||||
@@ -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<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 });
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user