fix(server-core): rebuild the cached driver when its configuration changes - #11453
Draft
MikeNitsenko wants to merge 1 commit into
Draft
fix(server-core): rebuild the cached driver when its configuration changes#11453MikeNitsenko wants to merge 1 commit into
MikeNitsenko wants to merge 1 commit into
Conversation
…anges
`getOrchestratorApi` builds its driver factory as a closure over the context of
the request that created the orchestrator, and the driver that factory resolves
is then cached for the life of the process. Nothing ever revisits either. When
`driverFactory` derives the connection from the security context — a per-user
OAuth token, say — the driver stays pinned to the token it was first built
with, and every new connection it opens after that token rotates fails to
authenticate. Only a redeploy clears it.
Track the latest request context per cached orchestrator and, when it changes,
check whether the factory now resolves a different configuration. If it does,
build a new driver and release the old one.
The check is layered so deployments that cannot be affected never leave the
fast path, and no user-supplied function is called more than it has to be:
1. No custom driverFactory, or one returning a constructed driver rather than
a config — nothing context-derived to compare, so reuse, exactly as before.
2. Security context byte-identical to what the cached driver was built from —
reuse without calling the factory at all. This is the common case;
requestId changes per request, credentials do not.
3. Context changed, so ask the factory. Most ignore it and return an identical
config — reuse, and remember the new context so step 2 short-circuits next
time.
4. Config genuinely changed — rebuild.
Configurations are compared by a truncated SHA-256 of a key-sorted
serialisation rather than kept verbatim, since the values include database
passwords and OAuth tokens. Anything that cannot be fingerprinted (a circular
structure, a constructed driver) yields null, which every caller reads as
"assume unchanged" so behaviour degrades to the previous resolve-once
semantics rather than to churn.
The replaced driver is released off the request path; `release` drains the
pool, so queries already running on it finish before its connections close.
This follows the documented contract of `contextToOrchestratorId` — that it is
the cache key for database connections. Two contexts resolving to different
connections while sharing an orchestrator id remain a misconfiguration; they
were already sharing one user's connection before this change.
Also updates the per-user OAuth recipe, whose `context_to_orchestrator_id`
returned the username alone and so could not survive a token rotation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11453 +/- ##
===========================================
- Coverage 83.95% 59.14% -24.81%
===========================================
Files 257 224 -33
Lines 80887 17965 -62922
Branches 0 3657 +3657
===========================================
- Hits 67908 10626 -57282
+ Misses 12979 6819 -6160
- Partials 0 520 +520
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:
|
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.
The stickiness half of CUB-3599. The console-server side fixes what triggers a bad credential; this fixes why a bad credential then survives until redeploy.
The problem
getOrchestratorApibuilds its driver factory as a closure over the context of the request that created the orchestrator, and the driver that factory resolves is cached for the life of the process. Nothing ever revisits either.So when
driverFactoryderives the connection from the security context — a per-user OAuth token, as our own per-user OAuth recipe instructs — the driver stays pinned to the token it was first built with. The JDBC pool drops to zero connections after ~30s idle, so the next query opens a fresh session with a token that expired an hour ago and gets403/401from the warehouse. Only a redeploy clears it.The fix
Track the latest request context per cached orchestrator, and when it changes, check whether the factory now resolves a different configuration. If it does, build a new driver and release the old one.
The check is layered so deployments that cannot be affected never leave the fast path, and no user-supplied function is called more often than it has to be:
driverFactory, or one returning a constructed driver rather than a config — nothing context-derived to compare. Reuse, exactly as before.requestIdchanges per request, credentials do not.Why this is safe
null, and every caller readsnullas "assume unchanged". Failure degrades to today's resolve-once behaviour, never to churn.releasedrains the pool, so queries already running on it finish before its connections close.contextToOrchestratorId— that it is the cache key for database connections. Two contexts resolving to different connections while sharing an orchestrator id remain a misconfiguration; they were already sharing one user's connection before this change.resolveDriveris split so a caller that has already invoked the factory (to compare) builds from that same result instead of invoking user code twice. Its public signature and behaviour are unchanged.Testing
test/unit/driver-config-fingerprint.test.ts(9 tests): property-order stability, credential-only changes,undefinedvs absent, arrays/dates/bigints, no plaintext in the digest,nullon circular input.test/unit/driver-cache-invalidation.test.ts(7 tests): rebuild on rotated credential, release of the replaced driver, reuse on unchanged context (and that the factory is not re-invoked), reuse when the factory ignores context, never rebuilding for a constructed driver, the@pre_aggalias following the rebuild, and no poisoned cache entry after a failed rebuild.cubejs-server-coreunit suite: 95 passing. The 10RefreshSchedulerfailures in my checkout are pre-existing — they reproduce identically on a cleanmaster(Unsupported env variable: "nativeSqlPlannerPreAggregations", aschema-compiler/sharedversion mismatch) and are unrelated to this diff.Not verified against a live warehouse; the tests stub driver construction so no database is required.
Also here
The per-user OAuth recipe is updated in the same PR — its
context_to_orchestrator_idreturned the username alone, which cannot survive a token rotation, and it gated on the credentialstatusflag in a way that fell back to the service account whenever a background refresh had failed. Both samples now derive the credential and the orchestrator id from one shared helper so they cannot drift.Follow-up, deliberately not here
OrchestratorStorage's LRU has nodispose, so evicted orchestrators are never released. Adding one would close connections under queries still running on an evicted orchestrator, which needs its own thought — filing separately rather than bundling it into a fix that needs to ship.🤖 Generated with Claude Code