Skip to content

v0.46.1.0 feat(minions,db): per-job process isolation + pool-starvation fixes (garrytan-agents#5, #6) - #4151

Merged
garrytan merged 18 commits into
masterfrom
garrytan/fix-issue-5
Aug 15, 2026
Merged

garrytan merged 18 commits into
masterfrom
garrytan/fix-issue-5

Conversation

@garrytan

Copy link
Copy Markdown
Owner

Summary

Fixes garrytan-agents#6 (client-side connection-pool starvation misdiagnosed as "DB unreachable") and implements garrytan-agents#5 (opt-in per-job process isolation). One PR, atomic bisect-friendly commits, #6 root-cause lane first.

Pool starvation: cancel, don't abandon (#6)

  • Every site that raced a query against a timer and abandoned the loser now cancels it via AbortSignal → postgres.js .cancel(), releasing the checked-out slot: the worker health probe, minion lock renewal (runLockRenewalTick + queue.renewLock), the cycle-drain renewal (extracted, guarded runDrainRenewalTick), submit-time queue probes (probeQueueState), and the DB-lock heartbeat (withRefreshingLock).
  • withReservedConnection routes long holds (CREATE INDEX CONCURRENTLY, non-transactional migration DDL, backfill BEGIN..COMMIT — the observed 353s COMMIT) to the direct session lane, capped at direct_pool_size − 1 concurrent reserves (no floor: at size ≤ 1 it stays on the read pool) so claim/renewal heartbeats always keep a slot; overflow falls back to today's behavior.
  • Explicit max_lifetime on all four client pools as a per-connection jitter function (matches the postgres.js built-in shape) + GBRAIN_POOL_MAX_LIFETIME_S escape hatch. Framed honestly: an env knob, not a starvation fix — max_lifetime can't reclaim a leaked checkout.

Honest diagnostics (#6)

  • The health probe disambiguates via the direct lane: pool_starved ("server IS reachable; the fault is in the transaction-pooler path") vs server_unreachable vs unknown — replacing the blanket "DB unreachable" that sent operators debugging database capacity at 10% utilization. Gauge counts are labeled a tracked subset; no invented waiter arithmetic.
  • Single-pool collapse (direct-lane kill switch) now warns loudly at worker startup; a verdict-interpretation table ships in docs/guides/queue-operations-runbook.md.

Per-job process isolation (#5)

  • gbrain jobs work --job-isolation process (supervisor pass-through, GBRAIN_JOB_ISOLATION env): each claimed job runs in a detached, tini-wrapped child via an internal jobs run-child entrypoint with its own bounded pools. The parent keeps claim/renewal/recording; results travel by atomic outcome file; force-eviction becomes a real process-group SIGTERM → +25s SIGKILL (Bun rejects negative pids — process.kill should accept negative int oven-sh/bun#15791 — so group signaling falls back to /bin/kill, with loud degradation logging).
  • Attempt semantics preserved and hardened: handler errors reconstruct to the exact classes the worker branches on; worker shutdown gives children the drain window and never burns an attempt (including cooperative handlers that report an error mid-deploy, and RSS-watchdog drains); spawn failures release with no attempt burned + a 3-consecutive-failure circuit breaker exits the worker instead of looping invisibly; reclaimed-job children no-op behind the lock-token fence.

Review hardening (in-branch)

  • 23-agent adversarial workflow (12 confirmed findings fixed), 5 ship specialists + red team (3 confirmed criticals fixed: the size=1 reserve-cap floor, silent group-kill failure, the spawn-failure blind loop), plus maintainability/security/perf/data-migration fixes (per-connection max_lifetime jitter, lease shape validation, async outcome decode, shared abort-reason constants).

Test Coverage

Coverage gate: PASS at 82% (47/57 traced paths; target 80). 13 new test files + 6 extended; every new branch in the isolation protocol, kill escalation, shutdown classification matrix, gauge seams, probe verdict matrix, and reserve routing is behavior-pinned (including real-process grandchild-death for the Bun /bin/kill fallback). Remaining gaps are operator-flow message tests + e2e-lane negative tests, filed in TODOS.md ("isolation test-gap follow-ups").

Key pins: child-job-runner (real children: success/env-contract, error/lease reconstruction,
SIGTERM-ignorer → group SIGKILL, both shutdown semantics, watchdog double-abort, exit 13/14,
pool-size env matrix) · run-child-entry (real PGLite + claim-minted token: fence exit-14,
SIGTERM signal separation, {value:x} wrap parity, missing-handler unrecoverable, orphan watchdog
with a real reaped pid) · worker-job-isolation (fenced completeJob/failJob, spawn-release,
serialization parity, constructor guard, spawn-failure circuit breaker) · db-probe verdict
matrix + dual-pool gating · pool-gauge leak guards (reject + sync-throw + begin()-throw) ·
reserved-routing semaphore (TOCTOU, size=1, ddl-throw fallback) · renewal/probe cancellation
(signal fired on timeout) · max_lifetime resolver + two real-pool wiring pins

Pre-Landing Review

31 findings (3 critical from red team, 28 informational from 5 specialists) — 3 critical fixed, 21 informational fixed/auto-fixed, 7 deferred with TODOS entries. Adversarial workflow earlier in the wave: 12 confirmed findings, all fixed. Checklist passes (SQL safety, races, trust boundary, enum completeness): clean.

Design Review

No frontend files changed — design review skipped.

Eval Results

No prompt-related files changed — evals skipped.

Scope Drift

Scope Check: CLEAN. Intent: fix garrytan-agents#5 + #6 per the approved plan. Delivered: exactly that + review-driven hardening of the same surfaces.

Plan Completion

50 plan items audited: 41 DONE, 6 CHANGED (equivalent-or-stronger shipped behavior — e.g. SIGTERM fires shutdownSignal only per the adversarial P2 fix), 2 deferred (CHANGELOG/version → this ship's release commit ✅ since landed; help-text test ✅ since added), 1 unverifiable (manual smoke — superseded by the ship gates). Both audit gaps were closed before push.

Verification Results

No dev server (CLI daemon project) — plan verification ran as: typecheck clean, 44/44 verify guards, wave-owned test files 50/50 in isolation at HEAD, full-suite parallel failures identical to the master baseline (env-dependent class; isolated A/B: 290 pass / 5 fail on BOTH).

TODOS

  • Completed: "cancel (not just abandon) timed-out submit-time queue probes" (P3) — removed with the fix.
  • Filed: 13 follow-ups under "Issues feat: OpenClaw plugin — Phase 1 core search #5+SQLite integration #6 follow-ups" (nested-checkout P1-companion audit, per-handler isolation policy, per-child RSS caps, connection-budget clamp, isolation test gaps, raceWithAbortTimeout DRY helper, lazy handler resolution, Sql-proxy instrumentation, doctor connection_routing, and more).

Documentation

Doc diff preview (two /document-release commits: 8024412 sweep, ec3d7e7 cross-model review fixes):

  • README.md — Job queue (Minions) capability paragraph now names opt-in per-job process isolation (gbrain jobs work --job-isolation process) and the health-probe layer verdicts (pool_starved vs server_unreachable), linking docs/guides/minions-deployment.md (sizing/rollout) and docs/guides/queue-operations-runbook.md (verdict triage).
  • docs/architecture/KEY_FILES.md — postgres-engine.ts entry gains the withReservedConnection direct-lane routing invariants (never inside an open tx; no minimum-1 semaphore floor; read-pool overflow; getPoolDiagnostics() tracked-subset contract). Corrections: run-child SIGTERM fires shutdownSignal ONLY (both signals only on parent death); third no-burn child class ChildNotClaimedError documented.
  • docs/guides/minions-deployment.md — group-SIGKILL platform caveat (Bun /bin/kill fallback), lock-token fencing scoped to queue writes, connection math relabeled (55 pooler-lane vs 18 direct session-lane at the worked example), no-per-child-RSS-cap note, GBRAIN_JOB_CHILD_CLI + the 3-consecutive-spawn-failure breaker documented.
  • docs/guides/queue-operations-runbook.md — verdict rides the TERMINAL probe line; server_unreachable hedged (both-lanes-failed is the evidence); pooler-layer fault added to the 0 in flight reading; jobs cancel described honestly (cooperative inline vs real kill under isolation).
  • docs/TESTING.md — e2e job-isolation legs described accurately; 13 new unit-test inventory entries.
  • CHANGELOG.md — 0.46.1.0 entry; llms-full.txt regenerated (freshness 12/12).

Test plan

  • bun run typecheck clean
  • bun run verify 44/44 guards green (lock-renewal shape, jsonb patterns, flag registry, KEY_FILES current-state, bootstrap stamps/templates)
  • Wave-owned + minions-adjacent test files: 50/50 in isolation at HEAD; review-fix batch 338 tests green
  • Full-suite parallel failures identical to master baseline (env-dependent class — isolated A/B: 290/5 on both)
  • e2e lane wired explicitly into .github/workflows/e2e.yml tier1 (job-isolation on real Postgres runs in CI)

🤖 Generated with Claude Code

garrytan and others added 18 commits August 15, 2026 07:38
…f orphaning pool slots (#6)

Three hot paths raced a live query against a timer and abandoned the loser,
leaving the query holding a checked-out pool slot for its full server-side
duration. Under a saturated transaction-mode pooler those orphaned slots
starve lock renewal ('lock-renewal-failed' cascades) and the health probe.

- Health probe: pass the deadline AbortController's signal into
  executeRaw('SELECT 1') so a hung probe is cancelled via postgres.js
  .cancel() (runUnsafe already wires signal -> pending.cancel()).
- Minion lock renewal: LockRenewalDeps.renewLock widened with optional
  { signal }; runLockRenewalTick aborts a per-call controller when the
  timeout wins the race; MinionQueue.renewLock forwards the signal to
  executeRawDirect. Optional-param widening keeps the 14 existing hermetic
  tests compiling untouched.
- Cycle drain renewal (synthesize.ts): the inline best-effort tick had no
  per-call timeout and no re-entrancy guard, so a hung renewLock stacked a
  fresh checked-out slot per interval firing. Extracted as exported
  runDrainRenewalTick (per-call signal + timeout + swallow) behind a
  tick-in-flight guard.

Tests: 2 new signal paths in worker-lock-renewal.test.ts, probe-signal
assertion in worker-supervised-db-probe.test.ts, new hermetic
minion-queue-renewlock-signal.test.ts + cycle-drain-renewal.test.ts.
scripts/check-worker-lock-renewal-shape.sh stays green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Same abandoned-racer class as the previous commit, in two more spots:

- probeQueueState raced probeQueueStateInner against its 1500ms budget but
  the losing wedge/age queries kept running on the pool after the race
  resolved — under pool exhaustion (the exact regime the probe exists to
  detect) the orphaned query held a slot and made the exhaustion worse. The
  timeout now aborts a per-probe signal threaded through queryWedgeSignals
  and the oldest-waiting age query. Closes the filed TODOS entry.
- withRefreshingLock raced handle.refresh() against heartbeatTimeoutMs the
  same way; DbLockHandle.refresh now accepts { signal } (Postgres forwards
  to executeRawDirect; PGLite ignores it — no pool to starve), the timeout
  aborts it, and a re-entrancy guard stops overlapping ticks (15s min
  cadence vs 30s default timeout could stack two).

Tests: new hermetic queue-probe-cancellation.test.ts (signal threading,
timeout-aborts, fast-path-not-aborted, fail-open contract).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Makes the pool connection lifetime explicit at every postgres() call site
(db.ts module singleton, engine instance pool, ConnectionManager read +
direct pools) and adds GBRAIN_POOL_MAX_LIFETIME_S as an incident escape
hatch (N seconds; 0 disables recycling).

NOT a behavior change at default: postgres.js (verified against the pinned
3.4.9) already defaults max_lifetime to 60*(30+rand*30) — 30-60 min,
jittered per pool — and max_lifetime only recycles connections as they
return to the pool; it cannot reclaim a leaked checkout. Framed accordingly:
explicitness + operator knob, not a fix for the starvation class (that is
the cancellation work in the two prior commits).

Tests: hermetic resolver suite (env forms, 0-disables, jitter bounds,
warn-once on invalid values, per-call jitter variance).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…server unreachable (#6)

"[health] DB unreachable" sent operators chasing database capacity while the
real fault was client-side: the server sat at ~10% of max_connections. The
probe now names the failing layer:

- New src/core/minions/db-probe.ts (hermetic, injected-deps — the
  lock-renewal-tick pattern): on read-pool probe failure, a 3s direct-lane
  SELECT 1 disambiguates. Direct OK -> verdict 'pool_starved' ("server IS
  reachable; the fault is in the transaction-pooler path — client pool
  exhaustion or a pooler-layer fault", deliberately an honest disjunction).
  Both fail -> 'server_unreachable'. No direct lane -> 'unknown'. Both
  probes carry AbortSignals — a hung probe is cancelled, never abandoned.
- New src/core/pool-gauge.ts: approximate in-flight counters at the engine's
  raw/direct/reserved/transaction seams, surfaced via a duck-typed
  PostgresEngine.getPoolDiagnostics() (no BrainEngine churn, no PGLite
  stub). Explicitly labeled a tracked SUBSET — template-path traffic is
  untracked and no waiter/available figures are derived (that would be
  invented telemetry). Counters use try/finally (runUnsafe throws
  synchronously on a pre-aborted signal) and clamp at zero.
- worker.ts probe adapter emits the verdict in every failure line and on the
  final unhealthy payload; exit semantics UNCHANGED (exiting on a starved
  pool is correct recovery — it frees all client-held slots).
- jobs.ts: verdict-aware fatal text, plus a startup warning when a
  Supabase-shaped engine is running single-pool (kill-switch collapse used
  to be silent — renewal + probes + workload all sharing one pool is the
  precondition for this incident class).
- Runbook: verdict interpretation table in queue-operations-runbook.md.

Tests: pool-gauge.test.ts (pure + engine seams incl. rejected-query and
sync-throw leak guards), db-probe.test.ts (full verdict matrix, signal
cancellation, fail-open diagnostics, no-waiter-wording pin).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… dual-pool is active (#6)

Long-hold reserved work — CREATE INDEX CONCURRENTLY (vector-index), non-
transactional migration DDL, and backfill BEGIN..COMMIT batches (the observed
353s COMMIT session) — previously reserved from the worker's shared READ
pool, pinning slots under the 5-min pooler statement_timeout. It now reserves
from the DIRECT session lane, whose 30-min statement_timeout and
maintenance_work_mem GUCs are the right fit, and stops competing with handler
workload.

Heartbeat protection: concurrent direct reserves are capped at
directPoolSize - 1 (default 2 of 3) via a per-process semaphore so
claim/renewLock always keep >= 1 direct slot; overflow falls back to the
read pool — exactly the pre-change behavior, so this commit is strictly
never-worse than master. (Deliberate rejection of queue-for-a-permit: that
would block migrations behind multi-minute index builds. Per-process is the
correct scope: each process owns its own direct pool, so a CLI migration
cannot starve a worker's heartbeats.) Never rerouted inside an open
transaction (same guard shape as executeRawDirect); kill-switch collapse
degrades to status quo. Callers unchanged.

Tests: postgres-engine-reserved-routing.test.ts — direct when active, read
when kill-switched/in-tx, semaphore cap + overflow + permit release on fn
throw and on reserve() failure.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ontext builder (#5)

Foundation for per-job process isolation (no behavior change yet):

- job-isolation.ts — the parent<->child protocol: atomic outcome-file codec
  (tmp+rename; 32MiB decode cap that throws UnrecoverableError so oversize
  results die LOUDLY on attempt 1 instead of retrying identically or being
  silently truncated; decode errors report byte counts, never file content),
  handler-error encode/reconstruct preserving the two instanceof branches
  executeJob dispatches on (UnrecoverableError, RateLeaseUnavailableError),
  child argv/env contract, child-CLI resolution (env override -> compiled
  binary -> bun-dev fallback -> null for fail-fast), and killProcessGroup —
  children run detached in their own process group because SIGKILL on a tini
  pid alone kills tini and orphans the handler grandchild (tini cannot
  forward SIGKILL), and Bun rejects negative pids in process.kill()
  (oven-sh/bun#15791) so group signaling falls back to POSIX /bin/kill.
- worker-exit-codes.ts — reserved run-child codes 13 (usage/PGLite),
  14 (not claimed / token mismatch), 15 (result-write failed). Result-file
  presence, not the exit code, classifies the normal path: a reported
  handler FAILURE is still exit 0.
- job-context.ts — MinionJobContext builder extracted verbatim from
  executeJob so the child wires the exact same token-fenced DB callbacks;
  worker.ts now calls it (behavioral no-op, full minions suite green).

Tests: job-isolation-protocol.test.ts — codec round-trip + all decode
failure paths, instanceof reconstruction, invocation resolution, and REAL
detached-process group-kill tests incl. the grandchild-death guarantee
(runs under bun test, so the Bun negative-pid fallback is exercised for
real, not mocked).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The child side of process isolation. `gbrain jobs run-child --job-id N`
(internal; spawned by the worker, absent from user help):

- re-reads the job row and validates status='active' + lock-token match
  before running anything — a reclaimed/cancelled job exits 14 with the
  handler never invoked (the DB stays ground truth; no payload
  serialization across the boundary);
- registers the same handler surface as the worker via
  registerBuiltinHandlers({quiet}) — which includes plugin discovery, so
  plugin subagent jobs isolate identically — resolved through the new
  MinionWorker.getHandler() accessor;
- builds the shared token-fenced MinionJobContext against the CHILD's own
  engine, runs the handler, and writes ONE atomic outcome file: handler
  failure is an encoded error outcome with exit 0 (a reported failure is a
  successful report); only write-failure exits 15;
- runs NO worker machinery (no probe/stall/lock timers — the parent owns
  liveness). Installs a SIGTERM handler (fires ctx.signal + shutdownSignal
  so handlers get the drain window to finish and report) and a
  parent-liveness watchdog polling process.kill(parentPid, 0) — a ppid
  check is dead code under tini — that aborts the handler and hard-exits
  after a grace so orphaned LLM-bound work stops burning spend;
- CLI layer owns engine.disconnect() + process.exit() (engine-ownership
  invariant); PGLite exits 13 (isolation is Postgres-only, like jobs work).

Flag registry regenerated for the internal job-id flag.

Tests: run-child-entry.test.ts against real in-memory PGLite with a REAL
claim-minted token — success (incl. a fenced updateProgress landing),
handler-failure outcome, token-mismatch never-runs, missing job, missing
handler, and the parent-death watchdog aborting a live handler.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hildren (#5)

The parent-side seam. executeJob swaps ONE line — handler(context) vs
runJobInChild(...) — and every existing reporting branch (completeJob,
failJob dead/delayed, lease release, infra-abort no-burn) is reused verbatim
on the child's reconstructed outcome. Blast radius of a stuck or crashing
handler drops from N in-flight jobs to exactly one.

child-job-runner.ts:
- detached spawn (own process group) + tini wrap when available; stdio
  ['ignore','inherit','inherit'] so handler logs stream to the operator;
  per-job lifecycle log lines (spawned / exited code+signal);
- per-job abort -> group SIGTERM now, group SIGKILL at +25s (inside the 30s
  force-evict window, which stays as an untouched backstop) — force-eviction
  is now a real kill, not an abandonment;
- worker shutdown -> same SIGTERM so the child's handlers get the drain
  window to finish AND report; a child that reported before the kill
  completes normally; one that couldn't throws ChildWorkerShutdownError,
  which the worker RELEASES with no attempt burned — routine deploys must
  not burn attempts (codex-2 #7);
- pre-exec spawn failure -> ChildSpawnInfraError, also released with no
  attempt burned (one bad CLI path must not dead-letter a queue);
- child env contract: fenced lock token, outcome path, parent pid for the
  orphan watchdog, GBRAIN_POOL_SIZE=3 + GBRAIN_DIRECT_POOL_SIZE=1 bounds
  (children run no heartbeats; sockets die with the process — the point).

worker.ts: MinionWorkerOpts gains jobIsolation / childCliInvocation /
childTiniPath (defaults preserve inline behavior exactly); when isolated the
parent-side MinionJobContext is not built at all (the child builds its own).

Tests: child-job-runner.test.ts (real .mjs children: success + env contract,
error/lease outcome reconstruction, crash, SIGTERM-ignorer -> group SIGKILL,
pre-aborted, spawn ENOENT, both shutdown semantics);
worker-job-isolation.test.ts (real PGLite worker end-to-end: claim -> child
-> fenced completeJob with the REAL claim token, failJob on error outcome,
crash burns attempt, spawn failure releases with zero attempts burned).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The user-facing surface for per-job process isolation:

- jobs work --job-isolation <inline|process> (space or = form; env fallback
  GBRAIN_JOB_ISOLATION; default inline — fully opt-in). With 'process' the
  worker resolves the child CLI ONCE at startup (GBRAIN_JOB_CHILD_CLI ->
  compiled binary -> bun-dev fallback) and REFUSES to start on an
  unresolvable/nonexistent path — a bad path discovered per-job would stall
  the queue one released claim at a time. detectTini() wraps children when
  available. Startup banner names the mode + child CLI; combining with
  --max-rss prints a note that the watchdog now covers the worker only.
- jobs supervisor --job-isolation passes through via buildWorkerArgs as a
  CONDITIONAL push — inline/omitted keeps existing deployments' worker argv
  byte-identical (pinned arrays in supervisor-build-worker-args.test.ts are
  untouched; two new cases added).
- pool_starved fatal text now names the flag as a remedy (handler
  connections die with each job's child).
- help text for work + supervisor + the jobs index; flag registry
  regenerated.
- NEW test/e2e/job-isolation.test.ts, wired into e2e.yml tier1 EXPLICITLY —
  the workflow runs only named files (no glob), so an unwired e2e file would
  be silent coverage loss. Legs: concurrency-3 isolated drain through real
  children against real Postgres (the child-pool topology), and the REAL
  `jobs run-child` CLI entrypoint end-to-end (engine bootstrap, quiet
  handler registry, token validation, outcome protocol). Follows the #4128
  ambient-URL-guard conventions (explicit env in the e2e lane).
- serialization parity (codex-2 #8): a non-JSONB-serializable result fails
  loudly in BOTH modes (inline completeJob serialization vs child exit 15) —
  isolation never falsely completes a job inline mode would have failed.

Tests: jobs-isolation-flag.test.ts (parser matrix), extended
supervisor-build-worker-args + worker-job-isolation, cli-flag-validation
green via regen, jobs-subcommand-help.serial green (engine-free help path).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ce; file the follow-ups (#5, #6)

- KEY_FILES.md: entries for the six new modules (job-isolation,
  child-job-runner, run-child, job-context, db-probe, pool-gauge) and
  current-state updates for worker/queue/supervisor/jobs/db/db-lock/
  lock-renewal-tick/synthesize.
- minions-deployment.md: a --job-isolation section modeled on --nice — how
  the parent/child split works, preserved error semantics, orphan story, and
  the sizing notes (pooler CLIENT connection math: concurrency 15 ~ 73;
  --max-rss covers the worker only; spawn cost guidance; the lock token is a
  fencing token, not a secret).
- TESTING.md: inventory entries for the 12 new unit files + the e2e lane
  (which is wired EXPLICITLY into e2e.yml tier1 — no glob exists).
- TODOS.md: filed the 10 follow-ups, headlined by the P1-companion
  nested-checkout audit (the strongest remaining #6 root-cause candidate —
  this wave mitigates the starvation class and fixes the diagnostic; it does
  not claim to close every leak path), plus per-handler isolation policy,
  per-child RSS caps, the connection-budget clamp, autopilot pass-through,
  connection-audit release events, the doctor connection_routing check, and
  Sql-proxy checkout instrumentation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ntext builder

The deadline-plumbing structural test grepped worker.ts for the literal
deadlineAtMs derivation, which moved verbatim into job-context.ts (the
builder shared by inline mode and 'jobs run-child'). The pin now checks the
derivation in job-context.ts AND that worker.ts calls buildJobContext — the
same contract, at its new home.

Full-suite triage note: an isolated A/B of the 22 files that failed in the
parallel full-suite run shows IDENTICAL results on this branch and on the
master base (290 pass / 5 fail — doctor-minions-check + unified-multimodal,
both env-dependent) — zero regression delta from this wave.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ntics, TOCTOU semaphore, gauge + parity gaps (#5, #6)

A 23-agent adversarial review (5 dimension finders + per-finding refuters)
over the wave's diff confirmed 12 defects; all fixed here:

- [P2] run-child conflated worker SIGTERM with the per-job abort: cooperative
  handlers bailed mid-deploy, reported an error outcome, and the parent
  BURNED an attempt per routine deploy — while signal-ignoring handlers got
  the no-burn release (the exact inversion of the shutdown guarantee).
  SIGTERM now fires ONLY shutdownSignal (inline signal-separation parity —
  handlers finish + report inside the drain window), parent death still
  aborts both, and the parent classifies an ERROR outcome that arrives
  during shutdown as ChildWorkerShutdownError (released, not burned; a
  genuinely-failing job coinciding with a deploy gets one free retry).
- [P2] the reserved-direct semaphore was a check-then-increment spanning
  `await ddl()` — same-tick concurrent reserves could overshoot the cap and
  starve the heartbeat slot it exists to protect. The permit is now taken in
  the same synchronous frame as the check.
- [P3] RSS-watchdog drain (gracefulShutdown aborts BOTH signals, reason
  'watchdog') was classified as a per-job abort and burned attempts on
  innocent isolated jobs. Shutdown classification now wins unless the
  per-job reason is job-targeted (timeout/cancel/lock-*).
- [P3] force-evict's failJob('dead') could race executeJob's own recording
  in isolation mode (group SIGKILL at 25s + slow decode > 30s window) and
  dead-letter a job with attempts remaining — skipped when isolated (the
  inFlight eviction, which is what unblocks the worker, stays).
- [P3] child bootstrap exits were burned as handler crashes: exit 13 →
  ChildSpawnInfraError (release), exit 14 → new ChildNotClaimedError
  (release; the claim is provably owned elsewhere).
- [P3] missing handler in the child was 'generic' (retried to max_attempts)
  vs inline's immediate dead-letter — now 'unrecoverable' (parity).
- [P3] result-shape parity: the {value: x} wrap now happens CHILD-side,
  before JSON serialization, so Date/toJSON results can't flip the wrap
  decision across the boundary.
- [P3] child env no longer raises a stricter user GBRAIN_POOL_SIZE (pooler
  MaxClients tuning respected; explicit GBRAIN_JOB_CHILD_POOL_SIZE wins;
  invalid values fall back instead of flowing to the 10-conn fallback).
- [P3] transaction() gauge used a chained .finally that a synchronous
  begin() throw (nested tx on a clone) would skip — now try/finally.
- [P3 vacuity x3] new pins: db-lock heartbeat cancellation wiring +
  re-entrancy, the synthesize drain-loop guard + tick call (the shape guard
  only covers worker.ts), and GBRAIN_POOL_MAX_LIFETIME_S reaching a REAL
  constructed pool (postgres() is lazy — no I/O).

New tests: error-outcome-during-shutdown, watchdog double-abort,
timeout-beats-shutdown precedence, bootstrap exit codes, pool-size env
matrix, SIGTERM-only-aborts-shutdown (in-process emit), child-side wrap.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dings (#5, #6)

Ship's review army (5 specialists + red team over the full diff; 0 critical
from specialists, 3 confirmed critical from red team) — all findings fixed:

Red team (critical):
- reserved-direct cap: removed the Math.max(1, size-1) floor — at
  direct_pool_size=1 it let a multi-minute reserve consume the ONLY direct
  session and starve claim/renewLock heartbeats (the #6 class reintroduced).
  cap = size - 1, direct routing only when cap >= 1; size<=1 uses the read
  pool (true status quo). Pinned by a size=1 routing test.
- silent group-kill failure: the SIGKILL escalation now logs loudly when
  delivery fails (distroless hosts without /bin/kill would otherwise void
  the kill guarantee with zero diagnostics while the job duplicated
  elsewhere), and skips the redundant signal when the child already exited.
- spawn-failure circuit breaker: a deterministically broken child CLI looped
  claim/release forever, invisible to the stall detector (every settle
  refreshes the progress clock). After 3 consecutive spawn/bootstrap
  failures the worker emits unhealthy(child_spawn_failing) for a
  process-manager restart; counter resets on any spawn that runs. Plus the
  predicate-mismatch guard: jobIsolation 'process' without childCliInvocation
  now throws at construction (it silently ran handlers inline while the
  evict path believed it was isolated).

Specialists (informational, all applied):
- performance: parent-side outcome decode is async (a 32MiB-capped file must
  not block the event loop running renewal ticks); /bin/kill by absolute
  path (also the security finding).
- security: lease payloads are shape-validated before reconstruction
  (corrupt outcome files degrade to generic); the child-CLI override is
  canonicalized to an absolute path so the fail-fast check validates the
  binary that actually spawns.
- data-migration: max_lifetime default is now a per-CONNECTION jitter
  FUNCTION (matching the postgres.js built-in shape — a pre-evaluated number
  synchronized every connection in a pool onto one recycle deadline);
  reserved.release() throws no longer leak the gauge or the direct permit.
- testing: child harnesses use a readiness handshake instead of fixed 400ms
  sleeps (CI-load flake); the orphan-watchdog test uses a real reaped pid
  (a magic high pid is allocatable under Linux pid_max); new pins for the
  dual-pool probe gating (probeDirect wired ONLY when isDualPoolActive),
  the executeRawDirect/transaction gauge seams incl. the sync begin()-throw
  leak guard, the ddl()-throw read-pool fallback, and the --job-isolation
  help text.
- maintainability: abort-reason literals shared via types.ts (dead
  'cancel'/'cancelled' entries dropped), DEFAULT_DIRECT_POOL_SIZE and
  CHILD_READ_POOL_MAX named, redundant dynamic imports removed, unrefTimer
  helper, getConnectionRouting shared accessor, docstring + fixture-header
  corrections.

Deferred with TODOS entries: raceWithAbortTimeout DRY helper (5 sites), lazy
handler resolution in run-child, e2e-lane negative tests for the run-child
bootstrap guards + operator-flow messages, behavioral withRefreshingLock test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Issues #5 + #6 wave: pool-starvation cancellation + diagnostics, and opt-in
per-job process isolation. Version locations: VERSION, package.json,
CHANGELOG.md, openclaw.plugin.json, BOOTSTRAP_FOR_AGENTS.md stamp, and the
regenerated bootstrap template tree.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The pre-push credential guard (correctly) blocks any URL-with-password shape
in a pushed diff, including fake placeholders. The never-connected fixture
URLs don't need a password at construction time — drop it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r v0.46.1.0

document-release sweep: the wave's docs covered the guides, TESTING, and the
new module entries but missed two spots — the README Job queue capability
paragraph (now names --job-isolation process and the probe verdicts, linking
both guides) and the KEY_FILES postgres-engine.ts entry (now carries the
withReservedConnection direct-lane routing invariants + getPoolDiagnostics
seam, pinned by test/postgres-engine-reserved-routing.test.ts). llms-full.txt
regenerated for the README edit.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex review of the shipped docs vs the diff, each finding verified against
the code before applying:

- minions-deployment: group-SIGKILL platform caveat (Bun /bin/kill fallback),
  lock-token fencing scoped to queue writes (handler side effects bounded by
  the watchdog, not the token), connection math relabeled (pooler-lane vs
  direct session-lane split), no-per-child-RSS-cap note, GBRAIN_JOB_CHILD_CLI
  + the 3-consecutive-spawn-failure breaker documented.
- queue-operations-runbook: verdict rides the TERMINAL probe line (not every
  N/3 line), server_unreachable hedged (both-lanes-failed is the evidence),
  pooler-layer fault added to the 0-in-flight reading, jobs cancel described
  as cooperative inline vs real kill under isolation.
- KEY_FILES: run-child SIGTERM fires shutdownSignal ONLY (both only on
  parent death); third no-burn child class (ChildNotClaimedError).
- TESTING: e2e concurrency leg uses the fixture (no child DB pools); only
  the run-child leg boots real child pools.
- CHANGELOG: one wording precision fix (reserved holds leave a heartbeat
  slot, not "always keep a free slot").

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add per-job process isolation: one stuck handler degrades the whole worker and crashes destroy all in-flight jobs

1 participant