mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-27 22:15:33 +00:00
6ae94301a6
* 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>
* test: fill A-H gaps for v0.41.26.1 lock-renewal cathedral
The original v0.41.26.1 wave shipped 64 hermetic unit tests on the
pure tick function, audit primitives, and redactor. Post-ship audit
flagged 8 wiring gaps the pure tests can't see — A (launchJob
wiring), B (executeJob skip-failJob), C (.catch on stored promise),
D (INFRASTRUCTURE_ABORT_REASONS export), E (universal grace-evict),
F (executeJob_rejected end-to-end), G (re-entrancy guard at worker
layer), H (gold-standard E2E regression).
Now closed:
- **test/worker-lock-renewal-e2e.serial.test.ts** (1 test, gap H):
the headline gold-standard regression. Real PGLite + real
MinionWorker + executeRaw wrap that injects renewLock failures on
demand. Pins that the worker process DOES NOT crash via
unhandledRejection under sustained renewLock throws, the handler
observes abort.signal.aborted = true with reason
'lock-renewal-failed', and the audit JSONL contains both `failure`
and `gave_up` events. The exact v0.41.22.1 production bug class.
Quarantined to its own file because bun:test serial + PGLite has an
unresolved interaction with multiple MinionWorker-driven tests in
the same file (second test's queue.add hangs indefinitely).
- **test/worker-lock-renewal-shape.test.ts** (18 tests, gaps A-G):
source-shape behavioral pins. Greps worker.ts function bodies for
the patterns the locked decisions promised: launchJob calls
runLockRenewalTick + resolveLockRenewalKnobs + uses
lockRenewalAudit; tickInFlight declared and gated correctly; stored
executeJob promise has .catch with logExecuteJobRejected + console
stderr; abort.signal.addEventListener fires for any abort (not just
timeout_ms); INFRASTRUCTURE_ABORT_REASONS used inside executeJob's
catch with return-early shape. Bug-pattern-specific so a refactor
that genuinely improves the shape passes; a refactor that
accidentally strips a guarantee fails loud.
All 83 lock-renewal wave tests pass in 5.2s. 205 existing minion +
worker tests still green. No production code changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: typecheck errors in worker-lock-renewal-e2e.serial.test.ts
CI verify failed on the new E2E gap-fill test (commit a8b282d4) due
to two TS errors that bun's runtime accepts but tsc rejects:
1. line 92: `originalExecuteRaw(sql, ...args)` with `args: unknown[]`
— TS can't prove args has <=2 elements, so the call site looks
like 1+1+N args against a function that accepts 1-3. Fixed by
destructuring the wrap params explicitly: `(sql, params?, opts?)`
matching the executeRaw signature, then calling
`originalExecuteRaw(sql, params, opts)` with named args.
2. line 145: `expect(abortReason).toBe('lock-renewal-failed')` where
`abortReason: string | null = null`. TS narrows the variable to
`null` because the closure assignment in worker.register isn't
observable to the inferrer. bun:test's `.toBe` overload then
picks the null variant and rejects the string literal. Fixed by
`as unknown as string` cast — the preceding `handlerAbortObserved`
assertion guarantees we entered the branch where abortReason was
assigned. Documented inline.
Local verify (29 checks) now green; E2E test still passes in 5.0s.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: @garrytan-agents <noreply@github.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
152 lines
6.3 KiB
TypeScript
152 lines
6.3 KiB
TypeScript
/**
|
|
* v0.41.22.2 — shared connection-info redactor contract.
|
|
*
|
|
* Pins the privacy promise the audit channel makes: any text passed
|
|
* through `redactConnectionInfo` does NOT contain Postgres DSNs,
|
|
* credentials, hostnames, or IPv4 octets. Real-world fixtures drawn
|
|
* from actual Supabase / PgBouncer / Node DNS error shapes so the
|
|
* tests fail loud if real-world error formats drift past the patterns.
|
|
*
|
|
* Pure function tests, no fs / no env / no PGLite.
|
|
*/
|
|
|
|
import { describe, it, expect } from 'bun:test';
|
|
import {
|
|
redactConnectionInfo,
|
|
getRedactionKinds,
|
|
} from '../../src/core/audit/redact-connection-info.ts';
|
|
|
|
describe('redactConnectionInfo: per-pattern coverage', () => {
|
|
it('case 1 — postgres:// URL with embedded credentials', () => {
|
|
const out = redactConnectionInfo(
|
|
'connection failed: postgres://garry:hunter2@db.example.com:5432/gbrain',
|
|
);
|
|
expect(out).toContain('<REDACTED:pg_url>');
|
|
expect(out).not.toContain('garry');
|
|
expect(out).not.toContain('hunter2');
|
|
expect(out).not.toContain('db.example.com');
|
|
expect(out).not.toContain('5432/gbrain');
|
|
});
|
|
|
|
it('case 2 — postgresql:// (with -ql) treated the same as postgres://', () => {
|
|
const out = redactConnectionInfo(
|
|
'using postgresql://user:pass@host:5432/db?sslmode=require',
|
|
);
|
|
expect(out).toContain('<REDACTED:pg_url>');
|
|
expect(out).not.toContain('user:pass');
|
|
expect(out).not.toContain('host:5432');
|
|
});
|
|
|
|
it('case 3 — password=secret AND pwd=secret both redacted', () => {
|
|
const a = redactConnectionInfo('FATAL: password=hunter2 connection denied');
|
|
expect(a).toContain('<REDACTED:password>');
|
|
expect(a).not.toContain('hunter2');
|
|
|
|
const b = redactConnectionInfo('conninfo: pwd=admin123 user=postgres');
|
|
expect(b).toContain('<REDACTED:password>');
|
|
expect(b).not.toContain('admin123');
|
|
});
|
|
|
|
it('case 4 — user=postgres mid-string', () => {
|
|
const out = redactConnectionInfo('conninfo: host=db user=postgres dbname=app');
|
|
expect(out).toContain('<REDACTED:user>');
|
|
expect(out).not.toContain('user=postgres');
|
|
});
|
|
|
|
it('case 5 — host=db.example.com', () => {
|
|
const out = redactConnectionInfo('conninfo: host=db.example.com port=5432');
|
|
expect(out).toContain('<REDACTED:host>');
|
|
expect(out).not.toContain('db.example.com');
|
|
});
|
|
|
|
it('case 6 — IPv4 inside PG error context', () => {
|
|
const out = redactConnectionInfo(
|
|
'connection to server at "db.example.com" (192.168.1.42), port 5432 failed',
|
|
);
|
|
expect(out).toContain('<REDACTED:ipv4>');
|
|
expect(out).not.toContain('192.168.1.42');
|
|
});
|
|
});
|
|
|
|
describe('redactConnectionInfo: false-positive defense', () => {
|
|
it('case 7 — version number v3.1.4.0 is NOT redacted (negative lookbehind/ahead)', () => {
|
|
const out = redactConnectionInfo('tree-sitter version v3.1.4.0 loaded');
|
|
expect(out).toContain('v3.1.4.0');
|
|
expect(out).not.toContain('<REDACTED:ipv4>');
|
|
});
|
|
|
|
it('case 7b — semver-like 1.2.3.4 NOT redacted when surrounded by version-y tokens', () => {
|
|
const out = redactConnectionInfo('tree-sitter@0.26.3.1 was loaded');
|
|
expect(out).toContain('0.26.3.1');
|
|
expect(out).not.toContain('<REDACTED:ipv4>');
|
|
});
|
|
|
|
it('case 8 — already-redacted text is idempotent', () => {
|
|
const once = redactConnectionInfo('user=postgres host=db port=5432');
|
|
const twice = redactConnectionInfo(once);
|
|
expect(twice).toBe(once);
|
|
});
|
|
|
|
it('case 9 — plain text without secrets passes through unchanged', () => {
|
|
const plain = 'Worker started; processing job 42; took 150ms';
|
|
expect(redactConnectionInfo(plain)).toBe(plain);
|
|
});
|
|
|
|
it('case 9b — empty string passes through', () => {
|
|
expect(redactConnectionInfo('')).toBe('');
|
|
});
|
|
});
|
|
|
|
describe('redactConnectionInfo: real-world fixtures', () => {
|
|
it('case 10 — real Supabase connection failure error: IP redacted, structure preserved', () => {
|
|
const real =
|
|
'PostgresError: connection to server at "aws-0-us-east-1.pooler.supabase.com" (3.215.142.117), port 6543 failed: FATAL: password authentication failed for user "postgres.abcdef123456"';
|
|
const out = redactConnectionInfo(real);
|
|
// IP is the load-bearing leak in this shape (publicly resolvable).
|
|
expect(out).not.toContain('3.215.142.117');
|
|
expect(out).toContain('<REDACTED:ipv4>');
|
|
// Structure preserved enough to debug from.
|
|
expect(out).toMatch(/PostgresError/);
|
|
expect(out).toMatch(/port 6543/);
|
|
// Documented limitation: bare-quoted hostname and bare-quoted
|
|
// username are NOT caught by the bare-field matchers. The bare
|
|
// hostname leak is mitigated by the `host=` pattern wrt conninfo
|
|
// strings; the bare-quoted form is not (filed as a v0.42 TODO if
|
|
// the threat model changes). Pinning current behavior so a future
|
|
// widening shows up as a test diff, not a silent change.
|
|
expect(out).toContain('aws-0-us-east-1.pooler.supabase.com');
|
|
});
|
|
|
|
it('case 11 — real getaddrinfo ENOTFOUND with no IP, just hostname', () => {
|
|
const real =
|
|
'Error: getaddrinfo ENOTFOUND db.example.invalid.host\n at GetAddrInfoReqWrap.onlookup [as oncomplete]';
|
|
const out = redactConnectionInfo(real);
|
|
// Hostname appears WITHOUT host= prefix, so the bare-field matcher
|
|
// doesn't catch it. The error structure is preserved; no false
|
|
// claims of credential leakage. This is a documented limitation.
|
|
expect(out).toContain('getaddrinfo ENOTFOUND');
|
|
// Smoke test: IPv4 (if any) gone.
|
|
expect(out).not.toMatch(/(?<![\d.])(?:\d{1,3}\.){3}\d{1,3}(?![\d.])/);
|
|
});
|
|
|
|
it('case 12 — pattern order: URL with embedded password redacts as pg_url first, not double-redacted', () => {
|
|
const out = redactConnectionInfo(
|
|
'failed to connect: postgres://admin:s3cr3t@host.example.com:5432/db',
|
|
);
|
|
// The URL pattern wins because it's listed first. The substring
|
|
// `password=` doesn't appear in the input (it's the URL-form), so
|
|
// we get exactly ONE redaction kind.
|
|
expect(out.match(/<REDACTED:pg_url>/g)?.length).toBe(1);
|
|
expect(out).not.toContain('<REDACTED:password>');
|
|
expect(out).not.toContain('admin');
|
|
expect(out).not.toContain('s3cr3t');
|
|
});
|
|
});
|
|
|
|
describe('getRedactionKinds: pattern-set surface', () => {
|
|
it('exposes all 5 expected kinds for surface-stability tests', () => {
|
|
const kinds = getRedactionKinds();
|
|
expect(kinds).toEqual(['pg_url', 'password', 'user', 'host', 'ipv4']);
|
|
});
|
|
});
|