Repository navigation
fix(engine): module-singleton shared-ownership — joiner must not tear down shared pool - #1599
Closed
jalagrange wants to merge 2 commits into
Closed
jalagrange wants to merge 2 commits into
jalagrange wants to merge 2 commits into
Conversation
…ull repair PR garrytan#1416 added "No database connection" to retry-matcher's retryable patterns, but the retry path never re-established the pool. When engine.disconnect() runs mid-cycle (e.g. between dream phases) the module-level singleton goes null; the in-flight batch flunks with "No database connection: connect() has not been called.", retries fire 3/3 against the same null singleton, and the batch is lost. Live repro on v0.41.26.0 against direct Postgres (not Supavisor): [extract.timeline_fs] connection blip, retrying (attempt 1/3): No database connection ... [extract.timeline_fs] connection blip, retrying (attempt 2/3): No database connection ... [extract.timeline_fs] connection blip, retrying (attempt 3/3): No database connection ... batch error (13 timeline rows lost): No database connection ... Symptoms: 192 exhausted batch retries in 24h (doctor batch_retry_health FAIL), cycle_freshness FAIL because conversation_facts_backfill's first engine.getConfig() also hits the dead singleton (immediate, no retry path). Two-file change: src/core/retry.ts Widen WithRetryOpts.onRetry to allow Promise<void> and await it in withRetry. The old fire-and-forget call meant async pre-retry repair work (e.g. engine.reconnect()) could not run atomically with the retry — the next attempt raced against unfinished repair. src/core/postgres-engine.ts In batchRetry's onRetry, call this.reconnect() before the next attempt sleeps. Mirrors the supervisor pool-restart pattern at minions/supervisor.ts:600. engine.reconnect() no-ops when _savedConfig is null (module-singleton mode without a saved engine config) so the call is defensively safe. withRetry only invokes onRetry after isRetryableConnError matches, so every onRetry call is for a connection-class error where reconnect is the right repair. Tests added in test/core/retry.test.ts: - async onRetry is awaited before next attempt (pins the contract batchRetry now depends on; without await, asserts event ordering would break) - onRetry that throws propagates (best-effort repair contract — the reconnect failure surfaces visibly rather than being swallowed) All 39 retry.test.ts tests pass; 94/94 across retry-adjacent suites (retry, retry-stress, retry-matcher, connection-resilience, doctor-batch-retry). Typecheck clean. Does not address the non-batch-wrapped path (conversation_facts_backfill's direct engine.getConfig fails with the same root cause). That's a separate, smaller fix — likely engine.ensureConnected() at phase start — intentionally left out of this PR to keep it surgical.
… down shared pool
Repro (cycle on direct Postgres):
- Cycle's main engine connects without poolSize → module path. db.ts
singleton initialized; _connectionStyle='module'.
- Lint phase's resolveLintContentSanity (src/commands/lint.ts:298) creates
a temp PostgresEngine to read 4 config keys, calls engine.connect({}).
Routes to module path; db.connect() early-returns because singleton is
already alive. Temp engine sets _connectionStyle='module' regardless,
having done nothing.
- Lint's try/finally calls await engine.disconnect() on the temp engine.
Pre-fix: falls through to db.disconnect() because _connectionStyle is
'module' — and silently nulls the shared singleton for the cycle.
- Sync, synthesize, and embed phases that run AFTER lint flunk with
"No database connection: connect() has not been called." Extract
survives because batchRetry's onRetry now reconnects (PR garrytan#1593) — but
the non-batch phases have no such cushion.
Live evidence captured by tracing db.disconnect() with a stack-trace probe
on a v0.41.26.0 install:
[DBG] db.disconnect called, sql=alive
Error
at disconnect (db.ts:228)
at disconnect (postgres-engine.ts:211)
at resolveLintContentSanity (lint.ts:319)
at async runLintCore (lint.ts:395)
at async runPhaseLint (cycle.ts:676)
at async runCycle (cycle.ts:1421)
Fix: track per-engine whether THIS instance actually initialized the
singleton (`_ownsModuleSingleton`) by snapshotting `db.isConnected()`
BEFORE calling `db.connect()`. Only the initializer is allowed to call
`db.disconnect()` on cleanup. Joiners no-op — the CLI's main engine
retains exclusive teardown rights.
Two-file change:
src/core/db.ts
Export new helper `isConnected()` returning whether the singleton
is live. Pure read, no side effect.
src/core/postgres-engine.ts
Add private `_ownsModuleSingleton` flag. In connect()'s module path,
snapshot `db.isConnected()` BEFORE `db.connect()` and set
`_ownsModuleSingleton = !singletonWasLive`. In disconnect(), gate
the `db.disconnect()` call on `_ownsModuleSingleton`. Reset both
flags on disconnect so the engine is reusable.
Test: test/postgres-engine-module-ownership.test.ts pins the contract
hermetically via mock.module on db.ts — 4 cases:
- first engine initializes; joiner does NOT disconnect on cleanup
- joiner.disconnect() before primary.disconnect() leaves singleton alive
(the exact lint-phase repro)
- joiner.disconnect() is idempotent
- solo engine round-trips correctly
99/99 tests pass across postgres-engine-module-ownership, engine-factory,
connection-resilience, connection-manager, retry. Typecheck clean.
Companion to PR garrytan#1593: that one repairs the batchRetry path so existing
data loss stops; this one fixes the ROOT cause so non-batch phases stop
flunking too. Together they close the dream-cycle singleton-null hole.
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
Companion to #1593 — that PR repairs the batch-retry path so existing data loss stops. This PR fixes the root cause: a temp engine that joins the shared module-level singleton silently kills it for the whole cycle on
disconnect().Repro (full dream cycle on direct Postgres)
poolSize→ module path.db.tssingleton initialized;_connectionStyle='module'.resolveLintContentSanity(src/commands/lint.ts:298) creates a tempPostgresEngineto read 4 config keys, callsengine.connect({}).db.connect()no-ops because singleton is already alive. Temp engine sets_connectionStyle='module'regardless — even though it did nothing.try/finallycallsawait engine.disconnect()on the temp engine. Pre-fix: falls through todb.disconnect()because_connectionStyle === 'module'— and silently nulls the shared singleton for the rest of the cycle.sync,synthesize, andembedphases that run AFTER lint flunk with"No database connection: connect() has not been called."extractsurvives because #1593'sbatchRetryonRetrycallsthis.reconnect()— but the non-batch phases have no such cushion.Live evidence
Traced
db.disconnect()on a v0.41.26.0 install with a temporary stack-trace probe:The first
db.disconnect()of the cycle comes from lint, not from anything the cycle author would expect.Fix
Track per-engine whether THIS instance actually initialized the singleton (
_ownsModuleSingleton) by snapshottingdb.isConnected()BEFORE callingdb.connect(). Only the initializer is allowed to calldb.disconnect()on cleanup. Joiners no-op — the CLI's main engine retains exclusive teardown rights.Two-file change
src/core/db.ts— export new helperisConnected():src/core/postgres-engine.ts— track ownership inconnect(), gate teardown indisconnect():Tests
test/postgres-engine-module-ownership.test.tspins the contract hermetically viamock.moduleondb.ts. Four cases:joiner.disconnect()BEFOREprimary.disconnect()leaves singleton alive (the exact lint-phase repro)joiner.disconnect()is idempotentEnd-to-end verification
Deployed both PRs to the same install + ran
gbrain dream:Before (master + nothing):
After (PR #1593 + this PR):
Zero
"No database connection"in the log. The remaining✗ embedis an unrelatedOPENAI_API_KEYconfig issue, not the singleton bug.Why ship both PRs
#1593 and this PR are independently load-bearing:
batchRetry's pre-existing retry loop has no reconnect, so even though the singleton no longer dies mid-cycle, any real (non-leak) connection blip still loses batches. #1593 is the defense-in-depth.extractrecovers, butsyncandsynthesizekeep flunking on every cycle. This PR is the root-cause fix.Either can land first; merging both gives a fully clean cycle.
Risk
Minimal — the only behavior change is that passive sharers stop tearing down the shared pool. The CLI's main engine, autopilot's long-lived engine, and any first-connecting engine still teardown exactly as before. Second/third-connecting engines (lint's content-sanity, frontmatter audit, any future similar pattern) now correctly no-op on cleanup.