mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-31 04:07:52 +00:00
v0.41.26.1 fix: lock-renewal cathedral — closes ~39 worker crashes/day (supersedes #1567)
Production worker daemons against Supabase / PgBouncer were crashing ~39 times/day with `unhandledRejection at renewLock`. PR #1567 proposed the right try/catch shape; this wave incorporates it and closes the entire bug class (4 inside-review + 8 outside-voice findings absorbed via 9 locked design decisions). What's fixed: - `setInterval(async () => await renewLock(...))` replaced with a sync wrapper around the new pure `runLockRenewalTick` function. No more unhandled rejections escaping the timer callback. - Second crash vector closed: `.catch()` on the stored `executeJob(...).finally(...)` promise so failJob/completeJob throws during the same outage can't propagate to `process.on('unhandledRejection')`. - Per-call `Promise.race` timeout (default `lockDuration/3`) bounds hung renewLock calls so the re-entrancy guard can't wedge indefinitely. - Time-based abort (NOT count-based) so the worker releases its lock BEFORE another worker can reclaim. With the prior 3-strike count + 30s lockDuration, a 15s window let other workers race. - Infrastructure aborts (`lock-renewal-failed`, `lock-lost`) don't burn job attempts — `executeJob`'s catch consults the exported `INFRASTRUCTURE_ABORT_REASONS` set and skips `failJob` so the stall detector reclaims cleanly. - Universal grace-eviction: 30s force-evict safety net now fires for ANY abort reason, not just `job.timeout_ms`. What's added: - `src/core/minions/lock-renewal-tick.ts` (NEW): pure extracted state-machine function + env-knob resolver. Three operator-tunable knobs via env (max-failures-for-audit, call-timeout-ms, safety-margin-ms) with stderr-warn-once on bad input + default fallback. - `src/core/audit/lock-renewal-audit.ts` (NEW): sibling of `batch-retry-audit.ts`. Four outcomes: failure / success_after_failure / gave_up / executeJob_rejected. JSONL at `~/.gbrain/audit/lock-renewal-YYYY-Www.jsonl`. - `src/core/audit/redact-connection-info.ts` (NEW): shared privacy helper. Strips Postgres URLs, host=, user=, password=, IPv4 from error messages before they hit audit JSONL. Wired into BOTH the new lock-renewal audit AND the existing batch-retry audit (privacy backfill — same risk class). - `scripts/check-worker-lock-renewal-shape.sh` (NEW): CI guard wired into `bun run verify`. Asserts the v0.41.22.1 bug pattern (`lockTimer = setInterval(async ...)`) stays absent AND the pure function call site survives refactors. Bug-pattern-specific so it doesn't fight legitimate refactors (codex C12). Tests: 64 new cases across 5 new test files. 182 existing minion + worker tests still pass. All hermetic — no PGLite, no real network, no `mock.module`. Plan + 9 decisions + codex outside-voice review at ~/.claude/plans/system-instruction-you-are-working-humming-nygaard.md Closes #1567 (incorporates the contributor's try/catch shape; closes the bug class structurally). Co-Authored-By: @garrytan-agents <noreply@github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
@garrytan-agents <noreply@github.com>
Claude Opus 4.7
parent
42d99b6fca
commit
2efaafc2ca
Executable
+93
@@ -0,0 +1,93 @@
|
||||
#!/usr/bin/env bash
|
||||
# v0.41.22.2 — CI guard against the v0.41.22.1 lock-renewal crash class.
|
||||
#
|
||||
# The bug pattern: `setInterval(async () => { await something() })` lets
|
||||
# any throw inside the async callback propagate to Node's process-level
|
||||
# `unhandledRejection` handler, which kills the worker with exit 1.
|
||||
# Production lost ~39 worker processes/day to this exact shape when
|
||||
# PgBouncer rotated connections during a renewLock call.
|
||||
#
|
||||
# This guard enforces two invariants on `src/core/minions/worker.ts`:
|
||||
#
|
||||
# 1. The BUG pattern is absent: no `setInterval(async ...)` literal.
|
||||
# A future refactor that inlines `setInterval(async () => { await
|
||||
# renewLock(...) })` again would re-introduce the v0.41.22.1
|
||||
# crash class via the exact original surface.
|
||||
#
|
||||
# 2. The GOOD pattern is present: launchJob calls `runLockRenewalTick`.
|
||||
# Without this call-site, the timer logic could be re-inlined via
|
||||
# a different shape AND bypass the first invariant. The
|
||||
# `runLockRenewalTick` extraction is also the only test seam that
|
||||
# gives the state machine behavioral coverage.
|
||||
#
|
||||
# Intentionally bug-pattern-specific, not implementation-specific: a
|
||||
# future refactor to `setTimeout`-recursion or `AbortController`-based
|
||||
# scheduling passes as long as the bug pattern stays absent (codex C12
|
||||
# from the v0.41.22.2 outside-voice review).
|
||||
#
|
||||
# Usage: scripts/check-worker-lock-renewal-shape.sh
|
||||
# Exit: 0 when shape is good, 1 when violations found.
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
ROOT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)"
|
||||
cd "$ROOT"
|
||||
|
||||
# Allow tests to override the target file for fixture-based meta-tests.
|
||||
TARGET="${GBRAIN_LOCK_RENEWAL_SHAPE_TARGET:-src/core/minions/worker.ts}"
|
||||
|
||||
if [ ! -f "$TARGET" ]; then
|
||||
echo "ERROR: shape guard target file not found: $TARGET"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Invariant 1: the LOCK-RENEWAL site must not use the bug shape.
|
||||
#
|
||||
# The bug-class regex is `setInterval(...async...)`, but it appears
|
||||
# legitimately elsewhere in worker.ts (the stall-detector loop at
|
||||
# line ~269 uses it with try/catch — codex C13 covers re-entrancy
|
||||
# guard for that path separately). To keep this guard from fighting
|
||||
# unrelated decisions, we narrow scope to the renewal timer
|
||||
# specifically by requiring the assignment shape `lockTimer = setInterval(`.
|
||||
#
|
||||
# A future refactor that renames `lockTimer` would slip past this
|
||||
# guard; that's an accepted tradeoff (the variable name has been
|
||||
# stable since v0.10 and is the load-bearing test seam for
|
||||
# `launchJob`'s `inFlight` accounting).
|
||||
#
|
||||
# Uses POSIX ERE + [[:space:]] for BSD-grep portability (macOS shipping
|
||||
# grep doesn't support -P / \s).
|
||||
if grep -Eq 'lockTimer[[:space:]]*=[[:space:]]*setInterval\([[:space:]]*async' "$TARGET"; then
|
||||
echo "ERROR: $TARGET contains the v0.41.22.1 bug pattern (\`setInterval(async ...)\`)."
|
||||
echo
|
||||
echo " Async timer callbacks let unhandledRejection escape to the"
|
||||
echo " process-level handler and crash the worker daemon."
|
||||
echo
|
||||
echo " Fix: wrap the timer callback synchronously around an IIFE that"
|
||||
echo " routes through src/core/minions/lock-renewal-tick.ts:"
|
||||
echo
|
||||
echo " setInterval(() => {"
|
||||
echo " if (tickInFlight) return;"
|
||||
echo " tickInFlight = true;"
|
||||
echo " void runLockRenewalTick(deps, state)"
|
||||
echo " .then(handleResult)"
|
||||
echo " .catch(handlePostError)"
|
||||
echo " .finally(() => { tickInFlight = false; });"
|
||||
echo " }, lockDurationMs / 2);"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Invariant 2: good pattern present. launchJob must call
|
||||
# `runLockRenewalTick` or the test seam is gone.
|
||||
if ! grep -q 'runLockRenewalTick' "$TARGET"; then
|
||||
echo "ERROR: $TARGET does not call \`runLockRenewalTick\`."
|
||||
echo
|
||||
echo " Lock-renewal logic must route through"
|
||||
echo " src/core/minions/lock-renewal-tick.ts so the state-machine"
|
||||
echo " behavior stays unit-testable (no PGLite needed, no"
|
||||
echo " setInterval / process plumbing in tests). Re-introduce the"
|
||||
echo " call site at launchJob's renewal timer."
|
||||
exit 1
|
||||
fi
|
||||
|
||||
echo "lock-renewal shape OK ($TARGET)"
|
||||
@@ -61,6 +61,7 @@ CHECKS=(
|
||||
"check:source-scope-onboard"
|
||||
"check:no-double-retry"
|
||||
"check:batch-audit-site"
|
||||
"check:worker-lock-renewal-shape"
|
||||
"typecheck"
|
||||
)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user