fix(retry): reconnect on null instance pool in ALL non-batch config accessors (takeover of #1891) (#2934)

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 <sinabina@Sinabinas-MacBook-Pro-4.local>
Co-authored-by: jalagrange <jalagrange@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

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<T>(fn: () => Promise<T>): Promise<T> {
+    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<string | null> {
     // #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<void> {
-    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<number> {
-    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<string[]> {
-    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

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<void> }).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<void> }).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
+  });
+});