fix(server-core): release and connection-test dedicated pre-aggregation drivers - #11465
Conversation
|
Claude finished @igorlukanin's task in 4m 21s —— View job Review: solid fix, one caller-side gap — 1 high, 1 medium, 3 low (4 inline comments)Full review
SummaryThe root-cause analysis is correct and the fix is well-aimed. Findings
Finding 1 is the only one I'd consider blocking. Note it is not introduced by the diff in isolation — the callers were always unguarded — but the diff is what makes that path reachable: before it, Nit: Tests
Gaps, in priority order:
Not verifiedDependencies aren't installed in this checkout (no |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| errors.forEach((error) => { | ||
| this.logger('Error during release', { | ||
| error: (error as Error)?.stack || error, | ||
| }); | ||
| }); | ||
|
|
||
| throw errors[0]; |
There was a problem hiding this comment.
Throwing here is right, but the callers aren't ready for it. Before this PR release() iterated seenDataSources, which is only ever populated by the readiness probe — so on most deployments it touched no internal driver and effectively could not throw. Now it releases every driver, so a single pool whose release() rejects makes release() reject, and every caller does its cleanup after an unguarded await:
OrchestratorStorage.releaseConnections()—this.storage.clear()is skipped, so the map keeps orchestrator APIs that are already spent: theirrequestedDriverswas cleared at the top ofrelease()and some pools are closed.getOrchestratorApi()will hand those back out (dead pools), and a secondrelease()on them is a no-op — the pools that did survive are now unreachable forever.CubejsServerCore.releaseConnections()(server.ts:901) —clearInterval(this.maxCompilerCacheKeep)andscheduledRefreshTimerInterval.cancel()are skipped, so a graceful shutdown can hang on a live timer instead of exiting.resetInstanceState()(server.ts:559) —reloadEnvVariables()andstartScheduledRefreshTimer()are skipped, leaving the dev server with no refresh timer.
The PR's intent ("shutdown must exit non-zero", "a reset must not reload over state it could not free") is satisfied by propagating the error after the local cleanup runs. Simplest fix is try { ... } finally { ... } in the three callers — at minimum OrchestratorStorage.releaseConnections:
public async releaseConnections() {
try {
await Promise.all([...this.storage.values()].map(api => api.release()));
} finally {
this.storage.clear();
}
}Worth a test too: the suite covers "one driver refusing to close does not strand the others" within one api, but not that the caller can still finish tearing down.
| const hasSeparatePreAggEnv = hasPreAggregationsEnvVars(dataSource); | ||
| const usePreAgg = preAggregations && hasSeparatePreAggEnv && !this.optsHandler.isCustomDriverFactory(); |
There was a problem hiding this comment.
Moving these two above the cache-hit early return puts them on the hot path: driverFactory is invoked per query and per pre-aggregation check, and hasPreAggregationsEnvVars('default') allocates a Set and iterates all of process.env on every call (env.ts:146). Previously it ran once per cache miss, i.e. once per data source.
The key does have to be computed before the lookup, so the ordering itself is necessary — but the inputs can be memoized per dataSource inside this closure (they only change across a reloadEnvVariables(), which discards the orchestrator anyway):
const preAggEnvCache: Record<string, boolean> = {};
// ...
const hasSeparatePreAggEnv = preAggEnvCache[dataSource] ??= hasPreAggregationsEnvVars(dataSource);
|
|
||
| await this.releaseDriverInstance(resolved); | ||
| }), | ||
| this.releaseDriver(this.options.externalDriverFactory), |
There was a problem hiding this comment.
The external driver still goes back through its factory — the exact pattern the PR removes for internal drivers. In server.ts the external closure caches into externalPreAggregationsDriverPromise, so if the external driver was never used (or its last resolution failed and nulled the cache), release() constructs an external driver, testConnection()s it, and immediately closes it. On a deployment configured with an external pre-aggregation store but no pre-aggregation traffic, shutdown opens a connection just to shut it down — and a failure there now rejects the whole release() (see the other thread).
Recording the external driver the same way (or tracking externalPreAggregationsDriverPromise resolution) would make the rule uniform: release closes what was built, never builds.
| // One entry per distinct request, overwriting the previous promise for the | ||
| // same one: a re-request returns the same connection, and the factory owns | ||
| // its cache, so the latest promise is the one worth holding. | ||
| this.requestedDrivers.set(`${dataSource}${preAggregations ? '@pre_agg' : ''}`, { |
There was a problem hiding this comment.
Nit, against the PR description's claim that "the @pre_agg suffix now appears once in the tree instead of twice under two different rules": it still appears twice — here, and in driverCacheKey(). The two genuinely key on different things (this one on the request, the helper on the credentials), which is defensible, but the driverCacheKey docstring says "cached and tracked under", implying this call site uses it. Either call the helper here with preAggregations and note in the doc why the tracking key may over-count (harmless: identity dedup settles release, and the tested set settles the probe), or narrow the helper's doc to the factory cache only.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11465 +/- ##
===========================================
- Coverage 79.45% 59.20% -20.26%
===========================================
Files 480 224 -256
Lines 98810 17925 -80885
Branches 3636 3642 +6
===========================================
- Hits 78511 10612 -67899
+ Misses 19778 6792 -12986
Partials 521 521
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary
A deployment with a dedicated pre-aggregation connection leaked that driver's connection pool on every orchestrator teardown, and never connection-tested it.
OrchestratorApi.release()andtestConnection()iteratedseenDataSources, which holds plain data source names. The driver factory caches a dedicated pre-aggregation driver separately, so that driver was unreachable from either path.addDataSeenSource()is also called from exactly one place — the standalone readiness probe, hardcoded todefault— so on most deploymentsrelease()reached no internal driver at all.OrchestratorApinow wraps the driver factory it is constructed with and records every driver actually requested, then releases those instances. Because the wrapper is what it passes toQueryOrchestrator, the query path, the pre-aggregation subsystem and the data source probe all record through it — nothing has to guess which drivers exist.driverCacheKey()decides when a pre-aggregation request earns its own connection, and it takes that decision as an argument rather than re-deriving it, so the cache key cannot disagree with the credentials actually used. That fixes a second, order-dependent duplicate: keying on the request flag alone meant a pre-aggregation build arriving before the first query cached separately, and the next query then built another driver for the same connection — which is exactly what a refresh worker does. It also covers the case where a customdriverFactorytakes precedence over pre-aggregation env vars, where the two requests resolve to identical credentials and must therefore share one driver. The two compensating aliasing writes this replaces are deleted; the@pre_aggsuffix now appears once in the tree instead of twice under two different rules.Behaviour change worth noting
testConnection()now covers dedicated pre-aggregation connections, so a broken one that was previously invisible to the readiness and liveness probes will now fail them. That is the point of the change, but it is the one way a deployment could newly report unhealthy. Probes are otherwise no chattier than before: requests that share a connection resolve to one driver and are tested once.Not in scope:
OrchestratorStorage's LRU has nodispose, so an evicted orchestrator is still never released. Closing that would need to avoid dropping pools under queries still running on the evicted orchestrator, which deserves its own change — this one makes it easier, sincerelease()can now neither construct a driver nor be stalled by one.Test plan
test/unit/driver-lifecycle.test.ts(15 tests): both drivers released and tested with a dedicated pre-aggregation connection; named data sources; a single driver for both request orders without one, and for a customdriverFactoryoverriding the env vars; release never constructing a driver (never-queried, and failed-to-resolve); a failing release reported without stranding the others; the readiness probe still forcing the primary connection when only the pre-aggregation driver exists; tracking bounded across repeated requests and probes;rollupOnlyModeunchanged.seenDataSources(the original bug), no identity dedup, release re-resolving through the factory, dropping the readiness-probe union, a cache key omitting the credentials term, a no-op release, swallowed release errors, and unbounded per-call tracking. Each is caught by a distinct group.cubejs-server-coreunit suites green:driver-lifecycle,index,OrchestratorApi,OptsHandler,CompilerApi— 91 tests.tscandeslintclean.RefreshScheduler.test.tsfails on unmodifiedmasterin this package (order-dependent); verified by stashing, unrelated to this diff and unchanged by it.