fix(db): retry on ECONNREFUSED — stop exiting 1 on a Postgres endpoint gap (BLO-21615)

[allyblockcast]

The bug

gbrain serve has had connect retry with exponential backoff since v0.21. It never fired for the most common transient failure in a container environment, because that error was classified as permanent.

connect ECONNREFUSED <ip>:<port> is the kernel's connect(2) refusal as surfaced by Node/Bun. It is a different string from Postgres's own connection refused prose, and it was in neither retry list:

/connection refused/i.test("connect ECONNREFUSED 10.99.216.174:5432")  // => false

So isRetryableDbConnectError returned false, connectWithRetry rethrew on attempt 1, and the process exited 1.

On Kubernetes, ECONNREFUSED on a ClusterIP is exactly what kube-proxy REJECTs with while a Service has zero ready endpoints — i.e. for the entire duration of any database pod restart. So every DB blip became a container restart.

Production impact

gbrain-mcp pod, admin-ui container: 125 restarts over 17 days. Each incarnation lived ~4 seconds and logged one line:

Cannot connect to database: connect ECONNREFUSED 10.99.216.174:5432.
Fix: Check your connection URL in ~/.gbrain/config.json

That is GBrainError's composed ${problem}: ${cause_description}. Fix: ${fix} from db.ts connect(), propagating out of connectWithRetry uncaught. The absence of any [connect] attempt N failed … retrying line in those logs is the tell: retry never engaged.

gbrain is a cross-run memory layer, so each restart landed as Unable to connect on whichever writer was mid-write.

The retry machinery was always correct and always wired in (connectEngine() calls connectWithRetry). Only the classification was wrong. The issue this came from hypothesised "no retry or backoff" — that turned out to be wrong, and the real defect is narrower and cheaper to fix.

The fix

1. Add /ECONNREFUSED/i to CONN_PATTERNS in retry-matcher.ts.
2. Delegate db.ts:isRetryableDbConnectError to retry-matcher.ts.

db.ts kept its own inline 5-pattern list — the exact drift retry-matcher.ts was written to end, per its own docstring:

Before this module these predicates lived inline at each site and drifted over time. One source of truth here.

db.ts was never migrated, and had fallen five patterns behind: no ECONNREFUSED, no 08xxx SQLSTATE class, no CONNECTION_ENDED, no 53300. Connect-time retry therefore recovered from strictly fewer conditions than every other retry site in the codebase. There is now one list, not two.

Verification

Tests pin the exact production error string at two levels — the matcher, and connectWithRetry surviving a two-attempt refusal window.

They were confirmed to fail on the pre-change source (not merely pass after). Stashing only the two src/ files:

(fail) db.isRetryableDbConnectError matches the production error
(fail) connectWithRetry survives a 2-attempt ECONNREFUSED window
  error: Cannot connect to database: connect ECONNREFUSED 10.99.216.174:5432. …
      at connectWithRetry (/tmp/gbrain/src/core/db.ts:359:20)

That stack is the production crash reproduced in a test. With the fix:

check	result
bun test test/retry-matcher.test.ts	25 pass / 0 fail
bun test test/core/retry.test.ts	37 pass / 0 fail
tsc --noEmit	exit 0
scripts/check-no-double-retry.sh	ok
scripts/check-exports-count.sh	ok (20 entries, matches baseline)
scripts/check-key-files-current-state.sh	ok
VERSION / package.json / CHANGELOG trio	all 0.42.51.1

Permanent errors — missing extension, missing relation, syntax error — are still not retried (asserted).

test/minions.test.ts additions could not be executed locally: that file's beforeEach needs a live Postgres. They are placed there to sit with the existing connectWithRetry tests; the equivalent assertions were run standalone and are reported above.

Notes for the reviewer

• The import in db.ts is mid-file, matching the repo's existing pattern (cli.ts does the same for buildGatewayConfig, with the same "imported not just re-exported so local call sites bind it" reasoning). It must be an import + re-export, not a bare export … from, because connectWithRetry calls the predicate locally.
• Pre-existing, not touched here: graphify-out/GRAPH_REPORT.md is missing a trailing newline on master, so scripts/check-trailing-newline.sh fails independently of this branch. Left alone to keep this diff focused — worth a separate one-byte fix.
• Not claimed: this does not address why the database was restarting. That is tracked separately; this change makes gbrain tolerate the blip regardless of cause.

Refs BLO-21615.

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-21615

[allyblockcast]

🔗 Paperclip issue: BLO-21615

[allyblockcast]

@ally please review at head b346fc6. Small diff, but three things are worth your scrutiny:

1. Is delegating db.ts:isRetryableDbConnectError to retry-matcher.ts a safe widening? It goes from 5 patterns to the full shared set (adds 08xxx SQLSTATEs, CONNECTION_ENDED, 53300, too many clients, No database connection). All are transient at connect time in my reading, and isRetryableConnError explicitly excludes statement/lock timeouts. But this changes retry behaviour for every connectWithRetry caller, not just serve — please sanity-check I have not made something retry that should fail fast. Specifically: password authentication failed was already there, but is a genuinely-wrong-password startup now retried 3× where an operator would rather see it immediately?

2. The mid-file import in db.ts. It must be import + export {} rather than export … from, because connectWithRetry binds the predicate locally. I followed cli.ts's existing precedent for a mid-file import, but say so if this repo would rather it moved to the top of the file.

3. Test placement. The two connectWithRetry-level tests are in test/minions.test.ts next to the existing ones, but that file's beforeEach needs a live Postgres, so I could not run them locally — I ran the equivalent assertions standalone instead (results in the PR body, including the confirmed pre-change failure). If those belong in a DB-free file, tell me where.

Context: this is the root cause of 125 restarts/17d on the gbrain-mcp admin-ui container. The retry+backoff was always wired in; ECONNREFUSED was simply classified non-retryable, so it rethrew on attempt 1. /connection refused/i does not match connect ECONNREFUSED ….

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b346fc6

Critical Issues (0)

Important Issues (1)

• [native-codex] test/minions.test.ts:2196 — The regression test recovers after exactly two refusals, which is the last failure the unchanged three-attempt loop permits. With immediate ECONNREFUSED responses, src/core/db.ts:352-371 still exhausts its default budget after only the 1s and 2s waits and exits on the third refusal. A normal database pod restart or endpoint gap can easily exceed three seconds, so this change delays rather than eliminates the crash-loop behavior described by the PR.
  • Give serve an elapsed-time/attempt budget that covers realistic restart gaps (preferably configurable), and add a test where refusal lasts beyond the current three-attempt boundary before recovery.

Suggestions (2)

• [pr-review-toolkit] src/core/db.ts:336 — The shared classifier's code-based 08xxx, CONNECTION_ENDED, and 53300 branches are not exercised through the production connect() wrapper, which converts the driver error to GBrainError text without preserving code/cause. Preserve or traverse the cause, or add a production-boundary test so the broader delegation claim is pinned.
• [pr-review-toolkit] test/minions.test.ts:2205 — Pass noRetry: false and attempts: 3 explicitly so the test is isolated from the supported GBRAIN_NO_RETRY_CONNECT=1 process setting.

Strengths

• Centralizing connection classification removes the duplicated matcher, and the exact production ECONNREFUSED text is covered at both predicate and retry-loop levels.
• Permanent schema and syntax failures remain fail-fast, and the version/changelog surfaces are consistent.

Recommended Action

1. Address the Important retry-budget issue before merge.
2. Consider the Suggestions while tightening the regression coverage.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot submit a formal review or approval on it. The exact head b346fc6a9029808b82c3ab553256a07182a59b56 must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b346fc6

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Centralizing the connect-error predicate removes the duplicate, already-drifted retry list while retaining the existing public export.
• Regression coverage exercises both the matcher and the actual retry loop using the production-shaped refusal message.
• The project CI matrix is green, including mechanical, LLM-skill, serial, sharded, and verification checks.

Recommended Action

1. Safe to merge from this review perspective.