From bb2e88c42a4969e16df7a43a9eb118aa031e89a4 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 21 Jun 2026 06:36:43 -0700 Subject: [PATCH 1/2] v0.42.52.0 fix(reliability): autopilot dead-job storm + supervisor wedge + sync/status/minion reliability (#2194 #2227 #1994 #1737 #1738 #1950 #1984) (#2287) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(supervisor): pin LOCK_HELD fence-exit is never counted as a crash (#2227) A duplicate supervisor loses the queue-scoped DB singleton lock (#1849) and exits LOCK_HELD before spawning a worker or emitting 'started'. summarizeCrashes counts only worker_exited, so the fence path is structurally uncountable. Pin it so a future refactor that logs worker_exited on the fence path fails here instead of silently re-introducing the crash-budget breaker-trip loop. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(autopilot): per-source cycle binds FS phases to source.local_path, not global repo (#2194 #2227) A per-source autopilot-cycle inherited the global sync.repo_path as brainDir while stamping DB freshness for source_id — mixed scope. FS phases (sync/lint/extract) ran against the wrong tree, so the failure-cooldown and freshness gates would attribute work to the wrong source. Resolve the source's local_path in the handler (reuse the archive-recheck SELECT) and bind brainDir to it; a pure-DB source gets null (FS phases skip) instead of falling through to the global checkout. Legacy no-source dispatch keeps the global repoPath. Prerequisite for the cooldown/split commits (codex outside-voice #8). Resolves TODOS:634. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(supervisor): detect a live supervisor via the DB lock under split $HOME (#2227) jobs supervisor status + doctor read the HOME-derived pidfile, so a supervisor started under a different $HOME (keeper=/root vs ops=/data) read as 'not running' while healthy — the false signal that drives an operator to spawn a duplicate. Both surfaces now fall back to the queue-scoped DB singleton lock (#1849), the HOME-independent authority, when the pidfile shows nothing. New isLockHolderLive keys on lock freshness (ttl + heartbeat steal-grace), never process.kill, so PID reuse can't false-positive (pid-liveness-alone-pid-reuse). Status surfaces the holder host/pid + recorded concurrency/max-rss from the latest started event. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(supervisor): degraded retry instead of permanent give-up on crash storm (#1994 #2227) max_crashes_exceeded gave up forever, so a transient DB-pooler blip that tripped the soft budget wedged the queue until a human restart (#2227's breaker-trips tail). Crossing the soft budget now enters degraded mode: keep respawning with capped exponential backoff (60s cap — a paced retry, not a hot loop) and emit a loud crash_budget_degraded health_warn. The existing stable-run reset clears the count once a respawn survives >5min, so a recovered DB self-heals. Permanent give-up fires only at a much-higher hard ceiling (maxCrashes × 10), tunable/disablable via GBRAIN_SUPERVISOR_HARD_STOP_CRASHES (0 = never). Resolves TODOS:92. Co-Authored-By: Claude Opus 4.8 (1M context) * feat(autopilot): clamp fan-out to worker concurrency + doctor warning (#2194) Fan-out resolved to 4 (Postgres) regardless of worker --concurrency, so surplus cycles queued behind the worker and raced the stalled-sweeper. Two fixes for the same mismatch: - resolveEffectiveFanoutMax clamps to max(1, concurrency-1) (reserve a slot), gated on a LIVE DB-lock holder so a stale started-audit row can't shrink throughput (codex #9/D5); no live holder → unknown → unclamped base. Escape hatch autopilot.fanout_clamp_to_concurrency. - doctor's autopilot_fanout_concurrency check warns when fan-out exceeds effective slots — the misconfig was silent before. Advisory (started-event concurrency), wired into both doctor surfaces. Co-Authored-By: Claude Opus 4.8 (1M context) * feat(autopilot): per-source failure cooldown — break the dead-job storm (#2194) Only SUCCESS gated dispatch, so a source whose cycle kept failing/timing-out re-fanned-out every 5-min tick forever (200+ dead jobs/24h). Now a failed source backs off with bounded exponential cooldown (10→120min). Read at DISPATCH from minion_jobs dead/failed rows (timeouts/RSS-kills dead-letter via SQL and never run handler code, so a write-only hook would miss them) AND re-checked at CLAIM time in the handler (codex #5: already-queued/retrying jobs). A success clears it (codex #7); null-source rows excluded (codex #6); engine-parity via executeRaw. Disable with autopilot.failure_cooldown_min=0. Fail-open if config/history reads error. Surfaced via fanout_cooldown_skipped + the fanout summary. Co-Authored-By: Claude Opus 4.8 (1M context) * feat(autopilot): split the cycle — per-source phases + one global-maintenance job (#2194 #2227) N per-source cycles each ran the brain-wide global phases (embed-all/orphans/ purge/…) concurrently, thrashing the same rows and taking the worker 4→10GB in <60s → RSS-kill → orphaned stalls. Split them: per-source jobs now run only source-scoped (+ mixed) phases and stamp last_source_cycle_at; a new autopilot-global-maintenance job runs the global phases ONCE per window (idempotency_key + maxWaiting:1 = structural single-flight) and stamps autopilot.last_global_at. This is the codex-endorsed design that replaced the rejected skip-and-stamp-fresh approach (codex #1/#2): no freshness poisoning, no starvation — global work always runs as its own job, never marked done when it wasn't. PHASE_SCOPE is now a runtime partition (GLOBAL ∪ NON_GLOBAL == ALL). last_full_cycle_at still written for doctor/legacy (no longer a global gate). Co-Authored-By: Claude Opus 4.8 (1M context) * fix(doctor): guard nullable engine in supervisor DB-lock fallback (#2227) Follow-up to the supervisor-visibility commit: doctor's engine binding is BrainEngine | null, so the inspectLock fallback must guard on a non-null engine (tsc TS2345). No behavior change — a null engine simply skips the DB-lock probe and falls back to the pidfile reading, as before. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(doctor): categorize autopilot_fanout_concurrency check as ops (#2194) Follow-up to the fan-out/concurrency commit: the doctor-categories drift guard requires every check name in doctor.ts to belong to exactly one category set. Add the new autopilot_fanout_concurrency check to OPS_CHECK_NAMES (infrastructure liveness, alongside wedged_queue/supervisor). Co-Authored-By: Claude Opus 4.8 (1M context) * docs: update KEY_FILES for the autopilot cycle split + supervisor degraded-retry (#2194 #2227) Post-ship document-release: refresh the KEY_FILES current-state entries that drifted — cycle.ts (GLOBAL/NON_GLOBAL phase split + last_source_cycle_at / autopilot.last_global_at), jobs.ts (per-source local_path brainDir, claim-time cooldown, autopilot-global-maintenance handler), supervisor.ts + child-worker (degraded retry instead of permanent give-up; hard ceiling), db-lock.ts (isLockHolderLive), handler-timeouts (new handler). Regenerated llms bundle. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(minions): handleTimeouts counts the timed-out run as a spent attempt (#1737) The per-job timeout_at dead-letter (handleTimeouts) set status='dead' without incrementing attempts_made, unlike the wall-clock and stall dead-letter siblings. It is the FIRST killer to fire for the long-lane handlers (subagent / embed-backfill / autopilot-cycle) because timeout_ms is stamped at submit, so a timed-out long job reported `attempts: 0/N (started: N)`. Mirror the siblings with attempts_made + 1 (terminal, no retry). Safe against double-count: the worker sweep runs handleStalled -> handleTimeouts -> handleWallClockTimeouts sequentially and awaited, each guarded on status='active', so the first to dead-letter excludes the row from the rest. Regression assertions added (test/minions.test.ts + e2e/minions-resilience.test.ts) so the increment can't be silently dropped. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(agent): recognize trailing switches in `agent run`, keep prompts freeform (#1738) parseRunFlags() broke flag parsing at the first positional token, so any flag after the prompt (`gbrain agent run "do X" --detach`) was swallowed into the prompt string and silently ignored. Now the no-value switches --detach/--follow/ --no-follow are hoisted when they trail the prompt, while everything else stays verbatim: an unknown --word is treated as prompt text (no "unknown flag" throw), a --switch mid-prompt is preserved, and `--` suppresses hoisting entirely for a literal escape. Value-flags now reject a missing or flag-shaped value (and --max-turns/--timeout-ms a non-number) instead of capturing undefined/NaN. Contract change: a prompt that starts with or trails an unguarded --word no longer errors; a literal trailing --detach needs `--`. Help text updated; tests revised + extended (test/agent-cli.test.ts). Co-Authored-By: Claude Opus 4.8 (1M context) * fix(sync): honest live-sync status + progress-aware stall-abort (#1950) Finishes the #2255 honest-freshness story for two gaps it left. (a) `gbrain sources status` printed "idle" while a sync proc held the per-source lock (the reported bug). New shared liveSyncStatus() helper in db-lock.ts reads the SAME live-lock signal `gbrain doctor` uses; runStatus now shows "running" (BACKFILL column + a sync_running field in --json) and suppresses the misleading "never synced" warning while a sync is live. One helper, so the surfaces can't drift (doctor/status retrofit tracked as a follow-up). (b) A sync wedged-but-alive kept refreshing its lock heartbeat (it fires on its own timer) and hadn't hit the wall-clock deadline, so only a manual pkill freed it. New in-band stall watchdog keys off FORWARD IMPORT PROGRESS (progress.tick), not the heartbeat: if no file completes for GBRAIN_SYNC_STALL_ABORT_SECONDS (default 900s), it aborts via a controller composed into opts.signal, so the drain returns partial() (last_commit unchanged, next run resumes from the checkpoint) and withRefreshingLock releases the lock. Limits, documented in code: a single file slower than the window trips it; a fully starved event loop won't fire the timer (the wall-clock hard deadline is that backstop). Tests: liveSyncStatus (live/expired/none/per-source) in db-lock-inspect; the resolveStallAbortSeconds env matrix in sync-hard-deadline. Co-Authored-By: Claude Opus 4.8 (1M context) * feat(status): version field + per-section --deadline-ms budget (#1984) `gbrain status` had no version in its JSON envelope and could hang on a slow connection with no way to get a partial answer. Two additions: - version: the StatusReport JSON now carries the local gbrain CLI version so a poller can pin behavior to a build. Thin-client also surfaces remote_version (the brain server's version), and the get_status_snapshot MCP op reports its version for that parity. - --deadline-ms=N / --fast: a shared wall-clock budget. Each section is raced against the REMAINING budget via Promise.race (NOT process-watchdog, which SIGKILLs and can't return partial output), so one slow/hung section can't strand the snapshot — it's marked stale and the rest still return. The envelope gains partial:true + stale_sections[]; exit code stays 0 (a snapshot was produced). Invalid --deadline-ms → exit 2. Tests: parseDeadlineFlag + withSectionDeadline (hermetic), the usage-error exit, version presence in the PGLite envelope, and the op's version key. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(sync): report stall_timeout distinctly + document in-flight limit (#1950) Pre-landing review (codex + adversarial): the stall watchdog aborted opts.signal but the per-iteration abort checks returned partial('timeout'), collapsing a wedge-reap into a user --timeout/SIGINT so JSON consumers couldn't tell them apart. Add a 'stall_timeout' reason (set via a stallAborted flag) on the three import-loop abort sites; deletes/renames-phase and checkpoint sites stay 'timeout'. Sharpen the watchdog comment: the abort is observed BETWEEN files, so a hang inside a single importFile is not interrupted until it returns (TODO: thread a cancellation signal through importFile). * fix(agent): `--` escape suppresses trailing-switch hoisting anywhere (#1738) Pre-landing review: the leading-flag loop breaks at the first positional, so the `escaped` flag only fired for a leading `--`. A `--` placed after a positional left trailing-switch hoisting active, so `agent run note -- body --detach` silently detached and dropped the `--` as junk. Suppress hoisting whenever a literal `--` appears in the prompt. Regression test added. * fix(status): deadline-ms usage-error + scoped stale_sections + cancel losing remote call (#1984) Pre-landing review (codex): (1) bare `--deadline-ms` with no value silently fell through to no-budget/--fast instead of a usage error; (2) thin-client timeout reported both sync+cycle stale even under `--section sync`, naming a section the caller excluded (local path was already correct); (3) the section race abandoned the remote promise locally but didn't cancel the in-flight MCP call — pass the budget as timeoutMs so the losing side actually cancels. Regression test added. * v0.42.52.0 fix(reliability): autopilot dead-job storm + supervisor wedge + sync/status/minion reliability (#2194 #2227 #1994 #1737 #1738 #1950 #1984) Bundles the already-reviewed autopilot/supervisor stabilization (#2194 #2227 #1994: cycle split, per-source failure cooldown, fan-out clamp, degraded supervisor retry, DB-lock live-supervisor detection) with four operational fixes: minion timeout attempt-accounting (#1737), agent-run trailing-flag parsing (#1738), honest live-sync sources status + progress-aware stall watchdog (#1950), and status version + --deadline-ms partial result (#1984). Co-Authored-By: Claude Opus 4.8 (1M context) * docs: document GBRAIN_SYNC_STALL_ABORT_SECONDS env knob (#1950) Post-ship doc sync (/document-release): add the sync stall watchdog env var to the CLAUDE.md sync-tuning table (Five → Six knobs) + regenerate the llms bundle. * test: quarantine #2249 fanout tests as *.serial (R1 env-isolation) (#2194) The cherry-picked autopilot-fanout-clamp + doctor-autopilot-fanout-concurrency tests mutate process.env.GBRAIN_AUDIT_DIR in beforeEach/afterEach, which the check:test-isolation R1 lint flags (parallel shards load multiple files per process). Rename to *.serial.test.ts (sanctioned quarantine — they run under --max-concurrency=1) instead of restructuring the reviewed test bodies. No logic change; both files stay green (9 tests). Fixes the failing verify CI check. --------- Co-authored-by: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 21 ++ CLAUDE.md | 3 +- TODOS.md | 21 ++ VERSION | 2 +- docs/architecture/KEY_FILES.md | 12 +- llms-full.txt | 3 +- package.json | 2 +- src/commands/agent.ts | 95 +++++- src/commands/autopilot-fanout.ts | 311 +++++++++++++++++- src/commands/autopilot.ts | 24 +- src/commands/doctor.ts | 68 +++- src/commands/jobs.ts | 128 ++++++- src/commands/sources.ts | 39 ++- src/commands/status.ts | 175 ++++++++-- src/commands/sync.ts | 80 ++++- src/core/cycle.ts | 32 +- src/core/db-lock.ts | 54 +++ src/core/doctor-categories.ts | 1 + src/core/minions/child-worker-supervisor.ts | 70 +++- src/core/minions/handler-timeouts.ts | 3 + src/core/minions/queue.ts | 9 + src/core/minions/supervisor.ts | 29 +- src/core/operations.ts | 5 +- test/agent-cli.test.ts | 58 +++- test/autopilot-cycle-source-checkout.test.ts | 108 ++++++ test/autopilot-failure-cooldown.test.ts | 151 +++++++++ test/autopilot-fanout-clamp.serial.test.ts | 110 +++++++ test/autopilot-fanout-wiring.test.ts | 7 +- test/autopilot-global-maintenance.test.ts | 146 ++++++++ test/child-worker-supervisor.test.ts | 68 +++- test/db-lock-inspect.test.ts | 38 +++ ...utopilot-fanout-concurrency.serial.test.ts | 84 +++++ test/e2e/autopilot-cooldown-parity.test.ts | 62 ++++ test/e2e/minions-resilience.test.ts | 4 + test/e2e/status-pglite.test.ts | 5 + test/get-status-snapshot-op.test.ts | 2 + test/minions.test.ts | 6 + test/status-sections.test.ts | 70 +++- test/supervisor-audit.test.ts | 42 +++ test/supervisor-db-lock.test.ts | 65 +++- test/supervisor.test.ts | 39 ++- test/sync-hard-deadline.test.ts | 22 ++ 42 files changed, 2169 insertions(+), 105 deletions(-) create mode 100644 test/autopilot-cycle-source-checkout.test.ts create mode 100644 test/autopilot-failure-cooldown.test.ts create mode 100644 test/autopilot-fanout-clamp.serial.test.ts create mode 100644 test/autopilot-global-maintenance.test.ts create mode 100644 test/doctor-autopilot-fanout-concurrency.serial.test.ts create mode 100644 test/e2e/autopilot-cooldown-parity.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 565627272..f0007b641 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,27 @@ All notable changes to GBrain will be documented in this file. +## [0.42.52.0] - 2026-06-18 + +**Autopilot stops manufacturing dead jobs and wedging its own queue, plus four operational rough edges get fixed: minion attempt-accounting, `agent run` flag parsing, honest `sources status`, and a budgeted `gbrain status`.** On a multi-source Postgres brain, autopilot could fan out a continuous stream of dead `autopilot-cycle` jobs while the supervisor periodically wedged the very queue it exists to keep alive. The root cause was one disease with several interacting parts; this wave addresses all of them, then cleans up four smaller reliability bugs found alongside. + +### Changed +- **Autopilot runs one brain-wide maintenance pass, not one per source.** The cycle is split: per-source jobs run only source-scoped phases, and a single `autopilot-global-maintenance` job runs the brain-wide phases once per window. This removes the per-cycle memory blow-up that was the shared root cause of the dead-job storm and the queue wedge. Per-source filesystem phases bind to the source's own path, so the freshness stamp and the work agree on which source ran. +- **The supervisor self-heals instead of giving up.** A transient database blip no longer trips the crash-budget breaker into a permanent stop; the supervisor degrades to capped-backoff retry and recovers, with a hard ceiling as the backstop. It detects a live sibling supervisor through the queue's database lock (not a `$HOME`-derived pidfile), so two supervisors under a split home directory can't both claim the queue. +- **`gbrain sources status` tells a running sync apart from an idle source.** A source holding a live sync lock now reads as actively syncing instead of "idle," matching the honest-freshness signal `gbrain doctor` already shows. + +### Added +- **Per-source failure cooldown + fan-out clamp.** A source that fails backs off (bounded exponential) instead of being re-dispatched every tick; per-tick fan-out is clamped to the worker concurrency, with a `gbrain doctor` check that warns on a mismatch. +- **`gbrain status --deadline-ms` / `--fast`.** A budgeted status snapshot returns whatever sections completed within the budget (marked partial) instead of hanging a poller; the JSON envelope also carries the CLI `version`. +- **A sync stall watchdog.** If the import drain makes no forward progress for `GBRAIN_SYNC_STALL_ABORT_SECONDS` (default 900), the run aborts and releases its per-source lock so the next `gbrain sync` resumes from the checkpoint — no manual `pkill`. It keys off import progress (not the lock heartbeat) and reports a distinct `stall_timeout` reason. (Limit: a hang inside a single file's import is observed between files, not mid-file; the wall-clock deadline remains the backstop there.) + +### Fixed +- **A timed-out minion run counts as a spent attempt.** Wall-clock dead-lettering already did; the per-job timeout path didn't, so long-lane jobs (subagent / embed-backfill / autopilot-cycle) could read `attempts: 0/N (started: N)`. Accounting is now honest across all dead-letter paths. +- **`gbrain agent run` no longer swallows flags after the prompt.** A trailing `--detach` / `--follow` is recognized instead of being captured into the prompt string; a `--word` inside the prompt stays verbatim, and an explicit `--` ends flag parsing anywhere. + +### To take advantage of v0.42.52.0 +`gbrain upgrade`. Existing brains pick up the cycle split, supervisor backoff, and per-source cooldown on the next autopilot tick (one catch-up global-maintenance pass on the first tick) — no migration, all on by default. Tune the sync stall watchdog with `GBRAIN_SYNC_STALL_ABORT_SECONDS` if 900s doesn't fit your largest files; budget a status poller with `gbrain status --fast` or `--deadline-ms=`. + ## [0.42.51.0] - 2026-06-17 **`gbrain sync` stops bottlenecking all its workers on a single database row, a malformed checkpoint can no longer wedge a source, and `gbrain doctor` tells an actively-running sync apart from a stuck one.** A slow source that fell behind HEAD could read as permanently stale even while it imported every cycle: sync was single-core-bound at the database layer, so handing it more workers didn't help, and the freshness check couldn't see that a sync was in fact running. diff --git a/CLAUDE.md b/CLAUDE.md index 30c36e6df..edf1a4b9b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -390,7 +390,7 @@ Progress banks into the append-only `op_checkpoint_paths` table (one row per dra path, written via the direct session pool so it survives `EMAXCONNSESSION`); a killed run resumes from the checkpoint and `last_commit` only advances on true completion. The per-source lock heartbeats through the direct pool and refuses to steal a live, -recently-refreshed holder. Five env knobs tune it (all env-only, incident-time escape +recently-refreshed holder. Six env knobs tune it (all env-only, incident-time escape hatches — no config-dashboard surface by design): | Env var | Default | What it does | @@ -400,6 +400,7 @@ hatches — no config-dashboard surface by design): | `GBRAIN_SYNC_MAX_CHECKPOINT_FAILURES` | 3 | Consecutive failed flushes (each already retried ~12s) before the run aborts with `reason: 'checkpoint_unavailable'` instead of importing work it can never bank. | | `GBRAIN_SYNC_YIELD_EVERY` | 64 | Yield the event loop (`setTimeout(0)`, NOT `setImmediate` — Bun starves the timers phase under a tight setImmediate loop) every N files so the lock-refresh `setInterval` heartbeat fires mid-import. | | `GBRAIN_LOCK_STEAL_GRACE_SECONDS` | derived (~600 at 30min TTL) | A holder that refreshed within this window is NOT stolen even if its TTL lapsed (starved-but-alive). Dead holders stop refreshing, age past the grace, and become stealable; TTL stays the backstop. | +| `GBRAIN_SYNC_STALL_ABORT_SECONDS` | 900 | Progress-aware stall watchdog (#1950): if the import drain makes no forward progress (keyed on file-import progress, NOT the lock heartbeat) for N seconds, abort the run and release the per-source lock so the next `gbrain sync` resumes from the checkpoint. Reports `reason: 'stall_timeout'`. Observed BETWEEN files; a hang inside one file's import isn't interrupted until it returns (the wall-clock hard deadline is that backstop). 0 disables. | ## Pace Mode (DB-contention-aware backfill pacing) diff --git a/TODOS.md b/TODOS.md index 8321af0e8..153805c36 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1,5 +1,26 @@ # TODOS +## reliability fix-wave follow-ups (filed v0.42.52.0) + +Deferred from the autopilot/supervisor + sync/status/minion reliability wave +(plan-eng-review + codex + adversarial diff review CLEARED). Both surfaced by the +ship-stage pre-landing review; neither blocks the wave. + +- [ ] **P2 — Thread a cancellation signal through `importFile` (#1950).** The sync + stall watchdog aborts `opts.signal`, but the per-iteration abort checks observe + it BETWEEN files — a hang inside one `importFile` call (e.g. a stuck embed + network request) isn't interrupted until that call returns. Thread an + `AbortSignal` into `importFromContent`/`importFromFile` and check it at the async + phase boundaries (post-parse, pre-embed, pre-DB-write) so an in-flight wedge is + reaped too. Core hot path (engine-parity + downstream-client surface) — scope it + on its own. Where: `src/core/import-file.ts`, `src/commands/sync.ts`. +- [ ] **P3 — Centralize live-sync liveness onto `liveSyncStatus` (#1950).** + `gbrain sources status` now uses the shared `liveSyncStatus(engine, sourceId)` + helper; retrofit `gbrain doctor` (its own inline lock probe) and `gbrain status` + onto the same helper so there's one source of truth for "is this source + syncing." Where: `src/core/db-lock.ts`, `src/commands/doctor.ts`, + `src/commands/status.ts`. + ## Pace Mode follow-ups (filed v0.42.49.0) Deferred from the paced-backfill wave (CEO + eng review CLEARED). Core shipped: diff --git a/VERSION b/VERSION index 6a6c1a02a..752fea66c 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.42.51.0 +0.42.52.0 diff --git a/docs/architecture/KEY_FILES.md b/docs/architecture/KEY_FILES.md index b2d7fe7d2..1753b4744 100644 --- a/docs/architecture/KEY_FILES.md +++ b/docs/architecture/KEY_FILES.md @@ -228,13 +228,13 @@ per-release `**vX.Y.Z:**` narration — CI enforces this - `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). `add()` plumbs `max_stalled` through with a `[1, 100]` clamp; omitted values let the schema DEFAULT (5) kick in. `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). The `maxWaiting` coalesce path 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. `claim` and `renewLock` issue their UPDATE via `engine.executeRawDirect` (not `executeRaw`) so the lock heartbeat runs on the direct session-mode pool that the transaction pooler won't recycle mid-hold; on PGLite this is identical to `executeRaw`. The two terminal dead-letter paths (`handleWallClockTimeouts` wall-clock kill and the stall dead-letter CTE in the stall sweep) BOTH increment `attempts_made` so a long job killed there reads as an honest attempt instead of `attempts 0 / started N`; the stall path also bumps `stalled_counter`, surfaced by `gbrain jobs get` as `Attempts: M/N (started: X, stalled: S/MaxS)`. At submit, `add()` stamps a default `timeout_ms` via `defaultTimeoutMsFor(jobName)` (from `handler-timeouts.ts`) when the caller passed none, so long handlers aren't wall-clock-killed mid-progress by the short null-default; an explicit `opts.timeout_ms` always wins. Guarded by `test/queue-lock-retry.test.ts` (claim never falls back to `executeRaw`), `test/postgres-execute-raw-direct.test.ts` (routing decision matrix), and `test/minions.test.ts` (attempt accounting + default-timeout stamping). - `src/core/minions/worker.ts` — MinionWorker class (handler registry, lock renewal, graceful shutdown, timeout safety net). Aborted jobs call `failJob` with reason (`timeout`/`cancel`/`lock-lost`/`shutdown`); `shutdownAbort` (instance field) fires on SIGTERM/SIGINT and propagates to `ctx.shutdownSignal` (shell handler listens; non-shell handlers don't). Per-job timeout fires `abort.abort(new Error('timeout'))` then a 30s grace-then-evict safety net force-evicts the job from `inFlight` and marks it dead if the handler ignores the abort signal (the moved-out generic abort listener now fires for ANY abort reason). The `launchJob` lock-renewal block is a thin sync wrapper around the pure `runLockRenewalTick` from `src/core/minions/lock-renewal-tick.ts` (NEVER `setInterval(async () => await renewLock(...))` — that shape was the unhandledRejection crash class during PgBouncer rotation). Gaps closed: (1) `cancelled` flag captured in the timer closure stops in-flight IIFEs writing misleading audit events after the job ended; (2) re-entrancy guard `tickInFlight` + per-call `Promise.race` timeout (`GBRAIN_LOCK_RENEWAL_CALL_TIMEOUT_MS`, default `lockDuration/3`); (3) time-based abort (`Date.now() - lastSuccessfulRenewalAt >= lockDuration - GBRAIN_LOCK_RENEWAL_SAFETY_MARGIN_MS`) so we release the lock BEFORE another worker can reclaim; (4) explicit `.catch()` on the stored `executeJob(...).finally(...)` promise closes the second unhandledRejection vector; (5) exported `INFRASTRUCTURE_ABORT_REASONS = new Set(['lock-renewal-failed', 'lock-lost'])` so executeJob's catch skips `failJob` for these (PgBouncer blips don't dead-letter healthy jobs; the stall detector reclaims). CI guard `scripts/check-worker-lock-renewal-shape.sh` (in `bun run verify`) asserts the bug pattern stays absent AND `launchJob` keeps calling `runLockRenewalTick`. Engine-ownership invariant: `start()` does NOT call `engine.disconnect()` on shutdown — the CLI handler in `src/commands/jobs.ts case 'work'` owns engine lifecycle via try/finally with loud error logging. RSS watchdog uses non-file-backed pages on Linux: exported `parseRssFromProcStatus(status)` (pure parser; field-presence regex so `RssAnon: 0 + RssShmem: 512` parses correctly) and `getAccurateRss(readStatus?)` (reads `/proc/self/status` for `RssAnon + RssShmem`, falls back to `process.memoryUsage().rss` on macOS / restricted containers / kernel <4.5); the default `getRss` in `WorkerOpts` is `getAccurateRss`. `checkMemoryLimit` tracks peak RSS, fires an 80%-of-cap soft-warn (once per crossing, carrying peak + in-flight job kinds), and on exceed sets `_rssWatchdogTriggered=true` (exposed via `get rssWatchdogTriggered()`) so `jobs work`'s finally can `process.exit(WORKER_EXIT_RSS_WATCHDOG)` after disconnect (drain self-identifying instead of an opaque code-0 exit). The poll loop wraps `claim` in try/catch: on a retryable conn error it reconnects ONCE and continues to the next tick rather than blind-retrying (a retry after `UPDATE...RETURNING` committed but the socket died would double-claim). `LockRenewalDeps` is wired with `reconnect` when the engine supports it. The self-health DB-liveness probe now runs EVEN under a supervisor (`GBRAIN_SUPERVISED=1`): the outer guard is `if (this.opts.healthCheckInterval > 0)` and only the STALL-detection block is wrapped in `if (!isSupervisedChild)` — so a supervised worker whose own pool dies self-exits `unhealthy(db_dead)` after `dbFailExitAfter` probes (the supervisor watches a different connection and can't see this worker's dead pool), while the supervisor's progress watchdog owns forward-progress (#1801). Pinned by `test/worker-lock-renewal.test.ts` (18), `test/audit/lock-renewal-audit.test.ts` (11), `test/scripts/check-worker-lock-renewal-shape.test.ts` (5), `test/worker-shutdown-disconnect.test.ts` (asserts `disconnectSpy).not.toHaveBeenCalled()`), `test/worker-rss.test.ts` (11), `test/worker-supervised-db-probe.test.ts` (3). -- `src/core/minions/supervisor.ts` — MinionSupervisor process manager. Spawns `gbrain jobs work` as a child, restarts on crash with exponential backoff, periodic health check. `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. Worker exit classifier emits `likely_cause` on `worker_exited` events: `oom_or_external_kill` (SIGKILL), `graceful_shutdown` (SIGTERM), `runtime_error` (code 1), `clean_exit` (code 0), `unknown`. 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 the in-process SIGCHLD reaper can't reach); exposes `isTiniDetected` read-only accessor. The spawn-and-respawn loop is the shared `ChildWorkerSupervisor` core: MinionSupervisor composes it via `runSuperviseLoop()` → `new ChildWorkerSupervisor({...})` and maps `ChildSupervisorEvent` back through `emit()` SupervisorEvent (JSONL audit consumers see byte-compatible output). PID lock, signal handlers, health check, and `process.exit` on max-crashes stay in MinionSupervisor. `code=0` leaves `crashCount` untouched (so a worker alternating real crashes + watchdog drains still trips `max_crashes`); `cleanRestartBudget` (default 10 restarts per 60s) caps the macOS/non-Linux-fallback tight-loop via `health_warn { reason: 'clean_restart_budget_exceeded' }` + backoff. `shutdown()` drains via `childSupervisor.killChild('SIGTERM')` + `awaitChildExit(35_000)`. Progress watchdog (#1801): `healthCheck()` restarts an alive-but-wedged child via `childSupervisor.restartCurrentChild(35_000)` when a queue has claimable work, 0 live-lock active jobs, and stale completions across `wedgeRestartChecks` (default 3) consecutive checks past `wedgeRestartMinutes` (default 15, 0 disables) + a `startupGraceMs` window; bounded by `wedgeRestartLoopBudget` (default 3 / `wedgeRestartLoopWindowMs`) which switches to a one-shot `wedge_restart_loop` alert. The wedge query is the exported `queryWedgeSignals(engine, queue, handlerNames)` — name+queue-scoped, `active_healthy` = live-lock only (an expired-lock active row does NOT mask the wedge), due-delayed counted. Claimable names are derived at start via a throwaway `registerBuiltinHandlers` worker (its new `quiet` opt). Flags `--wedge-restart-minutes` / `--wedge-restart-checks` + env `GBRAIN_WEDGE_RESTART_MINUTES` / `GBRAIN_WEDGE_RESTART_CHECKS`. The worker argv is built by the exported pure `buildWorkerArgs(opts)` (appends `--nice N` when `opts.nice_requested` is set, alongside `--concurrency`/`--queue`/`--max-rss`); the niceness apply RESULT (`nice_requested`/`nice_effective`/`nice_error`, computed by the CLI in jobs.ts — the supervisor doesn't call setPriority) rides on the `started`/`worker_spawned` audit emissions (#1815). Queue-scoped singleton (#1849): the real authority is a DB lock (`tryAcquireDbLock` from `src/core/db-lock.ts`) keyed on `supervisorLockId(queue)` = `gbrain-supervisor:` — keyed on the QUEUE ALONE because the lock row lives inside the target database, so the (database) half of the mutex is physical, not part of the key (an earlier revision mixed in a config-derived DB identity, which let two supervisors on the same physical DB via different-but-equivalent URLs compute different ids and both acquire — fixed). Two supervisors with different `$HOME`/`--pid-file` against the same `(database, queue)` no longer both run with conflicting `--max-rss`; the second exits `LOCK_HELD`. The pidfile-cleanup `process.on('exit')` listener is installed BEFORE the DB-lock acquisition so the `LOCK_HELD` early-exit can't strand the pidfile this process just created. The default pidfile is now brain-scoped (`supervisor-.pid`) so different brains under one HOME don't false-block. The lock refreshes on its own `setInterval` (TTL 5min, refresh 60s, max 3 failures = 180s < TTL); a refresh that fails past the threshold exits `LOCK_LOST` (code 4) rather than risk a split-brain. `shutdown()` releases the lock so a clean restart re-acquires immediately. The `started` audit now records `max_rss_mb` so `gbrain doctor`'s `supervisor_singleton` check can surface the effective cap. Exports `supervisorLockId()` and the pure `classifySupervisorSingleton({lockLive, lockHolderHost, lockHolderPid, localHost, localPid}) → 'no_lock'|'single'|'mismatch'` (host+pid compare, bare pid meaningless cross-host) that doctor consumes. Pinned by `test/supervisor.test.ts` (16 cases), `test/supervisor-tini.test.ts`, `test/supervisor-wedge.test.ts`, `test/supervisor-build-worker-args.test.ts`, and `test/supervisor-db-lock.test.ts`. -- `src/core/minions/child-worker-supervisor.ts` — shared spawn-and-respawn core reused by both `MinionSupervisor` (standalone `gbrain jobs supervisor` daemon) and `src/commands/autopilot.ts` (autopilot daemon), so the two consumers can't drift into parallel-loop bugs. Pure class: NO PID file, NO signal handlers, NO `process.exit`, NO health check. Lifecycle events fire via injected `onEvent: (ChildSupervisorEvent) => void`. Exit classifier: `code === 0` leaves `crashCount` UNCHANGED (preserves flap detection across mixed exit sequences); `code != 0` follows `runDuration > stableRunResetMs ? 1 : ++crashCount`. Clean-restart budget: sliding window of code=0 exits; when count exceeds `cleanRestartBudget` (default 10) inside `cleanRestartWindowMs` (default 60s), emits `health_warn { reason: 'clean_restart_budget_exceeded' }` and applies `cleanRestartBudgetBackoffMs` (default 1s). The exit classifier special-cases `WORKER_EXIT_RSS_WATCHDOG` — `likely_cause='rss_watchdog'`, and that exit does NOT bump `crashCount` (routes to its own breaker); a dedicated `_watchdogExitTimestamps` sliding window trips a loud `rss_watchdog_loop` health_warn naming the cap when N watchdog exits land inside the window, INDEPENDENT of the stable-run reset (which would otherwise hide a >5-min-run watchdog drain loop). New opts `watchdogLoopBudget` (3), `watchdogLoopWindowMs` (600000), `watchdogBackoffMs` (30000); `ChildSupervisorEvent` extended. Public read-only accessors `childAlive`, `inBackoff`, `crashCount`; `killChild(signal)` gates on liveness (`exitCode === null && signalCode === null`), NOT `.killed` — `.killed` flips true once a signal is *sent*, so the old `!this._child.killed` guard made a follow-up SIGKILL after an ignored SIGTERM a silent no-op (#1801; the bug also lived in the `shutdown()` drain). `restartCurrentChild(graceMs)` (#1801 wedge self-heal) captures the CURRENT child ref, SIGTERM→grace→SIGKILLs THAT ref (never the respawn — closes the timer-kills-fresh-worker race), and flags `_intentionalRestart` so the exit is `likelyCause='wedge_restart'`, leaves `crashCount` UNTOUCHED (never trips `max_crashes`; like `rss_watchdog`), and respawns immediately (`backoff ms:0 reason='wedge_restart'`). `awaitChildExit(timeoutMs)` short-circuits when `child.exitCode !== null || child.signalCode !== null` so fast-SIGTERM responders don't cause a 35s shutdown hang. Test hooks `_backoffFloorMs`, `_now`. `supervisor-audit.ts` adds `rss_watchdog` as a non-clean cause + its own `CrashSummary.by_cause` bucket, and `wedge_restart` to `CLEAN_EXIT_CAUSES` (a self-heal, not a crash; denylist preserved so future causes route to `legacy`). Pinned by `test/child-worker-supervisor.test.ts` (12 cases). +- `src/core/minions/supervisor.ts` — MinionSupervisor process manager. Spawns `gbrain jobs work` as a child, restarts on crash with exponential backoff, periodic health check. `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. Worker exit classifier emits `likely_cause` on `worker_exited` events: `oom_or_external_kill` (SIGKILL), `graceful_shutdown` (SIGTERM), `runtime_error` (code 1), `clean_exit` (code 0), `unknown`. 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 the in-process SIGCHLD reaper can't reach); exposes `isTiniDetected` read-only accessor. The spawn-and-respawn loop is the shared `ChildWorkerSupervisor` core: MinionSupervisor composes it via `runSuperviseLoop()` → `new ChildWorkerSupervisor({...})` and maps `ChildSupervisorEvent` back through `emit()` SupervisorEvent (JSONL audit consumers see byte-compatible output). PID lock, signal handlers, health check, and `process.exit` on the HARD crash ceiling stay in MinionSupervisor. Crossing the SOFT budget (`maxCrashes`) no longer permanently gives up (#1994/#2227): the core drops into degraded retry (capped backoff + a `crash_budget_degraded` health_warn) and self-heals when a respawn runs stably; permanent `process.exit(MAX_CRASHES)` fires only at the hard ceiling `resolveHardStopMaxCrashes(maxCrashes)` (default `maxCrashes × 10`, env `GBRAIN_SUPERVISOR_HARD_STOP_CRASHES`, `0` = never). Separately, `gbrain jobs supervisor status` + `gbrain doctor` detect a live supervisor through this queue lock (`inspectLock` + `isLockHolderLive`, freshness-keyed so PID reuse can't false-positive) when the `$HOME`-derived pidfile is absent, so a split-`$HOME` deployment no longer reads a healthy supervisor as "not running" (#2227). `code=0` leaves `crashCount` untouched (so a worker alternating real crashes + watchdog drains still trips `max_crashes`); `cleanRestartBudget` (default 10 restarts per 60s) caps the macOS/non-Linux-fallback tight-loop via `health_warn { reason: 'clean_restart_budget_exceeded' }` + backoff. `shutdown()` drains via `childSupervisor.killChild('SIGTERM')` + `awaitChildExit(35_000)`. Progress watchdog (#1801): `healthCheck()` restarts an alive-but-wedged child via `childSupervisor.restartCurrentChild(35_000)` when a queue has claimable work, 0 live-lock active jobs, and stale completions across `wedgeRestartChecks` (default 3) consecutive checks past `wedgeRestartMinutes` (default 15, 0 disables) + a `startupGraceMs` window; bounded by `wedgeRestartLoopBudget` (default 3 / `wedgeRestartLoopWindowMs`) which switches to a one-shot `wedge_restart_loop` alert. The wedge query is the exported `queryWedgeSignals(engine, queue, handlerNames)` — name+queue-scoped, `active_healthy` = live-lock only (an expired-lock active row does NOT mask the wedge), due-delayed counted. Claimable names are derived at start via a throwaway `registerBuiltinHandlers` worker (its new `quiet` opt). Flags `--wedge-restart-minutes` / `--wedge-restart-checks` + env `GBRAIN_WEDGE_RESTART_MINUTES` / `GBRAIN_WEDGE_RESTART_CHECKS`. The worker argv is built by the exported pure `buildWorkerArgs(opts)` (appends `--nice N` when `opts.nice_requested` is set, alongside `--concurrency`/`--queue`/`--max-rss`); the niceness apply RESULT (`nice_requested`/`nice_effective`/`nice_error`, computed by the CLI in jobs.ts — the supervisor doesn't call setPriority) rides on the `started`/`worker_spawned` audit emissions (#1815). Queue-scoped singleton (#1849): the real authority is a DB lock (`tryAcquireDbLock` from `src/core/db-lock.ts`) keyed on `supervisorLockId(queue)` = `gbrain-supervisor:` — keyed on the QUEUE ALONE because the lock row lives inside the target database, so the (database) half of the mutex is physical, not part of the key (an earlier revision mixed in a config-derived DB identity, which let two supervisors on the same physical DB via different-but-equivalent URLs compute different ids and both acquire — fixed). Two supervisors with different `$HOME`/`--pid-file` against the same `(database, queue)` no longer both run with conflicting `--max-rss`; the second exits `LOCK_HELD`. The pidfile-cleanup `process.on('exit')` listener is installed BEFORE the DB-lock acquisition so the `LOCK_HELD` early-exit can't strand the pidfile this process just created. The default pidfile is now brain-scoped (`supervisor-.pid`) so different brains under one HOME don't false-block. The lock refreshes on its own `setInterval` (TTL 5min, refresh 60s, max 3 failures = 180s < TTL); a refresh that fails past the threshold exits `LOCK_LOST` (code 4) rather than risk a split-brain. `shutdown()` releases the lock so a clean restart re-acquires immediately. The `started` audit now records `max_rss_mb` so `gbrain doctor`'s `supervisor_singleton` check can surface the effective cap. Exports `supervisorLockId()` and the pure `classifySupervisorSingleton({lockLive, lockHolderHost, lockHolderPid, localHost, localPid}) → 'no_lock'|'single'|'mismatch'` (host+pid compare, bare pid meaningless cross-host) that doctor consumes. Pinned by `test/supervisor.test.ts` (16 cases), `test/supervisor-tini.test.ts`, `test/supervisor-wedge.test.ts`, `test/supervisor-build-worker-args.test.ts`, and `test/supervisor-db-lock.test.ts`. +- `src/core/minions/child-worker-supervisor.ts` — shared spawn-and-respawn core reused by both `MinionSupervisor` (standalone `gbrain jobs supervisor` daemon) and `src/commands/autopilot.ts` (autopilot daemon), so the two consumers can't drift into parallel-loop bugs. Pure class: NO PID file, NO signal handlers, NO `process.exit`, NO health check. Lifecycle events fire via injected `onEvent: (ChildSupervisorEvent) => void`. Exit classifier: `code === 0` leaves `crashCount` UNCHANGED (preserves flap detection across mixed exit sequences); `code != 0` follows `runDuration > stableRunResetMs ? 1 : ++crashCount`. Clean-restart budget: sliding window of code=0 exits; when count exceeds `cleanRestartBudget` (default 10) inside `cleanRestartWindowMs` (default 60s), emits `health_warn { reason: 'clean_restart_budget_exceeded' }` and applies `cleanRestartBudgetBackoffMs` (default 1s). The exit classifier special-cases `WORKER_EXIT_RSS_WATCHDOG` — `likely_cause='rss_watchdog'`, and that exit does NOT bump `crashCount` (routes to its own breaker); a dedicated `_watchdogExitTimestamps` sliding window trips a loud `rss_watchdog_loop` health_warn naming the cap when N watchdog exits land inside the window, INDEPENDENT of the stable-run reset (which would otherwise hide a >5-min-run watchdog drain loop). New opts `watchdogLoopBudget` (3), `watchdogLoopWindowMs` (600000), `watchdogBackoffMs` (30000); `ChildSupervisorEvent` extended. Public read-only accessors `childAlive`, `inBackoff`, `crashCount`; `killChild(signal)` gates on liveness (`exitCode === null && signalCode === null`), NOT `.killed` — `.killed` flips true once a signal is *sent*, so the old `!this._child.killed` guard made a follow-up SIGKILL after an ignored SIGTERM a silent no-op (#1801; the bug also lived in the `shutdown()` drain). `restartCurrentChild(graceMs)` (#1801 wedge self-heal) captures the CURRENT child ref, SIGTERM→grace→SIGKILLs THAT ref (never the respawn — closes the timer-kills-fresh-worker race), and flags `_intentionalRestart` so the exit is `likelyCause='wedge_restart'`, leaves `crashCount` UNTOUCHED (never trips `max_crashes`; like `rss_watchdog`), and respawns immediately (`backoff ms:0 reason='wedge_restart'`). `awaitChildExit(timeoutMs)` short-circuits when `child.exitCode !== null || child.signalCode !== null` so fast-SIGTERM responders don't cause a 35s shutdown hang. Degraded-retry (#1994/#2227): the `run()` loop no longer fires `onMaxCrashesExceeded` at the soft `maxCrashes`; it announces `health_warn { reason: 'crash_budget_degraded' }` once per episode and keeps respawning with capped backoff (the 60s cap makes it a paced retry, not a hot loop), re-arming after a stable-run reset drops the count. Permanent give-up fires only at `hardStopMaxCrashes` (default `maxCrashes × HARD_STOP_CRASH_MULTIPLIER` = 10×; `0` disables). Test hooks `_backoffFloorMs`, `_now`. `supervisor-audit.ts` adds `rss_watchdog` as a non-clean cause + its own `CrashSummary.by_cause` bucket, and `wedge_restart` to `CLEAN_EXIT_CAUSES` (a self-heal, not a crash; denylist preserved so future causes route to `legacy`). Pinned by `test/child-worker-supervisor.test.ts` (12 cases). - `src/core/minions/spawn-helpers.ts` — pure `detectTini()` + `buildSpawnInvocation()` consumed by both `supervisor.ts` and `autopilot.ts` (resolves the DRY violation between the two spawn sites and makes 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. `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) and `test/supervisor-tini.test.ts` (4). - `src/core/minions/niceness.ts` — OS scheduling-priority (niceness) primitives for the `--nice` flag. `parseNiceValue(raw)` whole-string parses + range-validates to POSIX `[-20, 19]` (rejects `"3.5"`/`"10abc"` that `parseInt` would silently truncate). `applyNiceness(nice, setPriority?, getPriority?)` calls `os.setPriority(0, n)` and ALWAYS re-reads `os.getPriority(0)` afterwards — in both the success and the catch paths — so a denied renice (EPERM) or an `RLIMIT_NICE` clamp records the real effective value (e.g. `0`), not null; returns `{applied, requested, effective, error?}`. `getEffectiveNiceness(pid, getPriority?)` reads an arbitrary pid's niceness (null on dead/unreadable). `formatNice(n)` → `+10`/`0`/`-5`. Applied only at the CLI layer (jobs.ts) so worker.ts/supervisor.ts stay embeddable. Pinned by `test/niceness.test.ts`. - `src/core/minions/worker-registry.ts` — live worker registry backing niceness observability. Each running `gbrain jobs work` self-registers `worker-.json` under `gbrainPath('workers')` (brain-isolated via `GBRAIN_HOME`; entries tagged with `currentBrainId()` so multiple DBs under one home don't cross-report). `registerWorker(info)` is best-effort (never blocks the worker) and returns a cleanup fn the caller wires to BOTH the shutdown `finally` AND `process.on('exit')` (the unhealthy `process.exit(1)` bypasses the awaited finally). `readWorkers(getNice?)` enumerates the dir, drops confirmed-dead pids (`classifyLiveness`: ESRCH = dead/prune, EPERM = alive/keep), applies a pid-reuse start-time guard (`ps -o lstart`, rejects a pid whose process started >5s after the entry was written), and re-measures each live worker's niceness now. Reports the worker's REAL pid, sidestepping the tini-wrapper-pid problem. Pinned by `test/worker-registry.test.ts`. - `src/core/minions/supervisor-pid.ts` — `readSupervisorPid(pidFile) → {pid, running}`: the shared `existsSync → readFileSync → parseInt → process.kill(pid,0)` PID-file + liveness reader extracted from the three copies in `jobs.ts` (supervisor status), `jobs.ts` (stats), and `doctor.ts`. EPERM from the liveness probe counts as running. Pinned by `test/supervisor-pid.test.ts`. -- `src/core/minions/handler-timeouts.ts` (#1737) — per-handler default wall-clock budgets. `HANDLER_DEFAULT_TIMEOUT_MS` maps the long handlers (`subagent`, `subagent_aggregator`, `embed-backfill`, `autopilot-cycle`) to a 30-min default (the value `cycle/patterns.ts` already passed for subagents, generalized). `defaultTimeoutMsFor(jobName)` returns that default or `null` for short handlers (keep the tight null-default wall-clock). `MinionQueue.add()` stamps this onto `minion_jobs.timeout_ms` at submit time when the caller passed no `timeout_ms`, so a long job submitted without one isn't wall-clock-killed mid-progress; an explicit value always wins and already-queued jobs are NOT backfilled. Pinned by `test/minions.test.ts`. +- `src/core/minions/handler-timeouts.ts` (#1737) — per-handler default wall-clock budgets. `HANDLER_DEFAULT_TIMEOUT_MS` maps the long handlers (`subagent`, `subagent_aggregator`, `embed-backfill`, `autopilot-cycle`, `autopilot-global-maintenance`) to a 30-min default (the value `cycle/patterns.ts` already passed for subagents, generalized). `defaultTimeoutMsFor(jobName)` returns that default or `null` for short handlers (keep the tight null-default wall-clock). `MinionQueue.add()` stamps this onto `minion_jobs.timeout_ms` at submit time when the caller passed no `timeout_ms`, so a long job submitted without one isn't wall-clock-killed mid-progress; an explicit value always wins and already-queued jobs are NOT backfilled. Pinned by `test/minions.test.ts`. - `src/core/minions/types.ts` — `MinionJobInput` + `MinionJobStatus` + handler context types. `MinionJobInput.max_stalled` 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 + `inherit:`-resolved keys. 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`). `ShellJobParams.inherit?: string[]` is a free-form list of snake_case config-key names; the worker resolves each via `loadConfig()` and injects the value under the derived env key (`database_url` → `GBRAIN_DATABASE_URL`; else uppercased). Names persist in `minion_jobs.data` (and the shell-audit JSONL); values never do. The canonical validator `validateShellJobParams` (sibling `shell-validate.ts`) runs PRE-ENQUEUE in both submit surfaces — `gbrain jobs submit shell` (jobs.ts:271) AND the `submit_job` op for `name='shell'` (operations.ts:2085); the handler-entry re-validation here is defense-in-depth (closes the bug class where validation ran AFTER `queue.add()` persisted the row). The validator does NOT police which config keys the agent inherits — same-uid trust model treats the agent as a peer of the worker. @@ -258,7 +258,7 @@ per-release `**vX.Y.Z:**` narration — CI enforces this - `src/core/minions/attachments.ts` — Attachment validation (path traversal, null byte, oversize, base64, duplicate detection). - `src/commands/agent.ts` — `gbrain agent run [flags]` CLI. Submits `subagent` (or N children + 1 aggregator) under `{allowProtectedSubmit: true}`. Single-entry `--fanout-manifest` short-circuits. Children get `on_child_fail: 'continue'` + `max_stalled: 3`. `--follow` is the default on TTY; streams logs + polls `waitForCompletion` in parallel. Ctrl-C detaches, does not cancel. - `src/commands/agent-logs.ts` — `gbrain agent logs [--follow] [--since]`. Merges JSONL heartbeat audit + `subagent_messages` into a chronological timeline. `parseSince` accepts ISO-8601 or relative (`5m`, `1h`, `2d`). Transcript tail renders only for terminal jobs. -- `src/commands/jobs.ts` — `gbrain jobs` CLI subcommands + `gbrain jobs work` daemon. `case 'work'` wraps `worker.start()` in try/finally and owns engine lifecycle — calls `engine.disconnect()` on shutdown with loud error logging (the worker must not disconnect an engine it doesn't own; pool slots free immediately on shutdown rather than waiting for TCP keepalive). `jobs submit` surfaces the full `MinionJobInput` retry/backoff/timeout/idempotency surface as flags: `--max-stalled`, `--backoff-type fixed|exponential`, `--backoff-delay`, `--backoff-jitter`, `--timeout-ms`, `--idempotency-key`. `jobs smoke --sigkill-rescue` is the SIGKILL-rescue regression guard. `registerBuiltinHandlers` always registers `subagent` + `subagent_aggregator` (no env flag — `ANTHROPIC_API_KEY` is the cost gate, trust is via `PROTECTED_JOB_NAMES`) and loads `GBRAIN_PLUGIN_PATH` plugins at startup with a loud per-plugin line; `shell` handler still gated by `GBRAIN_ALLOW_SHELL_JOBS=1` (RCE surface). The `autopilot-cycle` handler forwards `job.data.phases` to `runCycle`, validated against `ALL_PHASES` from `src/core/cycle.ts` (invalid names filtered; empty/missing falls back to the default cycle). The `sync` handler resolves `sourceId` at entry from `sources.local_path` (mirrors `cycle.ts:480`) so multi-source brains read the per-source `last_commit` anchor; concurrency routes through `autoConcurrency()` in `src/core/sync-concurrency.ts` (PGLite stays serial); `noEmbed` default is `true`. `gbrain jobs supervisor status` at `jobs.ts:803-826` consumes `summarizeCrashes()` from `src/core/minions/handlers/supervisor-audit.ts` for parity with `gbrain doctor`: JSON adds `crashes_by_cause: {runtime_error, oom_or_external_kill, unknown, legacy}` + `clean_exits_24h`; human output gains per-cause + clean-exits lines. Pinned by 4 source-grep wiring assertions in `test/doctor.test.ts` requiring `crashes_by_cause` + `clean_exits_24h=` in both `doctor.ts` and `jobs.ts`. `gbrain jobs watch` decouples its two output axes: `--json` picks FORMAT (human default, never gated on isTTY), `--follow` picks LOOP (default `isTTY && !json`). Non-TTY with no flags prints ONE human snapshot then exits (clean for subagent/pipe/cron); `--follow` opts into a continuous stream (human plain per tick, or JSONL with `--json`); a TTY with no flags keeps the live ANSI dashboard. Resolution is the pure `resolveWatchMode(opts, isTTY): {json, follow, useAnsiDashboard}` in `src/commands/jobs-watch.ts`; the dispatch wires `--follow`. Pinned by `test/jobs-watch-mode.test.ts` (format×loop matrix incl. the TTY+`--json`-one-shot case) + `test/e2e/non-tty-output.serial.test.ts` (the `cmd `-backed per-source line-prefix helper. Exports `withSourcePrefix(id, fn)` (runs `fn` with `id` as active prefix; nested wraps replace then restore), `getSourcePrefix()` (read-only accessor; test seam), and `slog(...)` / `serr(...)` (prefix-aware `console.log`/`console.error`). Embedded-newline-safe: a multi-line string under prefix `[foo]` emits `[foo] line1\n[foo] line2`. Outside a wrap, `slog`/`serr` fall through to bare `console.log`/`console.error` so single-source callers see identical output (back-compat invariant). Use `src.id` (slug-validated by `sources add`) NOT `src.name` (free-form) to defeat log-injection through newline/control-character names. Coverage: `src/commands/sync.ts` performSync + callees, `src/commands/embed.ts` runEmbedCore + helpers, `src/core/progress.ts` emitHumanLine. - `src/core/cli-options.ts` — Global CLI flag parser. `parseGlobalFlags(argv)` returns `{cliOpts, rest}` with `--quiet` / `--progress-json` / `--progress-interval=` stripped. `getCliOptions()` / `setCliOptions()` expose a module-level singleton so commands reach resolved flags without parameter threading. `cliOptsToProgressOptions()` maps to reporter options. `childGlobalFlags()` returns the flag suffix to append to `execSync('gbrain ...')` calls in migration orchestrators. `OperationContext.cliOpts` extends shared-op dispatch for MCP callers. -- `src/core/db-lock.ts` — generic `tryAcquireDbLock(engine, lockId, ttlMinutes)` over the `gbrain_cycle_locks` table. Parameterized lock id so scopes nest cleanly: `gbrain-cycle` for the broad cycle (held by `cycle.ts`) and `gbrain-sync` (`SYNC_LOCK_ID`) for `performSync`'s narrower writer window. UPSERT-with-TTL semantics survive PgBouncer transaction pooling (unlike session-scoped `pg_try_advisory_lock`); crashed holders auto-release once their TTL expires. It also does automatic same-host dead-pid takeover: when the upsert finds a held, NOT-TTL-expired lock whose holder is on this host and provably dead, it reclaims via a guarded `DELETE WHERE id=$1 AND holder_pid=$2` + one normal-upsert retry returning the standard handle (refresh/release intact). The liveness check is the exported `classifyHolderLiveness(pid, host, ageMs, opts?)` / `isHolderDeadLocally(...)` (injectable `process.kill` seam; `HOLDER_TAKEOVER_GRACE_MS = 60_000` PID-reuse guard; EPERM classified as `alive` so a live process you don't own is never stolen). TTL-expired locks stay the upsert's job; cross-host stays TTL-only. `runBreakLock` (`src/commands/sync.ts`) consumes the same predicate. Background reaper (#1972): `reapDeadHolderLocks(engine)` is the periodic sweep the contention path lacked — it deletes locks whose holder is `isHolderDeadLocally`, scoped to the `gbrain-sync:*` / `gbrain-cycle`/`gbrain-cycle:*` namespaces ONLY (election/supervisor/reindex locks keep TTL-only behavior, untouched), via `deleteLockRowExact(engine, id, pid, acquiredAt)` — a snapshot-matched delete (`date_trunc('milliseconds', acquired_at) = $3`, so the ms a JS Date keeps survives) that's TOCTOU-safe against a reused PID taking the lock between SELECT and DELETE. `cycle.ts` runs it at cycle start (before the sync phase); `gbrain doctor --fix` runs it for no-autopilot brains. `selectLockRows(engine, opts?)` + a shared row→`LockSnapshot` mapper are the single canonical reader now backing `inspectLock` + `listStaleLocks` + the reaper (was triplicated). Pinned by `test/db-lock-auto-takeover.test.ts` + `test/db-lock-reap.test.ts`. +- `src/core/db-lock.ts` — generic `tryAcquireDbLock(engine, lockId, ttlMinutes)` over the `gbrain_cycle_locks` table. Parameterized lock id so scopes nest cleanly: `gbrain-cycle` for the broad cycle (held by `cycle.ts`) and `gbrain-sync` (`SYNC_LOCK_ID`) for `performSync`'s narrower writer window. UPSERT-with-TTL semantics survive PgBouncer transaction pooling (unlike session-scoped `pg_try_advisory_lock`); crashed holders auto-release once their TTL expires. It also does automatic same-host dead-pid takeover: when the upsert finds a held, NOT-TTL-expired lock whose holder is on this host and provably dead, it reclaims via a guarded `DELETE WHERE id=$1 AND holder_pid=$2` + one normal-upsert retry returning the standard handle (refresh/release intact). The liveness check is the exported `classifyHolderLiveness(pid, host, ageMs, opts?)` / `isHolderDeadLocally(...)` (injectable `process.kill` seam; `HOLDER_TAKEOVER_GRACE_MS = 60_000` PID-reuse guard; EPERM classified as `alive` so a live process you don't own is never stolen). TTL-expired locks stay the upsert's job; cross-host stays TTL-only. `runBreakLock` (`src/commands/sync.ts`) consumes the same predicate. Background reaper (#1972): `reapDeadHolderLocks(engine)` is the periodic sweep the contention path lacked — it deletes locks whose holder is `isHolderDeadLocally`, scoped to the `gbrain-sync:*` / `gbrain-cycle`/`gbrain-cycle:*` namespaces ONLY (election/supervisor/reindex locks keep TTL-only behavior, untouched), via `deleteLockRowExact(engine, id, pid, acquiredAt)` — a snapshot-matched delete (`date_trunc('milliseconds', acquired_at) = $3`, so the ms a JS Date keeps survives) that's TOCTOU-safe against a reused PID taking the lock between SELECT and DELETE. `cycle.ts` runs it at cycle start (before the sync phase); `gbrain doctor --fix` runs it for no-autopilot brains. `selectLockRows(engine, opts?)` + a shared row→`LockSnapshot` mapper are the single canonical reader now backing `inspectLock` + `listStaleLocks` + the reaper (was triplicated). `isLockHolderLive(snap, ttlMinutes)` (#2227) is the observability liveness predicate — freshness-keyed (`ttl_expired` plus the heartbeat steal-grace), never `process.kill`, so `gbrain jobs supervisor status` / `gbrain doctor` can report a live supervisor via its queue lock without a PID-reuse false-positive. Pinned by `test/db-lock-auto-takeover.test.ts` + `test/db-lock-reap.test.ts`. - `src/core/sync-concurrency.ts` — single source of truth for the parallel-sync policy. Exports `autoConcurrency(engine, fileCount, override?)` (PGLite always serial; explicit override clamped to >=1; auto path returns `DEFAULT_PARALLEL_WORKERS=4` when `fileCount > AUTO_CONCURRENCY_FILE_THRESHOLD=100`), `shouldRunParallel(workers, fileCount, explicit)` (explicit `--workers` bypasses the >50-file floor), and `parseWorkers(s)` (rejects `'0'`, `'-3'`, `'foo'`, `'1.5'`, trailing chars). Used by `performSync`, `performFullSync`, `runImport`, and the Minion `sync` handler so the sites can't drift. `DEFAULT_PARALLEL_SOURCES = 4` is a SEPARATE constant for the per-source fan-out under `gbrain sync --all` — kept distinct from `DEFAULT_PARALLEL_WORKERS` because total live Postgres connections per wave ≈ `DEFAULT_PARALLEL_SOURCES × DEFAULT_PARALLEL_WORKERS × 2 (per-file pool)` = 32 at both defaults (each per-file worker opens its own `PostgresEngine` with `poolSize = min(2, resolvePoolSize(2))`); `sync.ts` warns when `parallel × workers × 2 > 16`. `resolveWorkersWithClamp(engine, override, commandName, fileCount)` wraps `autoConcurrency` with a per-command stderr clamp warning on PGLite (per-(command, requested) dedup via module-scoped warned-once set with `_resetWorkersClampWarningsForTest()` seam) and is the canonical surface for every bulk-command `--workers N` flag (extract-conversation-facts, extract, edges-backfill, reindex-multimodal, reindex, reindex-code); embed.ts deliberately bypasses it and keeps `GBRAIN_EMBED_CONCURRENCY || 20`. `resolveMaxConnections()` (reads `GBRAIN_MAX_CONNECTIONS`, undefined when unset) + `clampWorkersForConnectionBudget(workers, perWorkerPool, maxConnections, parentPool)` back the opt-in single-sync connection-footprint clamp so a big sync stays under a low pooler cap (`parent_pool + workers×perWorkerPool ≤ budget`); `gbrain doctor`'s `pool_budget` check (`computePoolBudgetCheck` / `checkPoolBudget` in `src/commands/doctor.ts`) warns when the budget leaves no room for a worker, pointing at `GBRAIN_POOL_SIZE=2`. Pinned by `test/pglite-workers-clamp.test.ts`. - `src/core/worker-pool.ts` — Canonical sliding-pool + bounded-semaphore primitive (extracted from `src/commands/embed.ts` sliding-pool sites and `src/commands/eval-cross-modal.ts` `runWithLimit` semaphore). Two exports: `runSlidingPool({items, workers, onItem, signal?, onError?, failureLabel?, onProgress?})` + `runWithLimit({items, limit, fn, signal?})`. Atomicity invariant: `const idx = nextIdx++` is one synchronous JS statement (no `await` between read and write — guaranteed by the single-threaded event loop), documented in the module header AND enforced by `scripts/check-worker-pool-atomicity.sh` (wired into `bun run verify`), which rejects importing `worker_threads` in any consuming file and inserting `await` between the `nextIdx` read and write. `MUST_ABORT_ERROR_TAGS` set is seeded with `BUDGET_EXHAUSTED` from `src/core/budget/budget-tracker.ts`; tagged errors (matched via `err.tag === 'BUDGET_EXHAUSTED'` to avoid cross-module import) bypass `onError` and hard-abort the pool via `AbortController.abort()` to in-flight `onItem` — the budget cap is a structural ceiling under concurrency. `failures[]` shape is `{idx, label, error}` records (NOT full items; callers supply `failureLabel(item) => string`) for bounded memory under huge brains. Pinned by `test/worker-pool.test.ts` + `test/scripts/check-worker-pool-atomicity.test.ts`. Drives every `--workers N` bulk command. - `src/commands/embed.ts` extension — both inline sliding-pool sites (`embedAll` simple at `:458-467` and `embedAllStale` paginated + AbortSignal at `:586-632`) call `runSlidingPool` from the shared worker-pool helper. Invariant-level contract preserved (counts + cost + AbortSignal propagation + per-batch rate-limit retry via `embedBatchWithBackoff`); byte-equality on progress-event ORDERING is NOT promised. The `GBRAIN_EMBED_CONCURRENCY || 20` default is preserved and embed bypasses `resolveWorkersWithClamp` because the 20-worker default would otherwise silently change every brain's embed hot path. Pinned by `test/embed-helper-migration.test.ts` (asserts the helper is wired in AND the pre-migration `let nextIdx = 0` + `Promise.all(Array.from({length: numWorkers}, ...))` shapes are gone). @@ -305,7 +305,7 @@ per-release `**vX.Y.Z:**` narration — CI enforces this - `src/commands/import.ts` — `gbrain import` CLI + `runImport` library entrypoint. Uses a path-set checkpoint via `src/core/import-checkpoint.ts` (the walk still applies `sortNewestFirst()` for embed-cost ordering, but checkpoint correctness no longer depends on sort order). A file enters `completed: Set` only when its `processFile` returns success (including content-hash short-circuit no-ops); failed files never enter the set so the next run retries them automatically with no manual `~/.gbrain/import-checkpoint.json` delete. This closes three classes: parallel-import-with-slow-worker dropping the slow file on crash-resume (the slow file isn't in `completed` until its own `processFile` resolves), failed-file-bumps-counter-past-itself (failures don't add to `completed`), and sort-flip-drops-newest-N-on-cross-version-resume (order is no longer part of the checkpoint). Old positional checkpoints are detected and discarded with a stderr line on first resume (re-walking is cheap because `content_hash` short-circuits unchanged files). Checkpoint persists every 100 successful adds, not every 100 processed files. The `managedBookmark` opt (set by `performFullSync` when `runImport` is the full-sync engine) suppresses `runImport`'s own `sync.last_commit` advance so the shared `applySyncFailureGate` (`src/core/sync-failure-ledger.ts`) owns the bookmark + failure-ledger gating on that path — one gate decides advance/block/auto-skip across both sync paths. Pinned by `test/import-checkpoint.test.ts` + `test/import-resume.test.ts` (incl. the SLUG_MISMATCH retry regression). - `src/core/import-checkpoint.ts` — `loadCheckpoint(brainDir)`, `saveCheckpoint(brainDir, completed)`, `resumeFilter(files, completed, brainDir)`, `clearCheckpoint()`, plus the `ImportCheckpoint` type. Path-set format `{schema_version, brainDir, completed: string[]}`. Atomic write via `.tmp` + `rename()` so a mid-write crash never leaves a partial JSON. `loadCheckpoint` returns `null` on: missing file, malformed JSON, brainDir mismatch (ran against a different brain), and the old positional format (logged to stderr before discard). `resumeFilter` returns `{toProcess, skippedCount}` — pure, no I/O, deterministic. `clearCheckpoint` is no-op-on-missing for clean-exit cleanup. Honors `GBRAIN_HOME` via `gbrainPath()` so `withEnv({GBRAIN_HOME: tmpdir})` test isolation works without monkey-patching fs. Best-effort persistence — `saveCheckpoint` logs warnings on write errors but never throws. - `src/core/sort-newest-first.ts` — single source of truth for the descending-lex sort that `gbrain import` and `gbrain sync` both apply. Mutates in place (Array.prototype.sort semantics), returns the same array reference for fluent chaining. Empty/single-element inputs short-circuit. Future ordering changes flip one line here instead of touching two CLI commands. Pinned by `test/sort-newest-first.test.ts` (descending order, mixed prefixes, empty, single-element, in-place-mutation contract). -- `src/core/cycle.ts` — brain maintenance cycle primitive (9 phases). `runCycle(engine: BrainEngine | null, opts: CycleOpts): Promise` composes phases in semantic order: lint → backlinks → sync → synthesize → extract → patterns → recompute_emotional_weight → embed → orphans. `synthesize` runs after sync (cross-references see a fresh brain) and before extract (auto-link materializes its writes); `patterns` runs after extract so it reads a fresh graph (subagent put_page sets `ctx.remote=true` and skips auto-link/timeline by default, so extract is the canonical materialization); `recompute_emotional_weight` sees the union of `syncPagesAffected` + `synthesizeWrittenSlugs` incrementally, or all pages when neither anchor is set (full backfill via `gbrain dream --phase recompute_emotional_weight`). `CycleReport.schema_version: "1"` is stable; `totals` is additive (`pages_emotional_weight_recomputed`, `transcripts_processed`, `synth_pages_written`, `patterns_written`). Three callers: `gbrain dream` CLI, `gbrain autopilot` daemon inline path, the Minions `autopilot-cycle` handler. Coordination via `gbrain_cycle_locks` DB table + `~/.gbrain/cycle.lock` file lock with PID-liveness for PGLite. `yieldBetweenPhases` runs between phases; `yieldDuringPhase` is in-phase keepalive (synthesize/patterns renew the cycle-lock TTL during long waits). Engine nullable; lock-skip on read-only phase selections. `CycleOpts.signal?: AbortSignal` propagates the worker's abort signal with `checkAborted()` between every phase. `runPhaseSync` returns `pagesAffected` via `SyncPhaseResult` (threaded to `runPhaseExtract` as the 4th arg) and takes `willRunExtractPhase: boolean` setting `noExtract: phases.includes('extract')` so `gbrain dream --phase sync` doesn't silently lose extraction. `resolveSourceForDir(engine, brainDir)` threads `sourceId` to `performSync()` so sync reads the per-source `sources.last_commit` anchor (not the drift-prone global `config.sync.last_commit`). `CycleOpts.brainDir` is `string | null`; when null (checkout-less postgres/Supabase brain) the 6 filesystem phases (lint/backlinks/sync/synthesize/extract/patterns) skip with `details.reason: 'no_brain_dir'` and the DB-only phases run; `resolveSourceForDir` is null-tolerant. `cycleSourceId = opts.sourceId ?? resolveSourceForDir(engine, brainDir)` is the canonical per-source scope for `extract_facts`/`extract_atoms`/calibration so `gbrain dream --source repo-a` reconciles repo-a's facts even with no checkout (instead of scoping to `'default'` while stamping repo-a fresh). `deriveStatus` counts `edges_resolved`/`edges_ambiguous` as work so an edges-only cycle reports `ok` not `clean`; the `jobs.ts` `autopilot-cycle` + phase-wrapper handlers pass `null` (not `'.'`) when no repo is configured. Pinned by `test/dream-postgres.serial.test.ts` + `test/jobs-autopilot-cycle-braindir.serial.test.ts`. +- `src/core/cycle.ts` — brain maintenance cycle primitive (9 phases). `runCycle(engine: BrainEngine | null, opts: CycleOpts): Promise` composes phases in semantic order: lint → backlinks → sync → synthesize → extract → patterns → recompute_emotional_weight → embed → orphans. `synthesize` runs after sync (cross-references see a fresh brain) and before extract (auto-link materializes its writes); `patterns` runs after extract so it reads a fresh graph (subagent put_page sets `ctx.remote=true` and skips auto-link/timeline by default, so extract is the canonical materialization); `recompute_emotional_weight` sees the union of `syncPagesAffected` + `synthesizeWrittenSlugs` incrementally, or all pages when neither anchor is set (full backfill via `gbrain dream --phase recompute_emotional_weight`). `CycleReport.schema_version: "1"` is stable; `totals` is additive (`pages_emotional_weight_recomputed`, `transcripts_processed`, `synth_pages_written`, `patterns_written`). Three callers: `gbrain dream` CLI, `gbrain autopilot` daemon inline path, the Minions `autopilot-cycle` handler. Coordination via `gbrain_cycle_locks` DB table + `~/.gbrain/cycle.lock` file lock with PID-liveness for PGLite. `yieldBetweenPhases` runs between phases; `yieldDuringPhase` is in-phase keepalive (synthesize/patterns renew the cycle-lock TTL during long waits). Engine nullable; lock-skip on read-only phase selections. `CycleOpts.signal?: AbortSignal` propagates the worker's abort signal with `checkAborted()` between every phase. `runPhaseSync` returns `pagesAffected` via `SyncPhaseResult` (threaded to `runPhaseExtract` as the 4th arg) and takes `willRunExtractPhase: boolean` setting `noExtract: phases.includes('extract')` so `gbrain dream --phase sync` doesn't silently lose extraction. `resolveSourceForDir(engine, brainDir)` threads `sourceId` to `performSync()` so sync reads the per-source `sources.last_commit` anchor (not the drift-prone global `config.sync.last_commit`). `CycleOpts.brainDir` is `string | null`; when null (checkout-less postgres/Supabase brain) the 6 filesystem phases (lint/backlinks/sync/synthesize/extract/patterns) skip with `details.reason: 'no_brain_dir'` and the DB-only phases run; `resolveSourceForDir` is null-tolerant. `cycleSourceId = opts.sourceId ?? resolveSourceForDir(engine, brainDir)` is the canonical per-source scope for `extract_facts`/`extract_atoms`/calibration so `gbrain dream --source repo-a` reconciles repo-a's facts even with no checkout (instead of scoping to `'default'` while stamping repo-a fresh). `deriveStatus` counts `edges_resolved`/`edges_ambiguous` as work so an edges-only cycle reports `ok` not `clean`; the `jobs.ts` `autopilot-cycle` + phase-wrapper handlers pass `null` (not `'.'`) when no repo is configured. The cycle is SPLIT for autopilot fan-out (#2194/#2227): `PHASE_SCOPE` partitions `ALL_PHASES` into `GLOBAL_PHASES` (brain-wide: embed/orphans/purge/resolve_symbol_edges/grade_takes/calibration_profile/synthesize_concepts/skillopt) and `NON_GLOBAL_PHASES` (source + mixed). Per-source `autopilot-cycle` jobs run only `NON_GLOBAL_PHASES` and stamp `last_source_cycle_at`; the single `autopilot-global-maintenance` job runs `GLOBAL_PHASES` (no `sourceId`) and stamps the brain-level `autopilot.last_global_at` config key (`LAST_GLOBAL_AT_KEY`). `last_full_cycle_at` is still written alongside `last_source_cycle_at` on a per-source success for doctor/legacy readers (no longer a gate for the brain-wide phases). Pinned by `test/dream-postgres.serial.test.ts` + `test/jobs-autopilot-cycle-braindir.serial.test.ts` + `test/autopilot-global-maintenance.test.ts`. - `src/core/cycle/synthesize.ts` — Synthesize phase: conversation-transcript-to-brain pipeline. Reads `dream.synthesize.session_corpus_dir`, runs a cheap Haiku verdict (cached in `dream_verdicts`), then fans out one Sonnet subagent per worth-processing transcript with `allowed_slug_prefixes` (sourced from `skills/_brain-filing-rules.json` `dream_synthesize_paths.globs`). Orchestrator collects slugs from `subagent_tool_executions` (NOT `pages.updated_at`) and reverse-renders DB → markdown via `serializeMarkdown`. Cooldown via `dream.synthesize.last_completion_ts`, written ONLY on success. Idempotency key `dream:synth::`. `--dry-run` runs Haiku, skips Sonnet. Subagent never gets fs-write access. `renderPageToMarkdown` (exported) stamps `dream_generated: true` + `dream_cycle_date` into every reverse-write's frontmatter; `writeSummaryPage` does the same on the summary index — this marker is the explicit identity surface `isDreamOutput` checks in `transcript-discovery.ts`. `judgeSignificance` and `JudgeClient` are exported; `judgeSignificance` takes a `verdictModel` param loaded from `dream.synthesize.verdict_model` via `loadSynthConfig`. `splitTranscriptByBudget(content, contentHash, maxChars)` splits oversized transcripts at paragraph boundaries (`## Topic:` → `---` → `\n` ladder) using a deterministic offset seeded from the first 32 bits of `contentHash` so retries chunk identically; per-chunk char budget = `MODEL_CONTEXT_TOKENS[resolvedModel] × 0.9 × 3.5 chars/token` (non-Anthropic ids fall back to a 180K-token safe default + once-per-process stderr warn); operator overrides `dream.synthesize.max_prompt_tokens` (floor 100K, wins) and `dream.synthesize.max_chunks_per_transcript` (default 24). Per-chunk idempotency keys `dream:synth:::cof`; single-chunk transcripts preserve the legacy `dream:synth::` key byte-for-byte so existing brains skip with `already_synthesized_legacy_single_chunk` instead of re-spending Sonnet. `collectChildPutPageSlugs` raw-fetches every (job_id, slug) pair (not `SELECT DISTINCT`) and rewrites bare-hash6 slugs to `-c` for chunked children (orchestrator-side, zero Sonnet trust). Cap-hit skips don't write to `dream_verdicts` so raising the cap on next run re-attempts cleanly. Bounds INITIAL prompt size only; tool-loop turn-N accumulation is caught by terminal-error classification in `subagent.ts`. Verdict routing is gateway-routed: `makeJudgeClient(verdictModel)` (exported, replacing `makeHaikuClient()`) mirrors `tryBuildGatewayClient` in `src/core/think/index.ts` — a construction-time provider/key probe returns `null` on a clear miss (unknown provider id via `resolveRecipe` AIConfigError, or Anthropic provider with no key via `hasAnthropicKey()`). The verdict loop wraps `judgeSignificance` in try/catch for `AIConfigError` so mid-run provider failures surface as per-transcript `worth=false, reasons=['gateway error: ...']` instead of crashing the phase. Canonical config key `models.dream.synthesize_verdict` (per `PER_TASK_KEYS` in `src/core/model-config.ts`); `JudgeClient` signature preserved verbatim for test-seam stability; CI guard `scripts/check-gateway-routed-no-direct-anthropic.sh` prevents reintroducing `new Anthropic()` here or in `think/index.ts`. At the queue.add boundary (lines 395-404) a conditional `anthropic:` prefix is applied ONLY when the resolved model has no colon AND starts with `claude-` (because `resolveModel` returns bare ids from `TIER_DEFAULTS`/`DEFAULT_ALIASES` and the subagent validator requires `provider:model` form) — avoids changing the shared constants which would ripple across every `resolveModel` caller. Pinned by `test/cycle/synthesize-gateway-adapter.test.ts`, `test/e2e/dream-synthesize-pglite.test.ts` (gateway-adapter mid-run AIConfigError catch), `test/cycle/regression-pr-wave-r1-r2-r4.test.ts`. - `scripts/check-gateway-routed-no-direct-anthropic.sh` — CI guard that fails the build if `src/core/cycle/synthesize.ts` or `src/core/think/index.ts` reintroduces a runtime `new Anthropic()` constructor call or a value-shaped `import Anthropic from '@anthropic-ai/sdk'` import. Type-only imports (`import type Anthropic from '@anthropic-ai/sdk'`) stay allowed for adapter types; comment lines (`//` or ` *` prefixes) are excluded so historical JSDoc doesn't false-fire. Mirrors `scripts/check-jsonb-pattern.sh`. Wired into `bun run verify` and `bun run check:all`. Extend `GUARDED_FILES` when migrating another file off direct SDK construction. - `src/core/cycle/patterns.ts` — Patterns phase: cross-session theme detection over reflections within `dream.patterns.lookback_days` (default 30). Names a pattern only when ≥`dream.patterns.min_evidence` (default 3) reflections support it. Single Sonnet subagent; same allow-list path as synthesize. Runs AFTER `extract` so the graph is fresh. diff --git a/llms-full.txt b/llms-full.txt index 2dcb09aad..bddfbc198 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -539,7 +539,7 @@ Progress banks into the append-only `op_checkpoint_paths` table (one row per dra path, written via the direct session pool so it survives `EMAXCONNSESSION`); a killed run resumes from the checkpoint and `last_commit` only advances on true completion. The per-source lock heartbeats through the direct pool and refuses to steal a live, -recently-refreshed holder. Five env knobs tune it (all env-only, incident-time escape +recently-refreshed holder. Six env knobs tune it (all env-only, incident-time escape hatches — no config-dashboard surface by design): | Env var | Default | What it does | @@ -549,6 +549,7 @@ hatches — no config-dashboard surface by design): | `GBRAIN_SYNC_MAX_CHECKPOINT_FAILURES` | 3 | Consecutive failed flushes (each already retried ~12s) before the run aborts with `reason: 'checkpoint_unavailable'` instead of importing work it can never bank. | | `GBRAIN_SYNC_YIELD_EVERY` | 64 | Yield the event loop (`setTimeout(0)`, NOT `setImmediate` — Bun starves the timers phase under a tight setImmediate loop) every N files so the lock-refresh `setInterval` heartbeat fires mid-import. | | `GBRAIN_LOCK_STEAL_GRACE_SECONDS` | derived (~600 at 30min TTL) | A holder that refreshed within this window is NOT stolen even if its TTL lapsed (starved-but-alive). Dead holders stop refreshing, age past the grace, and become stealable; TTL stays the backstop. | +| `GBRAIN_SYNC_STALL_ABORT_SECONDS` | 900 | Progress-aware stall watchdog (#1950): if the import drain makes no forward progress (keyed on file-import progress, NOT the lock heartbeat) for N seconds, abort the run and release the per-source lock so the next `gbrain sync` resumes from the checkpoint. Reports `reason: 'stall_timeout'`. Observed BETWEEN files; a hang inside one file's import isn't interrupted until it returns (the wall-clock hard deadline is that backstop). 0 disables. | ## Pace Mode (DB-contention-aware backfill pacing) diff --git a/package.json b/package.json index 82b43c3bc..73eaf18a9 100644 --- a/package.json +++ b/package.json @@ -143,5 +143,5 @@ "bun": ">=1.3.10" }, "license": "MIT", - "version": "0.42.51.0" + "version": "0.42.52.0" } diff --git a/src/commands/agent.ts b/src/commands/agent.ts index 4a5ae5500..2b4c05d4a 100644 --- a/src/commands/agent.ts +++ b/src/commands/agent.ts @@ -74,8 +74,12 @@ SUBMITTING --follow Tail status until terminal (default on TTY) --detach Submit + print job id, exit immediately - Flags after \`run\` up to the first unrecognized token are parsed; the - remainder is the prompt. Use \`--\` to explicitly terminate flag parsing. + Flags before the prompt are parsed normally. The no-value switches + --detach, --follow and --no-follow are ALSO recognized when they trail + the prompt, so \`gbrain agent run "do X" --detach\` detaches. Any other + --word is treated as prompt text (no error). Use \`--\` to end flag + parsing and pass the rest verbatim: + gbrain agent run -- "literally --detach this, with --flags" VIEWING gbrain agent logs @@ -102,31 +106,86 @@ interface RunFlags { detach: boolean; } +/** No-value switches that may also trail the prompt and get hoisted out (#1738). */ +const BOOLEAN_TAIL_FLAGS = new Set(['--follow', '--no-follow', '--detach']); + +function applyBooleanFlag(flags: RunFlags, a: string): void { + if (a === '--follow') flags.follow = true; + else if (a === '--no-follow') flags.follow = false; + else { flags.detach = true; flags.follow = false; } // --detach +} + +/** Read the value for a value-flag, rejecting a missing or flag-shaped value. */ +function requireFlagValue(args: string[], i: number, flag: string): string { + const v = args[i]; + if (v === undefined || v.startsWith('--')) { + throw new Error(`gbrain agent run: ${flag} requires a value. Run \`gbrain agent run --help\`.`); + } + return v; +} + +function parseIntFlagValue(v: string, flag: string): number { + const n = parseInt(v, 10); + if (Number.isNaN(n)) { + throw new Error(`gbrain agent run: ${flag} expects a number, got "${v}".`); + } + return n; +} + +/** + * Parse `agent run` args into flags + prompt (#1738). + * + * args ──► [ leading flag zone ] [ ── ? ] [ prompt … (trailing booleans) ] + * + * Leading zone: known flags (value + boolean) are consumed left-to-right until + * the first positional token, an UNKNOWN --flag, or an explicit `--`. An + * unknown --flag is NOT an error — it begins the freeform prompt, so + * `agent run "--note: do X"` works without `--`. Value-flags missing their + * value throw a usage error instead of silently capturing `undefined`/`NaN`. + * + * Prompt zone: a trailing run of the no-value switches (--detach/--follow/ + * --no-follow) is hoisted out so `agent run "do X" --detach` detaches. Only + * trailing switches are hoisted; a `--word` elsewhere in the prompt stays + * verbatim. After an explicit `--`, nothing is hoisted. + */ function parseRunFlags(args: string[]): { flags: RunFlags; rest: string[] } { const flags: RunFlags = { follow: process.stdout.isTTY === true, detach: false, }; let i = 0; - while (i < args.length) { - const a = args[i]; - if (a === '--') { i++; break; } - if (!isKnownFlag(a!)) break; + let escaped = false; + for (; i < args.length; i++) { + const a = args[i]!; + if (a === '--') { i++; escaped = true; break; } + if (!a.startsWith('--')) break; + let known = true; switch (a) { - case '--subagent-def': flags.subagentDef = args[++i]; i++; break; - case '--model': flags.model = args[++i]; i++; break; - case '--max-turns': flags.maxTurns = parseInt(args[++i] ?? '', 10); i++; break; - case '--tools': flags.tools = (args[++i] ?? '').split(',').map(s => s.trim()).filter(Boolean); i++; break; - case '--timeout-ms': flags.timeoutMs = parseInt(args[++i] ?? '', 10); i++; break; - case '--fanout-manifest': flags.fanoutManifest = args[++i]; i++; break; - case '--follow': flags.follow = true; i++; break; - case '--no-follow': flags.follow = false; i++; break; - case '--detach': flags.detach = true; flags.follow = false; i++; break; - default: - throw new Error(`unknown flag: ${a}. Run \`gbrain agent run --help\` for usage.`); + case '--subagent-def': flags.subagentDef = requireFlagValue(args, ++i, a); break; + case '--model': flags.model = requireFlagValue(args, ++i, a); break; + case '--max-turns': flags.maxTurns = parseIntFlagValue(requireFlagValue(args, ++i, a), a); break; + case '--tools': flags.tools = requireFlagValue(args, ++i, a).split(',').map(s => s.trim()).filter(Boolean); break; + case '--timeout-ms': flags.timeoutMs = parseIntFlagValue(requireFlagValue(args, ++i, a), a); break; + case '--fanout-manifest': flags.fanoutManifest = requireFlagValue(args, ++i, a); break; + case '--follow': flags.follow = true; break; + case '--no-follow': flags.follow = false; break; + case '--detach': flags.detach = true; flags.follow = false; break; + default: known = false; break; + } + if (!known) break; // unknown --flag → first token of the (freeform) prompt + } + const rest = args.slice(i); + // An explicit `--` terminates flag parsing wherever it appears — leading + // zone (escaped) OR after a positional (the leading loop breaks before it, + // so `escaped` stays false). Honor both: when the prompt carries a literal + // `--`, hoist nothing, so `agent run note -- --detach` keeps `--detach` + // verbatim instead of silently flipping detach mode. + if (!escaped && !rest.includes('--')) { + while (rest.length > 0 && BOOLEAN_TAIL_FLAGS.has(rest[rest.length - 1]!)) { + applyBooleanFlag(flags, rest.pop()!); } } - return { flags, rest: args.slice(i) }; + return { flags, rest }; } export async function runAgentRun(engine: BrainEngine, args: string[]): Promise { diff --git a/src/commands/autopilot-fanout.ts b/src/commands/autopilot-fanout.ts index 364ac7d61..9c558ad57 100644 --- a/src/commands/autopilot-fanout.ts +++ b/src/commands/autopilot-fanout.ts @@ -32,9 +32,26 @@ import type { BrainEngine, SourceRow } from '../core/engine.ts'; import type { MinionQueue } from '../core/minions/queue.ts'; +import { NON_GLOBAL_PHASES, GLOBAL_PHASES, LAST_GLOBAL_AT_KEY } from '../core/cycle.ts'; const FULL_CYCLE_FLOOR_MIN = 60; +// #2194 fix #2: failure cooldown. A source whose autopilot-cycle keeps +// failing/timing-out re-dispatches every tick today (only SUCCESS gates +// dispatch), so the same handful of sources fail and re-fan-out forever — the +// self-perpetuating dead-job storm. Back a failed source off with bounded +// exponential cooldown so a chronically-slow source can't re-dispatch every +// tick. Disabled with autopilot.failure_cooldown_min=0. +const FAILURE_COOLDOWN_BASE_MIN = 10; +const FAILURE_COOLDOWN_CAP_MIN = 120; +const FAILURE_COOLDOWN_EXP_CAP = 4; // 2^4 = 16× base before the cap clamps + +/** Recent-failure record for one source (from minion_jobs dead/failed rows). */ +export interface SourceFailure { count: number; lastFailedAt: Date; } + +/** Resolved cooldown knobs. baseMin <= 0 means the cooldown is disabled. */ +export interface CooldownOpts { baseMin: number; capMin: number; } + export interface FanoutOpts { repoPath: string; slot: string; @@ -58,6 +75,8 @@ export interface FanoutResult { skipped_fresh: string[]; /** Source ids beyond the fanoutMax cap (will retry next tick). */ skipped_cap: string[]; + /** Source ids skipped because they're in failure cooldown (#2194 fix #2). */ + skipped_cooldown: string[]; /** True when this tick fell back to the legacy single-job path * (no sources rows / engine empty). */ legacy_fallback: boolean; @@ -83,6 +102,62 @@ export async function resolveFanoutMax(engine: BrainEngine): Promise { return engine.kind === 'pglite' ? 1 : 4; } +/** + * Read the worker concurrency the supervisor most recently STARTED with, from + * its `started` audit event (the lowest-coupling source — no extra lock-row + * column). Filesystem read; returns null when no supervisor has ever started + * (or the event lacks concurrency). Filtered by queue so a `shell`-queue + * supervisor's concurrency doesn't leak into the `default`-queue decision. + * + * ADVISORY use only (doctor warning). Behavior-changing callers (the fanout + * clamp) must additionally gate on a LIVE supervisor — see + * resolveEffectiveFanoutMax — because a stale `started` row can otherwise + * shrink fan-out for a supervisor that isn't running that config (codex #9/D5). + */ +export async function readSupervisorConcurrency(queue = 'default'): Promise { + try { + const { readSupervisorEvents } = await import('../core/minions/handlers/supervisor-audit.ts'); + const events = readSupervisorEvents({ sinceMs: 24 * 60 * 60 * 1000 }); + const started = events + .filter((e) => e.event === 'started' && (e.queue === undefined || e.queue === queue)) + .pop(); + const c = started?.concurrency; + return typeof c === 'number' && Number.isFinite(c) ? c : null; + } catch { + return null; + } +} + +/** + * Resolve fanoutMax CLAMPED to the worker's effective concurrency (#2194 fix #1). + * + * Fanning out more cycles than the worker can run guarantees waiters that then + * race the stalled-sweeper. Clamp to `max(1, concurrency - 1)` — reserving ≥1 + * slot for targeted sync/embed jobs that share the `default` queue. + * + * codex #9 / D5: the clamp is BEHAVIOR-changing, so it trusts only a + * proven-alive supervisor (live DB-lock holder, `ttl_expires_at`-gated). With + * no live holder the concurrency is UNKNOWN and we fall back to the unclamped + * default (4 pg / 1 pglite) — the safe direction (never starve on stale data). + * Operators can disable the clamp via `autopilot.fanout_clamp_to_concurrency`. + */ +export async function resolveEffectiveFanoutMax(engine: BrainEngine, queue = 'default'): Promise { + const base = await resolveFanoutMax(engine); + const clampCfg = await engine.getConfig('autopilot.fanout_clamp_to_concurrency'); + if (clampCfg === 'false' || clampCfg === '0') return base; // operator opt-out + try { + const { inspectLock, isLockHolderLive } = await import('../core/db-lock.ts'); + const { supervisorLockId, SUPERVISOR_LOCK_TTL_MIN } = await import('../core/minions/supervisor.ts'); + const snap = await inspectLock(engine, supervisorLockId(queue)); + if (!snap || !isLockHolderLive(snap, SUPERVISOR_LOCK_TTL_MIN)) return base; // no live holder → unknown → no clamp + const concurrency = await readSupervisorConcurrency(queue); + if (concurrency === null) return base; + return Math.max(1, Math.min(base, concurrency - 1)); + } catch { + return base; + } +} + /** * Read `last_full_cycle_at` ISO string from a source's config JSONB. * Returns null when missing or unparseable. Pure function over the row @@ -111,6 +186,133 @@ export function isSourceStale(src: SourceRow, now = Date.now(), floorMin = FULL_ return ageMin >= floorMin; } +/** + * Most recent SUCCESSFUL cycle for a source. Prefers `last_source_cycle_at` + * (per-source phases, written by the split cycle) and falls back to the legacy + * `last_full_cycle_at`, so this works before AND after the cycle split. + */ +export function readLastSuccessAt(src: SourceRow): Date | null { + const c = src.config ?? {}; + const raw = (typeof c.last_source_cycle_at === 'string' && c.last_source_cycle_at) + || (typeof c.last_full_cycle_at === 'string' && c.last_full_cycle_at) + || null; + if (!raw) return null; + const d = new Date(raw); + return Number.isFinite(d.getTime()) ? d : null; +} + +/** Bounded exponential cooldown window (minutes) for a given failure count. */ +export function cooldownMinForCount(count: number, opts: CooldownOpts): number { + if (count <= 0 || opts.baseMin <= 0) return 0; + const mult = Math.pow(2, Math.min(count - 1, FAILURE_COOLDOWN_EXP_CAP)); + return Math.min(opts.baseMin * mult, opts.capMin); +} + +/** + * Is a source currently in failure cooldown? Pure — drives both the dispatch + * gate and the claim-time guard. A SUCCESS at-or-after the most recent failure + * clears the cooldown (codex #7: operator repair / manual cycle re-eligibility), + * so a recovered source is never suppressed by stale failure history. + */ +export function isInFailureCooldown( + failure: SourceFailure | undefined, + lastSuccessAt: Date | null, + now: number, + opts: CooldownOpts, +): boolean { + if (opts.baseMin <= 0) return false; // disabled + if (!failure || failure.count <= 0) return false; + if (lastSuccessAt && lastSuccessAt.getTime() >= failure.lastFailedAt.getTime()) return false; + const cooldownMs = cooldownMinForCount(failure.count, opts) * 60_000; + return (now - failure.lastFailedAt.getTime()) < cooldownMs; +} + +/** + * Resolve cooldown knobs from config. `autopilot.failure_cooldown_min` overrides + * the base (0 = disable entirely — exactly today's behavior); + * `autopilot.failure_cooldown_cap_min` overrides the ceiling. + */ +export async function resolveFailureCooldownOpts(engine: BrainEngine): Promise { + let baseMin = FAILURE_COOLDOWN_BASE_MIN; + let capMin = FAILURE_COOLDOWN_CAP_MIN; + const baseCfg = await engine.getConfig('autopilot.failure_cooldown_min'); + if (baseCfg !== null && baseCfg !== undefined && baseCfg !== '') { + const n = parseInt(baseCfg, 10); + if (Number.isFinite(n) && n >= 0) baseMin = n; + } + const capCfg = await engine.getConfig('autopilot.failure_cooldown_cap_min'); + if (capCfg) { + const n = parseInt(capCfg, 10); + if (Number.isFinite(n) && n >= 1) capMin = n; + } + return { baseMin, capMin }; +} + +/** + * Read recent dead/failed autopilot-cycle jobs grouped by source. Read-at- + * dispatch (NOT a write hook) because timeouts/RSS-kills/stalls dead-letter via + * SQL in queue.ts and never run handler code — a write-only cooldown would miss + * the exact failures that drive the storm. Engine-parity-safe via executeRaw + * (one query, both engines); cutoff is precomputed in JS to avoid INTERVAL + * portability concerns. codex #6: rows with a null source_id are excluded. + */ +export async function readRecentSourceFailures( + engine: BrainEngine, + opts: { sinceMin?: number; sourceId?: string } = {}, +): Promise> { + const sinceMin = opts.sinceMin ?? FAILURE_COOLDOWN_CAP_MIN; + const cutoff = new Date(Date.now() - sinceMin * 60_000).toISOString(); + const map = new Map(); + try { + const params: unknown[] = [cutoff]; + let sql = + `SELECT data->>'source_id' AS source_id, + count(*)::int AS fail_count, + max(finished_at) AS last_failed_at + FROM minion_jobs + WHERE name = 'autopilot-cycle' + AND status IN ('dead','failed') + AND data->>'source_id' IS NOT NULL + AND finished_at IS NOT NULL + AND finished_at > $1`; + if (opts.sourceId) { params.push(opts.sourceId); sql += ` AND data->>'source_id' = $${params.length}`; } + sql += ` GROUP BY data->>'source_id'`; + const rows = await engine.executeRaw<{ source_id: string | null; fail_count: number; last_failed_at: string | Date }>(sql, params); + for (const r of rows) { + if (!r.source_id) continue; // codex #6 null-source guard (defensive) + const last = r.last_failed_at instanceof Date ? r.last_failed_at : new Date(r.last_failed_at); + if (!Number.isFinite(last.getTime())) continue; + map.set(r.source_id, { count: Number(r.fail_count) || 0, lastFailedAt: last }); + } + } catch { + // Pre-migration / transient DB error → no cooldown data (fail open: dispatch). + } + return map; +} + +/** + * Claim-time cooldown guard (codex #5 / D4): a job already queued or retrying + * (max_attempts:2) can reach the worker after the dispatch gate decided. The + * handler calls this immediately before runCycle; an in-cooldown claim becomes + * a no-op skip (NOT a failure — it must not re-arm the cooldown). Shares the + * exact cooldown math with the dispatch gate (DRY). + */ +export async function isSourceInCooldown(engine: BrainEngine, sourceId: string, now = Date.now()): Promise { + const opts = await resolveFailureCooldownOpts(engine); + if (opts.baseMin <= 0) return false; + const failures = await readRecentSourceFailures(engine, { sinceMin: opts.capMin, sourceId }); + const failure = failures.get(sourceId); + if (!failure) return false; + let lastSuccessAt: Date | null = null; + try { + const rows = await engine.executeRaw<{ config: Record | null }>( + `SELECT config FROM sources WHERE id = $1`, [sourceId], + ); + if (rows[0]) lastSuccessAt = readLastSuccessAt({ config: rows[0].config ?? {} } as SourceRow); + } catch { /* treat as no success */ } + return isInFailureCooldown(failure, lastSuccessAt, now, opts); +} + /** * Decide which sources to dispatch this tick. Pure function so tests can * exercise the freshness gate + cap math without an engine. @@ -126,11 +328,21 @@ export function selectSourcesForDispatch( fanoutMax: number, now = Date.now(), floorMin = FULL_CYCLE_FLOOR_MIN, -): { dispatch: SourceRow[]; skippedFresh: SourceRow[]; skippedCap: SourceRow[] } { + recentFailures: Map = new Map(), + cooldownOpts: CooldownOpts = { baseMin: FAILURE_COOLDOWN_BASE_MIN, capMin: FAILURE_COOLDOWN_CAP_MIN }, +): { dispatch: SourceRow[]; skippedFresh: SourceRow[]; skippedCap: SourceRow[]; skippedCooldown: SourceRow[] } { const stale: SourceRow[] = []; const fresh: SourceRow[] = []; + const cooldown: SourceRow[] = []; for (const s of sources) { - (isSourceStale(s, now, floorMin) ? stale : fresh).push(s); + if (!isSourceStale(s, now, floorMin)) { fresh.push(s); continue; } + // #2194 fix #2: a stale source that recently failed is held in cooldown so + // it can't re-dispatch every tick (the storm). Success clears it. + if (isInFailureCooldown(recentFailures.get(s.id), readLastSuccessAt(s), now, cooldownOpts)) { + cooldown.push(s); + continue; + } + stale.push(s); } // Oldest-first ordering: NULL last_full_cycle_at sorts before any timestamp. stale.sort((a, b) => { @@ -141,7 +353,7 @@ export function selectSourcesForDispatch( }); const dispatch = stale.slice(0, fanoutMax); const skippedCap = stale.slice(fanoutMax); - return { dispatch, skippedFresh: fresh, skippedCap }; + return { dispatch, skippedFresh: fresh, skippedCap, skippedCooldown: cooldown }; } /** @@ -193,10 +405,27 @@ export async function dispatchPerSource( } else { log(`[dispatch] job #${job.id} autopilot-cycle (legacy single-source)`); } - return { dispatched: [], skipped_fresh: [], skipped_cap: [], legacy_fallback: true }; + return { dispatched: [], skipped_fresh: [], skipped_cap: [], skipped_cooldown: [], legacy_fallback: true }; } - const { dispatch, skippedFresh, skippedCap } = selectSourcesForDispatch(sources, opts.fanoutMax); + // #2194 fix #2: load recent per-source failures + cooldown knobs so a + // chronically-failing source is backed off instead of re-dispatched every + // tick. Fail-open: cooldown is an optimization, not a correctness gate — if + // config/job-history reads fail (or the engine lacks them), dispatch proceeds + // with no cooldown rather than blocking. + let cooldownOpts: CooldownOpts = { baseMin: 0, capMin: FAILURE_COOLDOWN_CAP_MIN }; + let recentFailures = new Map(); + try { + cooldownOpts = await resolveFailureCooldownOpts(engine); + if (cooldownOpts.baseMin > 0) { + recentFailures = await readRecentSourceFailures(engine, { sinceMin: cooldownOpts.capMin }); + } + } catch { + cooldownOpts = { baseMin: 0, capMin: FAILURE_COOLDOWN_CAP_MIN }; + } + + const { dispatch, skippedFresh, skippedCap, skippedCooldown } = + selectSourcesForDispatch(sources, opts.fanoutMax, Date.now(), FULL_CYCLE_FLOOR_MIN, recentFailures, cooldownOpts); const dispatched: string[] = []; for (const src of dispatch) { @@ -208,6 +437,11 @@ export async function dispatchPerSource( repoPath: opts.repoPath, source_id: src.id, pull: !!remoteUrl, + // #2194 fix #3 (cycle split): per-source cycles run ONLY source-scoped + // (+ mixed) phases. The brain-wide global phases (embed, orphans, + // purge, …) run once in autopilot-global-maintenance, not N times + // concurrently here — the fix for the 4→10GB RSS blowout. + phases: NON_GLOBAL_PHASES, }, { queue: 'default', @@ -261,10 +495,77 @@ export async function dispatchPerSource( })); } + if (skippedCooldown.length > 0 && opts.jsonMode) { + emit(JSON.stringify({ + event: 'fanout_cooldown_skipped', + sources: skippedCooldown.map(s => s.id), + })); + } + return { dispatched, skipped_fresh: skippedFresh.map(s => s.id), skipped_cap: skippedCap.map(s => s.id), + skipped_cooldown: skippedCooldown.map(s => s.id), legacy_fallback: false, }; } + +const GLOBAL_FLOOR_MIN = 60; + +/** Is the brain-wide maintenance overdue? Null/unparseable → overdue. */ +export function isGlobalMaintenanceStale(lastGlobalAtIso: string | null, now = Date.now(), floorMin = GLOBAL_FLOOR_MIN): boolean { + if (!lastGlobalAtIso) return true; + const d = new Date(lastGlobalAtIso); + if (!Number.isFinite(d.getTime())) return true; + return (now - d.getTime()) / 60_000 >= floorMin; +} + +/** + * #2194 fix #3 / #2227 bug #3 — dispatch the single brain-wide maintenance job + * that runs the `global` cycle phases (embed, orphans, purge, …) ONCE per + * window, instead of N per-source cycles each running them concurrently (the + * RSS blowout). Single-flight is structural: one `idempotency_key` + + * `maxWaiting:1`, so a slow run never stacks. Gated on `autopilot.last_global_at` + * (stamped by the handler on success). Postgres-only fan-out concern; on PGLite + * the file lock already serializes, but the job is still correct there. + */ +export async function dispatchGlobalMaintenance( + engine: BrainEngine, + queue: MinionQueue, + opts: { repoPath: string; slot: string; timeoutMs: number; jsonMode: boolean; emit?: (l: string) => void; log?: (l: string) => void }, +): Promise<{ dispatched: boolean; reason: 'stale' | 'fresh' }> { + const emit = opts.emit ?? ((line) => process.stderr.write(line + '\n')); + const log = opts.log ?? ((line) => console.log(line)); + + let floorMin = GLOBAL_FLOOR_MIN; + const floorCfg = await engine.getConfig('autopilot.global_floor_min'); + if (floorCfg) { + const n = parseInt(floorCfg, 10); + if (Number.isFinite(n) && n >= 1) floorMin = n; + } + const lastGlobalAt = await engine.getConfig(LAST_GLOBAL_AT_KEY); + if (!isGlobalMaintenanceStale(lastGlobalAt, Date.now(), floorMin)) { + return { dispatched: false, reason: 'fresh' }; + } + + const job = await queue.add( + 'autopilot-global-maintenance', + { repoPath: opts.repoPath, phases: GLOBAL_PHASES }, + { + queue: 'default', + // Structural single-flight: one global job per slot; maxWaiting:1 coalesces + // any surplus so a slow brain-wide pass never stacks duplicates. + idempotency_key: `autopilot-global:${opts.slot}`, + max_attempts: 2, + timeout_ms: opts.timeoutMs, + maxWaiting: 1, + }, + ); + if (opts.jsonMode) { + emit(JSON.stringify({ event: 'dispatched', job_id: job.id, mode: 'global_maintenance', slot: opts.slot })); + } else { + log(`[dispatch] job #${job.id} autopilot-global-maintenance (brain-wide phases)`); + } + return { dispatched: true, reason: 'stale' }; +} diff --git a/src/commands/autopilot.ts b/src/commands/autopilot.ts index fd232eb6f..927b571e5 100644 --- a/src/commands/autopilot.ts +++ b/src/commands/autopilot.ts @@ -871,8 +871,12 @@ export async function runAutopilot(engine: BrainEngine, args: string[]) { // codex P1-3). Fresh-install brains with no sources rows fall // back to the legacy single autopilot-cycle so existing // behavior is preserved. - const { dispatchPerSource, resolveFanoutMax } = await import('./autopilot-fanout.ts'); - const fanoutMax = await resolveFanoutMax(engine); + const { dispatchPerSource, dispatchGlobalMaintenance, resolveEffectiveFanoutMax } = await import('./autopilot-fanout.ts'); + // #2194 fix #1: clamp fan-out to the worker's effective concurrency + // (reserve ≥1 slot), gated on a LIVE supervisor so a stale audit row + // can't shrink throughput (codex #9/D5). autopilot-cycle jobs run on + // the 'default' queue, so that's the concurrency we compare against. + const fanoutMax = await resolveEffectiveFanoutMax(engine, 'default'); const result = await dispatchPerSource(engine, queue, { repoPath, slot, @@ -880,6 +884,18 @@ export async function runAutopilot(engine: BrainEngine, args: string[]) { fanoutMax, jsonMode, }); + // #2194 fix #3 / #2227 bug #3: dispatch the single brain-wide + // maintenance job (embed/orphans/purge/…) once per window — the per- + // source cycles above no longer run global phases, so this is where + // the brain-wide work happens (single-flight, no RSS blowout). Only on + // the per-source path (legacy single-source still runs everything). + if (!result.legacy_fallback) { + try { + await dispatchGlobalMaintenance(engine, queue, { repoPath, slot, timeoutMs, jsonMode }); + } catch (e) { + if (jsonMode) process.stderr.write(JSON.stringify({ event: 'global_maintenance_dispatch_failed', error: e instanceof Error ? e.message : String(e) }) + '\n'); + } + } if (result.dispatched.length > 0 || result.legacy_fallback) { lastFullCycleAt = Date.now(); } @@ -889,6 +905,7 @@ export async function runAutopilot(engine: BrainEngine, args: string[]) { dispatched: result.dispatched, skipped_fresh: result.skipped_fresh, skipped_cap: result.skipped_cap, + skipped_cooldown: result.skipped_cooldown, legacy_fallback: result.legacy_fallback, fanout_max: fanoutMax, score, @@ -896,7 +913,8 @@ export async function runAutopilot(engine: BrainEngine, args: string[]) { } else if (!result.legacy_fallback) { console.log( `[dispatch] fanout: ${result.dispatched.length} dispatched, ` + - `${result.skipped_fresh.length} fresh, ${result.skipped_cap.length} capped ` + + `${result.skipped_fresh.length} fresh, ${result.skipped_cap.length} capped, ` + + `${result.skipped_cooldown.length} cooldown ` + `(score=${score}, max=${fanoutMax})`, ); } diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index f931b5486..f3b67ad30 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -695,6 +695,9 @@ export async function doctorReportRemote(engine: BrainEngine): Promise { + if (engine.kind !== 'postgres') { + return { name: 'autopilot_fanout_concurrency', status: 'ok', message: 'PGLite — single-writer, fan-out is 1' }; + } + try { + const { resolveFanoutMax, readSupervisorConcurrency } = await import('./autopilot-fanout.ts'); + const concurrency = await readSupervisorConcurrency('default'); + if (concurrency === null) { + return { name: 'autopilot_fanout_concurrency', status: 'ok', message: 'No supervisor observed — skipping fan-out/concurrency check' }; + } + const fanoutMax = await resolveFanoutMax(engine); + const effectiveSlots = Math.max(1, concurrency - 1); + if (fanoutMax > effectiveSlots) { + return { + name: 'autopilot_fanout_concurrency', + status: 'warn', + message: + `autopilot fan-out (${fanoutMax}/tick) exceeds worker concurrency (${concurrency}). ` + + `Surplus cycles queue behind the worker and race the stalled-sweeper. ` + + `Lower fan-out: \`gbrain config set autopilot.fanout_max_per_tick ${effectiveSlots}\`, ` + + `or raise the supervisor's \`--concurrency\` to ${fanoutMax + 1}. ` + + `(The clamp in autopilot does this automatically unless disabled.)`, + details: { fanout_max: fanoutMax, concurrency, effective_slots: effectiveSlots }, + }; + } + return { + name: 'autopilot_fanout_concurrency', + status: 'ok', + message: `fan-out ${fanoutMax}/tick within worker concurrency ${concurrency}`, + }; + } catch (e) { + return { name: 'autopilot_fanout_concurrency', status: 'ok', message: `Skipped (${e instanceof Error ? e.message : String(e)})` }; + } +} + export async function checkBatchRetryHealth(_engine: BrainEngine): Promise { try { // Codex M-10: surface bad env config at doctor time. @@ -4348,7 +4394,22 @@ export async function buildChecks( const pidStatus = readSupervisorPid(DEFAULT_PID_FILE); const supervisorPid = pidStatus.pid; - const running = pidStatus.running; + const pidfileRunning = pidStatus.running; + + // issue #2227 fix #1/#3: DEFAULT_PID_FILE is HOME-derived, so a supervisor + // started under a different $HOME reads as "not running" even when healthy. + // Consult the queue-scoped DB singleton lock (#1849, HOME-independent) before + // warning. PID-reuse-safe (isLockHolderLive keys on lock freshness). + let detectedViaDbLock = false; + if (!pidfileRunning && engine) { + try { + const { inspectLock, isLockHolderLive } = await import('../core/db-lock.ts'); + const { supervisorLockId, SUPERVISOR_LOCK_TTL_MIN } = await import('../core/minions/supervisor.ts'); + const snap = await inspectLock(engine, supervisorLockId('default')); + if (snap && isLockHolderLive(snap, SUPERVISOR_LOCK_TTL_MIN)) detectedViaDbLock = true; + } catch { /* pre-migration / transient: pidfile-only */ } + } + const running = pidfileRunning || detectedViaDbLock; const events = readSupervisorEvents({ sinceMs: 24 * 60 * 60 * 1000 }); const lastStart = events.filter(e => e.event === 'started').pop()?.ts ?? null; @@ -4396,7 +4457,7 @@ export async function buildChecks( checks.push({ name: 'supervisor', status: 'ok', - message: `running=true pid=${supervisorPid} last_start=${lastStart ?? 'unknown'} crashes_24h=${crashes24h} clean_exits_24h=${summary.clean_exits}`, + message: `running=true${detectedViaDbLock ? ' (detected via DB lock; pidfile not at the HOME-derived path)' : ` pid=${supervisorPid}`} last_start=${lastStart ?? 'unknown'} crashes_24h=${crashes24h} clean_exits_24h=${summary.clean_exits}`, }); } } @@ -7171,6 +7232,9 @@ export async function buildChecks( // waiting, zero live-lock active, stale completions) as a health error. progress.heartbeat('wedged_queue'); checks.push(await computeWedgedQueueCheck(engine)); + // #2194 fix #5 — autopilot fan-out vs worker concurrency mismatch. + progress.heartbeat('autopilot_fanout_concurrency'); + checks.push(await computeAutopilotFanoutConcurrencyCheck(engine)); // v0.40.4 graph_signals_coverage — global inbound-link density when // graph_signals is enabled in the active mode bundle. progress.heartbeat('graph_signals_coverage'); diff --git a/src/commands/jobs.ts b/src/commands/jobs.ts index 029b0c167..ac5de27ca 100644 --- a/src/commands/jobs.ts +++ b/src/commands/jobs.ts @@ -1020,10 +1020,38 @@ HANDLER TYPES (built in) const pidStatus = readSupervisorPid(pidFile); const supervisorPid = pidStatus.pid; - const running = pidStatus.running; + const pidfileRunning = pidStatus.running; const events = readSupervisorEvents({ sinceMs: 24 * 60 * 60 * 1000 }); const lastStart = events.filter(e => e.event === 'started').pop()?.ts ?? null; + + // issue #2227 fix #1/#3: the pidfile is HOME-derived, so a supervisor + // started under a different $HOME (keeper=/root vs ops=/data) reads as + // "not running" here even when it is healthy — the false signal that + // makes an operator spawn a duplicate. Fall back to the queue-scoped DB + // singleton lock (#1849), the HOME-independent authority. PID-reuse-safe: + // isLockHolderLive keys on lock freshness, never process.kill. + const supQueue = parseFlag(args, '--queue') ?? 'default'; + let detectedViaDbLock = false; + let dbLockHolder: { holder_pid: number; holder_host: string } | null = null; + if (!pidfileRunning) { + try { + const { inspectLock, isLockHolderLive } = await import('../core/db-lock.ts'); + const { supervisorLockId, SUPERVISOR_LOCK_TTL_MIN } = await import('../core/minions/supervisor.ts'); + const snap = await inspectLock(engine, supervisorLockId(supQueue)); + if (snap && isLockHolderLive(snap, SUPERVISOR_LOCK_TTL_MIN)) { + detectedViaDbLock = true; + dbLockHolder = { holder_pid: snap.holder_pid, holder_host: snap.holder_host }; + } + } catch { + // Pre-migration brains / transient DB errors: fall back to pidfile-only. + } + } + const running = pidfileRunning || detectedViaDbLock; + // Surface the supervisor's recorded config from the latest `started` + // event (concurrency + effective --max-rss) so split-$HOME deployments + // see what the live-but-pidfile-invisible supervisor is running. + const startedEvt = events.filter(e => e.event === 'started').pop() ?? null; // Shared classifier — same code path runs in `gbrain doctor` so the // two surfaces cannot drift on what counts as a crash. Supersedes // v0.35.4.0's binary `classifyWorkerExit({code})` on this surface; @@ -1038,15 +1066,20 @@ HANDLER TYPES (built in) nice_requested: w.nice_requested, nice: w.nice_now, })); - const supervisorNice = running && supervisorPid !== null + const supervisorNice = pidfileRunning && supervisorPid !== null ? getEffectiveNiceness(supervisorPid) : null; const status = { running, - supervisor_pid: supervisorPid, + detected_via: detectedViaDbLock ? 'db_lock' : (pidfileRunning ? 'pidfile' : null), + supervisor_pid: supervisorPid ?? dbLockHolder?.holder_pid ?? null, + db_lock_holder: dbLockHolder, pid_file: pidFile, + queue: supQueue, last_start: lastStart, + concurrency: typeof startedEvt?.concurrency === 'number' ? startedEvt.concurrency : null, + max_rss_mb: typeof startedEvt?.max_rss_mb === 'number' ? startedEvt.max_rss_mb : null, crashes_24h: summary.total, clean_exits_24h: summary.clean_exits, crashes_by_cause: summary.by_cause, @@ -1058,9 +1091,11 @@ HANDLER TYPES (built in) if (jsonMode) { console.log(JSON.stringify(status, null, 2)); } else { - console.log(`Supervisor: ${running ? 'running' : 'not running'}`); - if (supervisorPid) console.log(` PID: ${supervisorPid}`); + const via = detectedViaDbLock ? ' (detected via DB lock; pidfile not found at the configured path)' : ''; + console.log(`Supervisor: ${running ? 'running' : 'not running'}${via}`); + if (status.supervisor_pid) console.log(` PID: ${status.supervisor_pid}${detectedViaDbLock ? ` @ ${dbLockHolder?.holder_host}` : ''}`); console.log(` PID file: ${pidFile}`); + if (detectedViaDbLock && status.concurrency !== null) console.log(` Concurrency: ${status.concurrency}${status.max_rss_mb !== null ? ` (max-rss ${status.max_rss_mb}MB)` : ''}`); if (lastStart) console.log(` Last start: ${lastStart}`); console.log(` Crashes (24h): ${summary.total} (runtime=${summary.by_cause.runtime_error} oom=${summary.by_cause.oom_or_external_kill} unknown=${summary.by_cause.unknown} legacy=${summary.by_cause.legacy})`); console.log(` Clean exits (24h): ${summary.clean_exits}`); @@ -1607,6 +1642,13 @@ export async function registerBuiltinHandlers( // archived between fan-out and worker claim, skip cleanly. const rawSourceId = job.data.source_id; let sourceId: string | undefined; + // issue #2227/#2194 (TODOS:634, codex #8): a per-source cycle must run its + // FILESYSTEM phases (sync/lint/extract) against the SOURCE's own checkout, + // not the global brain's. Pre-fix it inherited `repoPath` (the default + // checkout) while writing DB freshness for `source_id` — mixed scope that + // made cooldown/freshness attribute to the wrong source. We resolve the + // source's `local_path` here and use it as the cycle's brainDir below. + let sourceLocalPath: string | null = null; if (rawSourceId !== undefined && rawSourceId !== null) { if (typeof rawSourceId !== 'string') { throw new Error(`autopilot-cycle: invalid source_id (not a string): ${JSON.stringify(rawSourceId)}`); @@ -1620,9 +1662,10 @@ export async function registerBuiltinHandlers( } // Archive recheck (codex r1 P1-5): cheap pre-cycle lookup. Returns // immediately if source is gone or archived; runCycle never even - // acquires a lock. - const rows = await engine.executeRaw<{ archived: boolean | null }>( - `SELECT archived FROM sources WHERE id = $1`, + // acquires a lock. Also fetches local_path so FS phases bind to the + // source's own checkout (the #2227/#2194 mixed-scope fix). + const rows = await engine.executeRaw<{ archived: boolean | null; local_path: string | null }>( + `SELECT archived, local_path FROM sources WHERE id = $1`, [rawSourceId], ); if (rows.length === 0) { @@ -1640,8 +1683,17 @@ export async function registerBuiltinHandlers( }; } sourceId = rawSourceId; + sourceLocalPath = typeof rows[0].local_path === 'string' && rows[0].local_path.length > 0 + ? rows[0].local_path + : null; } + // Effective checkout for FS phases. For a per-source cycle, bind to the + // SOURCE's local_path (or null → skip FS phases for a pure-DB source); + // NEVER fall through to the global repoPath, which would run sync/lint + // against the wrong tree. Legacy (no source_id) keeps the global repoPath. + const effectiveBrainDir: string | null = sourceId ? sourceLocalPath : repoPath; + // Allow callers to select phases via job data (e.g. skip embed for // fast cycles). Validates against ALL_PHASES to prevent injection. const { ALL_PHASES } = await import('../core/cycle.ts'); @@ -1653,8 +1705,23 @@ export async function registerBuiltinHandlers( // Pull default: legacy `true` for back-compat; explicit boolean wins. const pull = typeof job.data.pull === 'boolean' ? job.data.pull : true; + // #2194 fix #2 / codex #5 (D4): claim-time cooldown guard. A job already + // queued or retrying (max_attempts:2) can reach the worker after the + // dispatch gate decided to back this source off. Skip it here as a NO-OP + // (status 'skipped', NOT a failure — a failure would re-arm the cooldown). + if (sourceId) { + const { isSourceInCooldown } = await import('./autopilot-fanout.ts'); + if (await isSourceInCooldown(engine, sourceId)) { + return { + partial: false, + status: 'skipped', + report: { reason: 'source_in_cooldown', source_id: sourceId }, + }; + } + } + const report = await runCycle(engine, { - brainDir: repoPath, + brainDir: effectiveBrainDir, pull, signal: job.signal, // propagate abort so cycle bails on timeout/cancel ...(sourceId ? { sourceId } : {}), @@ -1672,6 +1739,49 @@ export async function registerBuiltinHandlers( }; }); + // #2194 fix #3 / #2227 bug #3 — brain-wide maintenance. Runs the `global` + // cycle phases (embed, orphans, purge, resolve_symbol_edges, grade_takes, + // calibration_profile, synthesize_concepts, skillopt) ONCE per window instead + // of N times concurrently across per-source cycles (the 4→10GB RSS blowout). + // No source_id → uses the legacy global cycle lock; stamps autopilot.last_global_at + // on success so the dispatch gate backs off. + worker.register('autopilot-global-maintenance', async (job) => { + const { runCycle, GLOBAL_PHASES, LAST_GLOBAL_AT_KEY, ALL_PHASES } = await import('../core/cycle.ts'); + const repoPath: string | null = typeof job.data.repoPath === 'string' + ? job.data.repoPath + : (await engine.getConfig('sync.repo_path')) ?? null; + + const validPhases = new Set(ALL_PHASES); + const requested = Array.isArray(job.data.phases) + ? (job.data.phases as string[]).filter((p) => validPhases.has(p as never)) + : GLOBAL_PHASES; + const phases = (requested.length > 0 ? requested : GLOBAL_PHASES) as typeof GLOBAL_PHASES; + + const report = await runCycle(engine, { + brainDir: repoPath, + pull: false, // brain-wide DB/maintenance work never git-pulls + signal: job.signal, + phases, + yieldBetweenPhases: async () => { await new Promise((r) => setImmediate(r)); }, + }); + + // Stamp last_global_at only on a non-failed run so a failed pass stays stale + // and re-dispatches next tick (self-healing retry). + if (report.status === 'ok' || report.status === 'clean' || report.status === 'partial') { + try { + await engine.setConfig(LAST_GLOBAL_AT_KEY, new Date().toISOString()); + } catch (e) { + console.warn(`[autopilot-global-maintenance] failed to stamp last_global_at: ${e instanceof Error ? e.message : String(e)}`); + } + } + + return { + partial: report.status === 'partial' || report.status === 'failed', + status: report.status, + report, + }; + }); + // Shell handler is always registered. Runtime env guard lives inside the // handler so claimed jobs emit a clear rejection log on workers missing // GBRAIN_ALLOW_SHELL_JOBS=1. diff --git a/src/commands/sources.ts b/src/commands/sources.ts index e6bac3b37..ff9874ccb 100644 --- a/src/commands/sources.ts +++ b/src/commands/sources.ts @@ -747,8 +747,26 @@ async function runStatus(engine: BrainEngine, args: string[]): Promise { // caught-up source reports lag 0 instead of growing wall-clock (v0.41.32.0). const metrics = await computeAllSourceMetrics(engine, sources, { probeContent: true }); + // #1950: a source holding a live (non-TTL-expired) per-source sync lock is + // actively syncing RIGHT NOW. Without this it printed "idle" while a sync + // proc was live (the bug reported). Read the SAME live-lock signal `gbrain + // doctor` uses, via the shared helper, so the two surfaces never disagree. + const { liveSyncStatus } = await import('../core/db-lock.ts'); + const syncRunning = new Map(); + await Promise.all( + metrics.map(async (m) => { + const live = await liveSyncStatus(engine, m.source_id); + if (live) syncRunning.set(m.source_id, live); + }), + ); + if (json) { - console.log(JSON.stringify({ schema_version: 1, sources: metrics }, null, 2)); + const enriched = metrics.map((m) => ({ + ...m, + sync_running: syncRunning.has(m.source_id), + sync_holder: syncRunning.get(m.source_id) ?? null, + })); + console.log(JSON.stringify({ schema_version: 1, sources: enriched }, null, 2)); return; } @@ -765,11 +783,15 @@ async function runStatus(engine: BrainEngine, args: string[]): Promise { const embed = `${m.embed_coverage_pct.toFixed(0)}%`; // v0.41.31: embed-backfill state (active beats queued beats idle) so a // cron operator sees deferred embedding work after `sync --all`. - const backfill = m.backfill_active > 0 - ? `active(${m.backfill_active})` - : m.backfill_queued > 0 - ? `queued(${m.backfill_queued})` - : 'idle'; + // #1950: a live sync lock wins the column — surfacing "actively syncing" + // matters more than deferred embed-backfill state. + const backfill = syncRunning.has(m.source_id) + ? 'running' + : m.backfill_active > 0 + ? `active(${m.backfill_active})` + : m.backfill_queued > 0 + ? `queued(${m.backfill_queued})` + : 'idle'; const fails = String(m.failed_jobs_24h); const queue = String(m.queue_depth); const pages = m.total_pages.toLocaleString(); @@ -780,7 +802,10 @@ async function runStatus(engine: BrainEngine, args: string[]): Promise { for (const m of metrics) { const warns: string[] = []; if (!m.local_path) warns.push('no local_path'); - if (m.lag_seconds === null) warns.push(`never synced — run \`gbrain sync --source ${m.source_id}\``); + // #1950: don't cry "never synced" while a sync lock is live — it's syncing now. + if (m.lag_seconds === null && !syncRunning.has(m.source_id)) { + warns.push(`never synced — run \`gbrain sync --source ${m.source_id}\``); + } if (m.embed_coverage_pct < 95 && m.total_chunks > 100) { warns.push(`${(100 - m.embed_coverage_pct).toFixed(1)}% un-embedded — run \`gbrain embed --stale --source ${m.source_id}\``); } diff --git a/src/commands/status.ts b/src/commands/status.ts index 55893a384..de223cf16 100644 --- a/src/commands/status.ts +++ b/src/commands/status.ts @@ -39,6 +39,7 @@ import type { BrainEngine } from '../core/engine.ts'; import { existsSync, readFileSync } from 'node:fs'; import { gbrainPath, loadConfig, isThinClient } from '../core/config.ts'; import { callRemoteTool, unpackToolResult } from '../core/mcp-client.ts'; +import { VERSION } from '../version.ts'; import { buildSyncStatusReport, type SyncStatusReport, @@ -104,6 +105,10 @@ export interface AutopilotStatus { export interface StatusReport { schema_version: typeof SCHEMA_VERSION; + /** #1984: the local gbrain CLI version, so a poller can pin behavior to a build. */ + version: string; + /** #1984: the remote brain server's version (thin-client only; present when reported). */ + remote_version?: string; generated_at: string; mode: 'local' | 'thin-client'; sync?: SyncStatusReport; @@ -113,6 +118,33 @@ export interface StatusReport { queue?: QueueCounts | { local_only_remote: true }; autopilot?: AutopilotStatus | { local_only_remote: true }; warnings?: string[]; + /** #1984: true when a --deadline-ms budget elided one or more sections. */ + partial?: boolean; + /** #1984: names of the sections skipped/timed-out under the deadline. */ + stale_sections?: string[]; +} + +/** + * #1984: race a section's work against the remaining deadline budget. Returns + * the value, or undefined if the budget elapses first (the underlying query is + * abandoned — the process is a one-shot, so a stranded read is fine). `ms` + * undefined / <=0 means "no deadline" and the promise is awaited as-is. + */ +export async function withSectionDeadline( + p: Promise, + ms: number | undefined, + onTimeout: () => void, +): Promise { + if (ms === undefined || ms <= 0) return p; + let timer: ReturnType | undefined; + const timeout = new Promise((resolve) => { + timer = setTimeout(() => { onTimeout(); resolve(undefined); }, ms); + }); + try { + return await Promise.race([p, timeout]); + } finally { + if (timer) clearTimeout(timer); + } } // --------------------------------------------------------------------------- @@ -305,6 +337,8 @@ function buildAutopilotStatus(): AutopilotStatus { interface BuildOpts { sections?: Set
; + /** #1984: total wall-clock budget; sections share it, later ones get the remainder. */ + deadlineMs?: number; } async function buildLocalReport( @@ -313,23 +347,44 @@ async function buildLocalReport( ): Promise { const want = (s: Section) => !opts.sections || opts.sections.has(s); const warnings: string[] = []; + const staleSections: string[] = []; const report: StatusReport = { schema_version: SCHEMA_VERSION, + version: VERSION, generated_at: new Date().toISOString(), mode: 'local', }; + // #1984: a shared budget so `gbrain status --deadline-ms=N` never blocks a + // poller past N. Each async section is raced against the REMAINING budget, so + // one slow/hung section (cross-region DB, lock contention) can't strand the + // whole snapshot — it's marked stale and the rest still return. + const deadlineAt = opts.deadlineMs && opts.deadlineMs > 0 ? Date.now() + opts.deadlineMs : null; + const remaining = (): number | undefined => + deadlineAt === null ? undefined : Math.max(1, deadlineAt - Date.now()); + const markStale = (name: Section) => { + staleSections.push(name); + report.partial = true; + warnings.push(`${name} section exceeded the --deadline-ms budget (returned stale)`); + }; + if (want('sync')) { try { - const sources = await engine.executeRaw<{ - id: string; - name: string; - local_path: string | null; - config: Record | null; - }>(`SELECT id, name, local_path, config FROM sources ORDER BY id`); - report.sync = await buildSyncStatusReport( - engine, - sources.map((s) => ({ id: s.id, name: s.name, local_path: s.local_path, config: s.config ?? {} })), + report.sync = await withSectionDeadline( + (async () => { + const sources = await engine.executeRaw<{ + id: string; + name: string; + local_path: string | null; + config: Record | null; + }>(`SELECT id, name, local_path, config FROM sources ORDER BY id`); + return buildSyncStatusReport( + engine, + sources.map((s) => ({ id: s.id, name: s.name, local_path: s.local_path, config: s.config ?? {} })), + ); + })(), + remaining(), + () => markStale('sync'), ); } catch (err) { warnings.push(`sync section failed: ${(err as Error).message}`); @@ -337,23 +392,24 @@ async function buildLocalReport( } if (want('cycle')) { try { - report.cycle = await buildCycleSnapshot(engine); + report.cycle = await withSectionDeadline(buildCycleSnapshot(engine), remaining(), () => markStale('cycle')); } catch (err) { warnings.push(`cycle section failed: ${(err as Error).message}`); } } if (want('locks')) { - report.locks = await buildLocks(engine); + report.locks = await withSectionDeadline(buildLocks(engine), remaining(), () => markStale('locks')); } if (want('workers')) { report.workers = buildWorkerSummary(); } if (want('queue')) { - report.queue = await buildQueueCounts(engine); + report.queue = await withSectionDeadline(buildQueueCounts(engine), remaining(), () => markStale('queue')); } if (want('autopilot')) { report.autopilot = buildAutopilotStatus(); } + if (staleSections.length > 0) report.stale_sections = staleSections; if (warnings.length > 0) report.warnings = warnings; return report; } @@ -366,20 +422,51 @@ async function buildThinClientReport( const warnings: string[] = []; const report: StatusReport = { schema_version: SCHEMA_VERSION, + version: VERSION, generated_at: new Date().toISOString(), mode: 'thin-client', }; if (want('sync') || want('cycle')) { try { - const raw = await callRemoteTool(cfg!, 'get_status_snapshot', {}); - const payload = unpackToolResult<{ - schema_version: number; - sync: SyncStatusReport; - cycle: CycleSnapshot; - }>(raw); - if (want('sync')) report.sync = payload.sync; - if (want('cycle')) report.cycle = payload.cycle; + const payload = await withSectionDeadline( + (async () => { + // #1984: pass the budget as the request timeout so the LOSING side of + // the race actually cancels the in-flight MCP call instead of leaking + // it (the section deadline only abandons the promise locally). + const raw = await callRemoteTool( + cfg!, + 'get_status_snapshot', + {}, + opts.deadlineMs && opts.deadlineMs > 0 ? { timeoutMs: opts.deadlineMs } : {}, + ); + return unpackToolResult<{ + schema_version: number; + version?: string; + sync: SyncStatusReport; + cycle: CycleSnapshot; + }>(raw); + })(), + opts.deadlineMs && opts.deadlineMs > 0 ? opts.deadlineMs : undefined, + () => { + report.partial = true; + // Only name the sections the caller actually requested; the remote + // fetch backs both sync+cycle, but `--section sync` must not report + // `cycle` (a section it excluded) as stale. Matches the local path. + const elided: Section[] = [ + ...(want('sync') ? (['sync'] as Section[]) : []), + ...(want('cycle') ? (['cycle'] as Section[]) : []), + ]; + report.stale_sections = [...(report.stale_sections ?? []), ...elided]; + warnings.push('remote snapshot exceeded the --deadline-ms budget (returned stale)'); + }, + ); + if (payload) { + // #1984: surface the brain server's version for thin-client parity. + if (payload.version) report.remote_version = payload.version; + if (want('sync')) report.sync = payload.sync; + if (want('cycle')) report.cycle = payload.cycle; + } } catch (err) { warnings.push(`remote snapshot failed: ${(err as Error).message}`); } @@ -401,7 +488,13 @@ function renderHuman(report: StatusReport): string { lines.push(''); lines.push('GBrain Status'); lines.push('============='); - lines.push(`Mode: ${report.mode} · ${report.generated_at}`); + const ver = report.remote_version && report.remote_version !== report.version + ? `v${report.version} (remote v${report.remote_version})` + : `v${report.version}`; + lines.push(`Mode: ${report.mode} · ${ver} · ${report.generated_at}`); + if (report.partial) { + lines.push(`⚠ partial snapshot — stale sections: ${(report.stale_sections ?? []).join(', ')} (--deadline-ms budget hit)`); + } lines.push(''); // Sync @@ -528,6 +621,36 @@ function renderHuman(report: StatusReport): string { * - Set
→ only these sections * - 'usage_error' → bad section name (caller exits 2) */ +/** #1984: `--fast` preset budget (ms) when no explicit `--deadline-ms` is given. */ +export const FAST_DEADLINE_MS = 2000; + +/** + * #1984: parse the status deadline budget. `--deadline-ms=N` (or `--deadline-ms N`) + * wins; bare `--fast` applies FAST_DEADLINE_MS. Returns undefined (no budget), + * a positive ms value, or 'usage_error' on a non-positive / non-numeric value. + */ +export function parseDeadlineFlag(args: string[]): number | undefined | 'usage_error' { + let raw: string | undefined; + for (let i = 0; i < args.length; i++) { + const a = args[i]; + if (a === '--deadline-ms') { + // #1984: a bare `--deadline-ms` with no following value is a usage error, + // not a silent fall-through to no-budget / --fast (which would mask a + // typo'd budget and let a poller hang it never meant to). + if (i + 1 >= args.length) return 'usage_error'; + raw = args[i + 1]; + break; + } + if (a.startsWith('--deadline-ms=')) { raw = a.slice('--deadline-ms='.length); break; } + } + if (raw == null) { + return args.includes('--fast') ? FAST_DEADLINE_MS : undefined; + } + const n = Number(raw); + if (!Number.isFinite(n) || n <= 0) return 'usage_error'; + return n; +} + export function parseSectionFlag(args: string[]): Set
| undefined | 'usage_error' { let raw: string | undefined; for (let i = 0; i < args.length; i++) { @@ -575,19 +698,25 @@ export async function runStatus( const sections = sectionFlag; const json = args.includes('--json'); + const deadlineMs = parseDeadlineFlag(args); + if (deadlineMs === 'usage_error') { + stderr('gbrain status: --deadline-ms must be a positive number of milliseconds\n'); + return { exitCode: 2 }; + } + const cfg = loadConfig(); const useThinClient = cfg ? isThinClient(cfg) : false; let report: StatusReport; try { if (useThinClient) { - report = await buildThinClientReport(cfg, { sections }); + report = await buildThinClientReport(cfg, { sections, deadlineMs }); } else { if (!engine) { stderr('gbrain status: no engine connected (DB unreachable?). Run `gbrain doctor` to diagnose.\n'); return { exitCode: 1 }; } - report = await buildLocalReport(engine, { sections }); + report = await buildLocalReport(engine, { sections, deadlineMs }); } } catch (err) { stderr(`gbrain status: snapshot failed: ${(err as Error).message}\n`); diff --git a/src/commands/sync.ts b/src/commands/sync.ts index 856900bf0..2f8d5e42a 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -212,7 +212,7 @@ export interface SyncResult { * cron operators can disambiguate timeout vs pull-timeout in monitoring. */ filesImported?: number; - reason?: 'timeout' | 'pull_timeout' | 'checkpoint_unavailable'; + reason?: 'timeout' | 'pull_timeout' | 'stall_timeout' | 'checkpoint_unavailable'; /** * v0.42.x (#1794): cumulative file paths durably banked to the checkpoint * across THIS run + prior resumed runs. Surfaced on every partial/blocked @@ -1440,7 +1440,7 @@ function buildPartialResult(opts: { modified: number; deleted: number; renamed: number; - reason: 'timeout' | 'pull_timeout' | 'checkpoint_unavailable'; + reason: 'timeout' | 'pull_timeout' | 'stall_timeout' | 'checkpoint_unavailable'; bankedFiles?: number; }): SyncResult { return { @@ -2050,7 +2050,7 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise => { + const partial = async (reason: 'timeout' | 'pull_timeout' | 'stall_timeout'): Promise => { deregisterCheckpointCleanup(); if (!checkpointDead) { try { await flushCheckpoint(); } catch { /* best effort — we're aborting */ } @@ -2409,6 +2409,48 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise | undefined; + if (stallSeconds > 0) { + const stallMs = stallSeconds * 1000; + const stallController = new AbortController(); + stallTimer = setInterval(() => { + if (Date.now() - progressAt.last >= stallMs) { + serr( + `[sync] no import progress for ${stallSeconds}s — aborting (stall watchdog). ` + + `The per-source lock will release; the next 'gbrain sync' resumes from the checkpoint.`, + ); + stallAborted = true; + stallController.abort(); + } + }, Math.min(5000, stallMs)); + // Don't keep the process alive on the watchdog alone. + (stallTimer as unknown as { unref?: () => void }).unref?.(); + opts = { ...opts, signal: composeAbortSignals(opts.signal, stallController.signal) }; + } + async function importOnePath(eng: BrainEngine, path: string): Promise { const filePath = join(syncRepoPath, path); if (!existsSync(filePath)) { @@ -2429,6 +2471,7 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise = process.env, +): number { + const raw = env.GBRAIN_SYNC_STALL_ABORT_SECONDS; + if (raw === undefined || raw === '') return DEFAULT_SYNC_STALL_ABORT_SEC; + const n = Number(raw); + if (!Number.isFinite(n)) return DEFAULT_SYNC_STALL_ABORT_SEC; + return n; // n <= 0 disables the watchdog +} + /** * Resolve the out-of-band hard-deadline for a `gbrain sync` invocation (#1633). * Pure + argv/env-only so it runs BEFORE `connectEngine` (so a connect-phase hang diff --git a/src/core/cycle.ts b/src/core/cycle.ts index 17a31bccb..8219b3b77 100644 --- a/src/core/cycle.ts +++ b/src/core/cycle.ts @@ -242,6 +242,25 @@ export const PHASE_SCOPE: Record = { skillopt: 'global', }; +/** + * #2194 fix #3 / #2227 bug #3 — the cycle split. + * + * Per-source autopilot cycles run ONLY the source-scoped (and mixed) phases; + * the brain-wide `global` phases (embed, orphans, purge, resolve_symbol_edges, + * grade_takes, calibration_profile, synthesize_concepts, skillopt) run ONCE in + * a separate `autopilot-global-maintenance` job instead of N times concurrently + * across per-source cycles (the 4→10GB RSS blowout). Single-flight is + * structural: one global job, not a skip-and-pretend-fresh hack (codex #1/#2). + * + * GLOBAL_PHASES ∪ NON_GLOBAL_PHASES == ALL_PHASES, with no overlap — pinned by + * test/autopilot-global-maintenance.test.ts. + */ +export const GLOBAL_PHASES: CyclePhase[] = ALL_PHASES.filter((p) => PHASE_SCOPE[p] === 'global'); +export const NON_GLOBAL_PHASES: CyclePhase[] = ALL_PHASES.filter((p) => PHASE_SCOPE[p] !== 'global'); + +/** Config key holding the ISO timestamp of the last successful global-maintenance run. */ +export const LAST_GLOBAL_AT_KEY = 'autopilot.last_global_at'; + /** * Phases that mutate state (filesystem or DB) and therefore should * coordinate via the cycle lock. Only orphans is truly read-only @@ -2305,12 +2324,21 @@ export async function runCycle( // the cost of missing a successful write (next cycle will redo work). if (opts.sourceId && engine && !dryRun && !aborted && (status === 'ok' || status === 'clean' || status === 'partial')) { try { + const nowIso = new Date().toISOString(); + // #2194 fix #3 (the cycle split): `last_source_cycle_at` is the NEW gate + // for per-source dispatch (source-scoped phases done). We ALSO keep + // `last_full_cycle_at` current so doctor's cycle-freshness check and any + // legacy reader stay valid — it's no longer a *gate* for the brain-wide + // phases (those gate on autopilot.last_global_at), so writing it on a + // source-only cycle does not re-introduce the freshness poisoning codex + // flagged in the rejected skip-based design. await engine.updateSourceConfig(opts.sourceId, { - last_full_cycle_at: new Date().toISOString(), + last_source_cycle_at: nowIso, + last_full_cycle_at: nowIso, }); } catch (e) { // Best-effort; cycle already succeeded by the time we get here. - console.warn(`[cycle] failed to write last_full_cycle_at for source ${opts.sourceId}: ${e instanceof Error ? e.message : String(e)}`); + console.warn(`[cycle] failed to write last_source_cycle_at for source ${opts.sourceId}: ${e instanceof Error ? e.message : String(e)}`); } } diff --git a/src/core/db-lock.ts b/src/core/db-lock.ts index 436b59e16..238237a5f 100644 --- a/src/core/db-lock.ts +++ b/src/core/db-lock.ts @@ -132,6 +132,28 @@ export function isHolderDeadLocally( return classifyHolderLiveness(holderPid, holderHost, ageMs, opts) === 'dead_eligible'; } +/** + * issue #2227: is a lock-row holder live enough to count as "running" for an + * observability surface (`gbrain jobs supervisor status`, `gbrain doctor`)? + * + * PID-reuse-safe by design (`pid-liveness-alone-pid-reuse`): keys on the lock's + * own freshness, NEVER `process.kill`. A live holder refreshes its TTL on a + * timer; a dead one stops, so its `ttl_expires_at` lapses and (after the steal + * grace) `last_refreshed_at` ages out. The primary signal is `!ttl_expired`; + * the heartbeat grace covers a starved-but-alive holder whose TTL briefly + * lapsed between refresh ticks (the #1794 thrash class), mirroring the + * steal-grace semantics `tryAcquireDbLock` already uses. Cross-host holders are + * still "running" for visibility (a supervisor exists, just elsewhere) — we + * report freshness, not takeover-eligibility, so host is not consulted here. + */ +export function isLockHolderLive(snap: LockSnapshot, ttlMinutes: number = DEFAULT_TTL_MINUTES): boolean { + if (!snap.ttl_expired) return true; + if (snap.ms_since_last_refresh !== null) { + return snap.ms_since_last_refresh < resolveStealGraceSeconds(ttlMinutes) * 1000; + } + return false; +} + /** * Try to acquire a named DB lock. * @@ -707,6 +729,38 @@ export function syncLockId(sourceId: string): string { */ export const SYNC_LOCK_ID = syncLockId('default'); +/** + * #1950: is `sourceId` actively holding a live (non-TTL-expired) sync lock? + * + * Centralizes the live-sync signal that `gbrain doctor` already computes inline + * so `gbrain sources status` (and future surfaces) read the SAME truth instead + * of each re-deriving it. Returns the holder when a live lock is held, else + * null (idle, or a stale/expired lock that's structurally available for the + * next acquire). Inspect failures swallow to null — a status surface should + * degrade to "no indicator", never crash. + * + * Honest scope: a live lock proves the holder process is heartbeating, NOT that + * the import is making forward progress — `withRefreshingLock` refreshes + * `last_refreshed_at` on its own timer regardless of import progress. So callers + * report "running", NOT "healthy"; a wedged-but-alive holder still reads as + * running here. Forward-progress stall detection lives in the sync drain loop + * (#1950 stall-abort); stale-lock triage lives in `gbrain doctor`. + */ +export async function liveSyncStatus( + engine: BrainEngine, + sourceId: string, +): Promise<{ holder_pid: number; holder_host: string } | null> { + try { + const snap = await inspectLock(engine, syncLockId(sourceId)); + if (snap && !snap.ttl_expired) { + return { holder_pid: snap.holder_pid, holder_host: snap.holder_host }; + } + return null; + } catch { + return null; + } +} + /** * v0.30.1 (T4 + A4): wrap long-running work in a refreshing TTL lock. * diff --git a/src/core/doctor-categories.ts b/src/core/doctor-categories.ts index e8adebb71..5f7532861 100644 --- a/src/core/doctor-categories.ts +++ b/src/core/doctor-categories.ts @@ -127,6 +127,7 @@ export const SKILL_CHECK_NAMES: ReadonlySet = new Set([ */ export const OPS_CHECK_NAMES: ReadonlySet = new Set([ 'alternative_providers', + 'autopilot_fanout_concurrency', 'autopilot_lock_scope', 'batch_retry_health', 'brainstorm_health', diff --git a/src/core/minions/child-worker-supervisor.ts b/src/core/minions/child-worker-supervisor.ts index c7ab8524d..ad294b74c 100644 --- a/src/core/minions/child-worker-supervisor.ts +++ b/src/core/minions/child-worker-supervisor.ts @@ -61,9 +61,12 @@ export type ChildSupervisorEvent = } | { kind: 'health_warn'; - reason: 'clean_restart_budget_exceeded' | 'rss_watchdog_loop'; + reason: 'clean_restart_budget_exceeded' | 'rss_watchdog_loop' | 'crash_budget_degraded'; count: number; - windowMs: number; + /** Present for window-scoped warnings (budget/watchdog loops). */ + windowMs?: number; + /** Present for crash_budget_degraded: the soft budget that was crossed. */ + max?: number; }; export interface ChildWorkerSupervisorOpts { @@ -73,8 +76,24 @@ export interface ChildWorkerSupervisorOpts { args: string[]; /** Child env. Defaults to a clone of process.env. */ env?: NodeJS.ProcessEnv; - /** Give up after this many consecutive code != 0 exits. */ + /** + * Soft crash budget. issue #1994 (#2227 tail): crossing this NO LONGER + * permanently gives up. Instead the supervisor enters DEGRADED mode — it + * keeps respawning with capped exponential backoff (60s cap) and emits a + * loud `crash_budget_degraded` health_warn — so a transient DB-pooler outage + * that trips the counter self-heals when the DB returns (the stable-run reset + * clears crashCount once a respawn runs > stableRunResetMs) instead of + * wedging the queue until a human restart. + */ maxCrashes: number; + /** + * Hard ceiling: permanently give up (fire onMaxCrashesExceeded) ONLY after + * this many consecutive crashes — the runaway backstop for a genuinely + * unrecoverable hot crash-loop that the stable-run reset never escapes. + * Default: maxCrashes * HARD_STOP_CRASH_MULTIPLIER. Set to 0 to disable + * permanent give-up entirely (retry-forever-with-backoff). + */ + hardStopMaxCrashes?: number; /** Stable-run reset window: code != 0 after this duration resets crashCount to 1. Default 5 min. */ stableRunResetMs?: number; @@ -130,6 +149,12 @@ export interface ChildWorkerSupervisorOpts { _now?: () => number; } +/** issue #1994: how many multiples of the soft crash budget before the hard + * permanent-give-up backstop fires. 10× the default soft budget of 10 = 100 + * consecutive crashes (each within the stable-run window) before we conclude + * it's a genuine code bug and stop. A transient outage recovers long before. */ +export const HARD_STOP_CRASH_MULTIPLIER = 10; + const DEFAULTS = { stableRunResetMs: 5 * 60 * 1000, cleanRestartBudget: 10, @@ -287,19 +312,50 @@ export class ChildWorkerSupervisor { /** * Run the spawn-and-respawn loop. Resolves when: * 1. composer.isStopping() returns true, OR - * 2. crashCount reaches maxCrashes (after firing onMaxCrashesExceeded). + * 2. crashCount reaches the HARD ceiling (after firing onMaxCrashesExceeded). + * + * issue #1994 (#2227 tail): crossing the SOFT budget (`maxCrashes`) no longer + * stops the supervisor. It enters degraded mode — keep respawning with capped + * exponential backoff (60s cap, so it's a paced retry, not a hot loop) and + * announce loudly — so a transient DB-pooler outage that trips the counter + * recovers on its own (a respawn that runs > stableRunResetMs resets + * crashCount to 1) instead of permanently wedging the queue. Permanent + * give-up fires only at the much-higher hard ceiling: the runaway backstop + * for a genuinely unrecoverable hot crash-loop. */ async run(): Promise { - while (!this.opts.isStopping() && this._crashCount < this.opts.maxCrashes) { + const hardStop = this.opts.hardStopMaxCrashes ?? + this.opts.maxCrashes * HARD_STOP_CRASH_MULTIPLIER; + let degradedAnnounced = false; + while (!this.opts.isStopping()) { await this.spawnOnce(); if (this.opts.isStopping()) return; - if (this._crashCount >= this.opts.maxCrashes) { - this.opts.onMaxCrashesExceeded(this._crashCount, this.opts.maxCrashes); + // Hard ceiling: permanent give-up (the runaway backstop). hardStop <= 0 + // disables it entirely (retry-forever-with-backoff for deployments that + // would rather never auto-stop a recoverable supervisor). + if (hardStop > 0 && this._crashCount >= hardStop) { + this.opts.onMaxCrashesExceeded(this._crashCount, hardStop); return; } + // Soft budget crossed → degraded mode. Announce once per degradation + // episode; re-arm after a stable-run reset drops us back under budget. + if (this._crashCount >= this.opts.maxCrashes) { + if (!degradedAnnounced) { + degradedAnnounced = true; + this.opts.onEvent({ + kind: 'health_warn', + reason: 'crash_budget_degraded', + count: this._crashCount, + max: this.opts.maxCrashes, + }); + } + } else { + degradedAnnounced = false; + } + await this.applyBackoff(); } } diff --git a/src/core/minions/handler-timeouts.ts b/src/core/minions/handler-timeouts.ts index 9cfa6919b..365e51160 100644 --- a/src/core/minions/handler-timeouts.ts +++ b/src/core/minions/handler-timeouts.ts @@ -34,6 +34,9 @@ export const HANDLER_DEFAULT_TIMEOUT_MS: Readonly> = { subagent_aggregator: THIRTY_MIN_MS, 'embed-backfill': THIRTY_MIN_MS, 'autopilot-cycle': THIRTY_MIN_MS, + // #2194 fix #3: brain-wide maintenance (embed-all/orphans/purge/…) can run + // longer than a single source cycle; give it the same 30-min budget. + 'autopilot-global-maintenance': THIRTY_MIN_MS, }; /** diff --git a/src/core/minions/queue.ts b/src/core/minions/queue.ts index 70234d276..e21eb8a41 100644 --- a/src/core/minions/queue.ts +++ b/src/core/minions/queue.ts @@ -647,9 +647,18 @@ export class MinionQueue { async handleTimeouts(): Promise { return this.engine.transaction(async (tx) => { const rows = await tx.executeRaw>( + // #1737: count the timed-out run as a spent attempt (terminal, no retry), + // mirroring handleWallClockTimeouts + handleStalled. handleTimeouts is the + // FIRST killer to fire for the long-lane handlers (timeout_ms stamped at + // submit), so without this the job reads `attempts: 0/N (started: N)`. + // Safe against double-count: the worker sweep runs handleStalled -> + // handleTimeouts -> handleWallClockTimeouts sequentially and awaited, and + // each guards on `status = 'active'`, so the first to set status='dead' + // excludes the row from the later sweeps. `UPDATE minion_jobs SET status = 'dead', error_text = 'timeout exceeded', + attempts_made = attempts_made + 1, lock_token = NULL, lock_until = NULL, finished_at = now(), diff --git a/src/core/minions/supervisor.ts b/src/core/minions/supervisor.ts index ec68dc499..9a71355d1 100644 --- a/src/core/minions/supervisor.ts +++ b/src/core/minions/supervisor.ts @@ -30,6 +30,7 @@ import { detectTini } from './spawn-helpers.ts'; import { resolveDefaultMaxRssMb } from './rss-default.ts'; import { ChildWorkerSupervisor, + HARD_STOP_CRASH_MULTIPLIER, type ChildSupervisorEvent, } from './child-worker-supervisor.ts'; import { @@ -191,6 +192,21 @@ export function buildWorkerArgs( * shutdown() drain window (issue #1801, D3). */ const WEDGE_RESTART_GRACE_MS = 35_000; +/** + * issue #1994: resolve the hard permanent-give-up ceiling. Default + * maxCrashes × HARD_STOP_CRASH_MULTIPLIER; operators override (or disable with + * 0 = never auto-stop) via GBRAIN_SUPERVISOR_HARD_STOP_CRASHES. A negative or + * non-integer override is ignored (falls back to the default). + */ +export function resolveHardStopMaxCrashes(maxCrashes: number): number { + const raw = process.env.GBRAIN_SUPERVISOR_HARD_STOP_CRASHES; + if (raw !== undefined && raw !== '') { + const n = Number(raw); + if (Number.isInteger(n) && n >= 0) return n; + } + return maxCrashes * HARD_STOP_CRASH_MULTIPLIER; +} + /** Calculate backoff: 1s, 2s, 4s, 8s, 16s, 32s, 60s cap. */ export function calculateBackoffMs(crashCount: number): number { const base = Math.min(1000 * Math.pow(2, Math.max(crashCount, 0)), 60_000); @@ -293,7 +309,11 @@ export const ExitCodes = { * pidfile path. TTL > refresh-interval × max-failures so we always exit * before our lock could lapse and let a second supervisor take over. */ -const SUPERVISOR_LOCK_TTL_MIN = 5; +// Exported (issue #2227) so observability surfaces (`gbrain jobs supervisor +// status`, `gbrain doctor`) compute the lock-freshness steal grace with the +// SAME TTL the supervisor refreshes against, when detecting a live supervisor +// via the DB lock instead of the (possibly split-$HOME) pidfile. +export const SUPERVISOR_LOCK_TTL_MIN = 5; const SUPERVISOR_LOCK_REFRESH_MS = 60_000; const SUPERVISOR_LOCK_REFRESH_MAX_FAILURES = 3; // 3 × 60s = 180s < 5min TTL @@ -825,6 +845,10 @@ export class MinionSupervisor { args: workerArgs, env, maxCrashes: this.opts.maxCrashes, + // issue #1994: hard permanent-give-up ceiling (the runaway backstop). + // Operators can raise/lower or disable (0 = never auto-stop) via + // GBRAIN_SUPERVISOR_HARD_STOP_CRASHES; default is maxCrashes × 10. + hardStopMaxCrashes: resolveHardStopMaxCrashes(this.opts.maxCrashes), _backoffFloorMs: this.opts._backoffFloorMs, isStopping: () => this.stopping, onMaxCrashesExceeded: (count, max) => { @@ -907,6 +931,9 @@ export class MinionSupervisor { // ("raise --max-rss") is one glance away. Peak RSS stays in the // worker's own stderr line (the supervisor never sees it). ...(event.reason === 'rss_watchdog_loop' ? { max_rss_mb: this.opts.maxRssMb } : {}), + // issue #1994: degraded mode crossed the soft crash budget — surface + // it so doctor/status show "retrying with backoff" instead of silence. + ...(event.max !== undefined ? { max_crashes: event.max } : {}), }); return; } diff --git a/src/core/operations.ts b/src/core/operations.ts index 2051c7bd9..9a8d65640 100644 --- a/src/core/operations.ts +++ b/src/core/operations.ts @@ -2417,7 +2417,10 @@ const get_status_snapshot: Operation = { } const sync = await buildSyncStatusReport(ctx.engine, sources); const cycle = await buildCycleSnapshot(ctx.engine); - return { schema_version: 1 as const, sync, cycle }; + // #1984: report the brain server's version so a thin-client `gbrain status` + // can surface remote_version alongside its own local CLI version. + const { VERSION } = await import('../version.ts'); + return { schema_version: 1 as const, version: VERSION, sync, cycle }; }, scope: 'admin', localOnly: false, diff --git a/test/agent-cli.test.ts b/test/agent-cli.test.ts index 5561490d0..e953b2f58 100644 --- a/test/agent-cli.test.ts +++ b/test/agent-cli.test.ts @@ -43,7 +43,7 @@ describe('parseRunFlags', () => { expect(rest).toEqual(['hello', 'world']); }); - test('flags before prompt are parsed, unknown token ends flag parsing', () => { + test('leading flags parsed; first positional begins the prompt', () => { const { flags, rest } = agentTesting.parseRunFlags([ '--model', 'claude-opus-4-7', '--max-turns', '30', 'summarize', 'everything', ]); @@ -69,8 +69,60 @@ describe('parseRunFlags', () => { expect(rest).toEqual(['--not-a-flag']); }); - test('unknown flag throws', () => { - expect(() => agentTesting.parseRunFlags(['--what', 'x'])).toThrow(/unknown flag/); + test('#1738: unknown --flag is prompt text, not an error', () => { + const { rest } = agentTesting.parseRunFlags(['--what', 'x']); + expect(rest).toEqual(['--what', 'x']); + }); + + test('#1738: trailing --detach is hoisted out of the prompt', () => { + const { flags, rest } = agentTesting.parseRunFlags(['do', 'the', 'thing', '--detach']); + expect(flags.detach).toBe(true); + expect(flags.follow).toBe(false); + expect(rest).toEqual(['do', 'the', 'thing']); + }); + + test('#1738: leading flags + trailing switch both apply', () => { + const { flags, rest } = agentTesting.parseRunFlags(['--model', 'm', 'summarize', '--detach', '--no-follow']); + expect(flags.model).toBe('m'); + expect(flags.detach).toBe(true); + expect(flags.follow).toBe(false); + expect(rest).toEqual(['summarize']); + }); + + test('#1738: a --switch mid-prompt (not trailing) stays verbatim', () => { + const { flags, rest } = agentTesting.parseRunFlags(['summarize', '--detach', 'the', 'doc']); + expect(flags.detach).toBe(false); + expect(rest).toEqual(['summarize', '--detach', 'the', 'doc']); + }); + + test('#1738: a freeform prompt starting with --word is preserved', () => { + const { rest } = agentTesting.parseRunFlags(['--note:', 'do', 'the', 'thing']); + expect(rest).toEqual(['--note:', 'do', 'the', 'thing']); + }); + + test('#1738: -- suppresses trailing-switch hoisting', () => { + const { flags, rest } = agentTesting.parseRunFlags(['--', 'do', 'x', '--detach']); + expect(flags.detach).toBe(false); + expect(rest).toEqual(['do', 'x', '--detach']); + }); + + test('#1738: -- AFTER a positional also suppresses hoisting (no silent detach flip)', () => { + // The leading-flag loop breaks at the first positional, so the `escaped` + // flag never fires for a `--` placed later. A literal `--` ANYWHERE must + // still mean "hoist nothing" — otherwise `agent run note -- body --detach` + // silently detaches and drops the `--` as junk. + const { flags, rest } = agentTesting.parseRunFlags(['note', '--', 'body', '--detach']); + expect(flags.detach).toBe(false); + expect(rest).toEqual(['note', '--', 'body', '--detach']); + }); + + test('#1738: value-flag missing its value throws a usage error', () => { + expect(() => agentTesting.parseRunFlags(['--model'])).toThrow(/requires a value/); + expect(() => agentTesting.parseRunFlags(['--model', '--detach', 'x'])).toThrow(/requires a value/); + }); + + test('#1738: numeric value-flag rejects a non-number', () => { + expect(() => agentTesting.parseRunFlags(['--max-turns', 'abc', 'x'])).toThrow(/expects a number/); }); test('--subagent-def + --timeout-ms parsed', () => { diff --git a/test/autopilot-cycle-source-checkout.test.ts b/test/autopilot-cycle-source-checkout.test.ts new file mode 100644 index 000000000..00387eed8 --- /dev/null +++ b/test/autopilot-cycle-source-checkout.test.ts @@ -0,0 +1,108 @@ +/** + * issue #2227/#2194 (TODOS:634, codex #8) — a per-source `autopilot-cycle` + * binds its filesystem phases to the SOURCE's own checkout (`local_path`), + * never the global brain's `sync.repo_path`. + * + * Pre-fix the handler fed `repoPath` (the global checkout) into runCycle even + * when `source_id` was set, so FS phases (sync/lint/extract) ran against the + * wrong tree while DB freshness was stamped for `source_id` — mixed scope. + * That made the failure-cooldown and freshness gates attribute work to the + * wrong source, the prerequisite codex flagged before the storm-breaker could + * be trusted. + * + * Drives the REAL handler captured from registerBuiltinHandlers (not a + * source-grep) so a reintroduced repoPath fallthrough fails here. PGLite + * in-memory. The report's `brain_dir` mirrors the cycle's effective brainDir + * (cycle.ts:2324), so it's the observable proxy for "which checkout did FS + * phases bind to". + */ + +import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test'; +import { mkdtempSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { registerBuiltinHandlers } from '../src/commands/jobs.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; + +let engine: PGLiteEngine; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +beforeEach(async () => { + await resetPgliteState(engine); +}); + +async function captureHandlers(): Promise Promise>> { + const handlers = new Map Promise>(); + const fakeWorker = { register(name: string, fn: (job: any) => Promise) { handlers.set(name, fn); } }; + await registerBuiltinHandlers(fakeWorker as never, engine); + return handlers; +} + +describe('autopilot-cycle handler — per-source checkout binding (#2227/#2194)', () => { + test('source_id with local_path → brainDir is the SOURCE checkout, not the global repo', async () => { + const sourceDir = mkdtempSync(join(tmpdir(), 'gbrain-src-')); + // A DIFFERENT global checkout must NOT win for a per-source job. + await engine.setConfig('sync.repo_path', '/some/global/brain/checkout'); + await engine.executeRaw( + `INSERT INTO sources (id, name, local_path, config, archived, created_at) + VALUES ('repo-a', 'Repo A', $1, '{}'::jsonb, false, now())`, + [sourceDir], + ); + + const handlers = await captureHandlers(); + const handler = handlers.get('autopilot-cycle')!; + // DB-only phase keeps the test cheap; brain_dir is stamped from opts.brainDir + // regardless of which phases run, so it still proves the binding. + const result = await handler({ + data: { source_id: 'repo-a', phases: ['resolve_symbol_edges'] }, + signal: undefined, + }); + + expect(result.report.brain_dir).toBe(sourceDir); + expect(result.report.brain_dir).not.toBe('/some/global/brain/checkout'); + }); + + test('source_id with NULL local_path → brainDir is null (FS phases skip), never the global repo', async () => { + // The mixed-scope bug: a pure-DB source must NOT fall through to repoPath. + await engine.setConfig('sync.repo_path', '/some/global/brain/checkout'); + await engine.executeRaw( + `INSERT INTO sources (id, name, local_path, config, archived, created_at) + VALUES ('db-only', 'DB Only', NULL, '{}'::jsonb, false, now())`, + [], + ); + + const handlers = await captureHandlers(); + const handler = handlers.get('autopilot-cycle')!; + const result = await handler({ + data: { source_id: 'db-only', phases: ['resolve_symbol_edges'] }, + signal: undefined, + }); + + expect(result.report.brain_dir).toBeNull(); + expect(result.report.brain_dir).not.toBe('/some/global/brain/checkout'); + }); + + test('legacy (no source_id) keeps the global repoPath — back-compat', async () => { + const globalDir = mkdtempSync(join(tmpdir(), 'gbrain-global-')); + await engine.setConfig('sync.repo_path', globalDir); + + const handlers = await captureHandlers(); + const handler = handlers.get('autopilot-cycle')!; + const result = await handler({ + data: { phases: ['resolve_symbol_edges'] }, + signal: undefined, + }); + + expect(result.report.brain_dir).toBe(globalDir); + }); +}); diff --git a/test/autopilot-failure-cooldown.test.ts b/test/autopilot-failure-cooldown.test.ts new file mode 100644 index 000000000..3b9ed9151 --- /dev/null +++ b/test/autopilot-failure-cooldown.test.ts @@ -0,0 +1,151 @@ +/** + * #2194 fix #2 — failure cooldown (the storm-breaker). + * + * A source whose autopilot-cycle keeps failing re-dispatched every 5-min tick + * (only SUCCESS gated dispatch), so the same handful of sources failed and + * re-fanned-out forever — 200+ dead jobs/24h. The cooldown backs a failed + * source off with bounded exponential delay, read at dispatch from minion_jobs + * (dead/failed rows) AND re-checked at claim time (codex #5). A success clears + * it (codex #7). These tests pin the pure math, the engine query, the + * null-source guard (codex #6), and the dispatch/claim-time gates. + */ + +import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; +import { + cooldownMinForCount, + isInFailureCooldown, + readRecentSourceFailures, + isSourceInCooldown, + selectSourcesForDispatch, + resolveFailureCooldownOpts, + type SourceFailure, + type CooldownOpts, +} from '../src/commands/autopilot-fanout.ts'; +import type { SourceRow } from '../src/core/engine.ts'; + +const OPTS: CooldownOpts = { baseMin: 10, capMin: 120 }; + +describe('cooldownMinForCount — bounded exponential', () => { + test('grows 10, 20, 40, 80 then caps at 120', () => { + expect(cooldownMinForCount(1, OPTS)).toBe(10); + expect(cooldownMinForCount(2, OPTS)).toBe(20); + expect(cooldownMinForCount(3, OPTS)).toBe(40); + expect(cooldownMinForCount(4, OPTS)).toBe(80); + expect(cooldownMinForCount(5, OPTS)).toBe(120); // 160 capped to 120 + expect(cooldownMinForCount(99, OPTS)).toBe(120); + }); + test('zero/negative count or disabled base → 0', () => { + expect(cooldownMinForCount(0, OPTS)).toBe(0); + expect(cooldownMinForCount(3, { baseMin: 0, capMin: 120 })).toBe(0); + }); +}); + +describe('isInFailureCooldown — pure decision', () => { + const now = Date.UTC(2026, 5, 16, 12, 0, 0); + const ago = (min: number) => new Date(now - min * 60_000); + + test('no failure record → not in cooldown', () => { + expect(isInFailureCooldown(undefined, null, now, OPTS)).toBe(false); + }); + test('disabled (baseMin 0) → never in cooldown', () => { + expect(isInFailureCooldown({ count: 5, lastFailedAt: ago(1) }, null, now, { baseMin: 0, capMin: 120 })).toBe(false); + }); + test('failed 5min ago, count 1 (cooldown 10) → in cooldown', () => { + expect(isInFailureCooldown({ count: 1, lastFailedAt: ago(5) }, null, now, OPTS)).toBe(true); + }); + test('failed 15min ago, count 1 (cooldown 10) → recovered by time', () => { + expect(isInFailureCooldown({ count: 1, lastFailedAt: ago(15) }, null, now, OPTS)).toBe(false); + }); + test('success at/after the latest failure → cleared (codex #7)', () => { + const failure: SourceFailure = { count: 3, lastFailedAt: ago(5) }; + expect(isInFailureCooldown(failure, ago(4), now, OPTS)).toBe(false); // success 4min ago > fail 5min ago + }); + test('success BEFORE the latest failure, still in window → suppressed', () => { + const failure: SourceFailure = { count: 3, lastFailedAt: ago(5) }; + expect(isInFailureCooldown(failure, ago(30), now, OPTS)).toBe(true); + }); +}); + +describe('selectSourcesForDispatch — cooldown bucket', () => { + const src = (id: string): SourceRow => ({ id, name: id, config: {} } as SourceRow); + test('a stale source in cooldown is held in skippedCooldown, not dispatched', () => { + const sources = [src('a'), src('b')]; + const failures = new Map([ + ['a', { count: 1, lastFailedAt: new Date(Date.now() - 60_000) }], // 1min ago, cooldown 10min + ]); + const r = selectSourcesForDispatch(sources, 4, Date.now(), 60, failures, OPTS); + expect(r.dispatch.map(s => s.id)).toEqual(['b']); + expect(r.skippedCooldown.map(s => s.id)).toEqual(['a']); + }); +}); + +describe('readRecentSourceFailures + isSourceInCooldown (PGLite)', () => { + let engine: PGLiteEngine; + beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); + }, 30000); + afterAll(async () => { await engine.disconnect(); }); + beforeEach(async () => { await resetPgliteState(engine); }); + + async function addJob(status: string, sourceId: string | null, finishedMinAgo: number): Promise { + const finished = new Date(Date.now() - finishedMinAgo * 60_000).toISOString(); + const data = sourceId === null ? {} : { source_id: sourceId }; + await engine.executeRaw( + `INSERT INTO minion_jobs (name, status, data, finished_at) VALUES ('autopilot-cycle', $1, $2, $3)`, + [status, data, finished], + ); + } + + test('groups dead/failed jobs by source with count + max(finished_at)', async () => { + await addJob('dead', 'repo-a', 5); + await addJob('failed', 'repo-a', 2); + await addJob('dead', 'repo-b', 10); + await addJob('completed', 'repo-a', 1); // not counted + const map = await readRecentSourceFailures(engine, { sinceMin: 120 }); + expect(map.get('repo-a')?.count).toBe(2); + expect(map.get('repo-b')?.count).toBe(1); + expect(map.has('repo-a')).toBe(true); + // last failed is the most recent of the two (2 min ago). + const lastA = map.get('repo-a')!.lastFailedAt.getTime(); + expect(Date.now() - lastA).toBeLessThan(4 * 60_000); + }); + + test('null source_id rows are excluded (codex #6)', async () => { + await addJob('dead', null, 3); + await addJob('dead', 'repo-c', 3); + const map = await readRecentSourceFailures(engine, { sinceMin: 120 }); + expect(map.has('repo-c')).toBe(true); + expect([...map.keys()].some(k => !k)).toBe(false); + expect(map.size).toBe(1); + }); + + test('failures older than the window are not counted', async () => { + await addJob('dead', 'repo-old', 500); // way outside a 120min window + const map = await readRecentSourceFailures(engine, { sinceMin: 120 }); + expect(map.has('repo-old')).toBe(false); + }); + + test('isSourceInCooldown: recent failure → true; cleared after a success stamp', async () => { + await engine.executeRaw( + `INSERT INTO sources (id, name, config, created_at) VALUES ('repo-cd', 'r', '{}'::jsonb, now())`, [], + ); + await addJob('dead', 'repo-cd', 1); // 1 min ago, count 1 → 10min cooldown + expect(await isSourceInCooldown(engine, 'repo-cd')).toBe(true); + + // Operator repairs + a successful cycle stamps last_source_cycle_at NOW. + await engine.updateSourceConfig('repo-cd', { last_source_cycle_at: new Date().toISOString() }); + expect(await isSourceInCooldown(engine, 'repo-cd')).toBe(false); + }); + + test('isSourceInCooldown returns false when cooldown disabled (failure_cooldown_min=0)', async () => { + await engine.setConfig('autopilot.failure_cooldown_min', '0'); + await addJob('dead', 'repo-dis', 1); + expect(await isSourceInCooldown(engine, 'repo-dis')).toBe(false); + const opts = await resolveFailureCooldownOpts(engine); + expect(opts.baseMin).toBe(0); + }); +}); diff --git a/test/autopilot-fanout-clamp.serial.test.ts b/test/autopilot-fanout-clamp.serial.test.ts new file mode 100644 index 000000000..f4b1df3e5 --- /dev/null +++ b/test/autopilot-fanout-clamp.serial.test.ts @@ -0,0 +1,110 @@ +/** + * #2194 fix #1 (codex #9 / D5): resolveEffectiveFanoutMax clamps the per-tick + * fan-out to the worker's effective concurrency (max(1, concurrency-1), + * reserving ≥1 slot) — but ONLY when a LIVE supervisor holds the queue lock. + * A stale `started` audit row must not shrink throughput for a supervisor that + * isn't running that config, so with no live holder the clamp is skipped and + * the unclamped base is used. + */ + +import { describe, test, expect, beforeAll, afterAll, beforeEach, afterEach } from 'bun:test'; +import { mkdtempSync, writeFileSync, rmSync } from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { tryAcquireDbLock } from '../src/core/db-lock.ts'; +import { supervisorLockId, SUPERVISOR_LOCK_TTL_MIN } from '../src/core/minions/supervisor.ts'; +import { computeSupervisorAuditFilename } from '../src/core/minions/handlers/supervisor-audit.ts'; +import { resolveEffectiveFanoutMax } from '../src/commands/autopilot-fanout.ts'; + +let engine: PGLiteEngine; +let auditDir: string; +const prevAuditDir = process.env.GBRAIN_AUDIT_DIR; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}, 30000); + +afterAll(async () => { + await engine.disconnect(); +}); + +beforeEach(async () => { + auditDir = mkdtempSync(join(tmpdir(), 'gbrain-clamp-')); + process.env.GBRAIN_AUDIT_DIR = auditDir; + await engine.executeRaw(`DELETE FROM gbrain_cycle_locks WHERE id LIKE 'gbrain-supervisor:%'`); + // base fan-out: override to 8 so the clamp's effect is visible on PGLite + // (whose natural default is 1). The clamp logic is engine-agnostic. + await engine.setConfig('autopilot.fanout_max_per_tick', '8'); + await engine.setConfig('autopilot.fanout_clamp_to_concurrency', 'true'); +}); + +afterEach(() => { + if (prevAuditDir === undefined) delete process.env.GBRAIN_AUDIT_DIR; + else process.env.GBRAIN_AUDIT_DIR = prevAuditDir; + try { rmSync(auditDir, { recursive: true, force: true }); } catch { /* noop */ } +}); + +function writeStarted(concurrency: number): void { + const file = join(auditDir, computeSupervisorAuditFilename()); + writeFileSync(file, JSON.stringify({ + event: 'started', ts: new Date().toISOString(), supervisor_pid: 4242, + queue: 'default', concurrency, + }) + '\n', 'utf8'); +} + +describe('resolveEffectiveFanoutMax — clamp gated on live supervisor (#2194/codex #9)', () => { + test('NO live holder → no clamp (stale audit row cannot shrink throughput)', async () => { + writeStarted(3); // audit says concurrency 3, but no live lock holder + const n = await resolveEffectiveFanoutMax(engine, 'default'); + expect(n).toBe(8); // unclamped base + }); + + test('live holder + concurrency 3 → clamp to max(1, 3-1) = 2', async () => { + writeStarted(3); + const holder = await tryAcquireDbLock(engine, supervisorLockId('default'), SUPERVISOR_LOCK_TTL_MIN); + expect(holder).not.toBeNull(); + try { + const n = await resolveEffectiveFanoutMax(engine, 'default'); + expect(n).toBe(2); + } finally { + await holder!.release(); + } + }); + + test('live holder but clamp disabled → unclamped base', async () => { + await engine.setConfig('autopilot.fanout_clamp_to_concurrency', 'false'); + writeStarted(3); + const holder = await tryAcquireDbLock(engine, supervisorLockId('default'), SUPERVISOR_LOCK_TTL_MIN); + try { + const n = await resolveEffectiveFanoutMax(engine, 'default'); + expect(n).toBe(8); + } finally { + await holder!.release(); + } + }); + + test('live holder + concurrency 1 → floor at 1 (never below 1)', async () => { + writeStarted(1); + const holder = await tryAcquireDbLock(engine, supervisorLockId('default'), SUPERVISOR_LOCK_TTL_MIN); + try { + const n = await resolveEffectiveFanoutMax(engine, 'default'); + expect(n).toBe(1); + } finally { + await holder!.release(); + } + }); + + test('live holder but no started event (concurrency unknown) → no clamp', async () => { + // lock row exists but audit has no concurrency → fall back to base. + const holder = await tryAcquireDbLock(engine, supervisorLockId('default'), SUPERVISOR_LOCK_TTL_MIN); + try { + const n = await resolveEffectiveFanoutMax(engine, 'default'); + expect(n).toBe(8); + } finally { + await holder!.release(); + } + }); +}); diff --git a/test/autopilot-fanout-wiring.test.ts b/test/autopilot-fanout-wiring.test.ts index cb783aefe..addf8dbe3 100644 --- a/test/autopilot-fanout-wiring.test.ts +++ b/test/autopilot-fanout-wiring.test.ts @@ -28,8 +28,11 @@ describe('autopilot.ts ↔ dispatchPerSource wiring', () => { ); }); - test('imports resolveFanoutMax (so PGLite gets fanoutMax=1 per codex P1-3)', () => { - expect(AUTOPILOT_SRC).toMatch(/resolveFanoutMax/); + test('imports resolveEffectiveFanoutMax (clamps to worker concurrency; PGLite base still 1)', () => { + // #2194 fix #1: autopilot now resolves the CLAMPED fan-out (gated on a live + // supervisor) instead of the raw resolveFanoutMax. The clamp wraps + // resolveFanoutMax, so PGLite's base-1 still holds (codex P1-3). + expect(AUTOPILOT_SRC).toMatch(/resolveEffectiveFanoutMax/); }); test('calls dispatchPerSource within the shouldFullCycle branch', () => { diff --git a/test/autopilot-global-maintenance.test.ts b/test/autopilot-global-maintenance.test.ts new file mode 100644 index 000000000..0ec76f290 --- /dev/null +++ b/test/autopilot-global-maintenance.test.ts @@ -0,0 +1,146 @@ +/** + * #2194 fix #3 / #2227 bug #3 — the cycle split. + * + * Per-source autopilot cycles run ONLY source-scoped (+ mixed) phases; the + * brain-wide `global` phases run ONCE in a separate autopilot-global-maintenance + * job. This replaces the rejected skip-and-stamp-fresh design (codex #1/#2): the + * split makes single-flight structural (one global job, not N concurrent embeds) + * and never marks a source "fresh" for global work it didn't do. These tests pin + * the phase partition, the dispatch gate, the per-source phase set, and the + * global handler stamping autopilot.last_global_at. + */ + +import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; +import { registerBuiltinHandlers } from '../src/commands/jobs.ts'; +import { + ALL_PHASES, + GLOBAL_PHASES, + NON_GLOBAL_PHASES, + PHASE_SCOPE, + LAST_GLOBAL_AT_KEY, +} from '../src/core/cycle.ts'; +import { + dispatchGlobalMaintenance, + isGlobalMaintenanceStale, + dispatchPerSource, +} from '../src/commands/autopilot-fanout.ts'; +import type { BrainEngine } from '../src/core/engine.ts'; + +describe('cycle phase partition (#2194 fix #3)', () => { + test('GLOBAL ∪ NON_GLOBAL == ALL_PHASES, no overlap', () => { + const union = new Set([...GLOBAL_PHASES, ...NON_GLOBAL_PHASES]); + expect(union.size).toBe(ALL_PHASES.length); + for (const p of ALL_PHASES) expect(union.has(p)).toBe(true); + // No phase in both. + const overlap = GLOBAL_PHASES.filter((p) => NON_GLOBAL_PHASES.includes(p)); + expect(overlap).toEqual([]); + }); + + test('every GLOBAL phase is PHASE_SCOPE==="global"; embed is global, lint is not', () => { + for (const p of GLOBAL_PHASES) expect(PHASE_SCOPE[p]).toBe('global'); + expect(GLOBAL_PHASES).toContain('embed'); + expect(GLOBAL_PHASES).toContain('orphans'); + expect(GLOBAL_PHASES).toContain('purge'); + expect(NON_GLOBAL_PHASES).toContain('lint'); + expect(NON_GLOBAL_PHASES).toContain('sync'); + expect(NON_GLOBAL_PHASES).not.toContain('embed'); + }); +}); + +describe('isGlobalMaintenanceStale', () => { + const now = Date.UTC(2026, 5, 16, 12, 0, 0); + test('null/unparseable → stale (must run)', () => { + expect(isGlobalMaintenanceStale(null, now)).toBe(true); + expect(isGlobalMaintenanceStale('not-a-date', now)).toBe(true); + }); + test('older than floor → stale; within floor → fresh', () => { + expect(isGlobalMaintenanceStale(new Date(now - 61 * 60_000).toISOString(), now, 60)).toBe(true); + expect(isGlobalMaintenanceStale(new Date(now - 10 * 60_000).toISOString(), now, 60)).toBe(false); + }); +}); + +describe('dispatchGlobalMaintenance — single-flight gate', () => { + function stubs(lastGlobalAt: string | null) { + const added: Array<{ name: string; data: any; opts: any }> = []; + const engine = { + kind: 'postgres' as const, + getConfig: async (k: string) => (k === LAST_GLOBAL_AT_KEY ? lastGlobalAt : null), + } as unknown as BrainEngine; + const queue = { + add: async (name: string, data: unknown, opts: Record) => { + added.push({ name, data, opts }); return { id: 1 }; + }, + } as any; + return { engine, queue, added }; + } + + test('stale (never run) → dispatches one global job with single-flight opts', async () => { + const { engine, queue, added } = stubs(null); + const r = await dispatchGlobalMaintenance(engine, queue, { repoPath: '/tmp', slot: 's1', timeoutMs: 1, jsonMode: true, emit: () => {} }); + expect(r.dispatched).toBe(true); + expect(added.length).toBe(1); + expect(added[0].name).toBe('autopilot-global-maintenance'); + expect(added[0].opts.idempotency_key).toBe('autopilot-global:s1'); + expect(added[0].opts.maxWaiting).toBe(1); // structural single-flight + expect(added[0].data.phases).toEqual(GLOBAL_PHASES); + }); + + test('fresh → does NOT dispatch', async () => { + const { engine, queue, added } = stubs(new Date().toISOString()); + const r = await dispatchGlobalMaintenance(engine, queue, { repoPath: '/tmp', slot: 's1', timeoutMs: 1, jsonMode: true, emit: () => {} }); + expect(r.dispatched).toBe(false); + expect(added.length).toBe(0); + }); +}); + +describe('dispatchPerSource — per-source jobs carry NON_GLOBAL phases (no embed)', () => { + test('each per-source job sets phases = NON_GLOBAL_PHASES', async () => { + const sources = [{ id: 'repo-a', name: 'a', config: {} }, { id: 'repo-b', name: 'b', config: {} }]; + const added: any[] = []; + const engine = { + kind: 'postgres' as const, + listAllSources: async () => sources, + getConfig: async () => null, + executeRaw: async () => [], + } as unknown as BrainEngine; + const queue = { add: async (name: string, data: unknown, opts: unknown) => { added.push({ name, data, opts }); return { id: added.length }; } } as any; + await dispatchPerSource(engine, queue, { repoPath: '/tmp', slot: 's', timeoutMs: 1, fanoutMax: 4, jsonMode: true, emit: () => {}, log: () => {} }); + expect(added.length).toBe(2); + for (const j of added) { + expect(j.data.phases).toEqual(NON_GLOBAL_PHASES); + expect(j.data.phases).not.toContain('embed'); + } + }); +}); + +describe('autopilot-global-maintenance handler stamps last_global_at (PGLite)', () => { + let engine: PGLiteEngine; + beforeAll(async () => { engine = new PGLiteEngine(); await engine.connect({}); await engine.initSchema(); }, 30000); + afterAll(async () => { await engine.disconnect(); }); + beforeEach(async () => { await resetPgliteState(engine); }); + + async function captureHandlers() { + const handlers = new Map Promise>(); + const fakeWorker = { register(name: string, fn: (job: any) => Promise) { handlers.set(name, fn); } }; + await registerBuiltinHandlers(fakeWorker as never, engine); + return handlers; + } + + test('runs global phases (no source_id) and stamps autopilot.last_global_at on success', async () => { + expect(await engine.getConfig(LAST_GLOBAL_AT_KEY)).toBeNull(); + const handlers = await captureHandlers(); + const handler = handlers.get('autopilot-global-maintenance'); + expect(handler).toBeTruthy(); + + const result = await handler!({ data: { phases: ['orphans', 'embed'] }, signal: undefined }); + // The cycle ran the requested global phases (DB-only on an empty brain). + expect(result.report.phases.some((p: any) => p.phase === 'orphans')).toBe(true); + expect(['ok', 'clean', 'partial']).toContain(result.report.status); + // Freshness stamped so the dispatch gate backs off. + const stamped = await engine.getConfig(LAST_GLOBAL_AT_KEY); + expect(stamped).not.toBeNull(); + expect(Number.isFinite(new Date(stamped!).getTime())).toBe(true); + }); +}); diff --git a/test/child-worker-supervisor.test.ts b/test/child-worker-supervisor.test.ts index 6b85a6527..37d2e8dc1 100644 --- a/test/child-worker-supervisor.test.ts +++ b/test/child-worker-supervisor.test.ts @@ -50,6 +50,7 @@ async function runUntilTerminal( h: Harness, overrides: Partial<{ maxCrashes: number; + hardStopMaxCrashes: number; _backoffFloorMs: number; cleanRestartBudget: number; cleanRestartWindowMs: number; @@ -71,6 +72,7 @@ async function runUntilTerminal( cliPath: h.workerScript, args: [], maxCrashes: overrides.maxCrashes ?? 3, + hardStopMaxCrashes: overrides.hardStopMaxCrashes, _backoffFloorMs: overrides._backoffFloorMs ?? 5, cleanRestartBudget: overrides.cleanRestartBudget, cleanRestartWindowMs: overrides.cleanRestartWindowMs, @@ -159,12 +161,15 @@ if [ $((NEXT % 2)) -eq 1 ]; then exit 1; else exit 0; fi try { const res = await runUntilTerminal(h, { maxCrashes: 3, + // issue #1994: the soft budget no longer gives up; pin the hard + // ceiling to 3 so this counting test still fires give-up at 3. + hardStopMaxCrashes: 3, _backoffFloorMs: 5, stopAfterEvents: 200, }); expect(res.maxCrashesFired).not.toBeNull(); - // 3 code!=0 exits → max_crashes=3 + // 3 code!=0 exits → hard ceiling=3 expect(res.maxCrashesFired!.count).toBe(3); const exits = res.events.filter((e) => e.kind === 'worker_exited'); @@ -235,6 +240,7 @@ esac const res = await runUntilTerminal(h, { maxCrashes: 3, + hardStopMaxCrashes: 3, // issue #1994: pin give-up to 3 for this counting test _backoffFloorMs: 5, _now: fakeNow, stopAfterEvents: 200, @@ -471,6 +477,66 @@ esac }); }); + // issue #1994 (#2227 tail): crossing the SOFT crash budget no longer + // permanently gives up. The supervisor enters degraded mode (capped backoff + // + loud warn) so a transient outage self-heals; permanent give-up fires only + // at the much-higher hard ceiling. + describe('degraded-mode crash backoff (issue #1994)', () => { + it('crossing the soft budget does NOT give up; it warns and keeps retrying to the hard ceiling', async () => { + const h = makeHarness('degraded-softbudget', 'exit 1'); + try { + const { events, maxCrashesFired } = await runUntilTerminal(h, { + maxCrashes: 3, // soft budget + hardStopMaxCrashes: 6, // hard ceiling + _backoffFloorMs: 1, + stopAfterEvents: 200, + }); + // Permanent give-up fired at the HARD ceiling (6), not the soft budget (3). + expect(maxCrashesFired).not.toBeNull(); + expect(maxCrashesFired!.count).toBe(6); + expect(maxCrashesFired!.max).toBe(6); + + // The soft-budget crossing announced degraded mode (at least once). + const degraded = events.filter( + (e): e is Extract => + e.kind === 'health_warn' && e.reason === 'crash_budget_degraded', + ); + expect(degraded.length).toBeGreaterThanOrEqual(1); + expect(degraded[0].max).toBe(3); + expect(degraded[0].count).toBeGreaterThanOrEqual(3); + + // It kept respawning past the soft budget (more than 3 crash exits). + const crashes = events.filter( + (e): e is Extract => + e.kind === 'worker_exited' && e.code === 1, + ); + expect(crashes.length).toBe(6); + } finally { + h.cleanup(); + } + }); + + it('hardStopMaxCrashes=0 disables permanent give-up (retry-forever-with-backoff)', async () => { + const h = makeHarness('degraded-noforever', 'exit 1'); + try { + const { events, maxCrashesFired } = await runUntilTerminal(h, { + maxCrashes: 3, + hardStopMaxCrashes: 0, // never permanently stop + _backoffFloorMs: 1, + stopAfterEvents: 40, // the safety net stops the test, not a give-up + }); + // Never gave up despite many crashes past the soft budget. + expect(maxCrashesFired).toBeNull(); + const crashes = events.filter( + (e) => e.kind === 'worker_exited' && (e as any).code === 1, + ); + expect(crashes.length).toBeGreaterThan(3); + } finally { + h.cleanup(); + } + }); + }); + describe('issue #1801 — restartCurrentChild + killChild liveness fix', () => { const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); // ESRCH = no such process (dead). EPERM = process exists but we can't diff --git a/test/db-lock-inspect.test.ts b/test/db-lock-inspect.test.ts index 61270f992..705575fe2 100644 --- a/test/db-lock-inspect.test.ts +++ b/test/db-lock-inspect.test.ts @@ -21,6 +21,8 @@ import { inspectLock, listStaleLocks, deleteLockRow, + liveSyncStatus, + syncLockId, } from '../src/core/db-lock.ts'; let engine: PGLiteEngine; @@ -183,3 +185,39 @@ describe('deleteLockRow', () => { await handle!.release(); }); }); + +describe('liveSyncStatus (#1950)', () => { + test('returns null when the source holds no lock (idle)', async () => { + const live = await liveSyncStatus(engine, 'never-synced'); + expect(live).toBeNull(); + }); + + test('returns the holder when a live (non-expired) sync lock is held', async () => { + const handle = await tryAcquireDbLock(engine, syncLockId('busy-source')); + expect(handle).not.toBeNull(); + const live = await liveSyncStatus(engine, 'busy-source'); + expect(live).not.toBeNull(); + expect(live!.holder_pid).toBe(process.pid); + expect(typeof live!.holder_host).toBe('string'); + expect(live!.holder_host.length).toBeGreaterThan(0); + await handle!.release(); + }); + + test('returns null for a stale (TTL-expired) lock — structurally available, not running', async () => { + await (engine as any).db.query( + `INSERT INTO gbrain_cycle_locks (id, holder_pid, holder_host, acquired_at, ttl_expires_at) + VALUES ($1, $2, $3, NOW() - INTERVAL '2 hours', NOW() - INTERVAL '1 hour')`, + [syncLockId('wedged-dead'), 31337, 'old-host'], + ); + const live = await liveSyncStatus(engine, 'wedged-dead'); + expect(live).toBeNull(); + }); + + test('is scoped per source — one source live does not mark another running', async () => { + const handle = await tryAcquireDbLock(engine, syncLockId('source-a')); + expect(handle).not.toBeNull(); + expect(await liveSyncStatus(engine, 'source-a')).not.toBeNull(); + expect(await liveSyncStatus(engine, 'source-b')).toBeNull(); + await handle!.release(); + }); +}); diff --git a/test/doctor-autopilot-fanout-concurrency.serial.test.ts b/test/doctor-autopilot-fanout-concurrency.serial.test.ts new file mode 100644 index 000000000..a52571a9c --- /dev/null +++ b/test/doctor-autopilot-fanout-concurrency.serial.test.ts @@ -0,0 +1,84 @@ +/** + * #2194 fix #5: `gbrain doctor` warns when autopilot's per-tick fan-out exceeds + * the worker's effective concurrency. Fanning out more cycles than there are + * worker slots guarantees waiters that race the stalled-sweeper — a silent + * misconfig the operator never saw before this check. + * + * Drives computeAutopilotFanoutConcurrencyCheck directly with a fake engine so + * the fan-out (config) and concurrency (audit) inputs are controllable without + * spawning a supervisor. The audit read is stubbed via GBRAIN_AUDIT_DIR + a + * hand-written started event. + */ + +import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import { mkdtempSync, writeFileSync, rmSync } from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; +import { computeAutopilotFanoutConcurrencyCheck } from '../src/commands/doctor.ts'; +import { computeSupervisorAuditFilename } from '../src/core/minions/handlers/supervisor-audit.ts'; + +// Minimal fake engine: postgres-kind + a config map for fanout override. +function fakeEngine(config: Record = {}) { + return { + kind: 'postgres' as const, + getConfig: async (k: string) => config[k] ?? null, + } as any; +} + +let auditDir: string; +const prevAuditDir = process.env.GBRAIN_AUDIT_DIR; + +beforeEach(() => { + auditDir = mkdtempSync(join(tmpdir(), 'gbrain-fanout-doctor-')); + process.env.GBRAIN_AUDIT_DIR = auditDir; +}); + +afterEach(() => { + if (prevAuditDir === undefined) delete process.env.GBRAIN_AUDIT_DIR; + else process.env.GBRAIN_AUDIT_DIR = prevAuditDir; + try { rmSync(auditDir, { recursive: true, force: true }); } catch { /* noop */ } +}); + +/** Write a `started` audit event with the given concurrency for queue 'default'. */ +function writeStarted(concurrency: number): void { + const file = join(auditDir, computeSupervisorAuditFilename()); + const line = JSON.stringify({ + event: 'started', + ts: new Date().toISOString(), + supervisor_pid: 4242, + queue: 'default', + concurrency, + }); + writeFileSync(file, line + '\n', 'utf8'); +} + +describe('computeAutopilotFanoutConcurrencyCheck (#2194 fix #5)', () => { + test('warns when fan-out (4) exceeds effective slots (concurrency 2 → 1)', async () => { + writeStarted(2); + const check = await computeAutopilotFanoutConcurrencyCheck(fakeEngine()); + expect(check.status).toBe('warn'); + expect(check.message).toContain('exceeds worker concurrency'); + expect(check.details).toMatchObject({ fanout_max: 4, concurrency: 2, effective_slots: 1 }); + }); + + test('ok when fan-out fits (override 1, concurrency 4)', async () => { + writeStarted(4); + const check = await computeAutopilotFanoutConcurrencyCheck( + fakeEngine({ 'autopilot.fanout_max_per_tick': '1' }), + ); + expect(check.status).toBe('ok'); + }); + + test('ok/skip when no supervisor has ever started (no noise on unsupervised brains)', async () => { + // No started event written. + const check = await computeAutopilotFanoutConcurrencyCheck(fakeEngine()); + expect(check.status).toBe('ok'); + expect(check.message).toContain('No supervisor observed'); + }); + + test('PGLite short-circuits (single-writer, fan-out is 1)', async () => { + const check = await computeAutopilotFanoutConcurrencyCheck({ kind: 'pglite', getConfig: async () => null } as any); + expect(check.status).toBe('ok'); + expect(check.message).toContain('PGLite'); + }); +}); diff --git a/test/e2e/autopilot-cooldown-parity.test.ts b/test/e2e/autopilot-cooldown-parity.test.ts new file mode 100644 index 000000000..b99c837b1 --- /dev/null +++ b/test/e2e/autopilot-cooldown-parity.test.ts @@ -0,0 +1,62 @@ +/** + * #2194 fix #2 — engine-parity for the failure-cooldown query. + * + * readRecentSourceFailures runs ONE SQL through engine.executeRaw (the + * engine-agnostic path), so PGLite and Postgres must return identical + * groupings. This pins that: it seeds the SAME dead/failed autopilot-cycle rows + * into both engines and asserts the per-source counts + the null-source + * exclusion (codex #6) match. PGLite always runs; Postgres runs only when + * DATABASE_URL is set (mirrors engine-parity.test.ts's gating). + */ + +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { PGLiteEngine } from '../../src/core/pglite-engine.ts'; +import type { BrainEngine } from '../../src/core/engine.ts'; +import { hasDatabase, setupDB, teardownDB, getEngine } from './helpers.ts'; +import { readRecentSourceFailures } from '../../src/commands/autopilot-fanout.ts'; + +const SKIP_PG = !hasDatabase(); + +async function seed(engine: BrainEngine): Promise { + const rows: Array<[string, Record, number]> = [ + ['dead', { source_id: 'repo-a' }, 5], + ['failed', { source_id: 'repo-a' }, 2], + ['dead', { source_id: 'repo-b' }, 10], + ['completed', { source_id: 'repo-a' }, 1], // excluded (not a failure) + ['dead', {}, 3], // excluded (null source_id, codex #6) + ]; + for (const [status, data, minAgo] of rows) { + await engine.executeRaw( + `INSERT INTO minion_jobs (name, status, data, finished_at) VALUES ('autopilot-cycle', $1, $2, $3)`, + [status, data, new Date(Date.now() - minAgo * 60_000).toISOString()], + ); + } +} + +function summarize(map: Map): Record { + const out: Record = {}; + for (const [k, v] of map) out[k] = v.count; + return out; +} + +describe('failure-cooldown query — PGLite', () => { + let engine: PGLiteEngine; + beforeAll(async () => { engine = new PGLiteEngine(); await engine.connect({}); await engine.initSchema(); await seed(engine); }, 30000); + afterAll(async () => { await engine.disconnect(); }); + + test('groups failures by source, excludes completed + null-source', async () => { + const map = await readRecentSourceFailures(engine, { sinceMin: 120 }); + expect(summarize(map)).toEqual({ 'repo-a': 2, 'repo-b': 1 }); + }); +}); + +(SKIP_PG ? describe.skip : describe)('failure-cooldown query — Postgres parity', () => { + let engine: BrainEngine; + beforeAll(async () => { await setupDB(); engine = await getEngine(); await seed(engine); }, 60000); + afterAll(async () => { await teardownDB(); }); + + test('Postgres returns the SAME groupings as PGLite', async () => { + const map = await readRecentSourceFailures(engine, { sinceMin: 120 }); + expect(summarize(map)).toEqual({ 'repo-a': 2, 'repo-b': 1 }); + }); +}); diff --git a/test/e2e/minions-resilience.test.ts b/test/e2e/minions-resilience.test.ts index c10dcfbc9..21614fdd5 100644 --- a/test/e2e/minions-resilience.test.ts +++ b/test/e2e/minions-resilience.test.ts @@ -138,6 +138,10 @@ describeE2E('E2E: Minions resilience (OpenClaw real-world patterns)', () => { const final = await queue.getJob(job.id); expect(final?.error_text).toMatch(/timeout exceeded/i); + // #1737 regression: the runaway run is counted as a spent attempt by + // handleTimeouts (it's the first killer to fire), so accounting reads + // honestly instead of `attempts: 0/1 (started: 1)`. + expect(final?.attempts_made).toBe(1); } finally { await a.disconnect(); await b.disconnect(); diff --git a/test/e2e/status-pglite.test.ts b/test/e2e/status-pglite.test.ts index e9ba80dd6..139f0db4f 100644 --- a/test/e2e/status-pglite.test.ts +++ b/test/e2e/status-pglite.test.ts @@ -82,6 +82,11 @@ describe('gbrain status E2E (PGLite)', () => { expect(parsed).toHaveProperty('workers'); expect(parsed).toHaveProperty('queue'); expect(parsed).toHaveProperty('autopilot'); + // #1984: version field present (local CLI version) for poller pinning; + // a normal (un-budgeted) run is never partial. + expect(typeof parsed.version).toBe('string'); + expect(parsed.version.length).toBeGreaterThan(0); + expect(parsed.partial).toBeUndefined(); }); test('dual cycle rows: last_full + last_targeted surface independently', async () => { diff --git a/test/get-status-snapshot-op.test.ts b/test/get-status-snapshot-op.test.ts index 38fd3c6ae..b07d8c5a0 100644 --- a/test/get-status-snapshot-op.test.ts +++ b/test/get-status-snapshot-op.test.ts @@ -64,6 +64,8 @@ describe('get_status_snapshot handler shape', () => { expect(result.schema_version).toBe(1); expect(result).toHaveProperty('sync'); expect(result).toHaveProperty('cycle'); + // #1984: the op now also reports the brain server's version (thin-client parity). + expect(typeof result.version).toBe('string'); expect(result).not.toHaveProperty('locks'); expect(result).not.toHaveProperty('workers'); expect(result).not.toHaveProperty('queue'); diff --git a/test/minions.test.ts b/test/minions.test.ts index ec990406a..012e91bdd 100644 --- a/test/minions.test.ts +++ b/test/minions.test.ts @@ -1300,6 +1300,12 @@ describe('MinionQueue: handleTimeouts', () => { const dead = await queue.getJob(job.id); expect(dead!.status).toBe('dead'); expect(dead!.error_text).toBe('timeout exceeded'); + // #1737 regression: the timed-out run counts as a spent attempt, mirroring + // the wall-clock + stall dead-letter paths. Without this the job reads + // `attempts: 0/N (started: N)`. Asserted on both the RETURNING row and the + // persisted row so a future refactor can't silently drop the increment. + expect(timedOut[0].attempts_made).toBe(1); + expect(dead!.attempts_made).toBe(1); }); test('handleTimeouts ignores stalled jobs (lock_until > now guard)', async () => { diff --git a/test/status-sections.test.ts b/test/status-sections.test.ts index 5de48dbac..ab36820fd 100644 --- a/test/status-sections.test.ts +++ b/test/status-sections.test.ts @@ -14,7 +14,65 @@ */ import { describe, test, expect } from 'bun:test'; -import { parseSectionFlag, runStatus } from '../src/commands/status.ts'; +import { + parseSectionFlag, + parseDeadlineFlag, + withSectionDeadline, + runStatus, + FAST_DEADLINE_MS, +} from '../src/commands/status.ts'; + +describe('parseDeadlineFlag (#1984)', () => { + test('no flag → undefined (no budget)', () => { + expect(parseDeadlineFlag([])).toBeUndefined(); + expect(parseDeadlineFlag(['--json'])).toBeUndefined(); + }); + + test('--fast applies the preset budget', () => { + expect(parseDeadlineFlag(['--fast'])).toBe(FAST_DEADLINE_MS); + }); + + test('--deadline-ms in both forms', () => { + expect(parseDeadlineFlag(['--deadline-ms', '500'])).toBe(500); + expect(parseDeadlineFlag(['--deadline-ms=750'])).toBe(750); + }); + + test('explicit --deadline-ms wins over --fast', () => { + expect(parseDeadlineFlag(['--fast', '--deadline-ms=100'])).toBe(100); + }); + + test('non-positive / non-numeric → usage_error', () => { + expect(parseDeadlineFlag(['--deadline-ms', '0'])).toBe('usage_error'); + expect(parseDeadlineFlag(['--deadline-ms', '-5'])).toBe('usage_error'); + expect(parseDeadlineFlag(['--deadline-ms', 'soon'])).toBe('usage_error'); + }); + + test('bare --deadline-ms with no value → usage_error (not a silent no-budget fallthrough)', () => { + expect(parseDeadlineFlag(['--deadline-ms'])).toBe('usage_error'); + expect(parseDeadlineFlag(['--fast', '--deadline-ms'])).toBe('usage_error'); + }); +}); + +describe('withSectionDeadline (#1984)', () => { + test('resolves the value when it beats the budget', async () => { + let timedOut = false; + const v = await withSectionDeadline(Promise.resolve(42), 1000, () => { timedOut = true; }); + expect(v).toBe(42); + expect(timedOut).toBe(false); + }); + + test('returns undefined + fires onTimeout when the budget elapses', async () => { + let timedOut = false; + const v = await withSectionDeadline(new Promise(() => {}), 10, () => { timedOut = true; }); + expect(v).toBeUndefined(); + expect(timedOut).toBe(true); + }); + + test('no budget (undefined/<=0) awaits the promise as-is', async () => { + expect(await withSectionDeadline(Promise.resolve('x'), undefined, () => {})).toBe('x'); + expect(await withSectionDeadline(Promise.resolve('y'), 0, () => {})).toBe('y'); + }); +}); describe('parseSectionFlag', () => { test('no --section flag → undefined (all sections)', () => { @@ -73,4 +131,14 @@ describe('runStatus exit codes', () => { expect(r.exitCode).toBe(1); expect(captured).toMatch(/snapshot failed|no engine connected/); }); + + test('#1984: invalid --deadline-ms → exit 2 (usage error)', async () => { + let captured = ''; + const r = await runStatus(null, ['--deadline-ms', '0'], { + stdout: () => {}, + stderr: (s: string) => { captured += s; }, + }); + expect(r.exitCode).toBe(2); + expect(captured).toContain('--deadline-ms'); + }); }); diff --git a/test/supervisor-audit.test.ts b/test/supervisor-audit.test.ts index b32d21186..1c79b3863 100644 --- a/test/supervisor-audit.test.ts +++ b/test/supervisor-audit.test.ts @@ -199,3 +199,45 @@ describe('summarizeCrashes — aggregation', () => { expect(summary.clean_exits).toBe(0); }); }); + +// issue #2227 bug #1/#2: a duplicate supervisor (different $HOME / --pid-file) +// passes the pidfile guard but loses the queue-scoped DB singleton lock +// (#1849) and `process.exit(ExitCodes.LOCK_HELD)`s. That fence-exit happens in +// supervisor.ts:start() BEFORE the worker is ever spawned and BEFORE the +// `started` event is emitted (the DB-lock acquire at :528 precedes the +// `started` emit at :555), so the loser contributes NO `worker_exited` events +// to the audit trail. The crash ledger (summarizeCrashes) moves ONLY on +// `worker_exited`, so a fence-collision can never burn the crash budget the +// guard exists to protect. The field report claimed 20 "unknown" crashes were +// fence exits; this pins that the fence path is structurally uncountable, so a +// future refactor that (wrongly) logs a `worker_exited` on the LOCK_HELD path +// would fail here instead of silently re-introducing the breaker-trip loop. +describe('summarizeCrashes — LOCK_HELD fence-exit is never a crash (issue #2227)', () => { + test('a losing duplicate supervisor (no worker_exited) contributes zero crashes', () => { + // The loser exits at the DB-lock fence before emitting `started`, so its + // audit contribution is empty. Even paired with the winner's healthy + // stream, total crashes stay 0. + const winnerHealthy: SupervisorEmission[] = [ + evt('started', { supervisor_pid: 4242, queue: 'default', concurrency: 3 }), + evt('worker_spawned', { worker_pid: 4243 }), + // ... no worker_exited yet (winner still running) + ]; + expect(summarizeCrashes(winnerHealthy).total).toBe(0); + }); + + test('started + stopped with no worker_exited (clean fence path) is zero crashes', () => { + // Defensive: even if a future change made the loser emit lifecycle events + // (started/shutting_down/stopped) around the LOCK_HELD exit, none of those + // are `worker_exited`, so the ledger must stay at 0. The only way this test + // fails is if someone logs a `worker_exited` on the fence path — which is + // exactly the regression #2227 fix #2 guards against. + const fenceStream: SupervisorEmission[] = [ + evt('started', { supervisor_pid: 5252 }), + evt('shutting_down', { reason: 'LOCK_HELD' }), + evt('stopped', { exit_code: 2 }), + ]; + const summary = summarizeCrashes(fenceStream); + expect(summary.total).toBe(0); + expect(summary.clean_exits).toBe(0); + }); +}); diff --git a/test/supervisor-db-lock.test.ts b/test/supervisor-db-lock.test.ts index 31829443f..f7b9afbbe 100644 --- a/test/supervisor-db-lock.test.ts +++ b/test/supervisor-db-lock.test.ts @@ -14,9 +14,26 @@ import { existsSync, unlinkSync } from 'fs'; import { join } from 'path'; import { tmpdir } from 'os'; import { PGLiteEngine } from '../src/core/pglite-engine.ts'; -import { tryAcquireDbLock } from '../src/core/db-lock.ts'; -import { MinionSupervisor, ExitCodes, supervisorLockId, classifySupervisorSingleton } from '../src/core/minions/supervisor.ts'; -import type { DbLockHandle } from '../src/core/db-lock.ts'; +import { tryAcquireDbLock, inspectLock, isLockHolderLive } from '../src/core/db-lock.ts'; +import { MinionSupervisor, ExitCodes, supervisorLockId, classifySupervisorSingleton, SUPERVISOR_LOCK_TTL_MIN } from '../src/core/minions/supervisor.ts'; +import type { DbLockHandle, LockSnapshot } from '../src/core/db-lock.ts'; + +// Build a LockSnapshot fixture for the isLockHolderLive matrix. Only ttl_expired +// and ms_since_last_refresh are consulted; the rest are filled for shape. +function snap(over: Partial): LockSnapshot { + return { + id: 'gbrain-supervisor:default', + holder_pid: 4242, + holder_host: 'box', + acquired_at: new Date(), + ttl_expires_at: new Date(), + age_ms: 1000, + ttl_expired: false, + last_refreshed_at: new Date(), + ms_since_last_refresh: 0, + ...over, + }; +} let engine: PGLiteEngine; @@ -147,6 +164,48 @@ describe('#1849 LOCK_HELD path does not strand the pidfile', () => { }); +describe('#2227 isLockHolderLive — PID-reuse-safe supervisor liveness', () => { + test('fresh TTL → live (the normal running case)', () => { + expect(isLockHolderLive(snap({ ttl_expired: false }), SUPERVISOR_LOCK_TTL_MIN)).toBe(true); + }); + + test('expired TTL but refreshed within the steal grace → live (starved-but-alive #1794)', () => { + // ttl lapsed but the holder heartbeat is recent → it is alive, just starved. + expect(isLockHolderLive(snap({ ttl_expired: true, ms_since_last_refresh: 5_000 }), SUPERVISOR_LOCK_TTL_MIN)).toBe(true); + }); + + test('expired TTL and stale heartbeat → dead (a gone supervisor stops refreshing)', () => { + expect(isLockHolderLive(snap({ ttl_expired: true, ms_since_last_refresh: 36_000_000 }), SUPERVISOR_LOCK_TTL_MIN)).toBe(false); + }); + + test('expired TTL and no heartbeat column → dead', () => { + expect(isLockHolderLive(snap({ ttl_expired: true, ms_since_last_refresh: null }), SUPERVISOR_LOCK_TTL_MIN)).toBe(false); + }); + + test('liveness NEVER consults process.kill (PID reuse cannot false-positive)', () => { + // A row whose holder_pid happens to be a live, unrelated process (PID reuse) + // but whose lock is stale must read as NOT live — proving freshness, not the + // PID probe, is the signal. holder_pid=1 (init, always alive) + expired/stale. + expect(isLockHolderLive(snap({ holder_pid: 1, ttl_expired: true, ms_since_last_refresh: 36_000_000 }), SUPERVISOR_LOCK_TTL_MIN)).toBe(false); + }); +}); + +describe('#2227 status detects a live supervisor via the DB lock (split-$HOME)', () => { + test('a live queue lock with no local pidfile reads as running via inspectLock', async () => { + // Simulate the keeper holding the queue lock under a different $HOME: there + // is a live lock row but the local pidfile path is empty. + const holder = await tryAcquireDbLock(engine, supervisorLockId('default'), SUPERVISOR_LOCK_TTL_MIN); + expect(holder).not.toBeNull(); + const live = await inspectLock(engine, supervisorLockId('default')); + expect(live).not.toBeNull(); + expect(isLockHolderLive(live!, SUPERVISOR_LOCK_TTL_MIN)).toBe(true); + await holder!.release(); + // After release the row is gone → not running. + const gone = await inspectLock(engine, supervisorLockId('default')); + expect(gone).toBeNull(); + }); +}); + describe('#1849 refresh-failure fails safe (F1A)', () => { test('exits LOCK_LOST after the failure threshold; tolerates a single blip', async () => { const sup = new MinionSupervisor(engine, { cliPath: '/bin/sh', healthInterval: 0, json: true }); diff --git a/test/supervisor.test.ts b/test/supervisor.test.ts index 80f95d577..f34e819fb 100644 --- a/test/supervisor.test.ts +++ b/test/supervisor.test.ts @@ -4,7 +4,7 @@ import { spawn } from 'child_process'; import { join } from 'path'; import { tmpdir } from 'os'; import { readSupervisorEvents, computeSupervisorAuditFilename } from '../src/core/minions/handlers/supervisor-audit.ts'; -import { calculateBackoffMs } from '../src/core/minions/supervisor.ts'; +import { calculateBackoffMs, resolveHardStopMaxCrashes } from '../src/core/minions/supervisor.ts'; const TEST_PID_FILE = '/tmp/gbrain-supervisor-test.pid'; @@ -58,6 +58,16 @@ function spawnSupervisor(h: IntegrationHarness, overrides: Record boolean, timeoutMs: number, tickMs = 20): Pro } describe('MinionSupervisor', () => { + describe('resolveHardStopMaxCrashes (issue #1994)', () => { + const KEY = 'GBRAIN_SUPERVISOR_HARD_STOP_CRASHES'; + afterEach(() => { delete process.env[KEY]; }); + + it('defaults to maxCrashes × 10 when no override', () => { + delete process.env[KEY]; + expect(resolveHardStopMaxCrashes(10)).toBe(100); + expect(resolveHardStopMaxCrashes(3)).toBe(30); + }); + + it('honors a valid non-negative integer override', () => { + process.env[KEY] = '0'; // 0 = disable permanent give-up + expect(resolveHardStopMaxCrashes(10)).toBe(0); + process.env[KEY] = '5'; + expect(resolveHardStopMaxCrashes(10)).toBe(5); + }); + + it('ignores a negative or non-integer override (falls back to default)', () => { + process.env[KEY] = '-1'; + expect(resolveHardStopMaxCrashes(10)).toBe(100); + process.env[KEY] = 'abc'; + expect(resolveHardStopMaxCrashes(10)).toBe(100); + }); + }); + describe('calculateBackoffMs', () => { it('returns ~1s for first crash', () => { const backoff = calculateBackoffMs(0); @@ -197,6 +232,8 @@ describe('MinionSupervisor', () => { // hit max-crashes, then exit via shutdown() with code 1. const h = makeHarness('max-crashes', 'exit 1'); try { + // hard ceiling defaults to SUP_MAX_CRASHES in the harness (see + // spawnSupervisor) so this give-up lifecycle still fires at 3 (#1994). const sup = spawnSupervisor(h, { SUP_MAX_CRASHES: '3' }); const { code } = await sup.exited; diff --git a/test/sync-hard-deadline.test.ts b/test/sync-hard-deadline.test.ts index c90e92ccc..33716c455 100644 --- a/test/sync-hard-deadline.test.ts +++ b/test/sync-hard-deadline.test.ts @@ -6,11 +6,33 @@ import { describe, test, expect } from 'bun:test'; import { resolveSyncHardDeadline, composeAbortSignals, + resolveStallAbortSeconds, + DEFAULT_SYNC_STALL_ABORT_SEC, HARD_DEADLINE_GRACE_SEC, } from '../src/commands/sync.ts'; const GRACE_MS = HARD_DEADLINE_GRACE_SEC * 1000; +describe('resolveStallAbortSeconds (#1950)', () => { + test('defaults to 900s when the env var is unset or empty', () => { + expect(resolveStallAbortSeconds({})).toBe(DEFAULT_SYNC_STALL_ABORT_SEC); + expect(resolveStallAbortSeconds({ GBRAIN_SYNC_STALL_ABORT_SECONDS: '' })).toBe(900); + }); + + test('honors a positive override', () => { + expect(resolveStallAbortSeconds({ GBRAIN_SYNC_STALL_ABORT_SECONDS: '120' })).toBe(120); + }); + + test('<=0 disables the watchdog (returned verbatim)', () => { + expect(resolveStallAbortSeconds({ GBRAIN_SYNC_STALL_ABORT_SECONDS: '0' })).toBe(0); + expect(resolveStallAbortSeconds({ GBRAIN_SYNC_STALL_ABORT_SECONDS: '-1' })).toBe(-1); + }); + + test('falls back to the default on a non-numeric value', () => { + expect(resolveStallAbortSeconds({ GBRAIN_SYNC_STALL_ABORT_SECONDS: 'nope' })).toBe(900); + }); +}); + describe('resolveSyncHardDeadline', () => { test('--no-hard-deadline disables everything (even with --timeout)', () => { const r = resolveSyncHardDeadline(['--source', 'x', '--timeout', '60', '--no-hard-deadline'], { isTty: false }); From 814258dda67945ffec9457a1e73980e947b7e462 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 24 Jun 2026 06:05:16 -0700 Subject: [PATCH 2/2] v0.42.53.0 fix(sync,db): #2339 op_checkpoints jsonb double-encode + bug-class sweep + CI guard (#2375) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sync): op_checkpoints pin write double-encodes jsonb — every sync aborts (#2339) recordCompleted bound JSON.stringify(array) to a $3::jsonb param via postgres.js .unsafe(), double-encoding it into a jsonb string scalar that violates the v119 op_checkpoints_completed_keys_array CHECK — aborting every multi-source sync on real Postgres at the first checkpoint write. PGLite parses the string silently, which is why unit tests stayed green and it shipped. Cast through $3::text::jsonb so the text->jsonb cast parses a genuine array. Adds a DATABASE_URL-gated parity test + a dedicated Postgres CI job so the guard can never silently skip. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(db): sweep positional jsonb double-encode sites + AST CI guard (#2324) Every executeRaw/.unsafe site that bound JSON.stringify(x) to a bare positional jsonb cast double-encodes on real Postgres (same class as #2339). Sweep them all to the text::jsonb form across query-cache, sources-ops, llm-base, calibration-profile, impact-capture, subagent, receipt-write, traversal-cache, symbol-resolver, and the agent/sources commands. Adds scripts/check-jsonb-params.mjs (AST-lite scanner for the positional form the legacy template grep misses, incl. generic-typed calls), wired into check-jsonb-pattern.sh, with a self-test. PGLite's native db.query is not scanned — it parses text to jsonb natively, so the bug can't occur there. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(search,eval): alias-hop injected results carry page_id (contradiction-probe crash) applyAliasHop injected synthetic SearchResults without page_id (the `as SearchResult` cast hid the missing field), so listActiveTakesForPages bound undefined/NaN into ANY($1::int[]) and crashed the whole contradiction probe on real Postgres. Stamp page_id=page.id at the injection site and add a finite-id filter in generateIntraPagePairs as a defensive backstop (mirrors hybrid.ts:63). Co-Authored-By: Claude Opus 4.8 (1M context) * docs(engines): positional jsonb binding rule (text::jsonb vs the double-encode trap) Co-Authored-By: Claude Opus 4.8 (1M context) * v0.42.53.0 fix(sync,db): #2339 op_checkpoints jsonb double-encode + bug-class sweep + CI guard Bumps VERSION + package.json to 0.42.53.0, adds the CHANGELOG entry, and regenerates llms-full.txt. Ships the #2339 sync-abort hotfix, the repo-wide positional jsonb double-encode sweep, the alias-hop contradiction-probe crash fix, and the new positional-form CI guard. Co-Authored-By: Claude Opus 4.8 (1M context) * docs: post-ship sync — jsonb invariant now covers the positional form + new guard CLAUDE.md JSONB invariant + KEY_FILES (sql-query, check-jsonb-pattern, op-checkpoint) now describe the #2339 positional double-encode class, the $N::text::jsonb fix, and the new check-jsonb-params.mjs guard. Regenerates llms-full.txt. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .github/workflows/e2e.yml | 43 +++++++ CHANGELOG.md | 15 +++ CLAUDE.md | 11 +- VERSION | 2 +- docs/ENGINES.md | 33 +++++ docs/architecture/KEY_FILES.md | 7 +- llms-full.txt | 44 ++++++- package.json | 2 +- scripts/check-jsonb-params.mjs | 128 +++++++++++++++++++ scripts/check-jsonb-pattern.sh | 14 ++ src/commands/agent.ts | 2 +- src/commands/sources.ts | 12 +- src/core/chunkers/symbol-resolver.ts | Bin 11437 -> 11443 bytes src/core/code-intel/traversal-cache.ts | 2 +- src/core/conversation-parser/llm-base.ts | 8 +- src/core/cycle/calibration-profile.ts | 2 +- src/core/eval-contradictions/runner.ts | 12 +- src/core/minions/handlers/subagent.ts | 20 +-- src/core/onboard/impact-capture.ts | 2 +- src/core/op-checkpoint.ts | 17 ++- src/core/search/hybrid.ts | 5 + src/core/search/query-cache.ts | 2 +- src/core/sources-ops.ts | 4 +- src/core/sql-query.ts | 25 ++-- src/core/takes-quality-eval/receipt-write.ts | 4 +- test/check-jsonb-params.test.ts | 83 ++++++++++++ test/e2e/op-checkpoint-jsonb-parity.test.ts | 80 ++++++++++++ 27 files changed, 524 insertions(+), 55 deletions(-) create mode 100644 scripts/check-jsonb-params.mjs create mode 100644 test/check-jsonb-params.test.ts create mode 100644 test/e2e/op-checkpoint-jsonb-parity.test.ts diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 74717c3ea..5142a47c2 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -20,6 +20,49 @@ concurrency: cancel-in-progress: true jobs: + jsonb-parity: + # Dedicated required guard for the JSONB double-encode bug-class (#2339). + # PGLite parses a double-encoded jsonb string silently, so this assertion can + # ONLY be made on real Postgres — a normal gated e2e file would skip without + # DATABASE_URL and let the bug ship green (as #2339 did). This job provisions + # Postgres and HARD-FAILS if DATABASE_URL is missing, so the guard can never + # silently skip. + name: JSONB parity (#2339 regression guard) + runs-on: ubuntu-latest + timeout-minutes: 15 + services: + postgres: + image: pgvector/pgvector:pg16 + env: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: postgres + POSTGRES_DB: gbrain_test + ports: + - 5432:5432 + options: >- + --health-cmd pg_isready + --health-interval 10s + --health-timeout 5s + --health-retries 5 + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + with: + bun-version: 1.3.13 + - run: bun install + - name: Require DATABASE_URL (no silent skip) + env: + DATABASE_URL: postgresql://postgres:postgres@localhost:5432/gbrain_test + run: | + if [ -z "$DATABASE_URL" ]; then + echo "::error::DATABASE_URL must be set for the jsonb-parity job — the #2339 guard would silently skip (the exact failure PGLite hides). Failing the job." >&2 + exit 1 + fi + - name: Run JSONB double-encode parity tests on real Postgres + env: + DATABASE_URL: postgresql://postgres:postgres@localhost:5432/gbrain_test + run: bun test test/e2e/op-checkpoint-jsonb-parity.test.ts test/e2e/jsonb-roundtrip.test.ts + tier1: name: Tier 1 (Mechanical) runs-on: ubuntu-latest diff --git a/CHANGELOG.md b/CHANGELOG.md index f0007b641..45dcd7ee8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,21 @@ All notable changes to GBrain will be documented in this file. +## [0.42.53.0] - 2026-06-23 + +**`gbrain sync` works again on managed Postgres brains: the durable-checkpoint pin write was encoding its value the wrong way, so every multi-source sync aborted at the very first checkpoint. Fixed, plus a repo-wide sweep of the same JSONB footgun and a new CI guard so it can't come back.** A recent release added a structural check on the sync checkpoint table; the pin write that runs before every drain bound its value as a string rather than a real array, so the check rejected it and the run bailed before importing anything. The bug was invisible on the embedded engine (its driver parses the value either way) and only bit managed Postgres. + +### Fixed +- **Multi-source sync no longer aborts at the first checkpoint.** The sync-target pin write now binds its value so Postgres stores a genuine JSONB array instead of a double-encoded string scalar. A dedicated Postgres CI job exercises this on a real database, because the embedded test engine masks the failure — which is exactly why it shipped. +- **The same JSONB double-encode footgun is swept across the codebase.** Every raw write that serialized a value into a JSONB column the bug-prone way is corrected to the safe form (search cache, source config, calibration profiles, subagent tool records, eval receipts, code-intel cache, symbol resolver, and others). Readers were already defensive, so existing rows self-heal as each is rewritten. +- **`gbrain eval suspected-contradictions` no longer crashes on an exact-alias query.** An alias-matched result was missing its page id, which aborted the whole probe on Postgres; the id is now carried through, with a finite-id filter as a defensive backstop. + +### Added +- **A CI guard for the positional JSONB double-encode pattern.** The existing guard caught only the template-string spelling; a new static check (`scripts/check-jsonb-params.mjs`) catches the positional-parameter form — the one behind this wave — across the codebase, with its own self-test. The embedded engine's native path is intentionally not flagged, since the bug can't occur there. + +### To take advantage of v0.42.53.0 +`gbrain upgrade`. Multi-source Postgres brains that had stopped syncing resume on the next `gbrain sync` — no migration, no manual step. Rows written in the double-encoded form before the fix self-heal as each is rewritten; re-running the affected write (or a sync) repairs them eagerly if you'd rather not wait. + ## [0.42.52.0] - 2026-06-18 **Autopilot stops manufacturing dead jobs and wedging its own queue, plus four operational rough edges get fixed: minion attempt-accounting, `agent run` flag parsing, honest `sources status`, and a budgeted `gbrain status`.** On a multi-source Postgres brain, autopilot could fan out a continuous stream of dead `autopilot-cycle` jobs while the supervisor periodically wedged the very queue it exists to keep alive. The root cause was one disease with several interacting parts; this wave addresses all of them, then cleans up four smaller reliability bugs found alongside. diff --git a/CLAUDE.md b/CLAUDE.md index edf1a4b9b..c23e645bc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -59,9 +59,14 @@ Per-file detail is in `docs/architecture/KEY_FILES.md`. - **Source isolation.** Every read-side op routes through `sourceScopeOpts(ctx)`; precedence is federated array (`ctx.auth.allowedSources`) > scalar (`ctx.sourceId`) > nothing. Don't hand-roll source filtering — a missed thread is a cross-source data leak. -- **JSONB: never `JSON.stringify` into a `::jsonb` cast.** postgres.js double-encodes it; - PGLite hides the bug. Pass raw objects to `engine.executeRaw`, or use `executeRawJsonb`. - Guarded by `scripts/check-jsonb-pattern.sh`. +- **JSONB: never `JSON.stringify` into a `::jsonb` cast.** postgres.js double-encodes it (a jsonb + string scalar); PGLite hides the bug. This bites BOTH spellings — the template form + (`${JSON.stringify(x)}::jsonb`) AND the positional form (`executeRaw(\`…$N::jsonb\`, [JSON.stringify(x)])`, + the #2339 class that aborted every sync). Fix: pass a raw object to `engine.executeRaw` / use + `executeRawJsonb` / `sql.json()`; or for the positional path bind through `$N::text::jsonb` (binds as + text, the cast parses it). Guarded by `scripts/check-jsonb-pattern.sh` (template grep) + + `scripts/check-jsonb-params.mjs` (positional AST scanner); the real backstop is the DATABASE_URL-gated + e2e parity tests, since PGLite can't surface the bug. Full rule in `docs/ENGINES.md`. - **Engine parity.** `src/core/postgres-engine.ts` and `src/core/pglite-engine.ts` move in lockstep — a new method/SQL shape lands in BOTH, pinned by `test/e2e/engine-parity.test.ts`. Forward-referenced columns/indexes go in the bootstrap probe set (guarded by diff --git a/VERSION b/VERSION index 752fea66c..161d3610f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.42.52.0 +0.42.53.0 diff --git a/docs/ENGINES.md b/docs/ENGINES.md index e8080ed32..7455ab279 100644 --- a/docs/ENGINES.md +++ b/docs/ENGINES.md @@ -176,6 +176,39 @@ RRF fusion, multi-query expansion, and 4-layer dedup are engine-agnostic. They o **Migration:** `gbrain migrate --to supabase` exports everything (pages, chunks, embeddings, links, tags, timeline) and imports into Supabase. `gbrain migrate --to pglite` goes the other direction. Bidirectional, lossless. +## JSONB writes: never double-encode (the #2339 trap) + +Writing a JS value into a `jsonb` column has exactly two correct forms. Get this +wrong and the write succeeds on PGLite but stores a **jsonb string scalar** on +real Postgres — `col ->> 'k'` returns NULL, `jsonb_array_elements` throws, and a +`jsonb_typeof = 'array'` CHECK rejects the row (this aborted every sync in #2339). + +| Form | Verdict | +|---|---| +| Template tag: `` sql`... ${sql.json(obj)}` `` (postgres-engine only) | ✅ native jsonb serialization | +| Positional raw call, raw object: `executeRawJsonb(engine, sql, scalars, [obj])` | ✅ object reaches the wire as jsonb | +| Positional raw call, stringified: `executeRaw(\`... $N::text::jsonb\`, [JSON.stringify(x)])` | ✅ binds as text, the cast parses it | +| Positional raw call, BARE cast: `executeRaw(\`... $N::jsonb\`, [JSON.stringify(x)])` | ❌ **double-encodes** under postgres.js `.unsafe()` | +| Template literal interpolation: `` `... ${JSON.stringify(x)}::jsonb` `` | ❌ double-encodes | + +**Why:** postgres.js `.unsafe(sql, params)` (the path behind `executeRaw` / +`executeRawDirect`) binds a JS **string** as a text param. A bare `$N::jsonb` +cast then wraps that already-JSON string into a jsonb scalar string instead of +parsing it. Casting through `$N::text::jsonb` forces a text→jsonb parse. +**PGLite's `db.query` parses text→jsonb natively, so it hides the bug** — which is +why a regression only shows up on Postgres (and why the parity test must run there). + +**Two CI guards enforce this, both wired into `scripts/check-jsonb-pattern.sh`:** +- the template-tag grep (`${JSON.stringify(x)}::jsonb`), and +- `scripts/check-jsonb-params.mjs`, an AST-lite scanner for the positional + `$N::jsonb` + `JSON.stringify` form the grep misses. Sanctioned escapes: + `$N::text::jsonb`, `$N::text[]`, `executeRawJsonb`, `sql.json`, or an inline + `jsonb-guard-ok` comment. + +The real backstop is `test/e2e/op-checkpoint-jsonb-parity.test.ts` + +`test/e2e/jsonb-roundtrip.test.ts`, which round-trip writes through real Postgres +and assert `jsonb_typeof` — the assertion PGLite cannot make. + ## Adding a new engine 1. Create `src/core/-engine.ts` implementing `BrainEngine` diff --git a/docs/architecture/KEY_FILES.md b/docs/architecture/KEY_FILES.md index 1753b4744..cb6b249f3 100644 --- a/docs/architecture/KEY_FILES.md +++ b/docs/architecture/KEY_FILES.md @@ -265,7 +265,7 @@ per-release `**vX.Y.Z:**` narration — CI enforces this - `src/mcp/dispatch.ts` — shared tool-call dispatch consumed by both stdio (`server.ts`) and HTTP transports. Exports `dispatchToolCall(engine, name, params, opts)`, `buildOperationContext(engine, params, opts)`, `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 `remote: true` (untrusted); local CLI callers pass `remote: false`. Also exports `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 sorted; 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 by probing. Raw payload visibility is opt-in via `gbrain serve --http --log-full-params` (loud stderr warning). New logging paths route through this helper, not `JSON.stringify(params)`. - `src/mcp/rate-limit.ts` — 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 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` — 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] [--bind HOST] [--log-full-params]`. Combines MCP SDK's `mcpAuthRouter` (authorize/token/register/revoke), a custom `client_credentials` handler running BEFORE the router (SDK's token endpoint throws `UnsupportedGrantTypeError` for CC; custom handler falls through for `auth_code` / `refresh_token`), `requireBearerAuth` middleware for `/mcp` with scope enforcement + `localOnly` rejection before op dispatch, 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 broadcasts every MCP request. `cookie-parser` wired (Express 5 has no built-in). Startup logging prints port, engine, issuer URL (honors `--public-url`), client count, DCR status, admin bootstrap token. The `/mcp` request handler's OperationContext literal sets `remote: true` explicitly (without it `submit_job`'s protected-name guard at `operations.ts:1391` saw a falsy undefined and a `read+write`-scoped OAuth token could submit `shell` jobs — RCE). `summarizeMcpParams` from `src/mcp/dispatch.ts` feeds both `mcp_request_log` writes and the SSE feed by default (raw via `--log-full-params`). Cookie `Secure` flag set behind HTTPS or a public-URL proxy; magic-link nonce store LRU-bounded; DCR disable routes through the `GBrainOAuthProvider` `dcrDisabled` constructor option (not a router monkey-patch); `transport.handleRequest` wrapped in try/catch to return a JSON-RPC 500 envelope; OperationError + unexpected exceptions unified through `buildError` / `serializeError` so `/mcp` always returns the same envelope. `/health` is liveness-only via `probeLiveness(sql, engineName, version, timeoutMs)` racing `sql\`SELECT 1\`` against the exported `HEALTH_TIMEOUT_MS = 3000` (returns the same `ProbeHealthResult` tagged-union as `probeHealth`, single timer-cleanup site, single 503 envelope); body shape `{status, version, engine}` only. Full stats moved to admin-only `/admin/api/full-stats` (gated by `requireAdmin`, calls `probeHealth(engine, ...)`) — keeps `getStats()`'s 6× count(*) off the public route so a saturated pool doesn't trigger orchestrator restart cascades. Every OAuth/admin/audit SQL call routes through `sqlQueryForEngine(engine)` from `src/core/sql-query.ts` so it works against PGLite; the four `mcp_request_log.params` INSERT sites (success / auth_failed / scope_denied / server-error) go through `executeRawJsonb(engine, ...)` so the column stores real objects (`params->>'op'` returns `search`, not the quoted string). `--bind HOST` defaults `127.0.0.1` (self-hosters pass `--bind 0.0.0.0`); a stderr WARN fires when `--public-url` is set without `--bind`; the banner prints a `Bind:` line. `AuthInfo.sourceId` + `AuthInfo.allowedSources` are the typed source of truth, populated by `oauth-provider.ts:verifyAccessToken` from the `oauth_clients` row. The HTTP MCP `tools/list` handler at `:837-849` uses `paramDefToSchema(v)` from `src/mcp/tool-defs.ts` so array params keep `items` (strict-mode OAuth clients otherwise reject the whole tool list). -- `src/core/sql-query.ts` — engine-aware tagged-template SQL adapter for OAuth/admin/auth infrastructure. `sqlQueryForEngine(engine)` returns a `SqlQuery` (`(strings, ...values) => Promise`) that walks the template, builds `$N` positional SQL, asserts every value is a `SqlValue` (string | number | bigint | boolean | Date | null), and routes through `engine.executeRaw(sql, params)` (Postgres via postgres.js `unsafe(sql, params)`, PGLite via `db.query(sql, params)`). Deliberately narrower than postgres.js's `sql` tag: no nested fragments, `sql.json()`, `sql.unsafe()`, `sql.begin()`, or array binding — the narrow scalar-only surface is the feature (keeps it from drifting into a partial postgres.js clone). JSONB writes go through `executeRawJsonb(engine, sql, scalarParams, jsonbParams)` which composes positional `$N::jsonb` casts and passes JS objects through; the double-encode bug class doesn't apply because positional binding through `unsafe()` reaches the wire protocol with the correct type oid (verified by `test/sql-query.test.ts` on PGLite, `test/e2e/auth-permissions.test.ts:67` on Postgres). `scripts/check-jsonb-pattern.sh` doesn't fire because `executeRawJsonb(...)` is a method call, not the banned literal-template-tag interpolation. Consumed by `src/commands/auth.ts`, `src/commands/serve-http.ts`, `src/core/oauth-provider.ts`, `src/commands/files.ts`, `src/mcp/http-transport.ts` so all five work uniformly against PGLite and Postgres. +- `src/core/sql-query.ts` — engine-aware tagged-template SQL adapter for OAuth/admin/auth infrastructure. `sqlQueryForEngine(engine)` returns a `SqlQuery` (`(strings, ...values) => Promise`) that walks the template, builds `$N` positional SQL, asserts every value is a `SqlValue` (string | number | bigint | boolean | Date | null), and routes through `engine.executeRaw(sql, params)` (Postgres via postgres.js `unsafe(sql, params)`, PGLite via `db.query(sql, params)`). Deliberately narrower than postgres.js's `sql` tag: no nested fragments, `sql.json()`, `sql.unsafe()`, `sql.begin()`, or array binding — the narrow scalar-only surface is the feature (keeps it from drifting into a partial postgres.js clone). JSONB writes go through `executeRawJsonb(engine, sql, scalarParams, jsonbParams)` which composes positional `$N::jsonb` casts and passes JS **objects** through; an object reaches the wire with the correct type oid, so executeRawJsonb is safe (verified by `test/sql-query.test.ts` on PGLite, `test/e2e/auth-permissions.test.ts:67` on Postgres). Positional binding is NOT universally immune, though: binding a `JSON.stringify(x)` **string** to a bare `$N::jsonb` via `unsafe()` double-encodes it into a jsonb string scalar on real Postgres (the #2339 class; PGLite hides it). Fixes: pass a raw object (executeRawJsonb / `sql.json`), or cast through `$N::text::jsonb`. `scripts/check-jsonb-pattern.sh` (template grep) doesn't fire on `executeRawJsonb(...)` because it passes objects; the positional `$N::jsonb` + `JSON.stringify` form is caught by `scripts/check-jsonb-params.mjs`. Consumed by `src/commands/auth.ts`, `src/commands/serve-http.ts`, `src/core/oauth-provider.ts`, `src/commands/files.ts`, `src/mcp/http-transport.ts` so all five work uniformly against PGLite and Postgres. - `src/commands/serve.ts` — `gbrain serve` stdio MCP entrypoint with idempotent shutdown across every parent-disconnect signal. Stdio EOF, SIGTERM, SIGINT, SIGHUP, and parent-process death (every reparent case — PID 1, launchd subreaper, systemd, tmux, or a parent shell with `PR_SET_CHILD_SUBREAPER`) all funnel into one `cleanup(reason)` that releases the engine and the PGLite write-lock dir within 5 seconds (otherwise the lock is held indefinitely after Claude Desktop / Cursor / launchd-managed gateways disconnect, forcing a 5-minute stale-lock wait on next start). Watchdog reparent check is `getParentPid() !== initialParentPid` (the `=== 1` check missed the subreaper case under launchd/systemd). Bun's `process.ppid` cache is stale across reparenting ([oven-sh/bun#30305](https://github.com/oven-sh/bun/issues/30305)) so `getParentPid()` runs `spawnSync('ps', ['-o', 'ppid=', '-p', PID])` per tick. Startup probe verifies `ps` is on PATH; if not (stripped containers, busybox), the watchdog skips installing AND emits a loud `[gbrain serve] watchdog disabled: ps unavailable ...` stderr line so operators see the degraded mode. Pinned by `test/serve-stdio-lifecycle.test.ts` (22 cases). Credit @Aragorn2046 + @seungsu-kr. - `src/core/oauth-provider.ts` — `GBrainOAuthProvider` implementing the MCP SDK's `OAuthServerProvider` + `OAuthRegisteredClientsStore`. Backed by raw SQL (works on both PGLite and Postgres — OAuth is infrastructure, not a BrainEngine concern). Full OAuth 2.1: `authorize` + `exchangeAuthorizationCode` with PKCE, `client_credentials`, `refresh_token` with rotation, `revokeToken`, `registerClient` (DCR validates redirect_uri is `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); refresh rotation also `DELETE...RETURNING` (§10.4 stolen-token detection). `pgArray()` escapes commas/quotes/braces 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 in try/catch and returns the count via `RETURNING 1` + array length. RFC hardening: `client_id` folded atomically into the `DELETE WHERE` for both auth-code exchange and refresh rotation (wrong-client paths don't burn the row); refresh-scope-subset enforced against the original grant on the row (RFC 6749 §6, so revoking a scope shrinks existing refresh tokens); `client_id` bound on `revokeToken` (RFC 7009 §2.1); `/token` `redirect_uri` validated against the `/authorize` value (RFC 6749 §4.1.3, empty-string treated as missing not wildcard); bare `catch {}` in `verifyAccessToken`/`getClient` replaced by `isUndefinedColumnError` from `src/core/utils.ts` (only SQLSTATE 42703 falls through to legacy; lock timeouts/network blips throw); `dcrDisabled` constructor option lets `serve-http.ts` disable `/register` without monkey-patching the router. Module-private `coerceTimestamp()` normalizes postgres-driver-as-string BIGINT columns to JS numbers at 5 read sites (`getClient` for RFC 7591 §3.2.1 numeric timestamps, `exchangeRefreshToken` + `verifyAccessToken` for the SDK's `typeof === 'number'` check); throws on NaN/Infinity (fail loud at boundary), returns undefined for SQL NULL (callers treat NULL as expired). Not promoted to `utils.ts` — generic BIGINT precision-loss risk. `registerClient` honors `token_endpoint_auth_method: "none"` (RFC 7591 §3.2.1): public PKCE clients store `client_secret_hash = NULL` and the response omits `client_secret`; confidential clients (`client_secret_post` / `client_secret_basic`) keep their one-time-reveal shape; `getClient` normalizes NULL `client_secret_hash` to JS `undefined` so the SDK's clientAuth path accepts public clients. `verifyAccessToken` JOINs `oauth_clients.source_id` (write scope, scalar) + `oauth_clients.federated_read` (read scope, TEXT[]) onto the returned `AuthInfo`; legacy brains degrade via `isUndefinedColumnError` fallback. - `admin/` — 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), Register (modal with scope checkboxes + grant type selector), Credentials reveal (Copy + Download JSON + 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. @@ -326,7 +326,8 @@ per-release `**vX.Y.Z:**` narration — CI enforces this - `scripts/check-progress-to-stdout.sh` — CI guard against regressing to `\r`-on-stdout progress. Wired into `bun run test` via `scripts/check-progress-to-stdout.sh && bun test` in package.json. - `docs/progress-events.md` — Canonical JSON event schema reference. Additive only. - `src/core/markdown.ts` — Frontmatter parsing + body splitter. `coerceFrontmatterString(v)` coerces a non-string `title`/`slug`/`type` to a deterministic string at parse time so a YAML-typed value never reaches `.toLowerCase()` and throws (the #1939 wedge: `title: 2024-06-01` parsed as a `Date`, `title: 1458` as a number, and the throw blocked the sync bookmark from advancing); a `Date` becomes its UTC ISO date (`2024-06-01`, machine-independent and matching the on-disk token, unlike `String(date)`), `null`/`undefined` become `''`, everything else uses `String()`. `splitBody` requires an explicit timeline sentinel (``, `--- timeline ---`, or `---` immediately before `## Timeline`/`## History`). Plain `---` in body text is a markdown horizontal rule, not a separator. `inferType` auto-types `/wiki/analysis/` → analysis, `/wiki/guides/` → guide, `/wiki/hardware/` → hardware, `/wiki/architecture/` → architecture, `/writing/` → writing (plus existing people/companies/deals/etc heuristics). -- `scripts/check-jsonb-pattern.sh` — CI grep guard. Fails the build if anyone reintroduces (a) the `${JSON.stringify(x)}::jsonb` interpolation pattern (postgres.js v3 double-encodes it), or (b) `max_stalled INTEGER NOT NULL DEFAULT 1` in any schema source file (must be DEFAULT 5 to preserve SIGKILL-rescue). Wired into `bun test`. +- `scripts/check-jsonb-pattern.sh` — CI grep guard. Fails the build if anyone reintroduces (a) the `${JSON.stringify(x)}::jsonb` interpolation pattern (postgres.js v3 double-encodes it), or (b) `max_stalled INTEGER NOT NULL DEFAULT 1` in any schema source file (must be DEFAULT 5 to preserve SIGKILL-rescue). It also invokes `scripts/check-jsonb-params.mjs` and propagates its exit code. Wired into `bun test`. +- `scripts/check-jsonb-params.mjs` — AST-lite CI guard for the POSITIONAL jsonb double-encode form the template grep above misses: an `executeRaw`/`executeRawDirect`/`.unsafe()` call whose balanced arg span binds `JSON.stringify(x)` into a bare `$N::jsonb` cast (the #2339 class). Walks each call's balanced span respecting strings/templates/comments, handles generic-typed calls (`executeRaw(`), and allows the sanctioned forms (`$N::text::jsonb`, `$N::text[]`, `executeRawJsonb`, `sql.json`, an inline `jsonb-guard-ok` comment). PGLite's native `db.query` is deliberately not scanned (it parses text→jsonb, so the bug can't occur there). Heuristic by design (whole-span correlation; can't see a `JSON.stringify` assigned to a variable before the call) — the real backstop is the DATABASE_URL-gated e2e parity tests. Scan roots overridable via argv for its self-test (`test/check-jsonb-params.test.ts`). - `scripts/check-source-id-projection.sh` — CI grep guard for the multi-source bug class. Greps `src/core/postgres-engine.ts` + `src/core/pglite-engine.ts` for `SELECT.*FROM pages` projections matching the `rowToPage` feeder shape (id + slug + type + title) and fails if `source_id` is missing. `Page.source_id` is required at the type level; a projection dropping the column produces `Page` rows with `source_id: undefined` while TypeScript's `: string` lies about it. Wired into `bun run verify` + `bun run check:all`. - `docker-compose.ci.yml` + `scripts/ci-local.sh` — Local CI gate. `bun run ci:local` spins up four `pgvector/pgvector:pg16` services (postgres-1..4) + `oven/bun:1` with named volumes (`gbrain-ci-pg-data-{1..4}`, `gbrain-ci-node-modules`, `gbrain-ci-bun-cache`), runs gitleaks on host, smoke-tests `scripts/run-e2e.sh` argv handling, runs guards + typecheck, then the Tier 1 default: 4-shard parallel unit + E2E (`xargs -P4`, one Postgres per shard; unit phase keeps `DATABASE_URL` unset). `--no-shard` falls back to the legacy unsharded sequential flow (debug aid); `--diff` runs the diff-aware selector unsharded. Also runs a `pgbouncer` service (`edoburu/pgbouncer`, `POOL_MODE: transaction`, `AUTH_TYPE: plain` — pg16 stores SCRAM verifiers, so the userlist must hold the plaintext password; `IGNORE_STARTUP_PARAMETERS` whitelists gbrain's `statement_timeout`/`idle_in_transaction_session_timeout` startup params the way the Supabase pooler does) fronting postgres-1 on host port `GBRAIN_CI_PGBOUNCER_PORT` (default 6543); every E2E invocation exports `GBRAIN_PGBOUNCER_URL` (pooled; dedicated `gbrain_pgbouncer` database so it never races the `gbrain_test` TRUNCATE fixtures) + `GBRAIN_PGBOUNCER_DIRECT_URL`, consumed by `test/e2e/pgbouncer-teardown.test.ts` — the transaction-mode teardown bug class (#1972/#2015/#2084) reproduced in the local gate instead of only in production. `--no-pull` skips upstream pulls; `--clean` nukes named volumes. Postgres host port defaults to 5434; override with `GBRAIN_CI_PG_PORT=NNNN`. Stronger gate than PR CI's 2-file Tier 1 set. - `scripts/select-e2e.ts` + `scripts/e2e-test-map.ts` — Diff-aware E2E test selector. Reads three git sources (committed `origin/master...HEAD`, working-tree `HEAD`, and `git ls-files --others --exclude-standard` for untracked NOT-gitignored files), classifies as EMPTY / DOC_ONLY / SRC. Fail-closed: EMPTY → all files; DOC_ONLY (every path matches the README/CLAUDE/AGENTS/CHANGELOG/TODOS allowlist) → empty stdout; SRC → escape-hatch paths (schema, package.json, skills/) trigger all, else the hand-tuned `E2E_TEST_MAP` glob narrows, and an unmapped src/ change still emits ALL files (never silently nothing). Pure-function exports `selectTests`, `classify`, `matchGlob`. `bun run ci:select-e2e` prints the current selection on stdout. `test/select-e2e.test.ts` covers all 4 branches plus 3 regression guards (skills/, untracked files, unmapped src/) — 24 cases. @@ -383,7 +384,7 @@ per-release `**vX.Y.Z:**` narration — CI enforces this - `src/commands/report.ts` — Structured report saver (audit trail for maintenance/enrichment) - `src/core/destructive-guard.ts` — three-layer protection against accidental data loss. `assessDestructiveImpact(engine, sourceId)` counts pages/chunks/embeddings/files for a source. `checkDestructiveConfirmation(impact, opts)` is the fail-closed gate (`--confirm-destructive` required when data is present; `--yes` alone is rejected). `softDeleteSource` / `restoreSource` / `listArchivedSources` / `purgeExpiredSources` drive the source-level archive lifecycle via `sources.archived BOOLEAN`, `archived_at TIMESTAMPTZ`, `archive_expires_at TIMESTAMPTZ`. Page-level analog: `BrainEngine.softDeletePage` / `restorePage` / `purgeDeletedPages` plus `pages.deleted_at TIMESTAMPTZ` and a partial purge index. The MCP `delete_page` op rewires to `softDeletePage`; ops `restore_page` (`scope: write`) and `purge_deleted_pages` (`scope: admin`, `localOnly: true`) round out the surface. Search visibility (`buildVisibilityClause` in `src/core/search/sql-ranking.ts`) hides soft-deleted pages and archived sources from `searchKeyword` / `searchKeywordChunks` / `searchVector` in both engines. The autopilot cycle's `purge` phase calls `purgeExpiredSources` + `engine.purgeDeletedPages(72)` so the 72h TTL is real. - `src/commands/pages.ts` — `gbrain pages purge-deleted [--older-than HOURS|Nd] [--dry-run] [--json]` operator escape hatch. Mirror of `gbrain sources purge` for the page-level lifecycle. Hard-deletes pages whose `deleted_at` is older than the cutoff; cascades to content_chunks/page_links/chunk_relations. -- `src/core/op-checkpoint.ts` — DB-backed checkpoint primitive for long-running ops. Migration v67 introduces `op_checkpoints (op TEXT, fingerprint TEXT, completed_keys JSONB, updated_at TIMESTAMPTZ, PK(op, fingerprint))`. Per-op fingerprint helpers (`embedFingerprint`, `extractFingerprint`, `reindexFingerprint`, `integrityFingerprint`, `purgeFingerprint`) compute `sha8(canonical-JSON(relevant-params))` so re-running with the same params resumes from `completed_keys` and re-running with different params (e.g. `--limit 100` vs `--limit 200`) starts fresh. Cross-worker safe on Postgres (DB row, no file-lock race); PGLite degrades gracefully. Replaces per-op file-backed JSON checkpoints scattered across `import.ts`, `embed.ts`, `reindex.ts`. The 7-day TTL GC runs in the cycle's `purge` phase. All writes (`recordCompleted`, `clearOpCheckpoint`) route through `engine.executeRawDirect` + `withRetry(BULK_RETRY_OPTS)` so they survive Supavisor pool exhaustion, and `recordCompleted` returns `boolean` (banked vs failed-after-retries) — the 9 non-sync consumers keep its REPLACE-into-`completed_keys` semantics. Resumable sync uses the additive `appendCompleted(key, deltaKeys)` / `appendCompletedOnce` (the latter no-retry for the SIGTERM path) which INSERT a delta into the `op_checkpoint_paths` child table (migration v115: `(op, fingerprint, path)` PK, FK to `op_checkpoints` ON DELETE CASCADE) via a single writable-CTE `unnest($3::text[])` write — O(delta), killing the old O(N²) full-set rewrite. `loadOpCheckpoint` returns the `UNION ALL` of legacy `completed_keys` + child-table paths (deduped in JS), so an in-flight upgrade loses nothing. The legacy arm is gated on `jsonb_typeof(completed_keys) = 'array'` so a non-array (scalar) parent row can't make `jsonb_array_elements_text` throw "cannot extract elements from a scalar" and take down the whole union (which would discard the valid child rows and lose all banked progress for the key); a third union arm flags the corruption so the loader logs it once and keeps the child rows. Migration v119 adds the `op_checkpoints_completed_keys_array` CHECK (`jsonb_typeof(completed_keys) = 'array'`) — a DB-enforced, always-on guard that makes the scalar-corruption class structurally impossible going forward; the migration repairs any pre-existing scalar to `'[]'` under `LOCK TABLE ... IN SHARE ROW EXCLUSIVE MODE` and `src/core/schema-embedded.ts` + `src/core/pglite-schema.ts` ship the same CHECK on fresh installs (a loader hit now implies schema drift, a disabled constraint, or an out-of-band writer). `syncFingerprint({sourceId, lastCommit})` keys the sync rows. Pinned by `test/op-checkpoint.test.ts` (incl. delta-append, union read, cascade clear, durable-write boolean, and the scalar-parent guard). `import-checkpoint.ts` was NOT migrated to this primitive — both checkpoint systems coexist without conflict; migrating requires async-propagating 4 sync call sites in `src/commands/import.ts` and rewriting 18 tests, deferred. +- `src/core/op-checkpoint.ts` — DB-backed checkpoint primitive for long-running ops. Migration v67 introduces `op_checkpoints (op TEXT, fingerprint TEXT, completed_keys JSONB, updated_at TIMESTAMPTZ, PK(op, fingerprint))`. Per-op fingerprint helpers (`embedFingerprint`, `extractFingerprint`, `reindexFingerprint`, `integrityFingerprint`, `purgeFingerprint`) compute `sha8(canonical-JSON(relevant-params))` so re-running with the same params resumes from `completed_keys` and re-running with different params (e.g. `--limit 100` vs `--limit 200`) starts fresh. Cross-worker safe on Postgres (DB row, no file-lock race); PGLite degrades gracefully. Replaces per-op file-backed JSON checkpoints scattered across `import.ts`, `embed.ts`, `reindex.ts`. The 7-day TTL GC runs in the cycle's `purge` phase. All writes (`recordCompleted`, `clearOpCheckpoint`) route through `engine.executeRawDirect` + `withRetry(BULK_RETRY_OPTS)` so they survive Supavisor pool exhaustion, and `recordCompleted` returns `boolean` (banked vs failed-after-retries) — the 9 non-sync consumers keep its REPLACE-into-`completed_keys` semantics. Resumable sync uses the additive `appendCompleted(key, deltaKeys)` / `appendCompletedOnce` (the latter no-retry for the SIGTERM path) which INSERT a delta into the `op_checkpoint_paths` child table (migration v115: `(op, fingerprint, path)` PK, FK to `op_checkpoints` ON DELETE CASCADE) via a single writable-CTE `unnest($3::text[])` write — O(delta), killing the old O(N²) full-set rewrite. `loadOpCheckpoint` returns the `UNION ALL` of legacy `completed_keys` + child-table paths (deduped in JS), so an in-flight upgrade loses nothing. The legacy arm is gated on `jsonb_typeof(completed_keys) = 'array'` so a non-array (scalar) parent row can't make `jsonb_array_elements_text` throw "cannot extract elements from a scalar" and take down the whole union (which would discard the valid child rows and lose all banked progress for the key); a third union arm flags the corruption so the loader logs it once and keeps the child rows. Migration v119 adds the `op_checkpoints_completed_keys_array` CHECK (`jsonb_typeof(completed_keys) = 'array'`) — a DB-enforced, always-on guard that makes the scalar-corruption class structurally impossible going forward; the migration repairs any pre-existing scalar to `'[]'` under `LOCK TABLE ... IN SHARE ROW EXCLUSIVE MODE` and `src/core/schema-embedded.ts` + `src/core/pglite-schema.ts` ship the same CHECK on fresh installs (a loader hit now implies schema drift, a disabled constraint, or an out-of-band writer). `recordCompleted` binds its array through `$3::text::jsonb` (NOT a bare `$3::jsonb`) so postgres.js `.unsafe()` doesn't double-encode `JSON.stringify(sorted)` into the scalar string that CHECK rejects — the #2339 bug that aborted every multi-source sync at the first pin write (PGLite parsed it silently, so it shipped). A DATABASE_URL-gated `test/e2e/op-checkpoint-jsonb-parity.test.ts` (its own CI job) asserts the array shape on real Postgres. `syncFingerprint({sourceId, lastCommit})` keys the sync rows. Pinned by `test/op-checkpoint.test.ts` (incl. delta-append, union read, cascade clear, durable-write boolean, and the scalar-parent guard). `import-checkpoint.ts` was NOT migrated to this primitive — both checkpoint systems coexist without conflict; migrating requires async-propagating 4 sync call sites in `src/commands/import.ts` and rewriting 18 tests, deferred. - `src/core/brain-score-recommendations.ts` — pure data layer consumed by both `gbrain doctor --remediation-plan` / `--remediate` and `gbrain features`. `computeRecommendations(checks, opts)` returns `Remediation[]` with stable `id`, content-hash `idempotency_key`, `severity`, `est_seconds`, `est_usd_cost`, `depends_on` (references stable ids, not check names — so plan order is reproducible). `classifyChecks(report)` triages every doctor check three-state into `remediable | human_only | blocked` (`human_only` covers RLS warnings and other human-judgment gates; `blocked` covers dependency chains where a parent check failed). `maxReachableScore(checks)` computes the ceiling for empty/under-configured brains (no entity pages → graph_coverage caps at 70; no embedding key → embedding_coverage caps at 60). Cost estimates pull from `anthropic-pricing.ts` (synthesize/patterns/consolidate) and `embedding-pricing.ts` (embed jobs). Pinned by `test/brain-score-recommendations.test.ts` (~27 cases incl. determinism, content-hash idempotency, DB-backed checkpoint provenance, three-state triage). - `src/commands/doctor.ts` extension — `--remediation-plan [--json] [--target-score N]` prints what would run (stable `id`, `idempotency_key`, `severity`, `est_seconds`, `est_usd_cost`, `depends_on`); `--remediate [--yes] [--target-score N] [--max-usd N]` submits each plan step as a Minion job in dependency order, re-checking score between steps. `--target-score N` defaults to 90; refuses to start when target exceeds `maxReachableScore()` and lists what's missing. `--max-usd N` is the cron-safety guard — submission refuses when the plan's `est_total_usd_cost` exceeds the cap. JSON envelope adds a `Check.remediation` field (additive, schema_version unchanged). Pinned by tests in `test/doctor.test.ts`. - `src/commands/jobs.ts` extension — registers 11 Minion handlers: `reindex`, `repair-jsonb`, `orphans`, `integrity`, `purge`, `synthesize` (PROTECTED), `patterns` (PROTECTED), `consolidate` (PROTECTED), `extract_facts`, `resolve_symbol_edges`, `recompute_emotional_weight`. Phase wrappers delegate to `runCycle({phases:[name]})` so `src/core/cycle.ts` stays the single source of truth for phase semantics. The standalone `sync` handler passes `noExtract: true` to match `runPhaseSync`'s contract (doctor's remediation plan emitting `[sync, extract]` would otherwise double-extract). diff --git a/llms-full.txt b/llms-full.txt index bddfbc198..d006b9a13 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -208,9 +208,14 @@ Per-file detail is in `docs/architecture/KEY_FILES.md`. - **Source isolation.** Every read-side op routes through `sourceScopeOpts(ctx)`; precedence is federated array (`ctx.auth.allowedSources`) > scalar (`ctx.sourceId`) > nothing. Don't hand-roll source filtering — a missed thread is a cross-source data leak. -- **JSONB: never `JSON.stringify` into a `::jsonb` cast.** postgres.js double-encodes it; - PGLite hides the bug. Pass raw objects to `engine.executeRaw`, or use `executeRawJsonb`. - Guarded by `scripts/check-jsonb-pattern.sh`. +- **JSONB: never `JSON.stringify` into a `::jsonb` cast.** postgres.js double-encodes it (a jsonb + string scalar); PGLite hides the bug. This bites BOTH spellings — the template form + (`${JSON.stringify(x)}::jsonb`) AND the positional form (`executeRaw(\`…$N::jsonb\`, [JSON.stringify(x)])`, + the #2339 class that aborted every sync). Fix: pass a raw object to `engine.executeRaw` / use + `executeRawJsonb` / `sql.json()`; or for the positional path bind through `$N::text::jsonb` (binds as + text, the cast parses it). Guarded by `scripts/check-jsonb-pattern.sh` (template grep) + + `scripts/check-jsonb-params.mjs` (positional AST scanner); the real backstop is the DATABASE_URL-gated + e2e parity tests, since PGLite can't surface the bug. Full rule in `docs/ENGINES.md`. - **Engine parity.** `src/core/postgres-engine.ts` and `src/core/pglite-engine.ts` move in lockstep — a new method/SQL shape lands in BOTH, pinned by `test/e2e/engine-parity.test.ts`. Forward-referenced columns/indexes go in the bootstrap probe set (guarded by @@ -2116,6 +2121,39 @@ RRF fusion, multi-query expansion, and 4-layer dedup are engine-agnostic. They o **Migration:** `gbrain migrate --to supabase` exports everything (pages, chunks, embeddings, links, tags, timeline) and imports into Supabase. `gbrain migrate --to pglite` goes the other direction. Bidirectional, lossless. +## JSONB writes: never double-encode (the #2339 trap) + +Writing a JS value into a `jsonb` column has exactly two correct forms. Get this +wrong and the write succeeds on PGLite but stores a **jsonb string scalar** on +real Postgres — `col ->> 'k'` returns NULL, `jsonb_array_elements` throws, and a +`jsonb_typeof = 'array'` CHECK rejects the row (this aborted every sync in #2339). + +| Form | Verdict | +|---|---| +| Template tag: `` sql`... ${sql.json(obj)}` `` (postgres-engine only) | ✅ native jsonb serialization | +| Positional raw call, raw object: `executeRawJsonb(engine, sql, scalars, [obj])` | ✅ object reaches the wire as jsonb | +| Positional raw call, stringified: `executeRaw(\`... $N::text::jsonb\`, [JSON.stringify(x)])` | ✅ binds as text, the cast parses it | +| Positional raw call, BARE cast: `executeRaw(\`... $N::jsonb\`, [JSON.stringify(x)])` | ❌ **double-encodes** under postgres.js `.unsafe()` | +| Template literal interpolation: `` `... ${JSON.stringify(x)}::jsonb` `` | ❌ double-encodes | + +**Why:** postgres.js `.unsafe(sql, params)` (the path behind `executeRaw` / +`executeRawDirect`) binds a JS **string** as a text param. A bare `$N::jsonb` +cast then wraps that already-JSON string into a jsonb scalar string instead of +parsing it. Casting through `$N::text::jsonb` forces a text→jsonb parse. +**PGLite's `db.query` parses text→jsonb natively, so it hides the bug** — which is +why a regression only shows up on Postgres (and why the parity test must run there). + +**Two CI guards enforce this, both wired into `scripts/check-jsonb-pattern.sh`:** +- the template-tag grep (`${JSON.stringify(x)}::jsonb`), and +- `scripts/check-jsonb-params.mjs`, an AST-lite scanner for the positional + `$N::jsonb` + `JSON.stringify` form the grep misses. Sanctioned escapes: + `$N::text::jsonb`, `$N::text[]`, `executeRawJsonb`, `sql.json`, or an inline + `jsonb-guard-ok` comment. + +The real backstop is `test/e2e/op-checkpoint-jsonb-parity.test.ts` + +`test/e2e/jsonb-roundtrip.test.ts`, which round-trip writes through real Postgres +and assert `jsonb_typeof` — the assertion PGLite cannot make. + ## Adding a new engine 1. Create `src/core/-engine.ts` implementing `BrainEngine` diff --git a/package.json b/package.json index 73eaf18a9..e59e46ac3 100644 --- a/package.json +++ b/package.json @@ -143,5 +143,5 @@ "bun": ">=1.3.10" }, "license": "MIT", - "version": "0.42.52.0" + "version": "0.42.53.0" } diff --git a/scripts/check-jsonb-params.mjs b/scripts/check-jsonb-params.mjs new file mode 100644 index 000000000..f3be2ddab --- /dev/null +++ b/scripts/check-jsonb-params.mjs @@ -0,0 +1,128 @@ +#!/usr/bin/env node +/** + * CI guard for the POSITIONAL jsonb double-encode footgun (#2339 / #2324 class). + * + * The legacy scripts/check-jsonb-pattern.sh only catches the template-tag form + * (`${JSON.stringify(x)}::jsonb`). It MISSES the positional-param form: + * + * engine.executeRaw(`... $3::jsonb ...`, [a, b, JSON.stringify(x)]) + * + * Under postgres.js `.unsafe(sql, params)` a JS STRING bound to a `$N::jsonb` + * param double-encodes — the text→jsonb cast wraps the already-JSON string into a + * jsonb *string scalar*. PGLite parses it silently, so the bug is invisible in + * unit tests and only bites on real Postgres (it aborted every sync in #2339). + * + * This scanner flags any executeRaw / executeRawDirect / .unsafe(...) call whose + * balanced argument span contains BOTH a positional `$N::jsonb` cast + * (NOT `$N::text::jsonb`, NOT `$N::text[]`) AND a `JSON.stringify(` — the exact + * double-encode shape. It is heuristic by design (whole-span correlation); the + * real backstop is the DATABASE_URL-gated e2e parity test. Keep both. + * + * Allowed forms (NOT flagged): + * - `$N::text::jsonb` + JSON.stringify (the fix: binds as text, cast parses it) + * - `$N::text[]` (the unnest path — arrays bind fine) + * - executeRawJsonb(...) (passes raw objects, not strings) + * - sql.json(x) (postgres.js native jsonb serializer) + * - a `jsonb-guard-ok` comment anywhere in the call span (explicit opt-out) + * + * Exit 0 = clean, 1 = violations found. Runs under node or bun. + */ +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import { join } from 'node:path'; + +// Default scan roots; overridable via argv so the guard's own test can point it +// at a fixture dir (e.g. `node check-jsonb-params.mjs /tmp/fixtures`). +const ROOTS = process.argv.slice(2).length > 0 ? process.argv.slice(2) : ['src', 'scripts']; +// executeRawDirect must precede executeRaw in the alternation so the longer name +// wins; executeRawJsonb is deliberately excluded (it passes objects). The +// optional `<...>` handles generic type args, e.g. `executeRaw<{ id: string }>(`. +// +// Only the postgres.js raw path is scanned (executeRaw/executeRawDirect/.unsafe). +// PGLite's native `this.db.query(...)` is intentionally NOT matched: its driver +// parses a text→jsonb cast natively, so the double-encode that bites postgres.js +// `.unsafe()` does not occur there (the `pglite-masks` invariant). The engine +// parity test pins that the resulting jsonb_typeof agrees across both engines. +const CALL_RE = /\b(executeRawDirect|executeRaw|unsafe)\s*(?:<[^>;]*>)?\s*\(/g; + +/** Walk from the '(' at openIdx and return [start,end) of the balanced span, + * respecting strings, template literals, and comments. */ +function findSpan(src, openIdx) { + let depth = 0; + let mode = 'code'; // code | line | block | sq | dq | tpl + for (let i = openIdx; i < src.length; i++) { + const c = src[i]; + const n = src[i + 1]; + if (mode === 'line') { if (c === '\n') mode = 'code'; continue; } + if (mode === 'block') { if (c === '*' && n === '/') { mode = 'code'; i++; } continue; } + if (mode === 'sq') { if (c === '\\') { i++; continue; } if (c === "'") mode = 'code'; continue; } + if (mode === 'dq') { if (c === '\\') { i++; continue; } if (c === '"') mode = 'code'; continue; } + if (mode === 'tpl') { if (c === '\\') { i++; continue; } if (c === '`') mode = 'code'; continue; } + // mode === 'code' + if (c === '/' && n === '/') { mode = 'line'; i++; continue; } + if (c === '/' && n === '*') { mode = 'block'; i++; continue; } + if (c === "'") { mode = 'sq'; continue; } + if (c === '"') { mode = 'dq'; continue; } + if (c === '`') { mode = 'tpl'; continue; } + if (c === '(') depth++; + else if (c === ')') { depth--; if (depth === 0) return [openIdx + 1, i]; } + } + return [openIdx + 1, src.length]; +} + +/** Blank out comments so a commented-out example doesn't trip the JSON.stringify probe. */ +function stripComments(s) { + return s.replace(/\/\/[^\n]*/g, '').replace(/\/\*[\s\S]*?\*\//g, ''); +} + +const violations = []; + +function scanFile(file) { + const src = readFileSync(file, 'utf8'); + CALL_RE.lastIndex = 0; + let m; + while ((m = CALL_RE.exec(src))) { + const method = m[1]; + const openIdx = m.index + m[0].length - 1; // index of the '(' + const [s, e] = findSpan(src, openIdx); + const span = src.slice(s, e); + if (/jsonb-guard-ok/.test(span)) continue; + if (!/JSON\.stringify\s*\(/.test(stripComments(span))) continue; + // A positional `$N::jsonb` that is NOT `$N::text::jsonb`. + const jsonbRe = /\$\d+\s*::\s*jsonb\b/g; + let j; + let badText = ''; + while ((j = jsonbRe.exec(span))) { + const pre = span.slice(Math.max(0, j.index - 12), j.index); + if (/::\s*text\s*$/.test(pre)) continue; // $N::text::jsonb is the fix — allowed + badText = j[0].replace(/\s+/g, ''); + break; + } + if (!badText) continue; + const line = src.slice(0, s).split('\n').length; + violations.push( + `${file}:${line} ${method}(...) binds JSON.stringify into ${badText} — use $N::text::jsonb or pass a raw object (executeRawJsonb / sql.json)`, + ); + } +} + +function walk(dir) { + let ents; + try { ents = readdirSync(dir); } catch { return; } + for (const ent of ents) { + if (ent === 'node_modules') continue; + const p = join(dir, ent); + const st = statSync(p); + if (st.isDirectory()) walk(p); + else if (p.endsWith('.ts') && !p.endsWith('.test.ts')) scanFile(p); + } +} + +for (const root of ROOTS) walk(root); + +if (violations.length) { + console.error('JSONB positional double-encode violations (#2339 class):\n'); + for (const v of violations) console.error(' ' + v); + console.error(`\n${violations.length} violation(s). Fix: bind through $N::text::jsonb (keeping JSON.stringify), or pass a raw object via executeRawJsonb / sql.json. See docs/ENGINES.md.`); + process.exit(1); +} +console.log('check-jsonb-params: clean (no positional $N::jsonb + JSON.stringify double-encodes)'); diff --git a/scripts/check-jsonb-pattern.sh b/scripts/check-jsonb-pattern.sh index 2403f7b83..d26acb0e2 100755 --- a/scripts/check-jsonb-pattern.sh +++ b/scripts/check-jsonb-pattern.sh @@ -44,3 +44,17 @@ if grep -rEn "$MAX_STALLED_PATTERN" src/schema.sql src/core/migrate.ts src/core/ fi echo "OK: max_stalled defaults are 5 in all schema sources" + +# v0.42.x (#2339 / #2324): positional `$N::jsonb` + JSON.stringify double-encode. +# The template-string grep above only catches `${JSON.stringify(x)}::jsonb`. It +# MISSES the positional-param form — executeRaw(`... $N::jsonb ...`, +# [JSON.stringify(x)]) — which is the exact shape that double-encoded the +# op_checkpoints pin and aborted every sync in #2339. The AST-lite scanner below +# catches it. `set -e` propagates its non-zero exit. +if command -v node >/dev/null 2>&1; then + node scripts/check-jsonb-params.mjs +elif command -v bun >/dev/null 2>&1; then + bun scripts/check-jsonb-params.mjs +else + echo "WARN: neither node nor bun on PATH; skipping check-jsonb-params.mjs" >&2 +fi diff --git a/src/commands/agent.ts b/src/commands/agent.ts index 2b4c05d4a..f0a7e37ce 100644 --- a/src/commands/agent.ts +++ b/src/commands/agent.ts @@ -310,7 +310,7 @@ async function runFanout(engine: BrainEngine, queue: MinionQueue, flags: RunFlag // do this after submission because each add() returns the committed // row's id; the aggregator's seed started with an empty array. await engine.executeRaw( - `UPDATE minion_jobs SET data = jsonb_set(data, '{children_ids}', $1::jsonb) WHERE id = $2`, + `UPDATE minion_jobs SET data = jsonb_set(data, '{children_ids}', $1::text::jsonb) WHERE id = $2`, [JSON.stringify(childIds), aggregator.id], ); diff --git a/src/commands/sources.ts b/src/commands/sources.ts index ff9874ccb..77caafff7 100644 --- a/src/commands/sources.ts +++ b/src/commands/sources.ts @@ -692,7 +692,7 @@ async function runFederate(engine: BrainEngine, args: string[], value: boolean): const config = parseConfig(src.config); config.federated = value; await engine.executeRaw( - `UPDATE sources SET config = $1::jsonb WHERE id = $2`, + `UPDATE sources SET config = $1::text::jsonb WHERE id = $2`, [JSON.stringify(config), id], ); console.log(`Source "${id}" is now ${value ? 'federated (appears in cross-source default search)' : 'isolated (only searched when explicitly named)'}.`); @@ -879,7 +879,7 @@ async function runWebhookSet(engine: BrainEngine, args: string[]): Promise cfg.webhook_secret = secret; cfg.github_repo = githubRepo; await engine.executeRaw( - `UPDATE sources SET config = $1::jsonb WHERE id = $2`, + `UPDATE sources SET config = $1::text::jsonb WHERE id = $2`, [JSON.stringify(cfg), id], ); @@ -935,7 +935,7 @@ async function runWebhookRotate(engine: BrainEngine, args: string[]): Promise( `INSERT INTO code_traversal_cache (symbol_qualified, depth, source_id, response_json, max_chunk_updated_at, xmin_max, cluster_generation) - VALUES ($1, $2, $3, $4::jsonb, $5::timestamptz, $6, $7) + VALUES ($1, $2, $3, $4::text::jsonb, $5::timestamptz, $6, $7) ON CONFLICT (symbol_qualified, depth, source_id) DO UPDATE SET response_json = EXCLUDED.response_json, diff --git a/src/core/conversation-parser/llm-base.ts b/src/core/conversation-parser/llm-base.ts index 78d0af205..6671b2cb0 100644 --- a/src/core/conversation-parser/llm-base.ts +++ b/src/core/conversation-parser/llm-base.ts @@ -266,13 +266,13 @@ async function writeDbCache( ): Promise { const [, , contentSha] = splitCacheKey(key); if (!contentSha) return; - // executeRaw with positional binding for JSONB. Per the sql-query.ts - // contract: object values passed via positional params reach the - // wire as proper jsonb when cast. + // #2339 class: this binds JSON.stringify(value) (a STRING) positionally, so it + // must cast through $4::text::jsonb — a bare $4::jsonb double-encodes under + // postgres.js .unsafe() (PGLite hides it). Pass a raw object to use $N::jsonb. await engine.executeRaw( `INSERT INTO conversation_parser_llm_cache (content_sha256, model_id, call_shape, value_json) - VALUES ($1, $2, $3, $4::jsonb) + VALUES ($1, $2, $3, $4::text::jsonb) ON CONFLICT (content_sha256, model_id, call_shape) DO NOTHING`, [contentSha, modelStr, shape, JSON.stringify(value)], ); diff --git a/src/core/cycle/calibration-profile.ts b/src/core/cycle/calibration-profile.ts index 05aff0234..f07724f48 100644 --- a/src/core/cycle/calibration-profile.ts +++ b/src/core/cycle/calibration-profile.ts @@ -356,7 +356,7 @@ class CalibrationProfilePhase extends BaseCyclePhase { active_bias_tags, model_id, cost_usd, judge_model_agreement ) VALUES ($1, $2, now(), false, $3, $4, $5, $6, $7, - $8::jsonb, $9::text[], + $8::text::jsonb, $9::text[], $10, $11, $12::text[], $13, NULL, NULL)`, [ diff --git a/src/core/eval-contradictions/runner.ts b/src/core/eval-contradictions/runner.ts index 7a16728af..adee2c5ef 100644 --- a/src/core/eval-contradictions/runner.ts +++ b/src/core/eval-contradictions/runner.ts @@ -171,11 +171,19 @@ async function generateIntraPagePairs( results: SearchResult[], ): Promise { if (results.length === 0) return []; - // Unique page_ids only. - const pageIds = Array.from(new Set(results.map((r) => r.page_id))); + // Unique, FINITE page_ids only. Defensive backstop for the alias-hop bug + // (#2339 sibling): an alias-injected synthetic result with an undefined/NaN + // page_id must never reach `ANY($1::int[])` — postgres.js rejects it with + // UNDEFINED_VALUE and aborts the whole probe. Mirrors the hybrid.ts:63 filter. + const pageIds = Array.from( + new Set( + results.map((r) => r.page_id).filter((n): n is number => typeof n === 'number' && Number.isFinite(n)), + ), + ); const takesByPage = await engine.listActiveTakesForPages(pageIds); const out: ContradictionPair[] = []; for (const r of results) { + if (typeof r.page_id !== 'number' || !Number.isFinite(r.page_id)) continue; const takes = takesByPage.get(r.page_id) ?? []; if (takes.length === 0) continue; const chunkMember = searchResultToMember(r); diff --git a/src/core/minions/handlers/subagent.ts b/src/core/minions/handlers/subagent.ts index fcead52f3..450dc8da0 100644 --- a/src/core/minions/handlers/subagent.ts +++ b/src/core/minions/handlers/subagent.ts @@ -878,7 +878,7 @@ async function runSubagentViaGateway(args: GatewayRunArgs): Promise( `INSERT INTO subagent_tool_executions (job_id, message_idx, tool_use_id, tool_name, input, status, schema_version, ordinal, gbrain_tool_use_id, provider_id) - VALUES ($1, $2, $3, $4, $5::jsonb, 'pending', 2, $6, $7, $8) + VALUES ($1, $2, $3, $4, $5::text::jsonb, 'pending', 2, $6, $7, $8) ON CONFLICT (job_id, message_idx, ordinal) DO UPDATE SET status = subagent_tool_executions.status RETURNING gbrain_tool_use_id::text AS gbrain_tool_use_id`, @@ -891,7 +891,7 @@ async function runSubagentViaGateway(args: GatewayRunArgs): Promise { await engine.executeRaw( `UPDATE subagent_tool_executions - SET status = 'complete', output = $1::jsonb, ended_at = now() + SET status = 'complete', output = $1::text::jsonb, ended_at = now() WHERE gbrain_tool_use_id::text = $2`, [JSON.stringify(output ?? null), gbrainToolUseId], ); @@ -1107,7 +1107,7 @@ async function persistMessage(engine: BrainEngine, jobId: number, msg: Persisted await engine.executeRaw( `INSERT INTO subagent_messages (job_id, message_idx, role, content_blocks, tokens_in, tokens_out, tokens_cache_read, tokens_cache_create, model) - VALUES ($1, $2, $3, $4::jsonb, $5, $6, $7, $8, $9) + VALUES ($1, $2, $3, $4::text::jsonb, $5, $6, $7, $8, $9) ON CONFLICT (job_id, message_idx) DO NOTHING`, [ jobId, @@ -1131,13 +1131,15 @@ async function persistToolExecPending( toolName: string, input: unknown, ): Promise { - // Serialize to JSON string for the ::jsonb cast. When `input` is already a - // string (e.g. pre-serialized), avoid double-encoding which produces a jsonb - // scalar string instead of a jsonb object — breaking `input->>'key'` lookups. + // Serialize to a JSON string, then bind through $5::text::jsonb. The value is + // ALWAYS a string here (pre-serialized input, or JSON.stringify) — binding a + // string to a bare $5::jsonb double-encodes it into a jsonb scalar string under + // postgres.js .unsafe() (#2339 class; PGLite hides it). The ::text cast makes + // the text→jsonb parse produce a real jsonb object. const jsonStr = typeof input === 'string' ? input : JSON.stringify(input); await engine.executeRaw( `INSERT INTO subagent_tool_executions (job_id, message_idx, tool_use_id, tool_name, input, status) - VALUES ($1, $2, $3, $4, $5::jsonb, 'pending') + VALUES ($1, $2, $3, $4, $5::text::jsonb, 'pending') ON CONFLICT (job_id, tool_use_id) DO NOTHING`, [jobId, messageIdx, toolUseId, toolName, jsonStr], ); @@ -1151,7 +1153,7 @@ async function persistToolExecComplete( ): Promise { await engine.executeRaw( `UPDATE subagent_tool_executions - SET status = 'complete', output = $3::jsonb, ended_at = now() + SET status = 'complete', output = $3::text::jsonb, ended_at = now() WHERE job_id = $1 AND tool_use_id = $2`, [jobId, toolUseId, typeof output === 'string' ? output : JSON.stringify(output)], ); @@ -1170,7 +1172,7 @@ async function persistToolExecFailed( // rejected upfront) and "pending row exists" (tool threw mid-execute). await engine.executeRaw( `INSERT INTO subagent_tool_executions (job_id, message_idx, tool_use_id, tool_name, input, status, error, ended_at) - VALUES ($1, $2, $3, $4, $5::jsonb, 'failed', $6, now()) + VALUES ($1, $2, $3, $4, $5::text::jsonb, 'failed', $6, now()) ON CONFLICT (job_id, tool_use_id) DO UPDATE SET status = 'failed', error = EXCLUDED.error, ended_at = now()`, [jobId, messageIdx, toolUseId, toolName, typeof input === 'string' ? input : JSON.stringify(input), error], diff --git a/src/core/onboard/impact-capture.ts b/src/core/onboard/impact-capture.ts index 7db93a772..f1f622af7 100644 --- a/src/core/onboard/impact-capture.ts +++ b/src/core/onboard/impact-capture.ts @@ -120,7 +120,7 @@ export async function writeImpactLogRow( remediation_id, metric_name, metric_before, metric_after, job_id, source_id, brain_id, started_at, idempotency_key, applied_by, details - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11::jsonb)`, + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11::text::jsonb)`, [ attribution.remediation_id, metricName, diff --git a/src/core/op-checkpoint.ts b/src/core/op-checkpoint.ts index f1c0c1e2e..8705bc73b 100644 --- a/src/core/op-checkpoint.ts +++ b/src/core/op-checkpoint.ts @@ -179,15 +179,22 @@ export async function recordCompleted( // REPLACE semantics (kept deliberately — #1794 V3). Callers like // extract-conversation-facts serialize a MUTABLE map through here and rely on // stale keys being REMOVED; an append would make them unremovable. The full - // set lands in the parent `completed_keys` JSONB column via a single UPSERT — - // exactly as before. JSON.stringify into `$3::jsonb` is correct (the text→jsonb - // cast yields a proper array; NOT the double-encode trap, which is the template - // form). Sync uses `appendCompleted` (below) instead, never this. + // set lands in the parent `completed_keys` JSONB column via a single UPSERT. + // #2339: bind through `$3::text::jsonb`, NOT `$3::jsonb`. Under postgres.js + // `.unsafe(sql, params)` (executeRawDirect's path) a JS string bound to a + // `$N::jsonb` param double-encodes — the text→jsonb cast wraps the already-JSON + // string into a jsonb *string scalar*, which fails the v119 + // `op_checkpoints_completed_keys_array CHECK (jsonb_typeof = 'array')` and aborts + // every sync on real Postgres (PGLite parses it silently, which hid the bug). + // Casting through `text` first binds it as a plain text param so the text→jsonb + // cast parses it into a genuine jsonb array. This is the positional-param form of + // the CLAUDE.md double-encode trap (the grep guard only caught the template form). + // Sync uses `appendCompleted` (below, `unnest($3::text[])`) instead, never this. const sorted = [...keys].sort(); return durableWrite(engine, key, 'write', () => engine.executeRawDirect( `INSERT INTO op_checkpoints (op, fingerprint, completed_keys, updated_at) - VALUES ($1, $2, $3::jsonb, now()) + VALUES ($1, $2, $3::text::jsonb, now()) ON CONFLICT (op, fingerprint) DO UPDATE SET completed_keys = EXCLUDED.completed_keys, updated_at = now()`, diff --git a/src/core/search/hybrid.ts b/src/core/search/hybrid.ts index ee1b08c6e..27da6b8c1 100644 --- a/src/core/search/hybrid.ts +++ b/src/core/search/hybrid.ts @@ -647,6 +647,11 @@ export async function applyAliasHop( if (!page) continue; injectScore += 1e-6; out.push({ + // #2339-sibling: include page_id. The `as SearchResult` cast hid its + // absence, so any consumer reading page_id off an alias-injected result got + // undefined — e.g. listActiveTakesForPages bound undefined/NaN into + // ANY($1::int[]) and crashed the contradiction probe on real Postgres. + page_id: page.id, slug: page.slug, title: page.title, type: page.type, diff --git a/src/core/search/query-cache.ts b/src/core/search/query-cache.ts index dd68a12e8..6d0558264 100644 --- a/src/core/search/query-cache.ts +++ b/src/core/search/query-cache.ts @@ -236,7 +236,7 @@ export class SemanticQueryCache { // the v0.40.3.0 IRON-RULE). await this.engine.executeRaw( `INSERT INTO query_cache (id, query_text, source_id, knobs_hash, embedding, results, meta, ttl_seconds, page_generations, max_generation_at_store, created_at) - VALUES ($1, $2, $3, $4, $5::vector, $6::jsonb, $7::jsonb, $8, $9::jsonb, $10, now()) + VALUES ($1, $2, $3, $4, $5::vector, $6::text::jsonb, $7::text::jsonb, $8, $9::text::jsonb, $10, now()) ON CONFLICT (id) DO UPDATE SET query_text = EXCLUDED.query_text, knobs_hash = EXCLUDED.knobs_hash, diff --git a/src/core/sources-ops.ts b/src/core/sources-ops.ts index 3cb1c0a9a..a7d95ee77 100644 --- a/src/core/sources-ops.ts +++ b/src/core/sources-ops.ts @@ -408,7 +408,7 @@ export async function addSource( try { await engine.executeRaw( `INSERT INTO sources (id, name, local_path, config) - VALUES ($1, $2, $3, $4::jsonb)`, + VALUES ($1, $2, $3, $4::text::jsonb)`, [opts.id, displayName, finalPath, JSON.stringify(config)], ); } catch (e) { @@ -454,7 +454,7 @@ export async function addSource( const displayName = opts.name ?? opts.id; await engine.executeRaw( `INSERT INTO sources (id, name, local_path, config) - VALUES ($1, $2, $3, $4::jsonb)`, + VALUES ($1, $2, $3, $4::text::jsonb)`, [opts.id, displayName, finalPath, JSON.stringify(config)], ); } diff --git a/src/core/sql-query.ts b/src/core/sql-query.ts index 2fcae4556..320820877 100644 --- a/src/core/sql-query.ts +++ b/src/core/sql-query.ts @@ -79,15 +79,22 @@ function assertSqlValue(value: unknown): asserts value is SqlValue { * auth/admin surface that a focused helper preserves the contract without * forcing every call site to remember which positions hold JSONB. * - * Why this is safe vs the v0.12.0 double-encode bug: the bug was specific - * to postgres.js's template-tag auto-stringify path interacting with - * sql.json() — not to positional binding through `unsafe()`. JS objects - * passed as positional params reach the wire protocol with the correct - * type oid (jsonb when cast in the SQL string), so there is no double- - * encode. The CI guard (scripts/check-jsonb-pattern.sh) doesn't fire - * because the source pattern is a method call (`executeRawJsonb(...)`), - * not the banned literal-template-tag interpolation pattern with - * JSON.stringify cast to jsonb. + * Why this is safe vs the double-encode bug: this helper binds a JS **object** + * (not a pre-stringified string) to each `$N::jsonb` position. postgres.js + * `unsafe()` and PGLite both serialize a JS object to the jsonb wire type + * correctly, so there is no double-encode. + * + * IMPORTANT (the #2339 distinction): positional binding is NOT universally safe. + * Binding `JSON.stringify(x)` (a **string**) to a `$N::jsonb` position via + * `unsafe()`/`executeRawDirect` DOES double-encode — the text→jsonb cast wraps + * the already-JSON string into a jsonb *string scalar* (PGLite hides it; real + * Postgres exposes it, and it broke every sync in #2339). The fixes are: pass a + * raw object (this helper), or cast through `$N::text::jsonb` so the string is + * parsed, never `$N::jsonb` + JSON.stringify. The legacy grep guard + * (scripts/check-jsonb-pattern.sh) only caught the template-tag form; the + * positional `$N::jsonb` + JSON.stringify form is caught by the AST guard + * scripts/check-jsonb-params.mjs. This helper's `executeRawJsonb(...)` method-call + * shape trips neither guard because it passes objects, which is correct. * * Usage: * await executeRawJsonb( diff --git a/src/core/takes-quality-eval/receipt-write.ts b/src/core/takes-quality-eval/receipt-write.ts index 5bb77f719..637670dae 100644 --- a/src/core/takes-quality-eval/receipt-write.ts +++ b/src/core/takes-quality-eval/receipt-write.ts @@ -32,8 +32,8 @@ export async function writeReceiptToDb(engine: BrainEngine, receipt: TakesQualit receipt_json, receipt_disk_path, created_at ) VALUES ( $1, $2, $3, $4, - $5, $6, $7, $8::jsonb, $9, - $10::jsonb, $11, $12::timestamptz + $5, $6, $7, $8::text::jsonb, $9, + $10::text::jsonb, $11, $12::timestamptz ) ON CONFLICT (receipt_sha8_corpus, receipt_sha8_prompt, receipt_sha8_models, receipt_sha8_rubric) DO NOTHING`, diff --git a/test/check-jsonb-params.test.ts b/test/check-jsonb-params.test.ts new file mode 100644 index 000000000..26e1b43ed --- /dev/null +++ b/test/check-jsonb-params.test.ts @@ -0,0 +1,83 @@ +/** + * Self-test for scripts/check-jsonb-params.mjs — the positional jsonb + * double-encode guard (#2339 / #2324 class). Verifies it catches the bug shape + * (including generic-typed calls and the `jsonStr` variable case is acknowledged + * as out of scope) and does NOT false-positive on the sanctioned forms. + * + * Fixtures are written to a temp dir and the scanner is pointed at it via argv, + * so this never touches the real src/ tree. + */ +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { mkdtempSync, writeFileSync, mkdirSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +const SCRIPT = join(import.meta.dir, '..', 'scripts', 'check-jsonb-params.mjs'); + +let root: string; +let badDir: string; +let goodDir: string; + +function runGuard(dir: string): { code: number; err: string } { + const res = Bun.spawnSync([process.execPath, SCRIPT, dir]); + return { code: res.exitCode, err: res.stderr.toString() + res.stdout.toString() }; +} + +beforeAll(() => { + root = mkdtempSync(join(tmpdir(), 'jsonb-guard-')); + badDir = join(root, 'bad'); + goodDir = join(root, 'good'); + mkdirSync(badDir, { recursive: true }); + mkdirSync(goodDir, { recursive: true }); + + // BAD: positional $1::jsonb bound to a JSON.stringify'd value. + writeFileSync( + join(badDir, 'bad.ts'), + "await engine.executeRaw(`INSERT INTO t (a) VALUES ($1::jsonb)`, [JSON.stringify(x)]);\n", + ); + // BAD: generic-typed executeRaw(...) must still be caught. + writeFileSync( + join(badDir, 'bad_generic.ts'), + "await engine.executeRaw<{ id: string }>(`UPDATE t SET a = $2::jsonb WHERE id = $1`, [id, JSON.stringify(x)]);\n", + ); + + // GOOD: the fix — $1::text::jsonb. + writeFileSync( + join(goodDir, 'good_text.ts'), + "await engine.executeRaw(`INSERT INTO t (a) VALUES ($1::text::jsonb)`, [JSON.stringify(x)]);\n", + ); + // GOOD: text[] array path (the appendCompleted unnest shape). + writeFileSync( + join(goodDir, 'good_array.ts'), + "await engine.executeRaw(`INSERT INTO t (a) SELECT unnest($1::text[])`, [JSON.stringify(arr)]);\n", + ); + // GOOD: executeRawJsonb passes a raw object, not a string — excluded. + writeFileSync( + join(goodDir, 'good_helper.ts'), + "await executeRawJsonb(engine, `INSERT INTO t (a) VALUES ($1::jsonb)`, [], [JSON.stringify(x)]);\n", + ); + // GOOD: explicit opt-out for a rare legitimate object-binding case. + writeFileSync( + join(goodDir, 'good_optout.ts'), + "await engine.executeRaw(`INSERT INTO t (a) VALUES ($1::jsonb)` /* jsonb-guard-ok */, [JSON.stringify(x)]);\n", + ); +}); + +afterAll(() => { + rmSync(root, { recursive: true, force: true }); +}); + +describe('check-jsonb-params guard', () => { + test('flags positional $N::jsonb + JSON.stringify (incl. generic-typed calls)', () => { + const { code, err } = runGuard(badDir); + expect(code).toBe(1); + expect(err).toContain('bad.ts'); + expect(err).toContain('bad_generic.ts'); + }); + + test('passes the sanctioned forms (::text::jsonb, ::text[], executeRawJsonb, opt-out)', () => { + const { code, err } = runGuard(goodDir); + expect(code).toBe(0); + expect(err).toContain('clean'); + }); +}); diff --git a/test/e2e/op-checkpoint-jsonb-parity.test.ts b/test/e2e/op-checkpoint-jsonb-parity.test.ts new file mode 100644 index 000000000..42f8abdc7 --- /dev/null +++ b/test/e2e/op-checkpoint-jsonb-parity.test.ts @@ -0,0 +1,80 @@ +/** + * E2E: op_checkpoints.completed_keys JSONB parity — #2339 regression guard. + * + * #2339: `recordCompleted` bound `JSON.stringify(array)` to a `$3::jsonb` param + * via postgres.js `.unsafe()` (executeRawDirect). That double-encodes the value + * into a jsonb *string scalar*, which violates the v119 + * `op_checkpoints_completed_keys_array CHECK (jsonb_typeof = 'array')` and aborts + * EVERY sync on real Postgres at the first checkpoint write. PGLite's driver + * parses the string silently, which is exactly why the unit suite stayed green + * and the bug shipped — so this assertion can ONLY be made on real Postgres. + * + * This file uses the standard `hasDatabase()` skip gate (consistent with the + * other e2e tests). The X2-A guarantee that it actually RUNS lives in a dedicated + * CI job (.github/workflows) that provisions a Postgres service so DATABASE_URL + * is always present there — rather than a fail-on-skip hack inside this file, + * which would red-fail legitimate DB-less local runs. + * + * Fix under test: `$3::text::jsonb` binds the value as text, so the text→jsonb + * cast parses it into a genuine jsonb array. + */ +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { hasDatabase, setupDB, teardownDB, getEngine, getConn } from './helpers.ts'; +import { recordCompleted, loadOpCheckpoint } from '../../src/core/op-checkpoint.ts'; + +const describeE2E = hasDatabase() ? describe : describe.skip; + +describeE2E('E2E: op_checkpoints completed_keys jsonb parity (#2339)', () => { + beforeAll(async () => { + await setupDB(); + }); + afterAll(async () => { + await teardownDB(); + }); + + const key = { op: 'sync-target', fingerprint: 'jsonb-parity-2339' }; + + test('recordCompleted stores completed_keys as a jsonb ARRAY, not a double-encoded scalar', async () => { + const engine = getEngine(); + + // Pre-fix this throws SQLSTATE 23514 (the CHECK), durableWrite exhausts its + // retries, and recordCompleted returns false. Post-fix it stores a real array. + const ok = await recordCompleted(engine, key, ['b/2.md', 'a/1.md', 'c/3.md']); + expect(ok).toBe(true); + + const sql = getConn(); + const [row] = await sql` + SELECT jsonb_typeof(completed_keys) AS t, + jsonb_array_length(completed_keys) AS len, + completed_keys ->> 0 AS first + FROM op_checkpoints + WHERE op = ${key.op} AND fingerprint = ${key.fingerprint} + `; + expect(row.t).toBe('array'); // pre-fix: 'string' (scalar) — the bug + expect(Number(row.len)).toBe(3); + expect(row.first).toBe('a/1.md'); // recordCompleted sorts the set + }, 30_000); + + test('loadOpCheckpoint round-trips the recorded set', async () => { + const engine = getEngine(); + const got = await loadOpCheckpoint(engine, key); + expect(new Set(got)).toEqual(new Set(['a/1.md', 'b/2.md', 'c/3.md'])); + }, 30_000); + + test('REPLACE semantics: re-recording a smaller set drops stale keys (stays an array)', async () => { + const engine = getEngine(); + const ok = await recordCompleted(engine, key, ['only/1.md']); + expect(ok).toBe(true); + + const got = await loadOpCheckpoint(engine, key); + expect(new Set(got)).toEqual(new Set(['only/1.md'])); + + const sql = getConn(); + const [row] = await sql` + SELECT jsonb_typeof(completed_keys) AS t + FROM op_checkpoints + WHERE op = ${key.op} AND fingerprint = ${key.fingerprint} + `; + expect(row.t).toBe('array'); + }, 30_000); +});