From bd2ba46a616855eee5a1a2399c8d2ba9f67144da Mon Sep 17 00:00:00 2001 From: Time Attakc <89218912+time-attack@users.noreply.github.com> Date: Fri, 17 Jul 2026 14:18:52 -0700 Subject: [PATCH] fix(retry): reconnect on null instance pool in ALL non-batch config accessors (takeover of #1891) (#2934) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports the still-unmerged remnant of PR #1891 (#1593 follow-up). Master's getConfig gained retry-with-reconnect in #1603, but its siblings — setConfig, unsetConfig, listConfigKeys — still touched `this.sql` bare, so the first config write/list after a mid-cycle instance-pool teardown threw the retryable "No database connection" (issue #1678) unhandled instead of rebuilding the pool. Adds the connRetry helper from #1891 (same retry+reconnect posture as batchRetry, but no batch audit JSONL — a config accessor is not a sized batch), refactors getConfig onto it, and wraps the other three. Writes are safe to retry: withRetry only retries connection-class failures and both writes are idempotent (upsert / delete). Co-authored-by: Sinabina Co-authored-by: jalagrange Co-authored-by: Claude Fable 5 --- src/core/postgres-engine.ts | 92 ++++++++++++------- test/postgres-engine-config-reconnect.test.ts | 86 +++++++++++++++++ 2 files changed, 146 insertions(+), 32 deletions(-) create mode 100644 test/postgres-engine-config-reconnect.test.ts diff --git a/src/core/postgres-engine.ts b/src/core/postgres-engine.ts index 817a7c1b7..2fe57e517 100644 --- a/src/core/postgres-engine.ts +++ b/src/core/postgres-engine.ts @@ -5244,52 +5244,80 @@ export class PostgresEngine implements BrainEngine { } // Config + + /** + * Single-statement sibling of {@link batchRetry} for the NON-batch config + * accessors that touch `this.sql` directly (#1603 / PR #1593 follow-up, + * PR #1891 by @jalagrange). + * + * Why not `batchRetry`: a config accessor is not a sized batch — routing it + * through `batchRetry` would emit bogus batch-retry audit JSONL (inflating + * the `batch_retry_health` doctor metric) and demand a fake `BatchAuditSite` + * enum member. This keeps the SAME retry + reconnect posture with no audit. + * + * Why it exists: the `sql` getter throws a RETRYABLE "No database + * connection" by design when an instance pool was torn down mid-cycle + * (#1678), precisely so a withRetry+reconnect caller rebuilds the pool and + * self-heals. `getConfig` got that wrapper in #1603; the sibling accessors + * did not — so the first config write/list after a mid-cycle disconnect + * threw unhandled (e.g. crashing the worker into a respawn loop). + * + * `fn` MUST re-read `this.sql` per invocation — `reconnect()` rebuilds the + * pool between attempts. Safe for the writes too: `withRetry` only retries + * connection-class failures (statement never committed), and both writes + * are idempotent (upsert / delete), so even a lost-ack replay converges. + */ + private async connRetry(fn: () => Promise): Promise { + const opts = this.getBulkRetryOpts(); + return withRetry(fn, { + maxRetries: opts.maxRetries, + delayMs: opts.delayMs, + delayMaxMs: opts.delayMaxMs, + jitter: BULK_RETRY_OPTS.jitter, + // Same reconnect posture as batchRetry: rebuild a dead instance pool + // before the next attempt. Race-safe via the engine's `_reconnecting` + // guard; fail-loud — a reconnect throw propagates as the real cause. + reconnect: (ctx) => this.reconnect(ctx), + }); + } + async getConfig(key: string): Promise { // #1603: a transient pooler drop on this read used to throw / fall through // to defaults silently — which on remote Postgres surfaces as the wrong - // search mode/knobs and empty-stdout queries. Retry-with-reconnect using the - // same tuned opts as the bulk writers. No auditSite: this is a single-row - // read, not a bulk write, so it must not emit batch-retry audit rows. - // `this.sql` is a getter, so each attempt sees the pool rebuilt by reconnect. - const opts = this.getBulkRetryOpts(); - return withRetry( - async () => { - const rows = await this.sql`SELECT value FROM config WHERE key = ${key}`; - return rows.length > 0 ? (rows[0].value as string) : null; - }, - { - maxRetries: opts.maxRetries, - delayMs: opts.delayMs, - delayMaxMs: opts.delayMaxMs, - jitter: BULK_RETRY_OPTS.jitter, - reconnect: (ctx) => this.reconnect(ctx), - }, - ); + // search mode/knobs and empty-stdout queries. + return this.connRetry(async () => { + const rows = await this.sql`SELECT value FROM config WHERE key = ${key}`; + return rows.length > 0 ? (rows[0].value as string) : null; + }); } async setConfig(key: string, value: string): Promise { - const sql = this.sql; - await sql` - INSERT INTO config (key, value) VALUES (${key}, ${value}) - ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value - `; + return this.connRetry(async () => { + await this.sql` + INSERT INTO config (key, value) VALUES (${key}, ${value}) + ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value + `; + }); } async unsetConfig(key: string): Promise { - const sql = this.sql; - const result = await sql`DELETE FROM config WHERE key = ${key}` as unknown as { count: number }; - return result.count ?? 0; + return this.connRetry(async () => { + const result = await this.sql`DELETE FROM config WHERE key = ${key}` as unknown as { count: number }; + return result.count ?? 0; + }); } async listConfigKeys(prefix: string): Promise { - const sql = this.sql; - // LIKE-escape literal % and _ so a config key with those chars resolves correctly. + // LIKE-escape literal % and _ so a config key with those chars resolves + // correctly. Pure string work — stays outside the retried thunk. const escaped = prefix.replace(/\\/g, '\\\\').replace(/%/g, '\\%').replace(/_/g, '\\_'); const pattern = `${escaped}%`; - const rows = await sql<{ key: string }[]>` - SELECT key FROM config WHERE key LIKE ${pattern} ESCAPE '\\' ORDER BY key - `; - return rows.map(r => r.key); + return this.connRetry(async () => { + const rows = await this.sql<{ key: string }[]>` + SELECT key FROM config WHERE key LIKE ${pattern} ESCAPE '\\' ORDER BY key + `; + return rows.map(r => r.key); + }); } // Migration support diff --git a/test/postgres-engine-config-reconnect.test.ts b/test/postgres-engine-config-reconnect.test.ts new file mode 100644 index 000000000..df44d47c9 --- /dev/null +++ b/test/postgres-engine-config-reconnect.test.ts @@ -0,0 +1,86 @@ +/** + * Non-batch config accessors must self-heal the same way the batch path does + * (takeover of PR #1891 by @jalagrange; #1593 follow-up). + * + * The config accessors touch `this.sql` directly. When an instance pool is + * torn down mid-cycle the getter throws a RETRYABLE "No database connection" + * (issue #1678) by design, so a withRetry+reconnect caller can rebuild the + * pool and recover. `getConfig` got that wrapper in #1603; `setConfig`, + * `unsetConfig`, and `listConfigKeys` did not — so the first config write or + * list after a mid-cycle disconnect threw unhandled. This pins that ALL four + * accessors now reconnect + retry, and that non-retryable errors are NOT + * masked by a reconnect. + * + * Pure: pokes private fields and stubs `reconnect` to simulate the pool + * rebuild; no real DB. + */ + +import { describe, it, expect } from 'bun:test'; +import { PostgresEngine } from '../src/core/postgres-engine.ts'; + +// A tagged-template-callable fake `sql` that resolves to the given value. +function fakeSql(result: unknown) { + return (..._args: unknown[]) => Promise.resolve(result); +} + +// Near-instant retry delays so the inter-attempt sleep does not slow the test. +// Shape matches `resolveBulkRetryOpts()` (the getBulkRetryOpts cache type). +const FAST_RETRY = { maxRetries: 3, delayMs: 1, delayMaxMs: 1, jitter: 'none' as const }; + +/** Engine with a torn-down instance pool; reconnect installs `poolResult`. */ +function makeTornDownEngine(poolResult: unknown): { engine: PostgresEngine; reconnects: () => number } { + const e = new PostgresEngine(); + (e as unknown as { _connectionStyle: string })._connectionStyle = 'instance'; + (e as unknown as { _sql: unknown })._sql = null; // instance pool torn down → getter throws retryable + (e as unknown as { _bulkRetryOptsCache: unknown })._bulkRetryOptsCache = FAST_RETRY; + let reconnectCalls = 0; + (e as unknown as { reconnect: () => Promise }).reconnect = async () => { + reconnectCalls++; + (e as unknown as { _sql: unknown })._sql = fakeSql(poolResult); + }; + return { engine: e, reconnects: () => reconnectCalls }; +} + +describe('PostgresEngine non-batch config accessors self-heal (PR #1891 takeover)', () => { + it('getConfig reconnects + retries a null instance pool, then returns the value', async () => { + const { engine, reconnects } = makeTornDownEngine([{ value: 'live-value' }]); + expect(await engine.getConfig('some.key')).toBe('live-value'); + expect(reconnects()).toBe(1); // exactly one reconnect closed the gap + }); + + it('setConfig reconnects + retries a null instance pool (idempotent upsert)', async () => { + const { engine, reconnects } = makeTornDownEngine([]); + await engine.setConfig('some.key', 'v'); + expect(reconnects()).toBe(1); + }); + + it('unsetConfig reconnects + retries a null instance pool, then returns the count', async () => { + const { engine, reconnects } = makeTornDownEngine({ count: 2 }); + expect(await engine.unsetConfig('some.key')).toBe(2); + expect(reconnects()).toBe(1); + }); + + it('listConfigKeys reconnects + retries a null instance pool, then returns keys', async () => { + const { engine, reconnects } = makeTornDownEngine([{ key: 'a.one' }, { key: 'a.two' }]); + expect(await engine.listConfigKeys('a.')).toEqual(['a.one', 'a.two']); + expect(reconnects()).toBe(1); + }); + + it('surfaces a non-retryable error without reconnecting (no masking)', async () => { + const e = new PostgresEngine(); + (e as unknown as { _connectionStyle: string })._connectionStyle = 'instance'; + (e as unknown as { _bulkRetryOptsCache: unknown })._bulkRetryOptsCache = FAST_RETRY; + // A live pool whose query throws a NON-retryable (non-connection) error. + (e as unknown as { _sql: unknown })._sql = () => + Promise.reject(new Error('syntax error at or near "SLECT"')); + + let reconnectCalls = 0; + (e as unknown as { reconnect: () => Promise }).reconnect = async () => { + reconnectCalls++; + }; + + await expect(e.getConfig('k')).rejects.toThrow('syntax error'); + await expect(e.setConfig('k', 'v')).rejects.toThrow('syntax error'); + expect(reconnectCalls).toBe(0); // non-retryable → no reconnect, error not masked + }); +});