From 9eac872136e4faaab6720da6d1d316158e2080f0 Mon Sep 17 00:00:00 2001
From: Ryan Ayers <rayers@dividia.net>
Date: Fri, 17 Jul 2026 13:32:23 -0500
Subject: [PATCH] fix(postgres-engine): build-then-swap reconnect() so a failed
 rebuild can't brick the engine (#1593) (#1906)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The instance-pool reconnect() did disconnect() (nulling _sql) BEFORE connect(),
so a connect() failure during a transient Postgres blip left _sql null for the
rest of the process — every subsequent non-retry-wrapped call then fell through
to the never-connected module singleton and threw 'No database connection',
crashing the autopilot worker into a respawn loop. Build-then-swap: snapshot the
live pool, build a fresh one, end the old only once the new validates, restore
on failure. Keeps upstream's reap-detection + pool-recovery audit. Confirmed by
jalagrange on closed PR #1593; this is the Layer-1 fix he left to us.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 src/core/postgres-engine.ts        | 28 +++++++++++++++++++++++++---
 test/connection-resilience.test.ts |  8 +++++++-
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/src/core/postgres-engine.ts b/src/core/postgres-engine.ts
index 3019d6f3f..817a7c1b7 100644
--- a/src/core/postgres-engine.ts
+++ b/src/core/postgres-engine.ts
@@ -5374,20 +5374,42 @@ export class PostgresEngine implements BrainEngine {
       logPoolRecovery(isReap ? 'reap_detected' : 'reconnect_other', ctx?.error);
     } catch { /* audit is best-effort */ }
 
+    // Instance pool: BUILD-THEN-SWAP. Snapshot the live pool, build a fresh one,
+    // and only tear the old one down once the new one is proven live. The naive
+    // disconnect()-then-connect() ordering nulls `_sql` BEFORE the rebuild, so a
+    // connect() failure during a transient blip leaves `_sql === null` for the
+    // rest of the process. A dead `_sql` falls through to the module-singleton
+    // accessor — which the autopilot process never connected — so every
+    // subsequent non-retry-wrapped call (getConfig, per-phase reads) throws
+    // "No database connection: connect() has not been called" and crashes the
+    // worker into a respawn loop (#1593 root-cause). Holding the old pool until
+    // the new one validates keeps the engine usable; postgres.js pools self-heal
+    // on the next query once Postgres is back, and batchRetry's backoff retries.
+    const oldSql = this._sql;
+    const oldManager = this.connectionManager;
     try {
-      // Instance pool: tear down old pool (best-effort — it may already be dead).
-      try { await this.disconnect(); } catch { /* swallow */ }
+      this._sql = null; // force connect() to build a fresh pool, not reuse
+      // connect() validates the new pool via `SELECT 1` before returning.
       await this.connect(this._savedConfig);
+      // New pool is live — discard the old one best-effort.
+      if (oldSql) { try { await oldSql.end({ timeout: 5 }); } catch { /* swallow */ } }
       try {
         const { logPoolRecovery } = await import('./audit/pool-recovery-audit.ts');
         logPoolRecovery('reconnect_succeeded');
       } catch { /* best-effort */ }
     } catch (err) {
+      // Rebuild failed: tear down the half-built pool (if any) and restore the
+      // prior live pool + manager so the engine stays usable.
+      if (this._sql && this._sql !== oldSql) {
+        try { await this._sql.end({ timeout: 5 }); } catch { /* swallow */ }
+      }
+      this._sql = oldSql;
+      this.connectionManager = oldManager;
       try {
         const { logPoolRecovery } = await import('./audit/pool-recovery-audit.ts');
         logPoolRecovery('reconnect_failed', err);
       } catch { /* best-effort */ }
-      throw err;
+      throw err; // let batchRetry's backoff handle the retry
     } finally {
       this._reconnecting = false;
     }
diff --git a/test/connection-resilience.test.ts b/test/connection-resilience.test.ts
index 655c8418c..e52809e7f 100644
--- a/test/connection-resilience.test.ts
+++ b/test/connection-resilience.test.ts
@@ -311,7 +311,13 @@ describe('Eng-review D3 — executeRaw has no per-call retry wrapper', () => {
     // can classify the triggering error for the pool-recovery audit. Match the
     // prefix so both `reconnect()` and `reconnect(ctx?)` satisfy the contract.
     expect(src).toContain('async reconnect(');
-    expect(src).toContain('await this.disconnect()');
+    // #1593 build-then-swap: reconnect() no longer disconnect()-then-connect()s
+    // on the instance-pool path (that nulled _sql, so a connect() failure during
+    // a transient blip left the engine permanently dead → worker respawn loop).
+    // It now snapshots the live pool, builds a fresh one, and ends the OLD pool
+    // only once the new one validates — restoring it on failure. Assert the
+    // old-pool teardown, which is the recovery contract this test guards.
+    expect(src).toContain('await oldSql.end(');
   });
 
   it('Supervisor still has the 3-strikes-then-reconnect path', () => {