v0.42.21.0 fix(postgres): module-singleton ownership — canonical landing for the dream-cycle "connect() has not been called" class (#1404/#1471/#1619) (#1805)

* fix(postgres): module-singleton ownership — borrower disconnect no longer nulls the cycle's connection (#1404/#1471/#1619)

gbrain dream on Postgres failed every DB phase with "No database connection:
connect() has not been called": a short-lived borrower probe engine (lint/doctor
config-lift, no poolSize) called db.disconnect() in its own disconnect(), nulling
the shared module singleton the long-lived cycle owner was still using. The module
`sql` is only ever nulled by db.disconnect() (postgres.js auto-reconnects its own
pool), so the failure was always a borrower-disconnect, never an idle-pooler drop.

Fix: db.connect() returns whether THIS call created the singleton (atomic — no
await between the null-check and the sql=postgres() assignment), PostgresEngine
stores it as _ownsModuleSingleton, and disconnect() only calls db.disconnect()
when it owns the connection. Borrowers no-op. Hardening: db.disconnect() snapshots
+nulls sql before awaiting end(); reconnect() shares one in-flight _reconnectPromise.

Tests: new postgres-engine-singleton-ownership.test.ts; expanded DB-gated e2e
matrix (owner/borrower, creation-not-role, symmetric CLI-exit, owner-reconnect-
with-live-borrower); module-style getter asymmetry; #1570 shared-recovery
regression updated to assert the fixed contract.

Co-Authored-By: nullhex-io <noreply@github.com>
Co-Authored-By: joelwp <noreply@github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* chore: bump version and changelog (v0.42.15.0)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: nullhex-io <noreply@github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Garry Tan
2026-06-03 08:59:39 -07:00
committed by GitHub
co-authored by nullhex-io Claude Opus 4.8
parent ec5fed2921
commit f3ade6c0c3
14 changed files with 454 additions and 98 deletions
-21
View File
@@ -130,27 +130,6 @@ describe('classifyWorkerExit', () => {
// --- Mock-based tests for reconnect logic ---
describe('PostgresEngine reconnect behavior', () => {
it('reconnect flag prevents concurrent reconnections', async () => {
// Simulate the _reconnecting guard
let reconnecting = false;
let reconnectCount = 0;
async function reconnect() {
if (reconnecting) return;
reconnecting = true;
try {
reconnectCount++;
await new Promise(r => setTimeout(r, 10));
} finally {
reconnecting = false;
}
}
// Fire 3 concurrent reconnects — only 1 should run
await Promise.all([reconnect(), reconnect(), reconnect()]);
expect(reconnectCount).toBe(1);
});
it('executeRaw retry does not infinite-loop on persistent connection failure', async () => {
// Simulate: first call fails (connection error), reconnect succeeds,
// but retry also fails with a NON-connection error
+23 -27
View File
@@ -49,46 +49,42 @@ describe.skipIf(skip)('v0.41.25.0 db-singleton shared-recovery regressions (#157
tmpAuditDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gbrain-1570-e2e-'));
});
test('CASE 1: shared singleton survives mid-operation disconnect via retry reconnect', async () => {
// Reproduce the dream-cycle scenario: caller A is mid-batch, caller B
// disconnects the module singleton, caller A's NEXT attempt enters
// retry and the reconnect callback rebuilds the singleton before the
// retry's fn fires. This is the symptom-fix contract we ship.
await db.connect({ database_url: DATABASE_URL! });
test('CASE 1: a borrower disconnect leaves the shared singleton ALIVE — no reconnect needed (#1471 ownership fix)', async () => {
// The dream-cycle scenario: caller A is mid-batch, caller B (a probe engine
// that BORROWED the singleton) disconnects. Pre-#1471, B's disconnect
// cascaded to db.disconnect() and nulled the singleton for A, so A's next
// call threw "connect() has not been called" and only batchRetry's reconnect
// could recover (and sync/synthesize, which never enter batchRetry, stayed
// broken). Post-#1471, B is a borrower (it joined the singleton beforeAll
// created) and its disconnect is a no-op — the singleton survives WITHOUT
// any reconnect, which is what protects the non-batch phases.
await db.connect({ database_url: DATABASE_URL! }); // already up from beforeAll → no-op
const engineA = new PostgresEngine();
await engineA.connect({ database_url: DATABASE_URL! });
await engineA.connect({ database_url: DATABASE_URL! }); // borrows
const engineB = new PostgresEngine();
await engineB.connect({ database_url: DATABASE_URL! });
await engineB.connect({ database_url: DATABASE_URL! }); // borrows
// Sanity: both engines share the live singleton.
expect((await engineA.sql`SELECT 1 as ok`)[0].ok).toBe(1);
expect((await engineB.sql`SELECT 1 as ok`)[0].ok).toBe(1);
// Engine B disconnects mid-operation (the "offending caller" scenario).
// This nulls the module singleton for engine A too.
// Engine B (a borrower) disconnects mid-operation. The bug fix: this MUST
// NOT null the singleton engine A is still using.
await engineB.disconnect();
// Engine A's direct unsafe call will throw — proving the bug class
// exists at the engine.sql layer.
let directThrew = false;
try {
await engineA.sql`SELECT 1`;
} catch {
directThrew = true;
}
expect(directThrew).toBe(true);
// Engine A's direct call now SUCCEEDS (pre-fix it threw). This is the
// inverted assertion — the path that used to "prove the bug exists" now
// proves the bug is gone. No reconnect, no retry: just works.
const afterBorrowerDisconnect = await engineA.sql`SELECT 1 as ok`;
expect(afterBorrowerDisconnect[0].ok).toBe(1);
// The retry layer's reconnect callback recovers. We exercise it via
// engine.reconnect() directly (which is what batchRetry's injected
// reconnect callback calls). After reconnect, engine A's next call
// succeeds.
// Defense-in-depth: reconnect() still works on a borrower (re-borrows the
// still-live singleton) — the genuine-transient-drop recovery path is intact.
await engineA.reconnect();
const afterRecovery = await engineA.sql`SELECT 1 as ok`;
expect(afterRecovery[0].ok).toBe(1);
expect((await engineA.sql`SELECT 1 as ok`)[0].ok).toBe(1);
// Cleanup
await engineA.disconnect();
await engineA.disconnect(); // borrower no-op; singleton torn down by afterAll
});
test('CASE 2: diagnostic audit records every mid-process disconnect call', async () => {
@@ -23,7 +23,7 @@
* (second call no-ops).
*/
import { describe, test, expect, beforeAll, afterAll } from 'bun:test';
import { describe, test, expect, beforeEach, afterAll } from 'bun:test';
import { PostgresEngine } from '../../src/core/postgres-engine.ts';
import * as db from '../../src/core/db.ts';
@@ -35,12 +35,13 @@ if (skip) {
console.log('Skipping postgres-engine-disconnect-idempotency E2E (DATABASE_URL not set)');
}
describe.skipIf(skip)('PostgresEngine.disconnect idempotency', () => {
beforeAll(async () => {
// Establish the module-level connection so we can verify it survives
// the instance-pool engine's double-disconnect.
const ok1 = (rows: unknown[]) => (rows[0] as { ok: number }).ok;
describe.skipIf(skip)('PostgresEngine.disconnect idempotency + module-singleton ownership (#1471)', () => {
// Every test builds its own module-singleton scenario from a clean slate, so
// order is irrelevant and the ownership cases don't leak state into each other.
beforeEach(async () => {
await db.disconnect();
await db.connect({ database_url: DATABASE_URL! });
}, 30_000);
afterAll(async () => {
@@ -48,40 +49,85 @@ describe.skipIf(skip)('PostgresEngine.disconnect idempotency', () => {
});
test('instance-pool engine: second disconnect() does NOT clobber module singleton', async () => {
const engine = new PostgresEngine();
await engine.connect({ database_url: DATABASE_URL!, poolSize: 2 });
// First disconnect — closes the engine's own pool.
await engine.disconnect();
// Sanity: module-level connection still alive (this is what
// helpers.ts's getConn() returns).
const before = await db.getConnection().unsafe('SELECT 1 as ok');
expect((before[0] as unknown as { ok: number }).ok).toBe(1);
// Second disconnect — pre-fix, this fell through to db.disconnect()
// and cleared the module-level singleton. Post-fix, it's a no-op.
await engine.disconnect();
// Module-level connection MUST still be alive.
const after = await db.getConnection().unsafe('SELECT 1 as ok');
expect((after[0] as unknown as { ok: number }).ok).toBe(1);
});
test('module-singleton engine: second disconnect() is a no-op', async () => {
// Re-establish module-level connection (idempotent; no-op if still
// connected from beforeAll).
// Establish a module-level baseline (the cycle's singleton).
await db.connect({ database_url: DATABASE_URL! });
const engine = new PostgresEngine();
// No poolSize → uses the module-level singleton.
await engine.connect({ database_url: DATABASE_URL! });
await engine.connect({ database_url: DATABASE_URL!, poolSize: 2 });
// First disconnect closes module-level singleton (this engine owned it).
await engine.disconnect();
expect(ok1(await db.getConnection().unsafe('SELECT 1 as ok'))).toBe(1);
// Second disconnect must NOT throw — should be a no-op since
// _connectionStyle was reset to null.
// Second disconnect — pre-fix this fell through to db.disconnect() and
// cleared the module-level singleton. Post-fix it's a no-op (instance style).
await engine.disconnect();
expect(ok1(await db.getConnection().unsafe('SELECT 1 as ok'))).toBe(1);
});
test('owner-first / borrower-second: a borrower disconnect must NOT null the owner singleton', async () => {
// The exact dream-cycle bug: the owner (cycle engine) creates the singleton,
// a probe (lint/doctor config-lift) borrows it, the probe disconnects, and
// pre-fix that nulled the singleton the owner was still using.
const owner = new PostgresEngine();
await owner.connect({ database_url: DATABASE_URL! }); // module branch → creates → owns
const borrower = new PostgresEngine();
await borrower.connect({ database_url: DATABASE_URL! }); // singleton exists → borrows
await borrower.disconnect(); // pre-fix: db.disconnect() → singleton null
// The owner must still be able to run DB work (this is sync/synthesize).
expect(ok1(await owner.executeRaw('SELECT 1 as ok'))).toBe(1);
expect(() => db.getConnection()).not.toThrow();
// And the owner — the true creator — DOES tear it down.
await owner.disconnect();
expect(() => db.getConnection()).toThrow(/No database connection/);
});
test('ownership tracks CREATION, not role: a probe that creates the singleton owns it', async () => {
// codex #2: ownership is not "the cycle engine" by name — it is whoever
// atomically created the pool. If a probe creates first, it owns; a later
// joiner is the borrower. (Safe in gbrain because the CLI engine is always
// the first creator and last to disconnect — the dominance invariant.)
const firstCreator = new PostgresEngine();
await firstCreator.connect({ database_url: DATABASE_URL! }); // creates → owns
const joiner = new PostgresEngine();
await joiner.connect({ database_url: DATABASE_URL! }); // joins → borrows
await joiner.disconnect(); // no-op on the singleton
expect(() => db.getConnection()).not.toThrow();
await firstCreator.disconnect(); // creator tears down
expect(() => db.getConnection()).toThrow(/No database connection/);
});
test('symmetric CLI-exit: a sole owner connect+disconnect tears the singleton down (no hang regression)', async () => {
const engine = new PostgresEngine();
await engine.connect({ database_url: DATABASE_URL! });
await engine.disconnect();
// Pool must be CLOSED so the CLI event loop drains and `gbrain init` /
// op-dispatch exit cleanly (the failure mode refcount would risk).
expect(() => db.getConnection()).toThrow(/No database connection/);
// Idempotent second disconnect.
await expect(engine.disconnect()).resolves.toBeUndefined();
});
test('owner reconnect with a live borrower: borrower resolves the rebuilt singleton', async () => {
const owner = new PostgresEngine();
await owner.connect({ database_url: DATABASE_URL! }); // owns
const borrower = new PostgresEngine();
await borrower.connect({ database_url: DATABASE_URL! }); // borrows
// Owner reconnect (the batchRetry path): tears down the old singleton and
// builds a fresh one, re-acquiring ownership via the atomic db.connect() token.
await owner.reconnect();
// The borrower's normal query path (this.sql → db.getConnection()) resolves
// the NEW singleton. (codex #4: the borrower's cached connectionManager pool
// is stale — that ddl()-only edge is a filed TODO, not this normal path.)
expect(ok1(await borrower.sql.unsafe('SELECT 1 as ok'))).toBe(1);
expect(ok1(await owner.executeRaw('SELECT 1 as ok'))).toBe(1);
await owner.disconnect();
expect(() => db.getConnection()).toThrow(/No database connection/);
});
});
@@ -40,4 +40,28 @@ describe('PostgresEngine.sql getter self-heal (issue #1678)', () => {
(e as unknown as { _sql: unknown })._sql = fakeSql;
expect(e.sql as unknown).toBe(fakeSql);
});
// #1471: the getter self-heal is INTENTIONALLY gated to instance style. A
// module-style engine with a null _sql must fall through to the loud legacy
// db.getConnection() error, NOT the instance "reaped pool" self-heal. Post
// ownership-fix, the module singleton never goes null via a borrower
// disconnect, so a null module singleton signals a genuine bug we want loud
// (never connected, or a real owner-side teardown) rather than papered over.
it('module-style + null _sql falls through to the loud legacy error, not the instance self-heal', () => {
const e = new PostgresEngine();
(e as unknown as { _connectionStyle: string })._connectionStyle = 'module';
(e as unknown as { _sql: unknown })._sql = null;
let thrown: unknown;
try {
void e.sql; // delegates to db.getConnection(), which throws (no module connect)
} catch (err) {
thrown = err;
}
expect(thrown).toBeDefined();
const msg = (thrown as Error).message;
// The intentional asymmetry: module style keeps the legacy loud message.
expect(msg).toContain('No database connection');
expect(msg).not.toContain('instance connection pool');
});
});
@@ -0,0 +1,153 @@
/**
* postgres-engine.ts module-singleton ownership guardrails (#1471).
*
* Root cause: when an engine connects via the module-singleton path (no
* poolSize), BOTH the engine that creates the shared db.ts `sql` singleton
* (the owner, e.g. the CLI cycle engine) and any engine constructed while
* that singleton already exists (a borrower, e.g. the probe engine in
* resolveLintContentSanity / doctor) got `_connectionStyle = 'module'`. The
* old disconnect() then called `db.disconnect()` for BOTH, so a short-lived
* borrower's teardown nulled the shared `sql` the owner was still using —
* every later cycle phase then threw "No database connection: connect() has
* not been called" and stranded the cycle lock.
*
* Fix: track ownership via a token returned atomically by db.connect(). Only
* the engine whose connect() actually created the singleton may db.disconnect()
* it; borrowers clear their own marker without touching the shared connection.
*
* Source-level, DB-free guardrails — matching the existing
* postgres-engine.test.ts convention (runtime mocking of postgres.js's
* tagged-template interface is painful under bun ESM; live behaviour is
* exercised by test/e2e/postgres-engine-disconnect-idempotency.test.ts).
*/
import { describe, test, expect } from 'bun:test';
import { readFileSync } from 'fs';
import { join } from 'path';
const ENGINE_SRC = readFileSync(
join(import.meta.dir, '..', 'src', 'core', 'postgres-engine.ts'),
'utf-8',
);
const DB_SRC = readFileSync(
join(import.meta.dir, '..', 'src', 'core', 'db.ts'),
'utf-8',
);
describe('postgres-engine / module-singleton ownership (#1471)', () => {
// Boolean assertions on purpose: matching a regex against the 4500-line
// source dumps the whole file into the failure message. `.test()` -> bool
// keeps RED output (and CI logs) readable.
test('db.connect() returns the ownership token (Promise<boolean>), not void', () => {
const sig = /export async function connect\(config: EngineConfig\): Promise<boolean>/.test(DB_SRC);
expect(sig).toBe(true);
});
test('db.connect() borrower path returns false; creator path returns true', () => {
const connect = stripComments(extractFn(DB_SRC, 'connect'));
// Borrower (singleton already exists) → return false.
expect(/if\s*\(\s*sql\s*\)\s*\{[\s\S]*?return false/.test(connect)).toBe(true);
// Creator (built + validated the pool) → return true.
expect(/return true/.test(connect)).toBe(true);
});
test('PostgresEngine tracks module-singleton ownership with a dedicated flag', () => {
expect(ENGINE_SRC.includes('_ownsModuleSingleton')).toBe(true);
});
test('connect() stores the ownership token from db.connect() — no separate pre-sample (TOCTOU guard)', () => {
const connect = stripComments(extractMethod(ENGINE_SRC, 'connect'));
// Ownership is the RETURN of db.connect(), assigned to the flag.
expect(/_ownsModuleSingleton\s*=\s*await\s+db\.connect\s*\(/.test(connect)).toBe(true);
// Regression guard against the original TOCTOU shape: no separate
// db.isConnected() probe sampled before db.connect().
expect(/db\.isConnected\s*\(/.test(connect)).toBe(false);
});
test('disconnect() calls db.disconnect() ONLY when this engine owns the singleton', () => {
const disconnect = stripComments(extractMethod(ENGINE_SRC, 'disconnect'));
// The shared-singleton teardown must be guarded by the ownership flag — a
// borrower clears its marker without nulling the owner's connection.
const guarded = /if\s*\(\s*this\._ownsModuleSingleton\s*\)\s*\{[\s\S]*?db\.disconnect\s*\(\s*\)/.test(disconnect);
expect(guarded).toBe(true);
// And the only db.disconnect() in the method is the guarded one (no
// unconditional clobber survives).
const calls = [...disconnect.matchAll(/db\.disconnect\s*\(\s*\)/g)];
expect(calls.length).toBe(1);
});
test('db.disconnect() snapshots + nulls the singleton BEFORE awaiting end() (codex #6)', () => {
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*\(/);
expect(snapshotIdx).toBeGreaterThanOrEqual(0);
expect(nullIdx).toBeGreaterThanOrEqual(0);
expect(endIdx).toBeGreaterThanOrEqual(0);
// null the module ref BEFORE the await, so a concurrent connect() can't
// join a pool that's already closing.
expect(nullIdx < endIdx).toBe(true);
});
test('reconnect() is connection-style aware: module-singleton path never tears down the shared pool (#1745)', () => {
const reconnect = stripComments(extractMethod(ENGINE_SRC, 'reconnect'));
// Module-singleton engines must NOT route through this.disconnect()/db.disconnect()
// on reconnect — they recover idempotently via db.connect() + setReadPool, so a
// transient blip can't null the shared singleton other phases are using.
expect(/this\._connectionStyle\s*!==\s*'instance'/.test(reconnect)).toBe(true);
expect(/db\.connect\(this\._savedConfig\)/.test(reconnect)).toBe(true);
expect(/setReadPool\(db\.getConnection\(\)\)/.test(reconnect)).toBe(true);
// The instance path keeps the `_reconnecting` re-entrancy guard.
expect(/this\._reconnecting\s*=\s*true/.test(reconnect)).toBe(true);
});
});
function stripComments(s: string): string {
return s
.replace(/\/\*[\s\S]*?\*\//g, '')
.replace(/(^|\s)\/\/[^\n]*/g, '$1');
}
// Balance a `{...}` body starting at the first `{` after `headerIdx`, where
// headerIdx points at (or before) the signature's parameter list. Skips the
// parameter-list parens so a `{` inside a param type isn't mistaken for the body.
function balanceBodyFrom(source: string, headerIdx: number, what: string): string {
let i = source.indexOf('(', headerIdx);
let pdepth = 0;
for (; i < source.length; i++) {
if (source[i] === '(') pdepth++;
else if (source[i] === ')') {
pdepth--;
if (pdepth === 0) { i++; break; }
}
}
i = source.indexOf('{', i);
if (i < 0) throw new Error(`no body brace for ${what}`);
const start = i;
let depth = 0;
for (; i < source.length; i++) {
if (source[i] === '{') depth++;
else if (source[i] === '}') {
depth--;
if (depth === 0) return source.slice(start, i + 1);
}
}
throw new Error(`unbalanced body for ${what}`);
}
// Class method body by name (async or not).
function extractMethod(source: string, name: string): string {
const openRe = new RegExp(`^\\s+(?:async\\s+)?${name}\\s*\\(`, 'm');
const match = openRe.exec(source);
if (!match) throw new Error(`method ${name} not found`);
return balanceBodyFrom(source, match.index, name);
}
// Top-level exported function body by name.
function extractFn(source: string, name: string): string {
const openRe = new RegExp(`export async function ${name}\\s*\\(`);
const match = openRe.exec(source);
if (!match) throw new Error(`function ${name} not found`);
return balanceBodyFrom(source, match.index, name);
}