diff --git a/src/core/connection-manager.ts b/src/core/connection-manager.ts index 73cd913b8..7073d101f 100644 --- a/src/core/connection-manager.ts +++ b/src/core/connection-manager.ts @@ -37,7 +37,7 @@ */ import postgres from 'postgres'; -import { resolvePrepare, resolveSessionTimeouts, resolvePoolSize } from './db.ts'; +import { resolvePrepare, resolveSessionTimeouts, resolvePoolSize, endPoolBounded } from './db.ts'; import { redactPgUrl } from './url-redact.ts'; import { logConnectionEvent } from './connection-audit.ts'; @@ -400,15 +400,21 @@ export class ConnectionManager { * (db.ts singleton path). Direct pool is always ours. */ async disconnect(): Promise { + // #1972: end both pools concurrently with a gbrain-owned hard bound, so the + // per-pool drains don't STACK (sequential 2s waits → ~4-6s once the + // instance/module pool is added downstream) and neither can hang teardown + // past the CLI's 10s force-exit. endPoolBounded never throws. + const ends: Promise[] = []; if (this._directPool) { - try { await this._directPool.end(); } catch { /* idempotent */ } + ends.push(endPoolBounded(this._directPool)); this._directPool = null; this._directInit = null; } if (this._readPool && !this._readPoolOwnedExternally) { - try { await this._readPool.end(); } catch { /* idempotent */ } + ends.push(endPoolBounded(this._readPool)); this._readPool = null; } + await Promise.all(ends); } /** diff --git a/src/core/db.ts b/src/core/db.ts index a38290509..99dae2136 100644 --- a/src/core/db.ts +++ b/src/core/db.ts @@ -7,6 +7,47 @@ import { verifySchema } from './schema-verify.ts'; let sql: ReturnType | null = null; let connectedUrl: string | null = null; +/** + * #1972: hard upper bound (seconds) on a single pool `.end()` drain. postgres.js + * accepts `{ timeout }` but applies it internally — against PgBouncer + * transaction-mode the drain can still hang, and a stubbed `.end()` ignores it + * entirely. So `endPoolBounded` ALSO wraps each end in a gbrain-owned + * Promise.race and passes this value as the postgres.js hint so a healthy drain + * still finishes fast. + */ +export const POOL_END_TIMEOUT_SECONDS = 2; + +/** + * #1972: end a postgres.js pool with a gbrain-owned hard bound. Resolves as soon + * as `.end()` settles OR after POOL_END_TIMEOUT_SECONDS + a small slack — so + * teardown never hangs (the prior bare `.end()` blocked until the CLI's 10s + * force-exit fired, which `process.exit()`s and truncated pending stdout, e.g. + * #1959's relational query came back empty). Never throws: a teardown that + * rejects is worse than one that races past a stuck socket. The race timer is + * the real guarantee; `{ timeout }` just lets a healthy drain return in ms. + * + * Note callers that close MULTIPLE pools should `Promise.all` them rather than + * awaiting sequentially, so the per-pool bounds run concurrently instead of + * stacking. + */ +export async function endPoolBounded( + pool: { end: (opts?: { timeout?: number }) => Promise }, +): Promise { + let timer: ReturnType | undefined; + const guard = new Promise((resolve) => { + timer = setTimeout(resolve, POOL_END_TIMEOUT_SECONDS * 1000 + 500); + timer.unref?.(); + }); + try { + await Promise.race([ + pool.end({ timeout: POOL_END_TIMEOUT_SECONDS }).catch(() => { /* idempotent / already-closed */ }), + guard, + ]); + } finally { + if (timer) clearTimeout(timer); + } +} + /** * Default pool size for Postgres connections. Users on the Supabase transaction * pooler (port 6543) or any multi-tenant pooler can lower this to avoid @@ -258,7 +299,7 @@ export async function disconnect(): Promise { const s = sql; sql = null; connectedUrl = null; - if (s) await s.end(); + if (s) await endPoolBounded(s); } export async function initSchema(): Promise { diff --git a/src/core/postgres-engine.ts b/src/core/postgres-engine.ts index ba0218a61..569b2f0fb 100644 --- a/src/core/postgres-engine.ts +++ b/src/core/postgres-engine.ts @@ -248,7 +248,9 @@ export class PostgresEngine implements BrainEngine { this.connectionManager = null; } if (this._sql) { - await this._sql.end(); + // #1972: gbrain-owned hard bound so a PgBouncer drain that never settles + // can't block teardown until the CLI's 10s force-exit truncates stdout. + await db.endPoolBounded(this._sql); this._sql = null; // After this point, _connectionStyle stays 'instance' so a second // disconnect() is a no-op rather than falling through and clearing diff --git a/test/postgres-disconnect-bounded.test.ts b/test/postgres-disconnect-bounded.test.ts new file mode 100644 index 000000000..68dea784f --- /dev/null +++ b/test/postgres-disconnect-bounded.test.ts @@ -0,0 +1,40 @@ +/** + * #1972 — gbrain-owned hard bound on pool teardown. + * + * The bug: `pool.end()` against PgBouncer transaction-mode never drains, so + * disconnect blocked until the CLI's 10s force-exit fired and truncated stdout. + * postgres.js's own `{ timeout }` is internal (a stub ignores it; it's not a + * guarantee we own), so `endPoolBounded` wraps every end in a Promise.race we + * control. These tests assert the bound is real (resolves even when `.end()` + * never settles) and that we still pass `{ timeout }` so a healthy drain is fast. + */ + +import { describe, test, expect } from 'bun:test'; +import { endPoolBounded, POOL_END_TIMEOUT_SECONDS } from '../src/core/db.ts'; + +describe('endPoolBounded', () => { + test('resolves fast when .end() settles quickly, forwarding { timeout }', async () => { + let calledWith: unknown; + const pool = { end: async (opts?: { timeout?: number }) => { calledWith = opts; } }; + const t0 = Date.now(); + await endPoolBounded(pool); + expect(Date.now() - t0).toBeLessThan(500); + expect(calledWith).toEqual({ timeout: POOL_END_TIMEOUT_SECONDS }); + }); + + test('resolves within the gbrain bound even when .end() NEVER settles', async () => { + // This is the PgBouncer hang: .end() returns a promise that never resolves. + // The bare `await pool.end()` would hang until the CLI's 10s force-exit. + const pool = { end: () => new Promise(() => { /* never resolves */ }) }; + const t0 = Date.now(); + await endPoolBounded(pool); + const elapsed = Date.now() - t0; + expect(elapsed).toBeGreaterThanOrEqual(POOL_END_TIMEOUT_SECONDS * 1000); + expect(elapsed).toBeLessThan(5000); // well under the CLI's 10s force-exit deadline + }); + + test('never throws when .end() rejects (teardown must not propagate)', async () => { + const pool = { end: async () => { throw new Error('pool boom'); } }; + await expect(endPoolBounded(pool)).resolves.toBeUndefined(); + }); +}); diff --git a/test/postgres-engine-singleton-ownership.test.ts b/test/postgres-engine-singleton-ownership.test.ts index 68e9ec78c..093be2507 100644 --- a/test/postgres-engine-singleton-ownership.test.ts +++ b/test/postgres-engine-singleton-ownership.test.ts @@ -81,7 +81,10 @@ describe('postgres-engine / module-singleton ownership (#1471)', () => { const disconnect = stripComments(extractFn(DB_SRC, 'disconnect')); const snapshotIdx = disconnect.search(/const\s+s\s*=\s*sql/); const nullIdx = disconnect.search(/\bsql\s*=\s*null/); - const endIdx = disconnect.search(/\bawait\s+s\.end\s*\(/); + // #1972: the pool end is now wrapped in the gbrain-owned hard bound + // `endPoolBounded(s)` instead of a bare `s.end()`. The ordering contract is + // unchanged: snapshot + null the singleton BEFORE awaiting the end. + const endIdx = disconnect.search(/\bawait\s+(?:s\.end\s*\(|endPoolBounded\s*\(\s*s\b)/); expect(snapshotIdx).toBeGreaterThanOrEqual(0); expect(nullIdx).toBeGreaterThanOrEqual(0); expect(endIdx).toBeGreaterThanOrEqual(0);