fix: update pool reliably upon config changes - #1269
Conversation
Coverage Report for CI Build 30382283150Coverage decreased (-0.09%) to 80.32%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions19 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a large, high-risk change to core database pool lifecycle and tenant config invalidation (revision-based reconciliation, a new invalidatable single-flight primitive, pool close/reconcile/retire semantics, and pinned-executor routing for pgvector) — no bugs were found, but given the concurrency and connection-routing correctness at stake, this warrants a human review rather than an automated approval.
What was reviewed: the config-revision ordering (compareConfigRevisions, PgPoolStrategy.isCurrent/reconcile) for stale-vs-newer races; the retire/close/reconcile pool lifecycle including drain tracking and the bounded retireAll loop; the tenant-config single-flight generation handoff (detached loads, missing-config redirects) for lost-update races; and the pgvector endpoint-scope keying used to avoid mixing connections across a pool swap.
Extended reasoning...
This PR reworks tenant database pool lifecycle management end-to-end: a monotonic config revision now flows from tenant config snapshots through PoolManager.getPool/reconcileExisting into PgPoolStrategy, replacing the previous ad hoc destroy/rebalance calls. It introduces createInvalidatableSingleFlightByKey to let a newer tenant-config load supersede a stale in-flight one without losing detached callers, adds pool close/reconcile/retire as distinct lifecycle states (vs. a single destroy), introduces a DatabaseUnavailable/503 error path for mid-request endpoint changes, and adds pinned-executor routing plus endpoint-scoped caching in the pgvector adapter so a pool swap can't mix connections across endpoints. Twenty-five files are touched, with the bulk of the logic living in src/internal/database/{pool,pg-connection,tenant}.ts.
Security risks
No injection, auth, or data-exposure surface is introduced. The main risk class here is correctness under concurrency: a race in revision comparison, single-flight generation tracking, or pool retirement could route a request to a stale/wrong database endpoint, leak a pool that should have been closed, or leave a tenant pool permanently unusable after a config update. None of these are classic security vulnerabilities, but a wrong-endpoint route for a multi-tenant storage service has some of the same blast radius (cross-tenant data reaching the wrong backend) if the revision/scope logic has a gap.
Level of scrutiny
This warrants the highest scrutiny tier for this repo: it changes the pooling and connection-lifecycle code that every multi-tenant request depends on, replaces existing production-tested destroy/rebalance semantics with new revision-ordering logic, and does so through several interacting new primitives (invalidatable single-flight, pool reconcile/retire, endpoint scoping) whose correctness depends on subtle interleavings that are hard to fully verify from the diff alone.
Other factors
The PR ships with a very extensive test suite covering many of the race conditions described above (detached-load handoff, obsolete-deletion vs recreation, retire-during-shutdown, endpoint-scope rotation for pgvector caches), which is a strong positive signal, but the sheer number of new interacting state machines (revision comparison, single-flight generations, pool lifecycle states) is exactly the kind of design decision that benefits from a second set of human eyes, particularly around edge cases not covered by tests (e.g., concurrent reconcile+retire interleavings, or admin-triggered deletion racing a live query using acquirePinnedExecutor).
050d3b2 to
30e1ca5
Compare
|
@claude review always |
There was a problem hiding this comment.
This PR reworks core database connection pool lifecycle and tenant config caching with revision-based reconciliation, invalidatable single-flight loads, and a new retire/reconcile/close-pool lifecycle — no bugs were found by the automated review, but given the scope and criticality of this change I think it's worth a human look before merging.
Extended reasoning...
Overview
The PR touches core multitenant database connection pooling (pg-connection.ts, pool.ts), tenant config caching and invalidation (tenant.ts), a new invalidatable single-flight primitive (single-flight.ts), a cache peek() addition (lru.ts/ttl.ts/monitoring.ts), and the pgvector adapter's per-endpoint capability/metric caching. It introduces a monotonic config revision that flows from tenant config load through pool reconciliation, a pool lifecycle split into close/reconcile/retire, and a bounded-rounds shutdown loop that drains pools created during retirement.
Security risks
No new external attack surface — this is internal connection-pool/cache-invalidation logic, not auth, crypto, or user-facing input handling. The main risk class is correctness/availability: a race or logic error here could route tenant traffic to a stale or wrong database endpoint, or leak/leave open connections. The diff includes several tests that specifically target detached-generation races (single-flight invalidation, overlapping notifications, retire vs. reconcile ordering), which is the right area to stress.
Level of scrutiny
This warrants a high level of scrutiny: it's a large, non-mechanical rewrite of the pool lifecycle and tenant-config invalidation path that all multitenant requests go through, with intricate concurrency invariants (revision comparisons, single-flight generation ownership, bounded retirement rounds) that are easy to get subtly wrong under concurrent invalidation. The human reviewers already in the thread (fenos, ferhatelmas) have been iterating on naming and design (isExternalPool semantics, retiringPools tracking, pgvector cache pinning) and marked their threads resolved, but the density of new invariants here is well beyond what should be rubber-stamped.
Other factors
Test coverage is extensive and specifically targets the race conditions this PR aims to fix (overlapping notifications, detached loads, obsolete deletions racing recreation, bounded shutdown rounds). Coveralls reports patch coverage of 95.6% with two pre-existing regressions in unrelated files. No bugs were surfaced by the automated bug hunt, but given the blast radius (all multitenant DB connections) I'd rather a human confirm the revision/reconciliation invariants hold before this merges.
Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
There was a problem hiding this comment.
Beyond the inline finding, I checked the two other candidates flagged this run: the dbUrl-change path no longer destroys the old pool synchronously, but the old cache entry is keyed by the stale dbUrl and will still be torn down automatically once its TTL cache entry expires — not an immediate destroy, but not a leak either. And PoolManager.rebalance(tenantId, ...) is now only exercised by tests (production callers were removed in favor of the per-request rebalance in getPool()), which is dead weight but not a functional bug.
Extended reasoning...
Verified against the diff: onTenantConfigChange (src/internal/database/tenant.ts) now only calls deleteTenantConfig and no longer calls destroyTenantPool, so a dbUrl change relies on the tenantPools TTL cache (src/internal/database/pool.ts, backed by @isaacs/ttlcache) to reap the orphaned old-dbUrl entry via its own expiry timer rather than an immediate destroy. Grepped for callers of PoolManager.rebalance(tenantId, ...) and found none outside pool.test.ts — rebalanceAll (used by src/start/server.ts) is unaffected and still live.
2846843 to
006adb0
Compare
Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
006adb0 to
0e7d778
Compare
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Tenant config could change (URL, max connections, delete) while processes still held stale pools or stale single-flight loads. Rebalance/destroy was incomplete and racy under concurrent invalidation.
What is the new behavior?
Pools are cached by tenant and database url so stale requests can't revert the endpoint.
Add invalidatable single flight so that an old load can't populate the cache with wrong config.