fix(postgres-engine): only the singleton owner may disconnect it (#1471)#1651
Closed
nullhex-io wants to merge 2 commits into
Closed
fix(postgres-engine): only the singleton owner may disconnect it (#1471)#1651nullhex-io wants to merge 2 commits into
nullhex-io wants to merge 2 commits into
Conversation
…rytan#1471) A PostgresEngine connecting via the module-singleton path (no poolSize) got _connectionStyle='module' whether it CREATED the shared db.ts sql singleton (owner, e.g. the CLI cycle engine) or merely found it already connected (borrower, e.g. the probe engine in resolveLintContentSanity / doctor). disconnect() then called db.disconnect() for both, so a short-lived borrower's teardown nulled the singleton the owner was still using - every later cycle phase threw "No database connection: connect() has not been called" and the cycle lock stranded in gbrain_cycle_locks. Track ownership: db.isConnected() lets connect() record whether THIS engine created the singleton; disconnect() only calls db.disconnect() when it owns it. Borrowers clear their own marker without touching the shared connection. Instance-pool engines (poolSize set) are unaffected. reconnect() stays correct: an owner disconnect+reconnect re-acquires ownership; a borrower re-borrows.
…nnect ownership Follow-up doc nits on the garrytan#1471 fix: - reconnect() doc no longer implies module-singleton engines lack saved config (both paths set _savedConfig; both reconnect, re-sampling ownership). - Note that concurrent module-path connects both claiming ownership is benign (db.disconnect() is idempotent once sql is null).
Author
Owner
|
Thanks for this! The Postgres module-singleton / "connect() has not been called" ownership bug was fixed canonically in #1805 (atomic owner-only-disconnect), with follow-ups in #1608 and #1572 — all merged to master. Closing as already-resolved. If you still reproduce after upgrading, please reopen, and thank you for the contribution. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Fixes #1471 (root cause). Should also resolve the symptom issues that stem from the same module-singleton-null-mid-cycle defect: #1404, #1413, #1491, #1619.
A
PostgresEngineconnecting via the module-singleton path (nopoolSize) is tagged_connectionStyle = 'module'whether it CREATED the shareddb.tssqlsingleton (the owner - the CLI cycle engine) or merely found it already connected (a borrower - the probe engine inresolveLintContentSanity, doctor, future probes). The olddisconnect()calleddb.disconnect()for BOTH, so a short-lived borrower'sfinally-block teardown nulled the singleton the owner cycle engine was still using. Every later phase then threwNo database connection: connect() has not been called, andrunCycle's finally stranded a row ingbrain_cycle_locks._connectionStylealone can't separate owner from borrower - both are'module'. This implements the ownership-flag fix proposed in #1471.Fix
db.ts: addisConnected()so an engine can tell, atconnect()time, whether the singleton already exists.postgres-engine.ts: add_ownsModuleSingleton.connect()samples ownership before delegating todb.connect();disconnect()callsdb.disconnect()only when this engine owns the singleton. A borrower clears its own marker without touching the shared connection.Scope:
poolSizeset) are unaffected - they never take the module branch.reconnect()stays correct: an owner's disconnect+reconnect re-acquires ownership (the singleton is null right after its owndb.disconnect()), and a borrower re-borrows the owner's (possibly refreshed) singleton.Tests
test/postgres-engine-singleton-ownership.test.ts: source-level guardrails (DB-free, matching the existingpostgres-engine.test.tsconvention, since runtime-mocking postgres.js's tagged-template interface is painful under bun ESM). Assertsdb.isConnected()is sampled beforedb.connect(), and that the onlydb.disconnect()indisconnect()is guarded by_ownsModuleSingleton.bun run typecheck: clean.postgres-engine,db-disconnect-audit,connection-manager.serial,autopilot-reconnect-classifier,worker-shutdown-disconnect.The live
gbrain dreamcycle is the e2e exercise of this path; the ownership logic above is what keeps the owner's singleton alive across a borrower probe's teardown.