From cfa2eb1161fda050b1d3517e35b4d9ceeca550fd Mon Sep 17 00:00:00 2001 From: MikeNitsenko Date: Sun, 2 Aug 2026 23:58:50 +0500 Subject: [PATCH 01/16] fix(server-core): rebuild the cached driver when its configuration changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- .../connect-to-data/oauth-authentication.mdx | 210 +++++++++++------ .../src/core/driver-config-fingerprint.ts | 93 ++++++++ .../cubejs-server-core/src/core/server.ts | 217 +++++++++++++++++- .../unit/driver-cache-invalidation.test.ts | 212 +++++++++++++++++ .../unit/driver-config-fingerprint.test.ts | 65 ++++++ 5 files changed, 719 insertions(+), 78 deletions(-) create mode 100644 packages/cubejs-server-core/src/core/driver-config-fingerprint.ts create mode 100644 packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts create mode 100644 packages/cubejs-server-core/test/unit/driver-config-fingerprint.test.ts diff --git a/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx b/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx index c18f0e2ef4a23..20e53301894e9 100644 --- a/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx +++ b/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx @@ -26,6 +26,19 @@ Because every user connects with different credentials, you also need per-user query orchestrator state. Without this, one user's cached connection could leak to another. + + +Cube caches one database connection per +[`context_to_orchestrator_id`][ref-context-to-orchestrator-id] for the +lifetime of the process. **Every input your `driver_factory` reads must +therefore also appear in the orchestrator ID.** OAuth access tokens rotate +(typically hourly), so an ID built from the username alone leaves the +cached connection pinned to the token it was first built with — new +database sessions then fail to authenticate until the next deploy. The +configuration below derives both from one helper so they cannot drift. + + + ## Prerequisites - A [Cube Cloud][ref-cube-cloud] deployment connected to an @@ -152,35 +165,61 @@ value with the correct `type` and driver-specific options. See the ```python cube.py from cube import config +from datetime import datetime, timezone import os +import time + +# Don't hand the driver a token that is about to expire. Drivers cache their +# connection settings, and the connection pool opens new sessions long after +# the driver was built, so "valid right now" is not enough. +EXPIRY_SKEW_SECONDS = 120 + + +def _parse_expiry(value): + if not value: + return None + try: + parsed = datetime.fromisoformat(str(value).replace("Z", "+00:00")) + except ValueError: + return None + if parsed.tzinfo is None: + parsed = parsed.replace(tzinfo=timezone.utc) + return parsed.timestamp() + + +def _credential(ctx: dict): + """Resolve the credential for this request, plus the key identifying it. + + driver_factory and context_to_orchestrator_id both call this, so the cached + connection can never drift from the token it was built with. + """ + # For other data sources, swap "databricks" for "snowflake", etc. + cube_cloud = (ctx.get("securityContext") or {}).get("cubeCloud") or {} + creds = (cube_cloud.get("userCredentials") or {}).get("databricks") or {} + + access_token = creds.get("accessToken") + expires_at = _parse_expiry(creds.get("accessTokenExpiresAt")) + + # Gate on the expiry rather than on `status`: a failed background refresh + # can flag the record while the token already in hand is still valid, and + # treating that as fatal drops the user onto the service account for no + # reason. + if access_token and expires_at and expires_at > time.time() + EXPIRY_SKEW_SECONDS: + # Key on the expiry, never on the token itself — the orchestrator ID is + # used as a cache prefix and appears in logs. + return access_token, f"u{int(expires_at)}" + + return os.environ["CUBEJS_DB_DATABRICKS_TOKEN"], "service-account" @config("driver_factory") def driver_factory(ctx: dict) -> dict: - # Extract the Cube Cloud security context, which contains - # per-user OAuth credentials when available. - # For other data sources, swap "databricks" for "snowflake", etc. - databricks_creds = ( - ctx - .get("securityContext", {}) - .get("cubeCloud", {}) - .get("userCredentials", {}) - .get("databricks", {}) - ) - - # Only use the OAuth token when the credential status is "active". - # An expired or revoked token falls back to the service account. - oauth_token = ( - databricks_creds.get("accessToken") - if databricks_creds.get("status") == "active" - else None - ) + token, _ = _credential(ctx) return { "type": "databricks-jdbc", "url": os.environ["CUBEJS_DB_DATABRICKS_URL"], - # Prefer the user's OAuth token; fall back to the service account token - "token": oauth_token or os.environ["CUBEJS_DB_DATABRICKS_TOKEN"], + "token": token, "acceptPolicy": True, "catalog": os.environ.get("CUBEJS_DB_DATABRICKS_CATALOG"), } @@ -188,15 +227,14 @@ def driver_factory(ctx: dict) -> dict: @config("context_to_orchestrator_id") def context_to_orchestrator_id(ctx: dict) -> str: - # Give each user a separate orchestrator instance (DB connections, - # execution queues, pre-aggregation caches) - username = ( - ctx - .get("securityContext", {}) - .get("cubeCloud", {}) - .get("username", "default") - ) - return f"CUBE_APP_{username}" + # One orchestrator per (user, credential): separate DB connections, + # execution queues and pre-aggregation caches, and a cache key that changes + # when the token rotates. + cube_cloud = (ctx.get("securityContext") or {}).get("cubeCloud") or {} + username = cube_cloud.get("username") or "default" + _, cache_key = _credential(ctx) + + return f"CUBE_APP_{username}_{cache_key}" ``` @@ -204,36 +242,55 @@ def context_to_orchestrator_id(ctx: dict) -> str: ```javascript cube.js -module.exports = { - driverFactory: ({ securityContext }) => { - // Extract the Cube Cloud security context, which contains - // per-user OAuth credentials when available. - // For other data sources, swap `databricks` for `snowflake`, etc. - const databricksCreds = - securityContext?.cubeCloud?.userCredentials?.databricks ?? {}; - - // Only use the OAuth token when the credential status is "active". - // An expired or revoked token falls back to the service account. - const oauthToken = - databricksCreds.status === "active" - ? databricksCreds.accessToken - : null; +// Don't hand the driver a token that is about to expire. Drivers cache their +// connection settings, and the connection pool opens new sessions long after +// the driver was built, so "valid right now" is not enough. +const EXPIRY_SKEW_MS = 120 * 1000; + +/** + * Resolve the credential for this request, plus the key identifying it. + * + * driverFactory and contextToOrchestratorId both call this, so the cached + * connection can never drift from the token it was built with. + */ +function resolveCredential(securityContext) { + // For other data sources, swap `databricks` for `snowflake`, etc. + const creds = securityContext?.cubeCloud?.userCredentials?.databricks ?? {}; + const expiresAt = Date.parse(creds.accessTokenExpiresAt ?? ""); + + // Gate on the expiry rather than on `status`: a failed background refresh can + // flag the record while the token already in hand is still valid, and + // treating that as fatal drops the user onto the service account for no + // reason. + if (creds.accessToken && expiresAt > Date.now() + EXPIRY_SKEW_MS) { + // Key on the expiry, never on the token itself — the orchestrator ID is + // used as a cache prefix and appears in logs. + return { token: creds.accessToken, cacheKey: `u${expiresAt}` }; + } + + return { + token: process.env.CUBEJS_DB_DATABRICKS_TOKEN, + cacheKey: "service-account", + }; +} - return { - type: "databricks-jdbc", - url: process.env.CUBEJS_DB_DATABRICKS_URL, - // Prefer the user's OAuth token; fall back to the service account token - token: oauthToken || process.env.CUBEJS_DB_DATABRICKS_TOKEN, - acceptPolicy: true, - catalog: process.env.CUBEJS_DB_DATABRICKS_CATALOG, - }; - }, - - // Give each user a separate orchestrator instance (DB connections, - // execution queues, pre-aggregation caches) +module.exports = { + driverFactory: ({ securityContext }) => ({ + type: "databricks-jdbc", + url: process.env.CUBEJS_DB_DATABRICKS_URL, + token: resolveCredential(securityContext).token, + acceptPolicy: true, + catalog: process.env.CUBEJS_DB_DATABRICKS_CATALOG, + }), + + // One orchestrator per (user, credential): separate DB connections, execution + // queues and pre-aggregation caches, and a cache key that changes when the + // token rotates. contextToOrchestratorId: ({ securityContext }) => { const username = securityContext?.cubeCloud?.username ?? "default"; - return `CUBE_APP_${username}`; + const { cacheKey } = resolveCredential(securityContext); + + return `CUBE_APP_${username}_${cacheKey}`; }, }; ``` @@ -248,21 +305,40 @@ module.exports = { credentials to `securityContext.cubeCloud.userCredentials.` (for example, `.databricks` or `.snowflake`). -2. **`driver_factory` resolves the credential** — If the credential status - is `active`, the user's OAuth token is used. Otherwise, Cube falls back - to the service account credential stored in environment variables. - -3. **Per-user orchestrator** — - [`context_to_orchestrator_id`][ref-context-to-orchestrator-id] returns - a unique key per username, so each user gets their own database - connection pool, execution queues, and pre-aggregation table cache. - Without this, Cube would share a single cached connection across all - users, causing one user's credentials to be reused for another user's - queries. +2. **`driver_factory` resolves the credential** — If the user has a token + that has not expired, it is used. Otherwise, Cube falls back to the + service account credential stored in environment variables. + +3. **Per-user, per-credential orchestrator** — + [`context_to_orchestrator_id`][ref-context-to-orchestrator-id] returns a + key derived from both the username and the credential in use, so each + user gets their own database connection pool, execution queues, and + pre-aggregation table cache — and a rotated token produces a fresh + connection instead of reusing one built from the previous token. Keying + on the username alone would share a single cached connection across + token rotations; omitting the username would share one across users. + +## Operational notes + +- **Expect one orchestrator per token rotation.** Each distinct + orchestrator ID holds its own connection pool, queues and + pre-aggregation table cache, and tokens typically rotate hourly. Watch + memory on deployments with many concurrent users, and note that the + first query after a rotation runs against a cold pre-aggregation cache. +- **Don't make [`context_to_app_id`][ref-context-to-appid] per-user.** The + data model is identical for every user — only the connection differs — + so a per-user app ID forces a full data-model recompile per user on + every replica for no benefit. Leave it unset, or return a constant if + your deployment already sets one. +- **Give the service account the minimum it needs to pass a connection + check.** If it has no access at all, liveness checks and any query that + falls back to it fail with an opaque authorization error from the driver + rather than something diagnosable. [ref-config]: /reference/configuration/config [ref-driver-factory]: /reference/configuration/config#driver_factory [ref-context-to-orchestrator-id]: /reference/configuration/config#context_to_orchestrator_id +[ref-context-to-appid]: /reference/configuration/config#context_to_app_id [ref-databricks-jdbc]: /admin/connect-to-data/data-sources/databricks-jdbc [ref-snowflake]: /admin/connect-to-data/data-sources/snowflake [ref-data-sources]: /admin/connect-to-data/data-sources diff --git a/packages/cubejs-server-core/src/core/driver-config-fingerprint.ts b/packages/cubejs-server-core/src/core/driver-config-fingerprint.ts new file mode 100644 index 0000000000000..bed7ee35bbf79 --- /dev/null +++ b/packages/cubejs-server-core/src/core/driver-config-fingerprint.ts @@ -0,0 +1,93 @@ +/** + * @copyright Cube Dev, Inc. + * @license Apache-2.0 + * @fileoverview Fingerprinting for driver configurations and security contexts. + */ + +import crypto from 'crypto'; + +/** + * Deterministic JSON used for fingerprinting. Object keys are emitted in sorted + * order so two structures that differ only in property order hash the same, and + * values JSON cannot represent are reduced to stable placeholders rather than + * silently disappearing. Throws on a circular structure, which callers treat as + * "not fingerprintable". + */ +function stableStringify(value: unknown, seen: Set): string { + if (value === undefined || value === null) { + return 'null'; + } + + const type = typeof value; + + if (type === 'string' || type === 'number' || type === 'boolean') { + return JSON.stringify(value); + } + + if (type === 'bigint') { + return JSON.stringify((value as bigint).toString()); + } + + // A closure's identity cannot be compared meaningfully across calls, so it + // contributes a constant. Two configs differing only in a function body are + // therefore treated as equal — deliberately conservative: it can only lead to + // reusing a connection, never to swapping one out unnecessarily. + if (type === 'function' || type === 'symbol') { + return JSON.stringify(`[${type}]`); + } + + if (value instanceof Date) { + return JSON.stringify(value.toISOString()); + } + + if (seen.has(value)) { + throw new Error('Circular structure cannot be fingerprinted'); + } + + seen.add(value); + + try { + if (Array.isArray(value)) { + return `[${value.map((item) => stableStringify(item, seen)).join(',')}]`; + } + + const entries = Object.keys(value as Record) + .sort() + .reduce((acc, key) => { + const entry = (value as Record)[key]; + + // Match JSON.stringify: undefined-valued properties are absent, so + // `{ a: undefined }` and `{}` fingerprint the same. + if (entry !== undefined) { + acc.push(`${JSON.stringify(key)}:${stableStringify(entry, seen)}`); + } + + return acc; + }, []); + + return `{${entries.join(',')}}`; + } finally { + seen.delete(value); + } +} + +/** + * A short, stable digest of `value`, or `null` when it cannot be fingerprinted. + * + * Hashed rather than kept verbatim because the values being compared include + * database passwords and OAuth access tokens: a raw copy would live for the + * lifetime of the process and surface in any heap dump. `null` means "cannot + * tell whether this changed", and every caller must treat that as "assume it + * did not" so behaviour falls back to the previous resolve-once semantics. + */ +export function fingerprint(value: unknown): string | null { + try { + return crypto + .createHash('sha256') + .update(stableStringify(value, new Set())) + .digest('hex') + .slice(0, 32); + } catch (e) { + return null; + } +} diff --git a/packages/cubejs-server-core/src/core/server.ts b/packages/cubejs-server-core/src/core/server.ts index 48b146c8abb70..4a3dbf228df9a 100644 --- a/packages/cubejs-server-core/src/core/server.ts +++ b/packages/cubejs-server-core/src/core/server.ts @@ -37,6 +37,7 @@ import { agentCollect } from './agentCollect'; import { OrchestratorStorage } from './OrchestratorStorage'; import { createLogger } from './logger'; import { OptsHandler } from './OptsHandler'; +import { fingerprint } from './driver-config-fingerprint'; import { driverDependencies, lookupDriverClass, @@ -74,6 +75,21 @@ import { const { version } = require('../../../package.json'); +/** + * What a cached driver was built from. `null` on either field means "cannot + * tell whether it changed", which is always read as "assume it did not". + */ +type DriverOrigin = { + securityContextFingerprint: string | null; + configFingerprint: string | null; +}; + +/** A `driverFactory` result together with the context that produced it. */ +type DriverFactoryResult = { + value: DriverConfig | BaseDriver; + securityContextFingerprint: string | null; +}; + function wrapToFnIfNeeded(possibleFn: T | ((a: R) => T)): (a: R) => T { if (typeof possibleFn === 'function') { return possibleFn; @@ -130,6 +146,19 @@ export class CubejsServerCore { protected readonly orchestratorStorage: OrchestratorStorage = new OrchestratorStorage(); + /** + * The request context each cached orchestrator most recently served. + * + * An orchestrator's driver factory closes over the context of the request + * that created it, and the driver it resolves is then cached for the life of + * the process. When that driver's configuration is derived from the context — + * a per-user OAuth token, say — it goes stale the moment the credential + * rotates. Tracking the latest context lets the factory notice. Keyed by the + * api instance so an entry disappears with the orchestrator it belongs to. + */ + protected readonly orchestratorRequestContexts = + new WeakMap(); + // eslint-disable-next-line @typescript-eslint/no-unused-vars protected repositoryFactory: ((context: RequestContext) => SchemaFileRepository) | (() => FileRepository); @@ -573,9 +602,22 @@ export class CubejsServerCore { const orchestratorId = await this.contextToOrchestratorId(context); if (this.orchestratorStorage.has(orchestratorId)) { - return this.orchestratorStorage.get(orchestratorId); + const cachedOrchestratorApi = this.orchestratorStorage.get(orchestratorId); + const cachedContextRef = this.orchestratorRequestContexts.get(cachedOrchestratorApi); + + // Keep the driver factory's view of the request context current. Without + // this it stays pinned to whichever request happened to create the + // orchestrator, and a driver built from context-derived credentials can + // never be rebuilt when they rotate. + if (cachedContextRef) { + cachedContextRef.current = context; + } + + return cachedOrchestratorApi; } + const requestContextRef: { current: RequestContext } = { current: context }; + /** * Hash table to store promises which will be resolved with the * datasource drivers. DriverFactoryByDataSource function is closure @@ -583,6 +625,12 @@ export class CubejsServerCore { */ const driverPromise: Record> = {}; + /** + * What each cached driver in `driverPromise` was built from, so a changed + * configuration can be detected. Keyed identically to `driverPromise`. + */ + const driverOrigin: Record = {}; + let externalPreAggregationsDriverPromise: Promise | null = null; const contextToDbType: DbTypeInternalFn = this.contextToDbType.bind(this); @@ -602,13 +650,57 @@ export class CubejsServerCore { */ async (dataSource = 'default', preAggregations = false) => { const factoryKey = preAggregations ? `${dataSource}@pre_agg` : dataSource; - if (driverPromise[factoryKey]) { - return driverPromise[factoryKey]; - } const hasSeparatePreAggEnv = hasPreAggregationsEnvVars(dataSource); const usePreAgg = preAggregations && hasSeparatePreAggEnv && !this.optsHandler.isCustomDriverFactory(); + const driverContext = (): DriverContext => ({ + ...requestContextRef.current, + dataSource, + preAggregations: usePreAgg || false, + }); + + // Already resolved by the staleness check below, so the factory is not + // asked twice for the same rebuild. + let resolvedFactoryResult: DriverFactoryResult | undefined; + + if (driverPromise[factoryKey]) { + const staleness = await this.resolveDriverStaleness( + driverOrigin[factoryKey], + driverContext(), + ); + + if (!staleness.stale) { + return driverPromise[factoryKey]; + } + + this.logger('Rebuilding driver on configuration change', { + dataSource, + preAggregations, + }); + + const replaced = driverPromise[factoryKey]; + + driverPromise[factoryKey] = null; + if (!preAggregations && !hasSeparatePreAggEnv) { + driverPromise[`${dataSource}@pre_agg`] = null; + } + + // Graceful: `release` drains the pool, so queries already running on + // the replaced driver finish before its connections are closed. It is + // deliberately not awaited — this request should not wait on the + // previous driver's in-flight work — and its failure must not fail + // this request. + replaced + .then((driver) => driver.release()) + .catch((error) => this.logger('Driver release error', { + dataSource, + error: (error as Error).stack || (error as Error).toString(), + })); + + resolvedFactoryResult = staleness.factoryResult; + } + if (preAggregations && hasSeparatePreAggEnv && this.optsHandler.isCustomDriverFactory()) { this.logger('Pre-aggregation driver conflict', { error: 'Both driverFactory and PRE_AGGREGATIONS env vars are defined. driverFactory will take precedence.', @@ -616,16 +708,38 @@ export class CubejsServerCore { }); } + // Shared by reference with the `@pre_agg` alias below, so both keys + // describe the one driver they both resolve to. Starts empty: until the + // factory has been called there is nothing to compare against, and + // `resolveDriverStaleness` reads that as "reuse". + const origin: DriverOrigin = { + securityContextFingerprint: null, + configFingerprint: null, + }; + + driverOrigin[factoryKey] = origin; + if (!preAggregations && !hasSeparatePreAggEnv) { + driverOrigin[`${dataSource}@pre_agg`] = origin; + } + driverPromise[factoryKey] = (async () => { let driver: BaseDriver | null = null; try { - driver = await this.resolveDriver( - { - ...context, - dataSource, - preAggregations: usePreAgg || false, - }, + const currentDriverContext = driverContext(); + const factoryResult = resolvedFactoryResult ?? { + value: await this.options.driverFactory(currentDriverContext), + securityContextFingerprint: fingerprint(currentDriverContext.securityContext), + }; + + origin.securityContextFingerprint = factoryResult.securityContextFingerprint; + origin.configFingerprint = isDriver(factoryResult.value) + ? null + : fingerprint(factoryResult.value); + + driver = await this.createDriverFromFactoryResult( + factoryResult.value, + currentDriverContext, orchestratorOptions, ); @@ -644,9 +758,11 @@ export class CubejsServerCore { ); } catch (e) { driverPromise[factoryKey] = null; + delete driverOrigin[factoryKey]; if (!preAggregations && !hasSeparatePreAggEnv) { driverPromise[`${dataSource}@pre_agg`] = null; + delete driverOrigin[`${dataSource}@pre_agg`]; } if (driver) { @@ -713,6 +829,7 @@ export class CubejsServerCore { } ); + this.orchestratorRequestContexts.set(orchestratorApi, requestContextRef); this.orchestratorStorage.set(orchestratorId, orchestratorApi); return orchestratorApi; @@ -877,7 +994,24 @@ export class CubejsServerCore { context: DriverContext, options?: OrchestratorInitedOptions, ): Promise { - const val = await this.options.driverFactory(context); + return this.createDriverFromFactoryResult( + await this.options.driverFactory(context), + context, + options, + ); + } + + /** + * Build a driver from whatever `driverFactory` returned. Split out of + * `resolveDriver` so a caller that has already invoked the factory — to + * compare its result against the cached driver's — can build from that same + * result instead of invoking a user-supplied function a second time. + */ + protected async createDriverFromFactoryResult( + val: DriverConfig | BaseDriver, + context: DriverContext, + options?: OrchestratorInitedOptions, + ): Promise { if (isDriver(val)) { return val; } else { @@ -895,6 +1029,67 @@ export class CubejsServerCore { } } + /** + * Decide whether a cached driver still reflects what `driverFactory` would + * resolve for the current request context. + * + * The check is deliberately layered so that deployments which cannot be + * affected never leave the fast path, and no user-supplied function is called + * more often than it has to be: + * + * 1. No custom `driverFactory`, or one that hands back a constructed driver + * rather than a config — nothing context-derived to compare. Reuse. + * 2. The security context is byte-for-byte 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. The security context changed, so ask the factory. Most factories ignore + * it and return an identical config — reuse, and remember the new context + * so step 2 short-circuits next time. + * 4. The config genuinely changed. Rebuild. + * + * Step 4 is what fixes a rotated per-user credential: previously the driver + * built from the first request's token was reused for the life of the + * process, so every new connection it opened failed to authenticate. + * + * Note this follows the documented contract of `contextToOrchestratorId` — + * that it is the cache key for database connections. Two contexts that + * resolve to different connections but share an orchestrator id are a + * misconfiguration; they were already sharing one user's connection before + * this change. + */ + protected async resolveDriverStaleness( + origin: DriverOrigin | undefined, + context: DriverContext, + ): Promise<{ stale: false } | { stale: true, factoryResult: DriverFactoryResult }> { + if ( + !origin || + origin.configFingerprint === null || + !this.optsHandler.isCustomDriverFactory() + ) { + return { stale: false }; + } + + const securityContextFingerprint = fingerprint(context.securityContext); + + if ( + securityContextFingerprint === null || + securityContextFingerprint === origin.securityContextFingerprint + ) { + return { stale: false }; + } + + const value = await this.options.driverFactory(context); + const configFingerprint = isDriver(value) ? null : fingerprint(value); + + if (configFingerprint === null || configFingerprint === origin.configFingerprint) { + origin.securityContextFingerprint = securityContextFingerprint; + + return { stale: false }; + } + + return { stale: true, factoryResult: { value, securityContextFingerprint } }; + } + public async testConnections() { return this.orchestratorStorage.testConnections(); } diff --git a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts new file mode 100644 index 0000000000000..6ebd5132830cf --- /dev/null +++ b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts @@ -0,0 +1,212 @@ +/* eslint-disable @typescript-eslint/no-empty-function */ +import { BaseDriver } from '@cubejs-backend/query-orchestrator'; +import type { DriverFactoryByDataSource } from '@cubejs-backend/query-orchestrator'; + +import { CreateOptions, CubejsServerCore } from '../../src'; + +type FakeDriver = BaseDriver & { + builtFrom: any; + release: jest.Mock; + testConnection: jest.Mock; +}; + +/** + * Stands in for real driver construction so these tests exercise the caching + * decisions without needing a database. Everything else — the driver factory + * closure, the fingerprinting, the rebuild — is the production code path. + */ +class TestServerCore extends CubejsServerCore { + public builtDrivers: FakeDriver[] = []; + + protected async createDriverFromFactoryResult( + val: any, + context: any, + options?: any, + ): Promise { + // A factory that hands back a constructed driver takes the real path — that + // branch is exactly what one of these tests is about. + if (val instanceof BaseDriver) { + return super.createDriverFromFactoryResult(val, context, options); + } + + const driver = { + builtFrom: val, + release: jest.fn(async () => {}), + testConnection: jest.fn(async () => {}), + setLogger: () => {}, + } as unknown as FakeDriver; + + this.builtDrivers.push(driver); + + return driver; + } +} + +/** + * Boot a core, resolve its orchestrator once, and hand back the driver factory + * the orchestrator was created with — the same closure the query orchestrator + * calls for every query. + */ +async function createCore(options: CreateOptions, securityContext: unknown) { + const core = new TestServerCore({ + contextToOrchestratorId: () => 'ORCHESTRATOR', + ...options, + }); + const spy = jest.spyOn(core, 'createOrchestratorApi'); + + await core.getOrchestratorApi({ requestId: 'req-1', securityContext }); + + const driverFactory = spy.mock.calls[0][0]; + + return { + core, + driverFactory, + /** Serve another request through the cached orchestrator. */ + request: (nextSecurityContext: unknown, requestId = 'req-n') => core.getOrchestratorApi({ requestId, securityContext: nextSecurityContext }), + }; +} + +describe('driver cache invalidation', () => { + beforeAll(() => { + process.env.CUBEJS_API_SECRET = 'api-secret'; + }); + + // The CUB-3599 regression: the orchestrator closed over the context of the + // request that created it, so a driver built from a per-user credential was + // reused for the life of the process. Every connection it opened after the + // token rotated failed to authenticate. + test('rebuilds the driver when a context-derived credential changes', async () => { + const { core, driverFactory, request } = await createCore({ + driverFactory: (ctx: any) => ({ type: 'postgres', password: ctx.securityContext.token }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + expect(first.builtFrom).toMatchObject({ password: 'token-a' }); + + await request({ token: 'token-b' }); + const second = await driverFactory('default'); + + expect(second).not.toBe(first); + expect(second.builtFrom).toMatchObject({ password: 'token-b' }); + expect(core.builtDrivers).toHaveLength(2); + }); + + test('releases the driver it replaced, so its pool is drained', async () => { + const { driverFactory, request } = await createCore({ + driverFactory: (ctx: any) => ({ type: 'postgres', password: ctx.securityContext.token }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + await request({ token: 'token-b' }); + await driverFactory('default'); + + // Released off the request path, so give the detached promise a tick. + await new Promise((resolve) => setImmediate(resolve)); + + expect(first.release).toHaveBeenCalledTimes(1); + }); + + test('reuses the driver when the security context is unchanged', async () => { + const factory = jest.fn((ctx: any) => ({ type: 'postgres', password: ctx.securityContext.token })); + const { driverFactory, request } = await createCore({ driverFactory: factory }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + // A different request, same user, same credential. + await request({ token: 'token-a' }, 'req-2'); + + expect(await driverFactory('default')).toBe(first); + // Not re-invoked: an unchanged security context short-circuits before the + // user's factory is called at all. + expect(factory).toHaveBeenCalledTimes(1); + }); + + test('reuses the driver when the factory ignores the security context', async () => { + const factory = jest.fn(() => ({ type: 'postgres', password: 'from-env' })); + const { core, driverFactory, request } = await createCore({ driverFactory: factory }, { user: 'a' }); + + const first = await driverFactory('default'); + + await request({ user: 'b' }); + const second = await driverFactory('default'); + + expect(second).toBe(first); + expect(core.builtDrivers).toHaveLength(1); + // Asked once more because the context changed, but the answer matched, so + // nothing was rebuilt. + expect(factory).toHaveBeenCalledTimes(2); + // ...and that answer is remembered, so a third request with the same + // context does not ask again. + await request({ user: 'b' }, 'req-3'); + await driverFactory('default'); + expect(factory).toHaveBeenCalledTimes(2); + }); + + test('never rebuilds when the factory returns a constructed driver', async () => { + class ConstructedDriver extends BaseDriver { + public release = jest.fn(async () => {}); + + public testConnection = jest.fn(async () => {}); + + public async query(): Promise { + return []; + } + } + + const driver = new ConstructedDriver(); + const factory = jest.fn(() => driver); + const { driverFactory, request } = await createCore({ driverFactory: factory }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + await request({ token: 'token-b' }); + + // A constructed driver carries no configuration to compare, so the previous + // resolve-once behaviour is preserved rather than guessed at. + expect(await driverFactory('default')).toBe(first); + expect(driver.release).not.toHaveBeenCalled(); + expect(factory).toHaveBeenCalledTimes(1); + }); + + test('keeps the pre-aggregation alias pointing at the rebuilt driver', async () => { + const { driverFactory, request } = await createCore({ + driverFactory: (ctx: any) => ({ type: 'postgres', password: ctx.securityContext.token }), + }, { token: 'token-a' }); + + await driverFactory('default'); + await request({ token: 'token-b' }); + + const rebuilt = await driverFactory('default'); + const preAgg = await driverFactory('default', true); + + expect(preAgg).toBe(rebuilt); + expect(preAgg.builtFrom).toMatchObject({ password: 'token-b' }); + }); + + test('a failed rebuild does not leave a poisoned cache entry', async () => { + let token = 'token-a'; + let shouldFail = false; + const { driverFactory, request } = await createCore({ + driverFactory: () => { + if (shouldFail) { + throw new Error('factory blew up'); + } + + return { type: 'postgres', password: token }; + }, + }, { token: 'token-a' }); + + await driverFactory('default'); + + token = 'token-b'; + shouldFail = true; + await request({ token: 'token-b' }); + await expect(driverFactory('default')).rejects.toThrow('factory blew up'); + + // The next attempt resolves from scratch rather than serving the failure. + shouldFail = false; + const recovered = await driverFactory('default'); + expect(recovered.builtFrom).toMatchObject({ password: 'token-b' }); + }); +}); diff --git a/packages/cubejs-server-core/test/unit/driver-config-fingerprint.test.ts b/packages/cubejs-server-core/test/unit/driver-config-fingerprint.test.ts new file mode 100644 index 0000000000000..b0572385cfd2d --- /dev/null +++ b/packages/cubejs-server-core/test/unit/driver-config-fingerprint.test.ts @@ -0,0 +1,65 @@ +import { fingerprint } from '../../src/core/driver-config-fingerprint'; + +describe('fingerprint', () => { + test('is stable across property order', () => { + expect(fingerprint({ type: 'databricks-jdbc', token: 'a', url: 'u' })) + .toEqual(fingerprint({ url: 'u', token: 'a', type: 'databricks-jdbc' })); + }); + + test('changes when a nested value changes', () => { + expect(fingerprint({ type: 'postgres', options: { password: 'one' } })) + .not.toEqual(fingerprint({ type: 'postgres', options: { password: 'two' } })); + }); + + // The case this exists for: a rotated per-user OAuth token has to be visible + // as a different configuration, or the cached driver is never rebuilt. + test('changes when only the credential changes', () => { + const base = { type: 'databricks-jdbc', url: 'jdbc:databricks://host', acceptPolicy: true }; + + expect(fingerprint({ ...base, token: 'token-issued-at-09:00' })) + .not.toEqual(fingerprint({ ...base, token: 'token-issued-at-10:00' })); + }); + + test('treats an absent property and an undefined one as equal', () => { + expect(fingerprint({ type: 'postgres', catalog: undefined })) + .toEqual(fingerprint({ type: 'postgres' })); + }); + + test('distinguishes arrays by order', () => { + expect(fingerprint({ scopes: ['a', 'b'] })).not.toEqual(fingerprint({ scopes: ['b', 'a'] })); + }); + + test('handles dates, bigints and nested structures', () => { + const value = { + when: new Date('2026-07-31T12:00:00.000Z'), + big: BigInt(42), + nested: [{ a: 1 }, { b: [true, null] }], + }; + + expect(fingerprint(value)).toEqual(fingerprint({ + nested: [{ a: 1 }, { b: [true, null] }], + big: BigInt(42), + when: new Date('2026-07-31T12:00:00.000Z'), + })); + expect(fingerprint(value)).not.toEqual(fingerprint({ ...value, big: BigInt(43) })); + }); + + test('does not expose the value it hashes', () => { + const digest = fingerprint({ type: 'postgres', password: 'super-secret' }); + + expect(digest).not.toContain('super-secret'); + expect(digest).toMatch(/^[0-9a-f]{32}$/); + }); + + test('returns null for a circular structure rather than throwing', () => { + const circular: Record = { type: 'postgres' }; + circular.self = circular; + + expect(fingerprint(circular)).toBeNull(); + }); + + test('returns a digest for null and undefined', () => { + expect(fingerprint(null)).toEqual(fingerprint(undefined)); + expect(fingerprint(null)).not.toBeNull(); + }); +}); From 23dbff0220e2c6e323270e8b5c8773606da5bc44 Mon Sep 17 00:00:00 2001 From: MikeNitsenko Date: Wed, 5 Aug 2026 19:11:58 +0500 Subject: [PATCH 02/16] fix(server-core): invalidate both pre-aggregation driver keys together MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `default` and `default@pre_agg` keys share one driver when the data source has no separate pre-aggregation credentials, but invalidation cleared only the key it was called with. A pre-aggregation build that observed a credential rotation first therefore left the other key serving the drained driver, released that driver a second time, and built an extra pool for what should be a single shared instance. Key alias writes and invalidation on which keys resolve to the driver being replaced rather than on which key was requested. Also report a rebuild count, so a deployment whose contextToOrchestratorId does not partition by whatever driverFactory reads — every user sharing one orchestrator, say — is diagnosable from the rebuild rate alone. Correct the per-user OAuth recipe too: it keyed context_to_orchestrator_id on the token expiry, which makes the rebuild moot and stands up a fresh orchestrator — pool, queues and pre-aggregation cache — on every rotation, leaking the pool when the LRU evicts it. The orchestrator has to partition by user, not by credential version. Co-Authored-By: Claude Opus 5 (1M context) --- .../connect-to-data/oauth-authentication.mdx | 137 +++++++++--------- .../src/core/driver-config-fingerprint.ts | 3 + .../cubejs-server-core/src/core/server.ts | 76 +++++++--- .../unit/driver-cache-invalidation.test.ts | 27 ++++ 4 files changed, 151 insertions(+), 92 deletions(-) diff --git a/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx b/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx index 20e53301894e9..f50343b78c850 100644 --- a/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx +++ b/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx @@ -29,13 +29,17 @@ connection could leak to another. Cube caches one database connection per -[`context_to_orchestrator_id`][ref-context-to-orchestrator-id] for the -lifetime of the process. **Every input your `driver_factory` reads must -therefore also appear in the orchestrator ID.** OAuth access tokens rotate -(typically hourly), so an ID built from the username alone leaves the -cached connection pinned to the token it was first built with — new -database sessions then fail to authenticate until the next deploy. The -configuration below derives both from one helper so they cannot drift. +[`context_to_orchestrator_id`][ref-context-to-orchestrator-id]. **The +orchestrator ID must therefore distinguish every user your +`driver_factory` can return a different connection for** — otherwise two +users share one pool and one user's credential is reused for another's +queries. + +Do not add the token itself to the ID. When the token rotates, Cube +notices that `driver_factory` now resolves a different configuration and +rebuilds the connection in place, so a username is enough. Keying on the +token instead creates a new orchestrator — with its own pool, queues and +pre-aggregation cache — on every rotation. @@ -169,15 +173,19 @@ from datetime import datetime, timezone import os import time -# Don't hand the driver a token that is about to expire. Drivers cache their -# connection settings, and the connection pool opens new sessions long after -# the driver was built, so "valid right now" is not enough. +# A token is handed to the driver once, but the pool keeps opening new sessions +# with it afterwards. Reject one that is too close to expiry to survive that +# gap, rather than one that is merely still valid at this instant. EXPIRY_SKEW_SECONDS = 120 def _parse_expiry(value): + """Seconds since the epoch, or None if the value is absent or unparseable.""" if not value: return None + if isinstance(value, (int, float)): + # Epoch milliseconds if the value is far too large to be seconds. + return value / 1000 if value > 1e11 else float(value) try: parsed = datetime.fromisoformat(str(value).replace("Z", "+00:00")) except ValueError: @@ -187,12 +195,8 @@ def _parse_expiry(value): return parsed.timestamp() -def _credential(ctx: dict): - """Resolve the credential for this request, plus the key identifying it. - - driver_factory and context_to_orchestrator_id both call this, so the cached - connection can never drift from the token it was built with. - """ +def _access_token(ctx: dict): + """The user's OAuth token, or None to fall back to the service account.""" # For other data sources, swap "databricks" for "snowflake", etc. cube_cloud = (ctx.get("securityContext") or {}).get("cubeCloud") or {} creds = (cube_cloud.get("userCredentials") or {}).get("databricks") or {} @@ -205,21 +209,19 @@ def _credential(ctx: dict): # treating that as fatal drops the user onto the service account for no # reason. if access_token and expires_at and expires_at > time.time() + EXPIRY_SKEW_SECONDS: - # Key on the expiry, never on the token itself — the orchestrator ID is - # used as a cache prefix and appears in logs. - return access_token, f"u{int(expires_at)}" + return access_token - return os.environ["CUBEJS_DB_DATABRICKS_TOKEN"], "service-account" + return None @config("driver_factory") def driver_factory(ctx: dict) -> dict: - token, _ = _credential(ctx) - + # Cube rebuilds this connection whenever the returned configuration + # changes, so returning a rotated token here is enough to replace it. return { "type": "databricks-jdbc", "url": os.environ["CUBEJS_DB_DATABRICKS_URL"], - "token": token, + "token": _access_token(ctx) or os.environ["CUBEJS_DB_DATABRICKS_TOKEN"], "acceptPolicy": True, "catalog": os.environ.get("CUBEJS_DB_DATABRICKS_CATALOG"), } @@ -227,14 +229,13 @@ def driver_factory(ctx: dict) -> dict: @config("context_to_orchestrator_id") def context_to_orchestrator_id(ctx: dict) -> str: - # One orchestrator per (user, credential): separate DB connections, - # execution queues and pre-aggregation caches, and a cache key that changes - # when the token rotates. + # One orchestrator per user: separate DB connections, execution queues and + # pre-aggregation caches. Deliberately not keyed on the token — see the + # warning above. cube_cloud = (ctx.get("securityContext") or {}).get("cubeCloud") or {} username = cube_cloud.get("username") or "default" - _, cache_key = _credential(ctx) - return f"CUBE_APP_{username}_{cache_key}" + return f"CUBE_APP_{username}" ``` @@ -242,56 +243,46 @@ def context_to_orchestrator_id(ctx: dict) -> str: ```javascript cube.js -// Don't hand the driver a token that is about to expire. Drivers cache their -// connection settings, and the connection pool opens new sessions long after -// the driver was built, so "valid right now" is not enough. +// A token is handed to the driver once, but the pool keeps opening new sessions +// with it afterwards. Reject one that is too close to expiry to survive that +// gap, rather than one that is merely still valid at this instant. const EXPIRY_SKEW_MS = 120 * 1000; -/** - * Resolve the credential for this request, plus the key identifying it. - * - * driverFactory and contextToOrchestratorId both call this, so the cached - * connection can never drift from the token it was built with. - */ -function resolveCredential(securityContext) { +/** The user's OAuth token, or undefined to fall back to the service account. */ +function accessToken(securityContext) { // For other data sources, swap `databricks` for `snowflake`, etc. const creds = securityContext?.cubeCloud?.userCredentials?.databricks ?? {}; - const expiresAt = Date.parse(creds.accessTokenExpiresAt ?? ""); + const raw = creds.accessTokenExpiresAt; + const expiresAt = typeof raw === "number" ? raw : Date.parse(raw ?? ""); // Gate on the expiry rather than on `status`: a failed background refresh can // flag the record while the token already in hand is still valid, and // treating that as fatal drops the user onto the service account for no - // reason. + // reason. NaN fails this comparison, so an unparseable expiry falls back too. if (creds.accessToken && expiresAt > Date.now() + EXPIRY_SKEW_MS) { - // Key on the expiry, never on the token itself — the orchestrator ID is - // used as a cache prefix and appears in logs. - return { token: creds.accessToken, cacheKey: `u${expiresAt}` }; + return creds.accessToken; } - return { - token: process.env.CUBEJS_DB_DATABRICKS_TOKEN, - cacheKey: "service-account", - }; + return undefined; } module.exports = { + // Cube rebuilds this connection whenever the returned configuration changes, + // so returning a rotated token here is enough to replace it. driverFactory: ({ securityContext }) => ({ type: "databricks-jdbc", url: process.env.CUBEJS_DB_DATABRICKS_URL, - token: resolveCredential(securityContext).token, + token: + accessToken(securityContext) ?? process.env.CUBEJS_DB_DATABRICKS_TOKEN, acceptPolicy: true, catalog: process.env.CUBEJS_DB_DATABRICKS_CATALOG, }), - // One orchestrator per (user, credential): separate DB connections, execution - // queues and pre-aggregation caches, and a cache key that changes when the - // token rotates. - contextToOrchestratorId: ({ securityContext }) => { - const username = securityContext?.cubeCloud?.username ?? "default"; - const { cacheKey } = resolveCredential(securityContext); - - return `CUBE_APP_${username}_${cacheKey}`; - }, + // One orchestrator per user: separate DB connections, execution queues and + // pre-aggregation caches. Deliberately not keyed on the token — see the + // warning above. + contextToOrchestratorId: ({ securityContext }) => + `CUBE_APP_${securityContext?.cubeCloud?.username ?? "default"}`, }; ``` @@ -309,22 +300,25 @@ module.exports = { that has not expired, it is used. Otherwise, Cube falls back to the service account credential stored in environment variables. -3. **Per-user, per-credential orchestrator** — +3. **Per-user orchestrator** — [`context_to_orchestrator_id`][ref-context-to-orchestrator-id] returns a - key derived from both the username and the credential in use, so each - user gets their own database connection pool, execution queues, and - pre-aggregation table cache — and a rotated token produces a fresh - connection instead of reusing one built from the previous token. Keying - on the username alone would share a single cached connection across - token rotations; omitting the username would share one across users. + key derived from the username, so each user gets their own database + connection pool, execution queues, and pre-aggregation table cache. + Without it, every user shares one cached connection and the first + user's credential is reused for everyone else's queries. + +4. **Rotation replaces the connection** — on the next request after a + rotation, Cube compares what `driver_factory` now resolves against what + the cached connection was built from. When they differ it builds a + replacement and drains the old pool, so in-flight queries finish on the + connection they started on. ## Operational notes -- **Expect one orchestrator per token rotation.** Each distinct - orchestrator ID holds its own connection pool, queues and - pre-aggregation table cache, and tokens typically rotate hourly. Watch - memory on deployments with many concurrent users, and note that the - first query after a rotation runs against a cold pre-aggregation cache. +- **One orchestrator per user, not per token.** The orchestrator survives + rotations, so pre-aggregation caches and queues stay warm and the + orchestrator count tracks your concurrent user count rather than growing + with every rotation. - **Don't make [`context_to_app_id`][ref-context-to-appid] per-user.** The data model is identical for every user — only the connection differs — so a per-user app ID forces a full data-model recompile per user on @@ -334,6 +328,11 @@ module.exports = { check.** If it has no access at all, liveness checks and any query that falls back to it fail with an opaque authorization error from the driver rather than something diagnosable. +- **Falling back is silent.** A missing or near-expired token sends the + query to the service account instead of failing, so results reflect the + service account's permissions rather than the user's. If that is not + acceptable for your deployment, raise an error in `driver_factory` + instead of returning the fallback credential. [ref-config]: /reference/configuration/config [ref-driver-factory]: /reference/configuration/config#driver_factory diff --git a/packages/cubejs-server-core/src/core/driver-config-fingerprint.ts b/packages/cubejs-server-core/src/core/driver-config-fingerprint.ts index bed7ee35bbf79..ebe3e426f1f68 100644 --- a/packages/cubejs-server-core/src/core/driver-config-fingerprint.ts +++ b/packages/cubejs-server-core/src/core/driver-config-fingerprint.ts @@ -85,6 +85,9 @@ export function fingerprint(value: unknown): string | null { return crypto .createHash('sha256') .update(stableStringify(value, new Set())) + // 32 hex chars = 128 bits, which is far more than an equality check over + // the handful of configurations one process resolves needs, and keeps the + // digest short enough to sit in a log line. .digest('hex') .slice(0, 32); } catch (e) { diff --git a/packages/cubejs-server-core/src/core/server.ts b/packages/cubejs-server-core/src/core/server.ts index 4a3dbf228df9a..8649b91ae233c 100644 --- a/packages/cubejs-server-core/src/core/server.ts +++ b/packages/cubejs-server-core/src/core/server.ts @@ -631,6 +631,15 @@ export class CubejsServerCore { */ const driverOrigin: Record = {}; + /** + * How many times each key has been rebuilt. Reported with the rebuild so a + * deployment whose `contextToOrchestratorId` does not partition by whatever + * `driverFactory` reads — every user sharing one orchestrator, say — is + * diagnosable: it rebuilds on request after request rather than once per + * credential rotation. + */ + const driverRebuilds: Record = {}; + let externalPreAggregationsDriverPromise: Promise | null = null; const contextToDbType: DbTypeInternalFn = this.contextToDbType.bind(this); @@ -660,6 +669,23 @@ export class CubejsServerCore { preAggregations: usePreAgg || false, }); + /** + * Every key that resolves to the one driver built here. Without separate + * pre-aggregation credentials `usePreAgg` is false whichever key was + * asked for, so both describe an identically configured driver and share + * a single instance — they must therefore be written, and invalidated, + * together. Doing it per requested key instead lets the two diverge into + * two pools where the deployment expects one. + */ + const aliasedKeys = hasSeparatePreAggEnv + ? [factoryKey] + : [dataSource, `${dataSource}@pre_agg`]; + + const invalidate = () => aliasedKeys.forEach((key) => { + driverPromise[key] = null; + delete driverOrigin[key]; + }); + // Already resolved by the staleness check below, so the factory is not // asked twice for the same rebuild. let resolvedFactoryResult: DriverFactoryResult | undefined; @@ -674,17 +700,26 @@ export class CubejsServerCore { return driverPromise[factoryKey]; } + driverRebuilds[factoryKey] = (driverRebuilds[factoryKey] || 0) + 1; + this.logger('Rebuilding driver on configuration change', { dataSource, preAggregations, + rebuildCount: driverRebuilds[factoryKey], }); const replaced = driverPromise[factoryKey]; - driverPromise[factoryKey] = null; - if (!preAggregations && !hasSeparatePreAggEnv) { - driverPromise[`${dataSource}@pre_agg`] = null; - } + // Clear every key pointing at the replaced driver, not just the one + // asked for: a surviving alias would keep handing out a driver whose + // pool is being drained, and would release it a second time when it + // was itself found stale. + Object.keys(driverPromise) + .filter((key) => driverPromise[key] === replaced) + .forEach((key) => { + driverPromise[key] = null; + delete driverOrigin[key]; + }); // Graceful: `release` drains the pool, so queries already running on // the replaced driver finish before its connections are closed. It is @@ -708,19 +743,18 @@ export class CubejsServerCore { }); } - // Shared by reference with the `@pre_agg` alias below, so both keys - // describe the one driver they both resolve to. Starts empty: until the - // factory has been called there is nothing to compare against, and + // Shared by reference across `aliasedKeys`, so every key describes the + // one driver they all resolve to. Starts empty: until the factory has + // been called there is nothing to compare against, and // `resolveDriverStaleness` reads that as "reuse". const origin: DriverOrigin = { securityContextFingerprint: null, configFingerprint: null, }; - driverOrigin[factoryKey] = origin; - if (!preAggregations && !hasSeparatePreAggEnv) { - driverOrigin[`${dataSource}@pre_agg`] = origin; - } + aliasedKeys.forEach((key) => { + driverOrigin[key] = origin; + }); driverPromise[factoryKey] = (async () => { let driver: BaseDriver | null = null; @@ -757,13 +791,7 @@ export class CubejsServerCore { `Unexpected return type, driverFactory must return driver (dataSource: "${dataSource}"), actual: ${getRealType(driver)}` ); } catch (e) { - driverPromise[factoryKey] = null; - delete driverOrigin[factoryKey]; - - if (!preAggregations && !hasSeparatePreAggEnv) { - driverPromise[`${dataSource}@pre_agg`] = null; - delete driverOrigin[`${dataSource}@pre_agg`]; - } + invalidate(); if (driver) { await driver.release(); @@ -773,12 +801,14 @@ export class CubejsServerCore { } })(); - // No separate pre-agg driver needed — share the same promise for both keys - if (!preAggregations && !hasSeparatePreAggEnv) { - driverPromise[`${dataSource}@pre_agg`] = driverPromise[factoryKey]; - } + const pending = driverPromise[factoryKey]; + + // No separate pre-agg driver needed — share the same promise across keys + aliasedKeys.forEach((key) => { + driverPromise[key] = pending; + }); - return driverPromise[factoryKey]; + return pending; }, { externalDriverFactory: this.options.externalDriverFactory && (async () => { diff --git a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts index 6ebd5132830cf..a4458a3e76346 100644 --- a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts +++ b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts @@ -184,6 +184,33 @@ describe('driver cache invalidation', () => { expect(preAgg.builtFrom).toMatchObject({ password: 'token-b' }); }); + // The `default` and `default@pre_agg` keys share one driver when the data + // source has no separate pre-aggregation credentials. A pre-aggregation build + // can be the first caller to observe a rotation, so invalidation has to clear + // both keys whichever one asked: clearing only the requested key left the + // other serving the drained driver, released it twice, and then built a + // second pool for what should be a single shared driver. + test('rebuilds once when a pre-aggregation build observes the rotation first', async () => { + const { core, driverFactory, request } = await createCore({ + driverFactory: (ctx: any) => ({ type: 'postgres', password: ctx.securityContext.token }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + await request({ token: 'token-b' }); + + const preAgg = await driverFactory('default', true); + const regular = await driverFactory('default'); + + await new Promise((resolve) => setImmediate(resolve)); + + expect(preAgg).toBe(regular); + expect(regular.builtFrom).toMatchObject({ password: 'token-b' }); + expect(core.builtDrivers).toHaveLength(2); + // Exactly once — a second release would run against an already-drained pool. + expect(first.release).toHaveBeenCalledTimes(1); + }); + test('a failed rebuild does not leave a poisoned cache entry', async () => { let token = 'token-a'; let shouldFail = false; From f92891d546268861be3fe89602f87ae9523469ad Mon Sep 17 00:00:00 2001 From: MikeNitsenko Date: Wed, 5 Aug 2026 20:22:27 +0500 Subject: [PATCH 03/16] fix(server-core): make the driver rebuild path concurrency-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review findings on #11453. The staleness check awaits the user's `driverFactory`, so two callers could both see the cached driver as stale. The second then read `driverPromise` *after* that await — picking up the driver the first had just built and returned — released it, and stood up a third pool. Its caller was left holding a driver whose pool was draining. Capture the entry before the await and re-enter when it has changed, so one rotation is one rebuild and one release however many queries are in flight. A probe failure no longer fails the request. That call is speculative on a path that used to be a pure cache hit, so a factory reading a secret store could turn a transient outage into failed queries. It now degrades to reuse, like anything else that cannot be compared. A driver the probe constructed and discards is released rather than leaked. Rebuilds are counted per alias set rather than per key, so a rotation seen through `default@pre_agg` and then `default` reads as one rebuild, and the log escalates once past a threshold to name `contextToOrchestratorId` as the likely cause. Deliberately warn rather than stop: an hourly rotation legitimately rebuilds ~24 times a day, and capping would silently restore the stale-credential bug this fixes. Also note the two config shapes fingerprinting makes a no-op — a credential behind a provider callback, and values behind prototype accessors — and say so on the recipe, which otherwise implies any rotated token is picked up. Mirror the Python expiry parser in the JavaScript sample: it read a numeric `accessTokenExpiresAt` as milliseconds, so epoch seconds would land in 1970 and drop every user onto the service account. Co-Authored-By: Claude Opus 5 (1M context) --- .../connect-to-data/oauth-authentication.mdx | 16 +- .../src/core/driver-config-fingerprint.ts | 9 + .../cubejs-server-core/src/core/server.ts | 328 +++++++++++------- .../unit/driver-cache-invalidation.test.ts | 118 ++++++- 4 files changed, 330 insertions(+), 141 deletions(-) diff --git a/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx b/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx index f50343b78c850..70e4313f5a65b 100644 --- a/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx +++ b/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx @@ -43,6 +43,15 @@ pre-aggregation cache — on every rotation. + + +Return the resolved credential from `driver_factory`, not a function that +fetches one. Cube compares the configuration values the factory returns, +and a function compares as unchanged however the credential behind it +rotates — the connection would never be rebuilt. + + + ## Prerequisites - A [Cube Cloud][ref-cube-cloud] deployment connected to an @@ -253,7 +262,12 @@ function accessToken(securityContext) { // For other data sources, swap `databricks` for `snowflake`, etc. const creds = securityContext?.cubeCloud?.userCredentials?.databricks ?? {}; const raw = creds.accessTokenExpiresAt; - const expiresAt = typeof raw === "number" ? raw : Date.parse(raw ?? ""); + // Epoch milliseconds if the value is far too large to be seconds. Reading + // seconds as milliseconds would land in 1970 and reject every token. + const expiresAt = + typeof raw === "number" + ? (raw > 1e11 ? raw : raw * 1000) + : Date.parse(raw ?? ""); // Gate on the expiry rather than on `status`: a failed background refresh can // flag the record while the token already in hand is still valid, and diff --git a/packages/cubejs-server-core/src/core/driver-config-fingerprint.ts b/packages/cubejs-server-core/src/core/driver-config-fingerprint.ts index ebe3e426f1f68..f1c4e963a0149 100644 --- a/packages/cubejs-server-core/src/core/driver-config-fingerprint.ts +++ b/packages/cubejs-server-core/src/core/driver-config-fingerprint.ts @@ -32,6 +32,12 @@ function stableStringify(value: unknown, seen: Set): string { // contributes a constant. Two configs differing only in a function body are // therefore treated as equal — deliberately conservative: it can only lead to // reusing a connection, never to swapping one out unnecessarily. + // + // The practical consequence is that a config carrying its credential as a + // provider callback rather than a resolved value fingerprints identically + // however the credential rotates, so such a driver is never rebuilt. A + // `driverFactory` that needs rotation to be noticed has to return the + // resolved value. if (type === 'function' || type === 'symbol') { return JSON.stringify(`[${type}]`); } @@ -51,6 +57,9 @@ function stableStringify(value: unknown, seen: Set): string { return `[${value.map((item) => stableStringify(item, seen)).join(',')}]`; } + // Own enumerable keys only, so a class instance holding its values behind + // prototype accessors fingerprints as `{}` — constant, and therefore another + // shape whose rotation goes unnoticed. Plain configs are unaffected. const entries = Object.keys(value as Record) .sort() .reduce((acc, key) => { diff --git a/packages/cubejs-server-core/src/core/server.ts b/packages/cubejs-server-core/src/core/server.ts index 8649b91ae233c..74e62ec3338d2 100644 --- a/packages/cubejs-server-core/src/core/server.ts +++ b/packages/cubejs-server-core/src/core/server.ts @@ -75,6 +75,13 @@ import { const { version } = require('../../../package.json'); +/** + * Rebuilds of one data source's driver before the log escalates to naming a + * likely misconfiguration. A rotating credential rebuilds a few times a day, so + * reaching this within a process means contexts are displacing each other. + */ +const DRIVER_REBUILD_WARN_THRESHOLD = 50; + /** * What a cached driver was built from. `null` on either field means "cannot * tell whether it changed", which is always read as "assume it did not". @@ -653,163 +660,196 @@ export class CubejsServerCore { (await this.orchestratorOptions(context)) || {}, ); - const orchestratorApi = this.createOrchestratorApi( + /** + * Driver factory function `DriverFactoryByDataSource`. Named so the rebuild + * path can re-enter it when another caller wins the race to replace a key. + */ + const resolveDataSourceDriver = async (dataSource = 'default', preAggregations = false): Promise => { + const factoryKey = preAggregations ? `${dataSource}@pre_agg` : dataSource; + + const hasSeparatePreAggEnv = hasPreAggregationsEnvVars(dataSource); + const usePreAgg = preAggregations && hasSeparatePreAggEnv && !this.optsHandler.isCustomDriverFactory(); + + const driverContext = (): DriverContext => ({ + ...requestContextRef.current, + dataSource, + preAggregations: usePreAgg || false, + }); + /** - * Driver factory function `DriverFactoryByDataSource`. + * Every key that resolves to the one driver built here. Without separate + * pre-aggregation credentials `usePreAgg` is false whichever key was + * asked for, so both describe an identically configured driver and share + * a single instance — they must therefore be written, and invalidated, + * together. Doing it per requested key instead lets the two diverge into + * two pools where the deployment expects one. */ - async (dataSource = 'default', preAggregations = false) => { - const factoryKey = preAggregations ? `${dataSource}@pre_agg` : dataSource; + const aliasedKeys = hasSeparatePreAggEnv + ? [factoryKey] + : [dataSource, `${dataSource}@pre_agg`]; - const hasSeparatePreAggEnv = hasPreAggregationsEnvVars(dataSource); - const usePreAgg = preAggregations && hasSeparatePreAggEnv && !this.optsHandler.isCustomDriverFactory(); + const invalidate = () => aliasedKeys.forEach((key) => { + driverPromise[key] = null; + delete driverOrigin[key]; + }); - const driverContext = (): DriverContext => ({ - ...requestContextRef.current, - dataSource, - preAggregations: usePreAgg || false, - }); + // Already resolved by the staleness check below, so the factory is not + // asked twice for the same rebuild. + let resolvedFactoryResult: DriverFactoryResult | undefined; - /** - * Every key that resolves to the one driver built here. Without separate - * pre-aggregation credentials `usePreAgg` is false whichever key was - * asked for, so both describe an identically configured driver and share - * a single instance — they must therefore be written, and invalidated, - * together. Doing it per requested key instead lets the two diverge into - * two pools where the deployment expects one. - */ - const aliasedKeys = hasSeparatePreAggEnv - ? [factoryKey] - : [dataSource, `${dataSource}@pre_agg`]; - - const invalidate = () => aliasedKeys.forEach((key) => { - driverPromise[key] = null; - delete driverOrigin[key]; - }); + const cached = driverPromise[factoryKey]; - // Already resolved by the staleness check below, so the factory is not - // asked twice for the same rebuild. - let resolvedFactoryResult: DriverFactoryResult | undefined; + if (cached) { + const staleness = await this.resolveDriverStaleness( + driverOrigin[factoryKey], + driverContext(), + ); - if (driverPromise[factoryKey]) { - const staleness = await this.resolveDriverStaleness( - driverOrigin[factoryKey], - driverContext(), - ); + // `resolveDriverStaleness` awaits the user's factory, so another caller + // may have replaced or invalidated this key in the meantime. Its work + // supersedes ours: start over rather than release a driver it has + // already handed out, or build a second pool alongside it. + if (driverPromise[factoryKey] !== cached) { + return resolveDataSourceDriver(dataSource, preAggregations); + } - if (!staleness.stale) { - return driverPromise[factoryKey]; - } + if (!staleness.stale) { + return cached; + } - driverRebuilds[factoryKey] = (driverRebuilds[factoryKey] || 0) + 1; + // Counted per alias set, not per key: a rotation seen first through + // `default@pre_agg` and then through `default` is one rebuild of one + // shared driver, and must not read as two counters at 1. + const rebuildKey = aliasedKeys[0]; + driverRebuilds[rebuildKey] = (driverRebuilds[rebuildKey] || 0) + 1; + const rebuildCount = driverRebuilds[rebuildKey]; + + this.logger('Rebuilding driver on configuration change', { + dataSource, + preAggregations, + rebuildCount, + }); - this.logger('Rebuilding driver on configuration change', { + // A credential rotation rebuilds a handful of times a day. Rebuilding + // this often means the orchestrator id does not partition by whatever + // the factory reads, so contexts that need different connections keep + // displacing each other's driver. + if (rebuildCount === DRIVER_REBUILD_WARN_THRESHOLD) { + this.logger('Driver rebuilt repeatedly', { dataSource, - preAggregations, - rebuildCount: driverRebuilds[factoryKey], + rebuildCount, + warning: 'Driver configuration keeps changing for one orchestrator. ' + + 'contextToOrchestratorId likely does not distinguish the contexts ' + + 'driverFactory returns different connections for.', }); - - const replaced = driverPromise[factoryKey]; - - // Clear every key pointing at the replaced driver, not just the one - // asked for: a surviving alias would keep handing out a driver whose - // pool is being drained, and would release it a second time when it - // was itself found stale. - Object.keys(driverPromise) - .filter((key) => driverPromise[key] === replaced) - .forEach((key) => { - driverPromise[key] = null; - delete driverOrigin[key]; - }); - - // Graceful: `release` drains the pool, so queries already running on - // the replaced driver finish before its connections are closed. It is - // deliberately not awaited — this request should not wait on the - // previous driver's in-flight work — and its failure must not fail - // this request. - replaced - .then((driver) => driver.release()) - .catch((error) => this.logger('Driver release error', { - dataSource, - error: (error as Error).stack || (error as Error).toString(), - })); - - resolvedFactoryResult = staleness.factoryResult; } - if (preAggregations && hasSeparatePreAggEnv && this.optsHandler.isCustomDriverFactory()) { - this.logger('Pre-aggregation driver conflict', { - error: 'Both driverFactory and PRE_AGGREGATIONS env vars are defined. driverFactory will take precedence.', - dataSource, + // Clear every key pointing at the replaced driver, not just the one + // asked for: a surviving alias would keep handing out a driver whose + // pool is being drained, and would release it a second time when it + // was itself found stale. + Object.keys(driverPromise) + .filter((key) => driverPromise[key] === cached) + .forEach((key) => { + driverPromise[key] = null; + delete driverOrigin[key]; }); - } - // Shared by reference across `aliasedKeys`, so every key describes the - // one driver they all resolve to. Starts empty: until the factory has - // been called there is nothing to compare against, and - // `resolveDriverStaleness` reads that as "reuse". - const origin: DriverOrigin = { - securityContextFingerprint: null, - configFingerprint: null, - }; + // Graceful: `release` drains the pool, so queries already running on + // the replaced driver finish before its connections are closed. It is + // deliberately not awaited — this request should not wait on the + // previous driver's in-flight work — and its failure must not fail + // this request. + cached + .then((driver) => driver.release()) + .catch((error) => this.logger('Driver release error', { + dataSource, + error: (error as Error).stack || (error as Error).toString(), + })); + + resolvedFactoryResult = staleness.factoryResult; + } - aliasedKeys.forEach((key) => { - driverOrigin[key] = origin; + if (preAggregations && hasSeparatePreAggEnv && this.optsHandler.isCustomDriverFactory()) { + this.logger('Pre-aggregation driver conflict', { + error: 'Both driverFactory and PRE_AGGREGATIONS env vars are defined. driverFactory will take precedence.', + dataSource, }); + } - driverPromise[factoryKey] = (async () => { - let driver: BaseDriver | null = null; - - try { - const currentDriverContext = driverContext(); - const factoryResult = resolvedFactoryResult ?? { - value: await this.options.driverFactory(currentDriverContext), - securityContextFingerprint: fingerprint(currentDriverContext.securityContext), - }; - - origin.securityContextFingerprint = factoryResult.securityContextFingerprint; - origin.configFingerprint = isDriver(factoryResult.value) - ? null - : fingerprint(factoryResult.value); - - driver = await this.createDriverFromFactoryResult( - factoryResult.value, - currentDriverContext, - orchestratorOptions, - ); - - if (typeof driver === 'object' && driver != null) { - if (driver.setLogger) { - driver.setLogger(this.logger); - } + // Shared by reference across `aliasedKeys`, so every key describes the + // one driver they all resolve to. Starts empty: until the factory has + // been called there is nothing to compare against, and + // `resolveDriverStaleness` reads that as "reuse". + const origin: DriverOrigin = { + securityContextFingerprint: null, + configFingerprint: null, + }; - await driver.testConnection(); + aliasedKeys.forEach((key) => { + driverOrigin[key] = origin; + }); - return driver; - } + const pending = (async () => { + let driver: BaseDriver | null = null; - throw new Error( - `Unexpected return type, driverFactory must return driver (dataSource: "${dataSource}"), actual: ${getRealType(driver)}` - ); - } catch (e) { - invalidate(); + try { + const currentDriverContext = driverContext(); + const factoryResult = resolvedFactoryResult ?? { + value: await this.options.driverFactory(currentDriverContext), + securityContextFingerprint: fingerprint(currentDriverContext.securityContext), + }; + + origin.securityContextFingerprint = factoryResult.securityContextFingerprint; + origin.configFingerprint = isDriver(factoryResult.value) + ? null + : fingerprint(factoryResult.value); + + driver = await this.createDriverFromFactoryResult( + factoryResult.value, + currentDriverContext, + orchestratorOptions, + ); - if (driver) { - await driver.release(); + if (typeof driver === 'object' && driver != null) { + if (driver.setLogger) { + driver.setLogger(this.logger); } - throw e; + await driver.testConnection(); + + return driver; + } + + throw new Error( + `Unexpected return type, driverFactory must return driver (dataSource: "${dataSource}"), actual: ${getRealType(driver)}` + ); + } catch (e) { + // Only if this build still owns the keys. A concurrent rebuild + // installs its own `origin`, and its driver must not be evicted + // because ours failed. + if (driverOrigin[factoryKey] === origin) { + invalidate(); } - })(); - const pending = driverPromise[factoryKey]; + if (driver) { + await driver.release(); + } - // No separate pre-agg driver needed — share the same promise across keys - aliasedKeys.forEach((key) => { - driverPromise[key] = pending; - }); + throw e; + } + })(); + + // No separate pre-agg driver needed — share the same promise across keys + aliasedKeys.forEach((key) => { + driverPromise[key] = pending; + }); + + return pending; + }; - return pending; - }, + const orchestratorApi = this.createOrchestratorApi( + resolveDataSourceDriver, { externalDriverFactory: this.options.externalDriverFactory && (async () => { if (externalPreAggregationsDriverPromise) { @@ -1108,10 +1148,40 @@ export class CubejsServerCore { return { stale: false }; } - const value = await this.options.driverFactory(context); + let value: DriverConfig | BaseDriver; + + try { + value = await this.options.driverFactory(context); + } catch (error) { + // This call is a probe, not the request's own resolution: a cache hit + // never used to invoke the factory at all, so letting a transient failure + // here propagate would fail a query the cached driver could have served. + // Degrade to reuse, as with anything else that cannot be compared. + this.logger('Driver staleness check error', { + dataSource: context.dataSource, + error: (error as Error).stack || (error as Error).toString(), + }); + + return { stale: false }; + } + const configFingerprint = isDriver(value) ? null : fingerprint(value); if (configFingerprint === null || configFingerprint === origin.configFingerprint) { + // A driver the factory constructed for this probe is about to be dropped, + // so hand back whatever it opened rather than leaking it. Only reachable + // for a factory that returns a config sometimes and a driver other times. + if (isDriver(value)) { + try { + await (value).release(); + } catch (error) { + this.logger('Driver release error', { + dataSource: context.dataSource, + error: (error as Error).stack || (error as Error).toString(), + }); + } + } + origin.securityContextFingerprint = securityContextFingerprint; return { stale: false }; diff --git a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts index a4458a3e76346..e4cf0ffe6e403 100644 --- a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts +++ b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts @@ -18,6 +18,9 @@ type FakeDriver = BaseDriver & { class TestServerCore extends CubejsServerCore { public builtDrivers: FakeDriver[] = []; + /** Set to fail the next driver construction, as a bad credential would. */ + public failNextBuild = false; + protected async createDriverFromFactoryResult( val: any, context: any, @@ -29,6 +32,12 @@ class TestServerCore extends CubejsServerCore { return super.createDriverFromFactoryResult(val, context, options); } + if (this.failNextBuild) { + this.failNextBuild = false; + + throw new Error('driver construction failed'); + } + const driver = { builtFrom: val, release: jest.fn(async () => {}), @@ -212,28 +221,115 @@ describe('driver cache invalidation', () => { }); test('a failed rebuild does not leave a poisoned cache entry', async () => { - let token = 'token-a'; + const { core, driverFactory, request } = await createCore({ + driverFactory: (ctx: any) => ({ type: 'postgres', password: ctx.securityContext.token }), + }, { token: 'token-a' }); + + await driverFactory('default'); + + // The rotation is detected, but building the replacement fails. + core.failNextBuild = true; + await request({ token: 'token-b' }); + await expect(driverFactory('default')).rejects.toThrow('driver construction failed'); + + // The next attempt resolves from scratch rather than serving the failure. + const recovered = await driverFactory('default'); + expect(recovered.builtFrom).toMatchObject({ password: 'token-b' }); + }); + + // The staleness check calls the factory speculatively, on a path that used to + // be a pure cache hit. A factory that reads a secret store can fail + // transiently, and that must not fail a query the cached driver can serve. + test('reuses the cached driver when the staleness probe throws', async () => { let shouldFail = false; - const { driverFactory, request } = await createCore({ - driverFactory: () => { + const { core, driverFactory, request } = await createCore({ + driverFactory: (ctx: any) => { if (shouldFail) { - throw new Error('factory blew up'); + throw new Error('secret store unreachable'); } - return { type: 'postgres', password: token }; + return { type: 'postgres', password: ctx.securityContext.token }; }, }, { token: 'token-a' }); - await driverFactory('default'); + const first = await driverFactory('default'); - token = 'token-b'; shouldFail = true; await request({ token: 'token-b' }); - await expect(driverFactory('default')).rejects.toThrow('factory blew up'); - // The next attempt resolves from scratch rather than serving the failure. + expect(await driverFactory('default')).toBe(first); + expect(core.builtDrivers).toHaveLength(1); + + // Once the factory recovers, the rotation is picked up as usual. shouldFail = false; - const recovered = await driverFactory('default'); - expect(recovered.builtFrom).toMatchObject({ password: 'token-b' }); + const rebuilt = await driverFactory('default'); + expect(rebuilt).not.toBe(first); + expect(rebuilt.builtFrom).toMatchObject({ password: 'token-b' }); + }); + + // Two queries in flight when a rotation lands both see the cached driver as + // stale. Only one may rebuild: the loser must not release the driver the + // winner has already handed to its caller, nor stand up a second pool. + test('concurrent callers rebuild once and release once', async () => { + const { core, driverFactory, request } = await createCore({ + driverFactory: (ctx: any) => ({ type: 'postgres', password: ctx.securityContext.token }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + await request({ token: 'token-b' }); + + const [a, b] = await Promise.all([ + driverFactory('default'), + driverFactory('default'), + ]); + + await new Promise((resolve) => setImmediate(resolve)); + + expect(a).toBe(b); + expect(a.builtFrom).toMatchObject({ password: 'token-b' }); + expect(core.builtDrivers).toHaveLength(2); + expect(first.release).toHaveBeenCalledTimes(1); + // The driver handed back is usable — not one whose pool is being drained. + expect(a.release).not.toHaveBeenCalled(); + }); + + test('reuses the driver when the security context cannot be fingerprinted', async () => { + const circular: any = { token: 'token-a' }; + circular.self = circular; + + const factory = jest.fn((ctx: any) => ({ type: 'postgres', password: ctx.securityContext.token })); + const { core, driverFactory, request } = await createCore({ driverFactory: factory }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + // A circular security context fingerprints as null, which every caller must + // read as "assume unchanged" rather than rebuilding blindly. + await request(circular); + + expect(await driverFactory('default')).toBe(first); + expect(core.builtDrivers).toHaveLength(1); + }); + + // The refresh scheduler's default context carries no security context at all, + // so it shares an orchestrator with API traffic on a deployment that does not + // partition by user. + test('treats an absent security context as a change in both directions', async () => { + const { core, driverFactory, request } = await createCore({ + driverFactory: (ctx: any) => ({ + type: 'postgres', + password: ctx.securityContext?.token ?? 'service-account', + }), + }, { token: 'token-a' }); + + const user = await driverFactory('default'); + expect(user.builtFrom).toMatchObject({ password: 'token-a' }); + + await request(undefined); + const scheduler = await driverFactory('default'); + + expect(scheduler).not.toBe(user); + expect(scheduler.builtFrom).toMatchObject({ password: 'service-account' }); + expect(core.builtDrivers).toHaveLength(2); }); }); From 6456197b2848ae6fcbc6b6f6d10b53f847fe5ff5 Mon Sep 17 00:00:00 2001 From: MikeNitsenko Date: Wed, 5 Aug 2026 20:31:37 +0500 Subject: [PATCH 04/16] fix(server-core): surface driver rebuilds at the default log level MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the re-review on #11453. `prodLogger`/`devLogger` only emit a plain-params message when it appears on an allowlist, so 'Rebuilding driver on configuration change' fell through the info, warn and error cases and was dropped below `trace`. Tearing down a connection pool is precisely the event an operator needs in order to correlate a latency blip or an auth error, and the threshold message arrives too late to reconstruct the rebuilds before it. It now carries `warning`, as the two neighbouring log calls already did. Bound the rebuild retry. Losing the race re-entered unconditionally, and where contexts keep displacing each other a request could lose every round, paying for a user-supplied factory call each time. Past three attempts it now takes whatever is cached — the same degrade-to-reuse fallback used everywhere else in this path, and strictly better than starving. Cover both rebuild logs in the tests, including that each carries the param that makes it visible, and that the escalation fires once and names contextToOrchestratorId. Verified by removing the param and watching the test fail. Co-Authored-By: Claude Opus 5 (1M context) --- .../cubejs-server-core/src/core/server.ts | 38 ++++++++++++++++++- .../unit/driver-cache-invalidation.test.ts | 38 +++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/packages/cubejs-server-core/src/core/server.ts b/packages/cubejs-server-core/src/core/server.ts index 74e62ec3338d2..a3c07228332e8 100644 --- a/packages/cubejs-server-core/src/core/server.ts +++ b/packages/cubejs-server-core/src/core/server.ts @@ -82,6 +82,13 @@ const { version } = require('../../../package.json'); */ const DRIVER_REBUILD_WARN_THRESHOLD = 50; +/** + * How many times one request will retry after losing the race to rebuild a + * driver before settling for whatever is cached. Bounds the work a single + * request can be made to do when contexts keep displacing each other's driver. + */ +const MAX_DRIVER_REBUILD_ATTEMPTS = 3; + /** * What a cached driver was built from. `null` on either field means "cannot * tell whether it changed", which is always read as "assume it did not". @@ -664,7 +671,11 @@ export class CubejsServerCore { * Driver factory function `DriverFactoryByDataSource`. Named so the rebuild * path can re-enter it when another caller wins the race to replace a key. */ - const resolveDataSourceDriver = async (dataSource = 'default', preAggregations = false): Promise => { + const resolveDataSourceDriver = async ( + dataSource = 'default', + preAggregations = false, + attempt = 0, + ): Promise => { const factoryKey = preAggregations ? `${dataSource}@pre_agg` : dataSource; const hasSeparatePreAggEnv = hasPreAggregationsEnvVars(dataSource); @@ -710,7 +721,24 @@ export class CubejsServerCore { // supersedes ours: start over rather than release a driver it has // already handed out, or build a second pool alongside it. if (driverPromise[factoryKey] !== cached) { - return resolveDataSourceDriver(dataSource, preAggregations); + const superseding = driverPromise[factoryKey]; + + // Retry, so this request ends up on a driver matching its own + // context — but bounded. Where contexts keep displacing each other + // this request could otherwise lose every round and pay for a + // user-supplied factory call each time. Past the bound, take what is + // cached: degrading to a reused driver is this design's fallback + // everywhere else, and it is strictly better than starving. + if (attempt < MAX_DRIVER_REBUILD_ATTEMPTS) { + return resolveDataSourceDriver(dataSource, preAggregations, attempt + 1); + } + + if (superseding) { + return superseding; + } + + // Invalidated rather than replaced, so there is nothing to reuse — + // fall through and build, which cannot recurse again. } if (!staleness.stale) { @@ -724,10 +752,16 @@ export class CubejsServerCore { driverRebuilds[rebuildKey] = (driverRebuilds[rebuildKey] || 0) + 1; const rebuildCount = driverRebuilds[rebuildKey]; + // Carries `warning` so it survives the default log level: a plain-params + // message matches no allowlist in `prodLogger`/`devLogger` and is + // dropped below `trace`. Tearing down a connection pool is an event an + // operator needs to be able to correlate against, and the threshold + // message below arrives too late to reconstruct the first rebuilds. this.logger('Rebuilding driver on configuration change', { dataSource, preAggregations, rebuildCount, + warning: 'Driver configuration changed; replacing the connection.', }); // A credential rotation rebuilds a handful of times a day. Rebuilding diff --git a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts index e4cf0ffe6e403..a257398f40ebe 100644 --- a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts +++ b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts @@ -57,8 +57,10 @@ class TestServerCore extends CubejsServerCore { * calls for every query. */ async function createCore(options: CreateOptions, securityContext: unknown) { + const logger = jest.fn(); const core = new TestServerCore({ contextToOrchestratorId: () => 'ORCHESTRATOR', + logger, ...options, }); const spy = jest.spyOn(core, 'createOrchestratorApi'); @@ -70,6 +72,9 @@ async function createCore(options: CreateOptions, securityContext: unknown) { return { core, driverFactory, + logger, + /** Log messages, with the params each was reported with. */ + logged: (message: string) => logger.mock.calls.filter(([msg]) => msg === message).map(([, params]) => params), /** Serve another request through the cached orchestrator. */ request: (nextSecurityContext: unknown, requestId = 'req-n') => core.getOrchestratorApi({ requestId, securityContext: nextSecurityContext }), }; @@ -294,6 +299,39 @@ describe('driver cache invalidation', () => { expect(a.release).not.toHaveBeenCalled(); }); + // Both rebuild logs have to survive the default log level, which drops any + // message carrying neither `error` nor `warning`. Without that param the + // rebuild — a connection pool being torn down — is invisible in production. + test('reports every rebuild, and escalates once it looks like a misconfiguration', async () => { + const { driverFactory, request, logged } = await createCore({ + driverFactory: (ctx: any) => ({ type: 'postgres', password: ctx.securityContext.token }), + }, { token: 'token-0' }); + + await driverFactory('default'); + + for (let i = 1; i <= 50; i++) { + // eslint-disable-next-line no-await-in-loop + await request({ token: `token-${i}` }, `req-${i}`); + // eslint-disable-next-line no-await-in-loop + await driverFactory('default'); + } + + const rebuilds = logged('Rebuilding driver on configuration change'); + + expect(rebuilds).toHaveLength(50); + expect(rebuilds.every((params) => params.warning)).toBe(true); + expect(rebuilds[0]).toMatchObject({ dataSource: 'default', rebuildCount: 1 }); + // Counted per alias set, so the 50th rotation reads as 50, not as a pair of + // separate counters for `default` and `default@pre_agg`. + expect(rebuilds[49]).toMatchObject({ rebuildCount: 50 }); + + const escalations = logged('Driver rebuilt repeatedly'); + + expect(escalations).toHaveLength(1); + expect(escalations[0]).toMatchObject({ rebuildCount: 50 }); + expect(escalations[0].warning).toContain('contextToOrchestratorId'); + }); + test('reuses the driver when the security context cannot be fingerprinted', async () => { const circular: any = { token: 'token-a' }; circular.self = circular; From 9c46ab101914fde9adbf99304a1fb10a62b53ef8 Mon Sep 17 00:00:00 2001 From: MikeNitsenko Date: Wed, 5 Aug 2026 20:39:13 +0500 Subject: [PATCH 05/16] fix(server-core): never reuse or re-release a driver once ownership is lost MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the re-review finding on #11453. Exhausting the retry bound where the key had been invalidated rather than replaced fell through into the reuse/rebuild block, which still operates on `cached`. The winning caller's build had failed, so it had already released that driver: a non-stale verdict handed back a driver whose pool was drained, and a stale one released it a second time and logged a spurious release error. The comment claimed the fall-through reached the build; it did not. The reuse and rebuild bodies now hang off an else-chain from the ownership check, so a caller that has lost the key cannot touch `cached` at all and goes straight to building — carrying the probe's result when it already resolved one, so the factory is not asked twice. Covered by a test that drives four lost races and a failing concurrent build, asserting the returned driver has not been released and that nothing was released twice. Verified against a deliberately reverted build, where it is the only failing case. Co-Authored-By: Claude Opus 5 (1M context) --- .../cubejs-server-core/src/core/server.ts | 129 +++++++++--------- .../unit/driver-cache-invalidation.test.ts | 67 +++++++++ 2 files changed, 133 insertions(+), 63 deletions(-) diff --git a/packages/cubejs-server-core/src/core/server.ts b/packages/cubejs-server-core/src/core/server.ts index a3c07228332e8..0cb3e07dd7f60 100644 --- a/packages/cubejs-server-core/src/core/server.ts +++ b/packages/cubejs-server-core/src/core/server.ts @@ -718,11 +718,12 @@ export class CubejsServerCore { // `resolveDriverStaleness` awaits the user's factory, so another caller // may have replaced or invalidated this key in the meantime. Its work - // supersedes ours: start over rather than release a driver it has - // already handed out, or build a second pool alongside it. - if (driverPromise[factoryKey] !== cached) { - const superseding = driverPromise[factoryKey]; + // supersedes ours, and `cached` is no longer ours to reuse or release: + // it has either been handed to that caller's requests or already + // released by it. + const superseding = driverPromise[factoryKey]; + if (superseding !== cached) { // Retry, so this request ends up on a driver matching its own // context — but bounded. Where contexts keep displacing each other // this request could otherwise lose every round and pay for a @@ -737,71 +738,73 @@ export class CubejsServerCore { return superseding; } - // Invalidated rather than replaced, so there is nothing to reuse — - // fall through and build, which cannot recurse again. - } - - if (!staleness.stale) { + // Invalidated rather than replaced — the winning caller's own build + // failed, so it released `cached` and left nothing to reuse. Build + // below, which cannot recurse again, carrying the probe's result when + // it already resolved one so the factory is not asked twice. + resolvedFactoryResult = staleness.stale ? staleness.factoryResult : undefined; + } else if (!staleness.stale) { return cached; - } - - // Counted per alias set, not per key: a rotation seen first through - // `default@pre_agg` and then through `default` is one rebuild of one - // shared driver, and must not read as two counters at 1. - const rebuildKey = aliasedKeys[0]; - driverRebuilds[rebuildKey] = (driverRebuilds[rebuildKey] || 0) + 1; - const rebuildCount = driverRebuilds[rebuildKey]; - - // Carries `warning` so it survives the default log level: a plain-params - // message matches no allowlist in `prodLogger`/`devLogger` and is - // dropped below `trace`. Tearing down a connection pool is an event an - // operator needs to be able to correlate against, and the threshold - // message below arrives too late to reconstruct the first rebuilds. - this.logger('Rebuilding driver on configuration change', { - dataSource, - preAggregations, - rebuildCount, - warning: 'Driver configuration changed; replacing the connection.', - }); - - // A credential rotation rebuilds a handful of times a day. Rebuilding - // this often means the orchestrator id does not partition by whatever - // the factory reads, so contexts that need different connections keep - // displacing each other's driver. - if (rebuildCount === DRIVER_REBUILD_WARN_THRESHOLD) { - this.logger('Driver rebuilt repeatedly', { + } else { + // Counted per alias set, not per key: a rotation seen first through + // `default@pre_agg` and then through `default` is one rebuild of one + // shared driver, and must not read as two counters at 1. + const rebuildKey = aliasedKeys[0]; + driverRebuilds[rebuildKey] = (driverRebuilds[rebuildKey] || 0) + 1; + const rebuildCount = driverRebuilds[rebuildKey]; + + // Carries `warning` so it survives the default log level: a + // plain-params message matches no allowlist in + // `prodLogger`/`devLogger` and is dropped below `trace`. Tearing down + // a connection pool is an event an operator needs to be able to + // correlate against, and the threshold message below arrives too late + // to reconstruct the first rebuilds. + this.logger('Rebuilding driver on configuration change', { dataSource, + preAggregations, rebuildCount, - warning: 'Driver configuration keeps changing for one orchestrator. ' - + 'contextToOrchestratorId likely does not distinguish the contexts ' - + 'driverFactory returns different connections for.', - }); - } - - // Clear every key pointing at the replaced driver, not just the one - // asked for: a surviving alias would keep handing out a driver whose - // pool is being drained, and would release it a second time when it - // was itself found stale. - Object.keys(driverPromise) - .filter((key) => driverPromise[key] === cached) - .forEach((key) => { - driverPromise[key] = null; - delete driverOrigin[key]; + warning: 'Driver configuration changed; replacing the connection.', }); - // Graceful: `release` drains the pool, so queries already running on - // the replaced driver finish before its connections are closed. It is - // deliberately not awaited — this request should not wait on the - // previous driver's in-flight work — and its failure must not fail - // this request. - cached - .then((driver) => driver.release()) - .catch((error) => this.logger('Driver release error', { - dataSource, - error: (error as Error).stack || (error as Error).toString(), - })); + // A credential rotation rebuilds a handful of times a day. Rebuilding + // this often means the orchestrator id does not partition by whatever + // the factory reads, so contexts that need different connections keep + // displacing each other's driver. + if (rebuildCount === DRIVER_REBUILD_WARN_THRESHOLD) { + this.logger('Driver rebuilt repeatedly', { + dataSource, + rebuildCount, + warning: 'Driver configuration keeps changing for one orchestrator. ' + + 'contextToOrchestratorId likely does not distinguish the contexts ' + + 'driverFactory returns different connections for.', + }); + } - resolvedFactoryResult = staleness.factoryResult; + // Clear every key pointing at the replaced driver, not just the one + // asked for: a surviving alias would keep handing out a driver whose + // pool is being drained, and would release it a second time when it + // was itself found stale. + Object.keys(driverPromise) + .filter((key) => driverPromise[key] === cached) + .forEach((key) => { + driverPromise[key] = null; + delete driverOrigin[key]; + }); + + // Graceful: `release` drains the pool, so queries already running on + // the replaced driver finish before its connections are closed. It is + // deliberately not awaited — this request should not wait on the + // previous driver's in-flight work — and its failure must not fail + // this request. + cached + .then((driver) => driver.release()) + .catch((error) => this.logger('Driver release error', { + dataSource, + error: (error as Error).stack || (error as Error).toString(), + })); + + resolvedFactoryResult = staleness.factoryResult; + } } if (preAggregations && hasSeparatePreAggEnv && this.optsHandler.isCustomDriverFactory()) { diff --git a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts index a257398f40ebe..bb18e293fc7fc 100644 --- a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts +++ b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts @@ -21,6 +21,29 @@ class TestServerCore extends CubejsServerCore { /** Set to fail the next driver construction, as a bad credential would. */ public failNextBuild = false; + /** + * Runs before each staleness probe, standing in for another caller that wins + * the race while this one is awaiting the factory. Re-entrant probes skip it, + * so the hook can drive the driver factory itself. + */ + public onStalenessProbe: (() => Promise) | undefined; + + private inStalenessHook = false; + + protected async resolveDriverStaleness(origin: any, context: any): Promise { + if (this.onStalenessProbe && !this.inStalenessHook) { + this.inStalenessHook = true; + + try { + await this.onStalenessProbe(); + } finally { + this.inStalenessHook = false; + } + } + + return super.resolveDriverStaleness(origin, context); + } + protected async createDriverFromFactoryResult( val: any, context: any, @@ -332,6 +355,50 @@ describe('driver cache invalidation', () => { expect(escalations[0].warning).toContain('contextToOrchestratorId'); }); + // Losing the race enough times to exhaust the retry bound, where the winner's + // own build then failed, leaves the key invalidated rather than replaced. The + // driver this caller started from has already been released by that winner, so + // it can be neither handed back nor released again — the only safe move is to + // build. Reaching it needs four lost races and a failed build, hence the hook. + test('builds instead of reusing a released driver when the retry bound is exhausted', async () => { + const { core, driverFactory, request, logged } = await createCore({ + driverFactory: (ctx: any) => ({ type: 'postgres', password: ctx.securityContext.token }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + let round = 0; + + core.onStalenessProbe = async () => { + round += 1; + + // A different context takes the orchestrator over, then rebuilds — which + // replaces the key for the first three rounds. On the fourth that rebuild + // fails, so it invalidates the key and releases what it replaced. + await request({ token: `concurrent-${round}` }, `req-${round}`); + + if (round === 4) { + core.failNextBuild = true; + } + + await Promise.resolve(driverFactory('default')).catch(() => {}); + }; + + const resolved = await driverFactory('default'); + + core.onStalenessProbe = undefined; + await new Promise((resolve) => setImmediate(resolve)); + + // Four rounds, so the bound was genuinely exhausted rather than short-circuited. + expect(round).toBe(4); + expect(resolved).not.toBe(first); + // The returned driver is usable: not one some other caller already drained. + expect(resolved.release).not.toHaveBeenCalled(); + // And nothing was released twice on the way there. + expect(core.builtDrivers.every((driver) => driver.release.mock.calls.length <= 1)).toBe(true); + expect(logged('Driver release error')).toHaveLength(0); + }); + test('reuses the driver when the security context cannot be fingerprinted', async () => { const circular: any = { token: 'token-a' }; circular.self = circular; From e63ba898c15d308cd36f0aaa84f9e5e2f26b29b4 Mon Sep 17 00:00:00 2001 From: MikeNitsenko Date: Fri, 14 Aug 2026 00:04:57 +0500 Subject: [PATCH 06/16] fix(server-core): rate-limit driver rebuilds, and never release the factory's own driver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rebuild tears down a connection pool, so its rate cannot be left to how often contexts happen to differ. A `driverFactory` returning a configuration that is not stable across calls — a credential minted per call, one carrying a nonce — resolves as changed every time, and that deployment works today only because the driver is resolved once and the difference is never noticed. Bound it to one rebuild per data source per 30s: inside the window the cached driver is served without asking the factory at all, exactly as before, and the suppression is reported once per window rather than once per query. Also drop the release of a driver the staleness probe received from the factory. That branch cannot be reached — `assertDriverFactoryResult` rejects a factory that returns a config on one call and a driver on the next, and one that returns drivers consistently records a null config fingerprint on its first build and is never probed — but releasing a value the factory owns could drain a pool it hands out as a singleton. Co-Authored-By: Claude Opus 5 (1M context) --- .../cubejs-server-core/src/core/server.ts | 138 +++++++++++--- .../unit/driver-cache-invalidation.test.ts | 171 ++++++++++++++++++ 2 files changed, 283 insertions(+), 26 deletions(-) diff --git a/packages/cubejs-server-core/src/core/server.ts b/packages/cubejs-server-core/src/core/server.ts index 0cb3e07dd7f60..e30a3096c804e 100644 --- a/packages/cubejs-server-core/src/core/server.ts +++ b/packages/cubejs-server-core/src/core/server.ts @@ -89,6 +89,27 @@ const DRIVER_REBUILD_WARN_THRESHOLD = 50; */ const MAX_DRIVER_REBUILD_ATTEMPTS = 3; +/** + * How long a data source keeps a freshly rebuilt driver before another + * configuration change may replace it. Inside the window the cached driver is + * reused without even asking the factory, exactly as before this file learned + * to rebuild at all. + * + * A rebuild tears down a connection pool, so the rate has to be bounded by + * something other than how often contexts happen to differ. Without this, a + * deployment whose `driverFactory` returns a configuration that is not stable + * across calls — a credential minted per call, say, or one carrying a nonce — + * would rebuild on request after request. That deployment works today, because + * the driver is resolved once and the difference is never noticed; it must not + * be turned into pool churn. + * + * The cost is that a credential which rotates twice inside one window is picked + * up a window late rather than immediately. At half a minute against + * credentials that live for an hour, that is not a tradeoff worth agonising + * over — and the alternative, before this change, was "not until redeploy". + */ +const DRIVER_REBUILD_MIN_INTERVAL_MS = 30 * 1000; + /** * What a cached driver was built from. `null` on either field means "cannot * tell whether it changed", which is always read as "assume it did not". @@ -104,6 +125,18 @@ type DriverFactoryResult = { securityContextFingerprint: string | null; }; +/** Rebuild history of the one driver an alias set resolves to. */ +type DriverRebuildState = { + count: number; + lastRebuildAt: number; + /** + * Whether the suppression window opened by that rebuild has already been + * logged. Reset by each rebuild, so a thrashing deployment reports once per + * window rather than once per query. + */ + suppressionReported: boolean; +}; + function wrapToFnIfNeeded(possibleFn: T | ((a: R) => T)): (a: R) => T { if (typeof possibleFn === 'function') { return possibleFn; @@ -646,13 +679,13 @@ export class CubejsServerCore { const driverOrigin: Record = {}; /** - * How many times each key has been rebuilt. Reported with the rebuild so a - * deployment whose `contextToOrchestratorId` does not partition by whatever - * `driverFactory` reads — every user sharing one orchestrator, say — is - * diagnosable: it rebuilds on request after request rather than once per - * credential rotation. + * Rebuild history per alias set, which both rate-limits rebuilds and makes + * a deployment whose `contextToOrchestratorId` does not partition by + * whatever `driverFactory` reads — every user sharing one orchestrator, say + * — diagnosable: it keeps resolving a changed configuration rather than + * doing so once per credential rotation. */ - const driverRebuilds: Record = {}; + const driverRebuilds: Record = {}; let externalPreAggregationsDriverPromise: Promise | null = null; @@ -704,11 +737,51 @@ export class CubejsServerCore { delete driverOrigin[key]; }); + /** + * Rebuilds are counted and rate-limited per alias set, not per key: a + * rotation seen first through `default@pre_agg` and then through `default` + * is one rebuild of one shared driver, and must not read as two counters + * at 1 — nor rebuild twice. + */ + const rebuildKey = aliasedKeys[0]; + // Already resolved by the staleness check below, so the factory is not // asked twice for the same rebuild. let resolvedFactoryResult: DriverFactoryResult | undefined; const cached = driverPromise[factoryKey]; + const rebuildState = driverRebuilds[rebuildKey]; + + if ( + cached && + rebuildState && + Date.now() - rebuildState.lastRebuildAt < DRIVER_REBUILD_MIN_INTERVAL_MS + ) { + // Inside the window this is a plain cache hit: the factory is not asked + // whether anything changed, because acting on the answer is what has to + // be rate-limited and asking a user-supplied function on every query is + // not free either. A configuration that really did change is picked up + // by the first resolution after the window closes. + if (!rebuildState.suppressionReported) { + rebuildState.suppressionReported = true; + + // Carries `warning` so it survives the default log level, as the + // rebuild it follows does. + this.logger('Driver rebuild suppressed', { + dataSource, + preAggregations, + rebuildCount: rebuildState.count, + warning: 'Driver was rebuilt less than ' + + `${DRIVER_REBUILD_MIN_INTERVAL_MS / 1000}s ago; reusing it without ` + + 'rechecking its configuration. Sustained suppression means the ' + + 'configuration is not stable across driverFactory calls, or that ' + + 'contextToOrchestratorId does not distinguish the contexts ' + + 'driverFactory returns different connections for.', + }); + } + + return cached; + } if (cached) { const staleness = await this.resolveDriverStaleness( @@ -746,12 +819,23 @@ export class CubejsServerCore { } else if (!staleness.stale) { return cached; } else { - // Counted per alias set, not per key: a rotation seen first through - // `default@pre_agg` and then through `default` is one rebuild of one - // shared driver, and must not read as two counters at 1. - const rebuildKey = aliasedKeys[0]; - driverRebuilds[rebuildKey] = (driverRebuilds[rebuildKey] || 0) + 1; - const rebuildCount = driverRebuilds[rebuildKey]; + // Opens a fresh suppression window, so the next configuration change + // for this alias set waits it out rather than tearing down the pool + // this rebuild is about to stand up. + // + // Re-read rather than reusing what was captured before the staleness + // probe awaited: reaching here means no concurrent rebuild landed, but + // the count is the one piece of state that would silently lose an + // increment if that ever stopped being true. + const state = driverRebuilds[rebuildKey] + || { count: 0, lastRebuildAt: 0, suppressionReported: false }; + + state.count += 1; + state.lastRebuildAt = Date.now(); + state.suppressionReported = false; + driverRebuilds[rebuildKey] = state; + + const rebuildCount = state.count; // Carries `warning` so it survives the default log level: a // plain-params message matches no allowlist in @@ -1158,6 +1242,11 @@ export class CubejsServerCore { * built from the first request's token was reused for the life of the * process, so every new connection it opened failed to authenticate. * + * A `stale: true` verdict is permission to rebuild, not an instruction to: the + * caller rate-limits rebuilds per data source, because the rate at which a + * configuration appears to change is a property of user code, while the cost + * of acting on it is a connection pool. + * * Note this follows the documented contract of `contextToOrchestratorId` — * that it is the cache key for database connections. Two contexts that * resolve to different connections but share an orchestrator id are a @@ -1202,23 +1291,20 @@ export class CubejsServerCore { return { stale: false }; } + // `null` for a constructed driver, which carries no configuration to + // compare — and, like every other `null` here, is read as "assume + // unchanged". Nothing is released on that path: the value belongs to the + // factory, which may be handing out a singleton it expects to keep working. + // + // No factory can actually reach it. One that returns drivers consistently + // recorded a null config fingerprint on its first build and is rejected by + // the guard above before the factory is ever called; one that switches from + // configs to drivers is rejected by `OptsHandler.assertDriverFactoryResult`, + // and that throw is caught above as a probe failure. It is handled because + // the type admits it, not because it happens. const configFingerprint = isDriver(value) ? null : fingerprint(value); if (configFingerprint === null || configFingerprint === origin.configFingerprint) { - // A driver the factory constructed for this probe is about to be dropped, - // so hand back whatever it opened rather than leaking it. Only reachable - // for a factory that returns a config sometimes and a driver other times. - if (isDriver(value)) { - try { - await (value).release(); - } catch (error) { - this.logger('Driver release error', { - dataSource: context.dataSource, - error: (error as Error).stack || (error as Error).toString(), - }); - } - } - origin.securityContextFingerprint = securityContextFingerprint; return { stale: false }; diff --git a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts index bb18e293fc7fc..eedcf6190ae08 100644 --- a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts +++ b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts @@ -10,6 +10,28 @@ type FakeDriver = BaseDriver & { testConnection: jest.Mock; }; +/** + * Rebuilds are rate-limited per data source, so a test that rebuilds more than + * once has to say how much time passed in between. Patching `Date.now` rather + * than using fake timers keeps the real microtask scheduling these tests depend + * on to interleave concurrent resolutions. + */ +function fakeClock() { + let now = Date.parse('2026-01-01T00:00:00Z'); + const spy = jest.spyOn(Date, 'now').mockImplementation(() => now); + + return { + /** Longer than any rebuild interval the server enforces. */ + advancePastRebuildInterval: () => { + now += 60 * 1000; + }, + advance: (ms: number) => { + now += ms; + }, + restore: () => spy.mockRestore(), + }; +} + /** * Stands in for real driver construction so these tests exercise the caching * decisions without needing a database. Everything else — the driver factory @@ -104,10 +126,22 @@ async function createCore(options: CreateOptions, securityContext: unknown) { } describe('driver cache invalidation', () => { + let clock: ReturnType; + beforeAll(() => { process.env.CUBEJS_API_SECRET = 'api-secret'; }); + // Frozen by default, so a test that rebuilds twice has to be explicit about + // the time in between rather than passing on whatever the wall clock did. + beforeEach(() => { + clock = fakeClock(); + }); + + afterEach(() => { + clock.restore(); + }); + // The CUB-3599 regression: the orchestrator closed over the context of the // request that created it, so a driver built from a per-user credential was // reused for the life of the process. Every connection it opened after the @@ -237,6 +271,11 @@ describe('driver cache invalidation', () => { await request({ token: 'token-b' }); const preAgg = await driverFactory('default', true); + + // Past the rebuild interval, so it is the alias bookkeeping that keeps these + // two on one driver rather than the rate limit masking a second rebuild. + clock.advancePastRebuildInterval(); + const regular = await driverFactory('default'); await new Promise((resolve) => setImmediate(resolve)); @@ -333,6 +372,9 @@ describe('driver cache invalidation', () => { await driverFactory('default'); for (let i = 1; i <= 50; i++) { + // Each rotation is its own, well clear of the previous rebuild, so all 50 + // are acted on rather than rate-limited. + clock.advancePastRebuildInterval(); // eslint-disable-next-line no-await-in-loop await request({ token: `token-${i}` }, `req-${i}`); // eslint-disable-next-line no-await-in-loop @@ -382,6 +424,13 @@ describe('driver cache invalidation', () => { } await Promise.resolve(driverFactory('default')).catch(() => {}); + + // Each round's rebuild has to sit outside the rate-limiting window by the + // time the caller that lost the race retries — otherwise that retry is a + // cache hit rather than another probe, and the bound is never reached. + // Which is the rate limit working: displacement this rapid is what it + // exists to stop, so provoking the bound means stepping past it. + clock.advancePastRebuildInterval(); }; const resolved = await driverFactory('default'); @@ -416,6 +465,128 @@ describe('driver cache invalidation', () => { expect(core.builtDrivers).toHaveLength(1); }); + // A `driverFactory` whose configuration is not stable across calls — a + // credential minted per call, a nonce — resolves as changed every time. That + // deployment works today, because the driver is resolved once and the + // difference is never noticed, and it must not become a pool teardown per + // query. Rebuilds are therefore rate-limited per data source, and inside the + // window the cached driver is served exactly as it was before. + test('rate-limits rebuilds, reporting the suppression once per window', async () => { + let nonce = 0; + const factory = jest.fn(() => { + nonce += 1; + + return { type: 'postgres', password: `token-${nonce}` }; + }); + const { core, driverFactory, request, logged } = await createCore( + { driverFactory: factory }, + { user: 'a' }, + ); + + await driverFactory('default'); + + clock.advancePastRebuildInterval(); + await request({ user: 'b' }); + const rebuilt = await driverFactory('default'); + + expect(core.builtDrivers).toHaveLength(2); + + const callsBeforeSuppression = factory.mock.calls.length; + + // Three more contexts inside the window. Each would resolve a different + // configuration, and none may replace the driver. + for (const user of ['c', 'd', 'e']) { + // eslint-disable-next-line no-await-in-loop + await request({ user }); + // eslint-disable-next-line no-await-in-loop + expect(await driverFactory('default')).toBe(rebuilt); + } + + expect(core.builtDrivers).toHaveLength(2); + expect(rebuilt.release).not.toHaveBeenCalled(); + // Not even asked: acting on the answer is what is rate-limited, and calling + // user code per query to discard the result would be its own cost. + expect(factory).toHaveBeenCalledTimes(callsBeforeSuppression); + + const suppressions = logged('Driver rebuild suppressed'); + + // Once per window, not once per query. + expect(suppressions).toHaveLength(1); + expect(suppressions[0]).toMatchObject({ dataSource: 'default', rebuildCount: 1 }); + expect(suppressions[0].warning).toContain('driverFactory'); + }); + + test('rebuilds again once the interval has passed', async () => { + const { core, driverFactory, request } = await createCore({ + driverFactory: (ctx: any) => ({ type: 'postgres', password: ctx.securityContext.token }), + }, { token: 'token-a' }); + + await driverFactory('default'); + + clock.advancePastRebuildInterval(); + await request({ token: 'token-b' }); + const rebuilt = await driverFactory('default'); + + // A second rotation inside the window is held back... + await request({ token: 'token-c' }); + expect(await driverFactory('default')).toBe(rebuilt); + + // ...and picked up by the first resolution after it closes, so the rate + // limit delays a rotation rather than dropping it. + clock.advancePastRebuildInterval(); + const latest = await driverFactory('default'); + + expect(latest).not.toBe(rebuilt); + expect(latest.builtFrom).toMatchObject({ password: 'token-c' }); + expect(core.builtDrivers).toHaveLength(3); + }); + + // The probe can never observe a factory switching from configs to a + // constructed driver — `OptsHandler` rejects the second shape — but what that + // rejection must not do is fail a query. It surfaces as a probe failure, and + // the driver the deployment is already using keeps serving. Pinned because the + // staleness check is what put user code on a path that used to be a pure cache + // hit, and the driver it hands back must never be one nobody owns. + test('keeps serving the cached driver when the factory changes its return shape', async () => { + class ConstructedDriver extends BaseDriver { + public release = jest.fn(async () => {}); + + public testConnection = jest.fn(async () => {}); + + public async query(): Promise { + return []; + } + } + + const constructed = new ConstructedDriver(); + let returnDriver = false; + const factory = jest.fn((ctx: any) => (returnDriver + ? constructed + : { type: 'postgres', password: ctx.securityContext.token })); + + const { core, driverFactory, request, logged } = await createCore( + { driverFactory: factory }, + { token: 'token-a' }, + ); + + const built = await driverFactory('default'); + + returnDriver = true; + clock.advancePastRebuildInterval(); + await request({ token: 'token-b' }); + + expect(await driverFactory('default')).toBe(built); + + await new Promise((resolve) => setImmediate(resolve)); + + expect(core.builtDrivers).toHaveLength(1); + expect(built.release).not.toHaveBeenCalled(); + // Not touched either: whatever the factory constructed belongs to the + // factory, which may be handing out a singleton it expects to keep working. + expect(constructed.release).not.toHaveBeenCalled(); + expect(logged('Driver staleness check error')).toHaveLength(1); + }); + // The refresh scheduler's default context carries no security context at all, // so it shares an orchestrator with API traffic on a deployment that does not // partition by user. From 1f8d972699a42f0fe25e6de8ee0dc2130feacac3 Mon Sep 17 00:00:00 2001 From: MikeNitsenko Date: Fri, 14 Aug 2026 00:05:07 +0500 Subject: [PATCH 07/16] docs: fix the per-user OAuth recipe's fallback when no expiry is advertised MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both samples gated solely on the token's expiry, so a provider that advertises none sent every user to the service account permanently. Fall back to `status` when there is no expiry to compare against. Also note the version the in-place connection rebuild requires — the page tells you not to key the orchestrator ID on the token because Cube now rebuilds instead, which is not true of a deployment on an older version — and that replacements are rate-limited. Co-Authored-By: Claude Opus 5 (1M context) --- .../connect-to-data/oauth-authentication.mdx | 45 ++++++++++++++++--- 1 file changed, 38 insertions(+), 7 deletions(-) diff --git a/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx b/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx index 70e4313f5a65b..6a3974d41f67b 100644 --- a/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx +++ b/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx @@ -45,6 +45,17 @@ pre-aggregation cache — on every rotation. +Rebuilding the connection when the configuration changes requires Cube +v1.7.20 or later. On earlier versions the connection resolved by the first +request is kept until the deployment restarts, so a rotated token is only +picked up by a redeploy — including the token itself in the orchestrator ID +is then the only way to pick one up without restarting, at the cost of a +fresh orchestrator per rotation. + + + + + Return the resolved credential from `driver_factory`, not a function that fetches one. Cube compares the configuration values the factory returns, and a function compares as unchanged however the credential behind it @@ -211,16 +222,22 @@ def _access_token(ctx: dict): creds = (cube_cloud.get("userCredentials") or {}).get("databricks") or {} access_token = creds.get("accessToken") + if not access_token: + return None + expires_at = _parse_expiry(creds.get("accessTokenExpiresAt")) # Gate on the expiry rather than on `status`: a failed background refresh # can flag the record while the token already in hand is still valid, and # treating that as fatal drops the user onto the service account for no # reason. - if access_token and expires_at and expires_at > time.time() + EXPIRY_SKEW_SECONDS: - return access_token + if expires_at is not None: + return access_token if expires_at > time.time() + EXPIRY_SKEW_SECONDS else None - return None + # No expiry advertised, so there is nothing to compare against and `status` + # is all there is to go on. Requiring an expiry here would send every user + # of a provider that omits it to the service account. + return access_token if creds.get("status") == "active" else None @config("driver_factory") @@ -261,6 +278,11 @@ const EXPIRY_SKEW_MS = 120 * 1000; function accessToken(securityContext) { // For other data sources, swap `databricks` for `snowflake`, etc. const creds = securityContext?.cubeCloud?.userCredentials?.databricks ?? {}; + + if (!creds.accessToken) { + return undefined; + } + const raw = creds.accessTokenExpiresAt; // Epoch milliseconds if the value is far too large to be seconds. Reading // seconds as milliseconds would land in 1970 and reject every token. @@ -272,12 +294,15 @@ function accessToken(securityContext) { // Gate on the expiry rather than on `status`: a failed background refresh can // flag the record while the token already in hand is still valid, and // treating that as fatal drops the user onto the service account for no - // reason. NaN fails this comparison, so an unparseable expiry falls back too. - if (creds.accessToken && expiresAt > Date.now() + EXPIRY_SKEW_MS) { - return creds.accessToken; + // reason. + if (!Number.isNaN(expiresAt)) { + return expiresAt > Date.now() + EXPIRY_SKEW_MS ? creds.accessToken : undefined; } - return undefined; + // No expiry advertised, so there is nothing to compare against and `status` is + // all there is to go on. Requiring an expiry here would send every user of a + // provider that omits it to the service account. + return creds.status === "active" ? creds.accessToken : undefined; } module.exports = { @@ -329,6 +354,12 @@ module.exports = { ## Operational notes +- **Replacements are rate-limited.** A data source is replaced at most once + every 30 seconds, and inside that window the current connection is reused + without asking `driver_factory` again. A credential that rotates hourly is + never held up by this; one that resolves to a different value on every call + degrades to reuse instead of reconnecting on every query. + - **One orchestrator per user, not per token.** The orchestrator survives rotations, so pre-aggregation caches and queues stay warm and the orchestrator count tracks your concurrent user count rather than growing From 75e0d0426c7eb4f4dcd3af4f7c760e3693a39837 Mon Sep 17 00:00:00 2001 From: MikeNitsenko Date: Fri, 14 Aug 2026 00:16:48 +0500 Subject: [PATCH 08/16] docs: address PR review on the OAuth recipe and driver rebuild scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `status` fallback covers an expiry that cannot be read as well as one that was never advertised — the previous version of that sample fell back to the service account on a garbled value, so say which case is being tolerated rather than leaving the flip implicit. Make the version gate self-verifying: an operator whose rotations only land on redeploy is on a version that predates the rebuild, whatever number this note ends up carrying. Document that `externalDriverFactory` and `contextToDbType` deliberately keep resolving from the creating request's context. Neither is the shape the rebuild is for — the external store is one connection the deployment owns, and a data source's type does not change per user — so the asymmetry with `resolveDataSourceDriver` reads as a decision rather than an oversight. Co-Authored-By: Claude Opus 5 (1M context) --- .../connect-to-data/oauth-authentication.mdx | 24 ++++++++++--------- .../cubejs-server-core/src/core/server.ts | 12 ++++++++++ 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx b/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx index 6a3974d41f67b..993ce445c05af 100644 --- a/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx +++ b/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx @@ -46,11 +46,11 @@ pre-aggregation cache — on every rotation. Rebuilding the connection when the configuration changes requires Cube -v1.7.20 or later. On earlier versions the connection resolved by the first -request is kept until the deployment restarts, so a rotated token is only -picked up by a redeploy — including the token itself in the orchestrator ID -is then the only way to pick one up without restarting, at the cost of a -fresh orchestrator per rotation. +v1.7.20 or later — if a rotated token is only picked up when you redeploy, +your version predates it. On earlier versions the connection resolved by the +first request is kept until the deployment restarts, so including the token +itself in the orchestrator ID is the only way to pick up a rotation without +restarting, at the cost of a fresh orchestrator per rotation. @@ -234,9 +234,10 @@ def _access_token(ctx: dict): if expires_at is not None: return access_token if expires_at > time.time() + EXPIRY_SKEW_SECONDS else None - # No expiry advertised, so there is nothing to compare against and `status` - # is all there is to go on. Requiring an expiry here would send every user - # of a provider that omits it to the service account. + # No expiry advertised, or one that cannot be read, so there is nothing to + # compare against and `status` is all there is to go on. Requiring an expiry + # here would send every user of a provider that omits it to the service + # account. return access_token if creds.get("status") == "active" else None @@ -299,9 +300,10 @@ function accessToken(securityContext) { return expiresAt > Date.now() + EXPIRY_SKEW_MS ? creds.accessToken : undefined; } - // No expiry advertised, so there is nothing to compare against and `status` is - // all there is to go on. Requiring an expiry here would send every user of a - // provider that omits it to the service account. + // No expiry advertised, or one that cannot be read, so there is nothing to + // compare against and `status` is all there is to go on. Requiring an expiry + // here would send every user of a provider that omits it to the service + // account. return creds.status === "active" ? creds.accessToken : undefined; } diff --git a/packages/cubejs-server-core/src/core/server.ts b/packages/cubejs-server-core/src/core/server.ts index e30a3096c804e..a8ff92cc81ea0 100644 --- a/packages/cubejs-server-core/src/core/server.ts +++ b/packages/cubejs-server-core/src/core/server.ts @@ -972,6 +972,18 @@ export class CubejsServerCore { const orchestratorApi = this.createOrchestratorApi( resolveDataSourceDriver, { + // Deliberately outside the staleness check that `resolveDataSourceDriver` + // applies to `requestContextRef.current`: this and `contextToDbType` + // below keep resolving from `context`, the request that created the + // orchestrator, and resolve once for its lifetime. + // + // Both were pinned that way before rebuilding existed, and neither is + // the shape the rebuild is for. The external store is Cube Store or a + // shared pre-aggregation warehouse — one connection the deployment owns, + // not one derived from who is asking — and a data source's type does not + // change per user, only its credentials do. Widening the rebuild to + // cover them would mean tearing down the pre-aggregation store's pool on + // a per-user signal that says nothing about it. externalDriverFactory: this.options.externalDriverFactory && (async () => { if (externalPreAggregationsDriverPromise) { return externalPreAggregationsDriverPromise; From b5d9ed9ad3c0bb844bc8248e46042e7c29d6835a Mon Sep 17 00:00:00 2001 From: MikeNitsenko Date: Wed, 19 Aug 2026 00:51:39 +0500 Subject: [PATCH 09/16] fix(server-core): replace a driver whose credential stopped rotating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebuilding on a changed configuration only covers the half of CUB-3599 where a credential rotates to something new. A credential whose refresh is failing upstream stops rotating instead: `driverFactory` keeps resolving an identical configuration while the connection built from it is already dead, which nothing here can tell apart from a credential nobody has touched. The pool then fails every session it opens until the deployment restarts — the shape a customer's outage actually took, with the factory still being called with a 21-hour-old context and every new session rejected by the warehouse. Two ways out, neither of which a comparison can reach: `DriverConfig.expiresAt` states the connection's lifetime, and it is enforced before the configuration is compared — so an unchanged configuration is still replaced once its deadline passes. Accepted as epoch milliseconds or seconds, an ISO 8601 string, or a Date, because the value is usually copied straight off a credential and the ecosystems disagree about units. Unreadable values are dropped rather than thrown: a lifetime is an optimisation over comparing, and failing a deployment's queries over a malformed one would be worse than the behaviour it had before the field existed. It is excluded from the fingerprint, so a factory recomputing the deadline on every call does not read as a changed connection, and stripped before the configuration reaches the driver's constructor, where every other key is passed through. A factory that keeps throwing now gives up its driver. A probe failure is the factory declining to produce a connection for this context: one is transient — a secret store blinking, a timeout — and reusing what is cached is right, but a `driverFactory` written to fail closed on an unusable credential is stating that this connection must not serve queries. Honouring that only when the factory happens to return is how an expired credential goes on being served from a pool nobody rebuilds. After three consecutive failures spanning at least 30s — both conditions, so a burst under load cannot tear down a working pool — the driver is released and the next request calls the factory itself, surfacing its own error to the caller. `Rebuilding driver on configuration change` becomes `Rebuilding driver` with a `reason`, now that it has two. The eviction the rebuild path did inline moved into `replaceCachedDriver`, so the new path cannot drift from it — a second release of the same driver is exactly what that code is careful about. Tests: seven more in driver-cache-invalidation (a lifetime elapsing with the configuration unchanged and how it is reported, the driver held until it does, an unstable lifetime that must not churn the pool, a deadline extended under an unchanged configuration, sustained refusal releasing and surfacing, a recovered factory resetting the count) and nine in driver-config-expiry covering the parsing, the strip, and that neither reaches the driver's options. Co-Authored-By: Claude Opus 5 --- .../src/core/driver-config-expiry.ts | 83 ++++++ .../cubejs-server-core/src/core/server.ts | 237 +++++++++++++++--- packages/cubejs-server-core/src/core/types.ts | 17 ++ .../unit/driver-cache-invalidation.test.ts | 195 +++++++++++++- .../test/unit/driver-config-expiry.test.ts | 129 ++++++++++ 5 files changed, 622 insertions(+), 39 deletions(-) create mode 100644 packages/cubejs-server-core/src/core/driver-config-expiry.ts create mode 100644 packages/cubejs-server-core/test/unit/driver-config-expiry.test.ts diff --git a/packages/cubejs-server-core/src/core/driver-config-expiry.ts b/packages/cubejs-server-core/src/core/driver-config-expiry.ts new file mode 100644 index 0000000000000..c6769171a7e35 --- /dev/null +++ b/packages/cubejs-server-core/src/core/driver-config-expiry.ts @@ -0,0 +1,83 @@ +/** + * @copyright Cube Dev, Inc. + * @license Apache-2.0 + * @fileoverview The optional lifetime a `driverFactory` can put on the + * configuration it returns. + */ + +import type { DriverConfig } from './types'; + +/** + * Above this a number is read as epoch milliseconds, below it as epoch seconds. + * 1e11 milliseconds is 1973 and 1e11 seconds is the year 5138, so nothing + * anyone can mean today is ambiguous. Both spellings are accepted because the + * value is usually copied straight off a credential, and the ecosystems + * disagree: JavaScript counts milliseconds, Python's `time.time()` seconds. + */ +const MILLISECONDS_THRESHOLD = 1e11; + +/** + * A driver lifetime as a POSIX timestamp in milliseconds, or undefined when + * none was given or the value cannot be read as a moment in time. + * + * Unreadable input is dropped rather than rejected: an expiry is an + * optimisation over comparing configurations, and failing a deployment's + * queries over a malformed one would be a worse outcome than the behaviour it + * had before the field existed. + */ +export function parseDriverExpiry(value: unknown): number | undefined { + if (value === undefined || value === null) { + return undefined; + } + + if (value instanceof Date) { + const time = value.getTime(); + + return Number.isNaN(time) ? undefined : time; + } + + if (typeof value === 'number') { + if (!Number.isFinite(value) || value <= 0) { + return undefined; + } + + return value > MILLISECONDS_THRESHOLD ? value : value * 1000; + } + + if (typeof value === 'string') { + const text = value.trim(); + + if (!text) { + return undefined; + } + + // A stringified timestamp, which `Date.parse` would read as a year. + if (/^\d+(\.\d+)?$/.test(text)) { + return parseDriverExpiry(Number(text)); + } + + const parsed = Date.parse(text); + + return Number.isNaN(parsed) ? undefined : parsed; + } + + return undefined; +} + +/** + * The same configuration without its lifetime, for the two places that must not + * see it: the fingerprint that decides whether the connection changed, and the + * options handed to the driver's own constructor. + * + * Returns the input untouched when there is nothing to strip, so the common + * case allocates nothing. + */ +export function withoutDriverExpiry(config: DriverConfig): DriverConfig { + if (!config || typeof config !== 'object' || !('expiresAt' in config)) { + return config; + } + + const { expiresAt: _lifetime, ...rest } = config; + + return rest; +} diff --git a/packages/cubejs-server-core/src/core/server.ts b/packages/cubejs-server-core/src/core/server.ts index a8ff92cc81ea0..3793a7945ba54 100644 --- a/packages/cubejs-server-core/src/core/server.ts +++ b/packages/cubejs-server-core/src/core/server.ts @@ -38,6 +38,7 @@ import { OrchestratorStorage } from './OrchestratorStorage'; import { createLogger } from './logger'; import { OptsHandler } from './OptsHandler'; import { fingerprint } from './driver-config-fingerprint'; +import { parseDriverExpiry, withoutDriverExpiry } from './driver-config-expiry'; import { driverDependencies, lookupDriverClass, @@ -111,12 +112,44 @@ const MAX_DRIVER_REBUILD_ATTEMPTS = 3; const DRIVER_REBUILD_MIN_INTERVAL_MS = 30 * 1000; /** - * What a cached driver was built from. `null` on either field means "cannot - * tell whether it changed", which is always read as "assume it did not". + * Consecutive staleness checks that could not resolve a configuration before + * the cached driver is given up rather than reused. + * + * A probe failure is the factory declining to produce a connection for this + * context. One is transient — a secret store blinking, a timeout — and reusing + * what is cached is right. Sustained refusal is not: a `driverFactory` written + * to fail closed on an unusable credential is stating that this connection must + * not serve queries, and honouring that only when the factory happens to return + * is how an expired credential goes on serving errors from a pool nobody + * rebuilds. + */ +const MAX_CONSECUTIVE_PROBE_FAILURES = 3; + +/** + * How long those failures must span before the driver is given up. + * + * The count alone is not a duration: under load three concurrent probes can + * fail inside the same blink of a dependency. Requiring both keeps a burst from + * tearing down a working pool while still bounding how long a refusal can be + * ignored. + */ +const PROBE_FAILURE_GRACE_MS = 30 * 1000; + +/** + * What a cached driver was built from. `null` on either fingerprint means + * "cannot tell whether it changed", which is always read as "assume it did + * not"; `expiresAt` is undefined when the configuration named no lifetime. */ type DriverOrigin = { securityContextFingerprint: string | null; configFingerprint: string | null; + expiresAt: number | undefined; +}; + +/** Consecutive probe failures for one alias set, and when they started. */ +type DriverProbeFailures = { + count: number; + firstFailureAt: number; }; /** A `driverFactory` result together with the context that produced it. */ @@ -125,6 +158,19 @@ type DriverFactoryResult = { securityContextFingerprint: string | null; }; +/** Why a cached driver is being replaced, for the operator reading the log. */ +type DriverStalenessReason = 'configuration change' | 'lifetime elapsed'; + +/** + * The verdict on a cached driver. `factoryResult` is present only when the + * probe already resolved one, so the rebuild does not call the factory twice; + * `probeFailed` marks the reuse that happened because the factory threw, which + * the caller counts. + */ +type DriverStaleness = + | { stale: false, probeFailed?: boolean } + | { stale: true, reason: DriverStalenessReason, factoryResult?: DriverFactoryResult }; + /** Rebuild history of the one driver an alias set resolves to. */ type DriverRebuildState = { count: number; @@ -137,6 +183,20 @@ type DriverRebuildState = { suppressionReported: boolean; }; +/** + * Fingerprint of everything in a driver configuration that identifies the + * connection — which is all of it except the lifetime. + * + * The lifetime is excluded deliberately. It is enforced on its own, and it is + * the one field a factory is expected to return a different value for on every + * call, being a deadline recomputed from whatever credential it just read. + * Including it would read each of those calls as a changed connection and + * rebuild the pool on a timer. + */ +function driverConfigFingerprint(value: DriverConfig): string | null { + return fingerprint(withoutDriverExpiry(value)); +} + function wrapToFnIfNeeded(possibleFn: T | ((a: R) => T)): (a: R) => T { if (typeof possibleFn === 'function') { return possibleFn; @@ -687,6 +747,13 @@ export class CubejsServerCore { */ const driverRebuilds: Record = {}; + /** + * Consecutive staleness probes that threw, per alias set. Reset by any + * probe or build that resolves a configuration, so only *sustained* refusal + * reaches the bound. + */ + const driverProbeFailures: Record = {}; + let externalPreAggregationsDriverPromise: Promise | null = null; const contextToDbType: DbTypeInternalFn = this.contextToDbType.bind(this); @@ -737,6 +804,34 @@ export class CubejsServerCore { delete driverOrigin[key]; }); + /** + * Drop every key pointing at `driver` and release it off the request path. + * + * Every key, not just the one asked for: a surviving alias would keep + * handing out a driver whose pool is being drained, and would release it a + * second time when it was itself found stale. + * + * `release` drains the pool, so queries already running on the replaced + * driver finish before its connections close. It is deliberately not + * awaited — this request should not wait on the previous driver's + * in-flight work — and its failure must not fail this request. + */ + const replaceCachedDriver = (driver: Promise) => { + Object.keys(driverPromise) + .filter((key) => driverPromise[key] === driver) + .forEach((key) => { + driverPromise[key] = null; + delete driverOrigin[key]; + }); + + driver + .then((resolved) => resolved.release()) + .catch((error) => this.logger('Driver release error', { + dataSource, + error: (error as Error).stack || (error as Error).toString(), + })); + }; + /** * Rebuilds are counted and rate-limited per alias set, not per key: a * rotation seen first through `default@pre_agg` and then through `default` @@ -816,8 +911,50 @@ export class CubejsServerCore { // below, which cannot recurse again, carrying the probe's result when // it already resolved one so the factory is not asked twice. resolvedFactoryResult = staleness.stale ? staleness.factoryResult : undefined; - } else if (!staleness.stale) { - return cached; + // `=== false` rather than `!`: this package compiles with + // `strictNullChecks` off, where the negation does not narrow the union + // and `probeFailed` below would not typecheck. + } else if (staleness.stale === false) { + if (!staleness.probeFailed) { + delete driverProbeFailures[rebuildKey]; + + return cached; + } + + const failures = driverProbeFailures[rebuildKey] + || { count: 0, firstFailureAt: Date.now() }; + + failures.count += 1; + driverProbeFailures[rebuildKey] = failures; + + const failingForMs = Date.now() - failures.firstFailureAt; + + // Transient, as far as anything here can tell. Reuse, exactly as + // before this bound existed. + if ( + failures.count < MAX_CONSECUTIVE_PROBE_FAILURES || + failingForMs < PROBE_FAILURE_GRACE_MS + ) { + return cached; + } + + this.logger('Releasing driver after repeated staleness check failures', { + dataSource, + preAggregations, + failureCount: failures.count, + warning: 'driverFactory has failed every staleness check for ' + + `${Math.round(failingForMs / 1000)}s. Releasing the connection it ` + + 'built rather than serving queries on a configuration it will no ' + + 'longer produce; the next request calls the factory itself, so a ' + + 'factory that fails closed on an unusable credential surfaces its ' + + 'own error.', + }); + + delete driverProbeFailures[rebuildKey]; + replaceCachedDriver(cached); + + // Falls through to the build below, which calls the factory itself: + // it either recovers, or throws where the caller can see it. } else { // Opens a fresh suppression window, so the next configuration change // for this alias set waits it out rather than tearing down the pool @@ -843,11 +980,12 @@ export class CubejsServerCore { // a connection pool is an event an operator needs to be able to // correlate against, and the threshold message below arrives too late // to reconstruct the first rebuilds. - this.logger('Rebuilding driver on configuration change', { + this.logger('Rebuilding driver', { dataSource, preAggregations, rebuildCount, - warning: 'Driver configuration changed; replacing the connection.', + reason: staleness.reason, + warning: `Replacing the connection — ${staleness.reason}.`, }); // A credential rotation rebuilds a handful of times a day. Rebuilding @@ -864,28 +1002,8 @@ export class CubejsServerCore { }); } - // Clear every key pointing at the replaced driver, not just the one - // asked for: a surviving alias would keep handing out a driver whose - // pool is being drained, and would release it a second time when it - // was itself found stale. - Object.keys(driverPromise) - .filter((key) => driverPromise[key] === cached) - .forEach((key) => { - driverPromise[key] = null; - delete driverOrigin[key]; - }); - - // Graceful: `release` drains the pool, so queries already running on - // the replaced driver finish before its connections are closed. It is - // deliberately not awaited — this request should not wait on the - // previous driver's in-flight work — and its failure must not fail - // this request. - cached - .then((driver) => driver.release()) - .catch((error) => this.logger('Driver release error', { - dataSource, - error: (error as Error).stack || (error as Error).toString(), - })); + delete driverProbeFailures[rebuildKey]; + replaceCachedDriver(cached); resolvedFactoryResult = staleness.factoryResult; } @@ -905,6 +1023,7 @@ export class CubejsServerCore { const origin: DriverOrigin = { securityContextFingerprint: null, configFingerprint: null, + expiresAt: undefined, }; aliasedKeys.forEach((key) => { @@ -921,10 +1040,17 @@ export class CubejsServerCore { securityContextFingerprint: fingerprint(currentDriverContext.securityContext), }; + const factoryConfig = isDriver(factoryResult.value) + ? undefined + : factoryResult.value; + origin.securityContextFingerprint = factoryResult.securityContextFingerprint; - origin.configFingerprint = isDriver(factoryResult.value) - ? null - : fingerprint(factoryResult.value); + origin.configFingerprint = factoryConfig + ? driverConfigFingerprint(factoryConfig) + : null; + origin.expiresAt = factoryConfig + ? parseDriverExpiry(factoryConfig.expiresAt) + : undefined; driver = await this.createDriverFromFactoryResult( factoryResult.value, @@ -939,6 +1065,10 @@ export class CubejsServerCore { await driver.testConnection(); + // Resolved a configuration and stood a connection up on it, so + // whatever the probes were failing on has passed. + delete driverProbeFailures[rebuildKey]; + return driver; } @@ -1218,7 +1348,10 @@ export class CubejsServerCore { if (isDriver(val)) { return val; } else { - const { type, ...rest } = val; + // Without the lifetime: it describes when to replace this driver, not + // how to connect, and every other key here is passed to the driver's own + // constructor. + const { type, ...rest } = withoutDriverExpiry(val); const opts = Object.keys(rest).length ? rest : { @@ -1268,9 +1401,21 @@ export class CubejsServerCore { protected async resolveDriverStaleness( origin: DriverOrigin | undefined, context: DriverContext, - ): Promise<{ stale: false } | { stale: true, factoryResult: DriverFactoryResult }> { + ): Promise { + if (!origin) { + return { stale: false }; + } + + // Checked first, and without asking the factory: a credential that has + // stopped rotating resolves to the same configuration indefinitely while + // the connection built from it is already dead. That is the one staleness a + // comparison cannot see, which is why a configuration may state its own + // lifetime. + if (origin.expiresAt !== undefined && Date.now() >= origin.expiresAt) { + return { stale: true, reason: 'lifetime elapsed' }; + } + if ( - !origin || origin.configFingerprint === null || !this.optsHandler.isCustomDriverFactory() ) { @@ -1294,13 +1439,15 @@ export class CubejsServerCore { // This call is a probe, not the request's own resolution: a cache hit // never used to invoke the factory at all, so letting a transient failure // here propagate would fail a query the cached driver could have served. - // Degrade to reuse, as with anything else that cannot be compared. + // Degrade to reuse, as with anything else that cannot be compared — but + // report it, because a factory that keeps refusing is not transient and + // the caller gives the driver up once these stop being occasional. this.logger('Driver staleness check error', { dataSource: context.dataSource, error: (error as Error).stack || (error as Error).toString(), }); - return { stale: false }; + return { stale: false, probeFailed: true }; } // `null` for a constructed driver, which carries no configuration to @@ -1314,15 +1461,29 @@ export class CubejsServerCore { // configs to drivers is rejected by `OptsHandler.assertDriverFactoryResult`, // and that throw is caught above as a probe failure. It is handled because // the type admits it, not because it happens. - const configFingerprint = isDriver(value) ? null : fingerprint(value); + const config = isDriver(value) ? undefined : value; + const configFingerprint = config ? driverConfigFingerprint(config) : null; if (configFingerprint === null || configFingerprint === origin.configFingerprint) { origin.securityContextFingerprint = securityContextFingerprint; + // The connection is unchanged, but its deadline may not be — the lifetime + // is excluded from the fingerprint, so a credential re-issued with the + // same value and a later expiry compares equal. Carrying the new deadline + // over is what keeps that from rebuilding on the old one, once per window, + // forever. + if (config) { + origin.expiresAt = parseDriverExpiry(config.expiresAt); + } + return { stale: false }; } - return { stale: true, factoryResult: { value, securityContextFingerprint } }; + return { + stale: true, + reason: 'configuration change', + factoryResult: { value, securityContextFingerprint }, + }; } public async testConnections() { diff --git a/packages/cubejs-server-core/src/core/types.ts b/packages/cubejs-server-core/src/core/types.ts index 3d1cfee7d79ee..5f2cba0b96514 100644 --- a/packages/cubejs-server-core/src/core/types.ts +++ b/packages/cubejs-server-core/src/core/types.ts @@ -159,6 +159,23 @@ export type DriverOptions = { export type DriverConfig = { type: DatabaseType, + /** + * When the connection this configuration describes stops being usable, as + * epoch milliseconds (or seconds), an ISO 8601 string, or a `Date`. + * + * A driver is resolved once and then cached, so a connection built from a + * rotating credential is replaced only when that credential *changes*. One + * that stops rotating — an expired token whose refresh is failing upstream — + * resolves to an identical configuration indefinitely while the connection + * built from it is already dead, and no comparison can see that. Setting this + * states the lifetime outright, so the driver is replaced when it elapses + * whether or not anything about the configuration changed. + * + * Optional, and deliberately not part of the configuration's identity: a + * factory may return a fresh deadline on every call without that reading as a + * changed connection. + */ + expiresAt?: number | string | Date, } & DriverOptions; export type DriverFactoryFn = (context: DriverContext) => diff --git a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts index eedcf6190ae08..2e69a0955394c 100644 --- a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts +++ b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts @@ -381,10 +381,11 @@ describe('driver cache invalidation', () => { await driverFactory('default'); } - const rebuilds = logged('Rebuilding driver on configuration change'); + const rebuilds = logged('Rebuilding driver'); expect(rebuilds).toHaveLength(50); expect(rebuilds.every((params) => params.warning)).toBe(true); + expect(rebuilds.every((params) => params.reason === 'configuration change')).toBe(true); expect(rebuilds[0]).toMatchObject({ dataSource: 'default', rebuildCount: 1 }); // Counted per alias set, so the 50th rotation reads as 50, not as a pair of // separate counters for `default` and `default@pre_agg`. @@ -608,4 +609,196 @@ describe('driver cache invalidation', () => { expect(scheduler.builtFrom).toMatchObject({ password: 'service-account' }); expect(core.builtDrivers).toHaveLength(2); }); + // The other half of CUB-3599, and the half no comparison can reach: the + // credential stopped rotating instead of rotating to something new. The + // factory keeps resolving an identical configuration while the connection + // built from it is already dead, so only a stated lifetime can replace it. + test('rebuilds when the stated lifetime elapses, configuration unchanged', async () => { + const expiresAt = Date.now() + 60 * 60 * 1000; + const { core, driverFactory } = await createCore({ + driverFactory: () => ({ type: 'postgres', password: 'frozen-token', expiresAt }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + // Same request context, same configuration — and past the deadline. + clock.advance(61 * 60 * 1000); + + const second = await driverFactory('default'); + + expect(second).not.toBe(first); + expect(core.builtDrivers).toHaveLength(2); + expect(first.release).toHaveBeenCalled(); + }); + + test('reports the lifetime rebuild as its own reason', async () => { + const expiresAt = Date.now() + 60 * 60 * 1000; + const { driverFactory, logged } = await createCore({ + driverFactory: () => ({ type: 'postgres', password: 'frozen-token', expiresAt }), + }, { token: 'token-a' }); + + await driverFactory('default'); + clock.advance(61 * 60 * 1000); + await driverFactory('default'); + + const rebuilds = logged('Rebuilding driver'); + + expect(rebuilds).toHaveLength(1); + expect(rebuilds[0]).toMatchObject({ reason: 'lifetime elapsed', rebuildCount: 1 }); + }); + + test('keeps the driver until its lifetime elapses', async () => { + const expiresAt = Date.now() + 60 * 60 * 1000; + const { core, driverFactory } = await createCore({ + driverFactory: () => ({ type: 'postgres', password: 'frozen-token', expiresAt }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + clock.advance(59 * 60 * 1000); + + expect(await driverFactory('default')).toBe(first); + expect(core.builtDrivers).toHaveLength(1); + }); + + // A deadline recomputed from the current clock differs on every call. It is + // excluded from the configuration's identity precisely so that a factory + // written that way does not rebuild the pool on a timer. + test('a lifetime that changes on every call is not a configuration change', async () => { + const { core, driverFactory, request } = await createCore({ + driverFactory: () => ({ + type: 'postgres', + password: 'stable-token', + expiresAt: Date.now() + 60 * 60 * 1000, + }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + for (let i = 1; i <= 5; i++) { + clock.advancePastRebuildInterval(); + // eslint-disable-next-line no-await-in-loop + await request({ token: `token-${i}` }, `req-${i}`); + // eslint-disable-next-line no-await-in-loop + expect(await driverFactory('default')).toBe(first); + } + + expect(core.builtDrivers).toHaveLength(1); + }); + + // The lifetime is not fingerprinted, so a credential re-issued with the same + // value and a later deadline compares equal. Carrying the new deadline over is + // what stops that driver from being rebuilt on the old one, once per window, + // for as long as the process runs. + test('carries a later deadline over when the configuration is unchanged', async () => { + let expiresAt = Date.now() + 60 * 60 * 1000; + const { core, driverFactory, request } = await createCore({ + driverFactory: () => ({ type: 'postgres', password: 'stable-token', expiresAt }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + // The credential is re-issued 30 minutes in: same token, later deadline. + clock.advance(30 * 60 * 1000); + expiresAt = Date.now() + 60 * 60 * 1000; + await request({ token: 'token-b' }); + + expect(await driverFactory('default')).toBe(first); + + // Past the original deadline, inside the new one. + clock.advance(45 * 60 * 1000); + + expect(await driverFactory('default')).toBe(first); + expect(core.builtDrivers).toHaveLength(1); + }); + + // A `driverFactory` that fails closed on an unusable credential is stating + // that the connection must not serve queries. Reusing the cached driver + // forever because the refusal arrives as a throw is how an expired credential + // goes on being served from a pool nobody rebuilds. + test('gives the driver up when the factory keeps refusing', async () => { + let shouldFail = false; + const { core, driverFactory, request, logged } = await createCore({ + driverFactory: (ctx: any) => { + if (shouldFail) { + throw new Error('credential is unusable'); + } + + return { type: 'postgres', password: ctx.securityContext.token }; + }, + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + shouldFail = true; + + // Two refusals inside the grace window: still transient as far as this can + // tell, so the cached driver is reused. + await request({ token: 'token-b' }, 'req-2'); + expect(await driverFactory('default')).toBe(first); + + await request({ token: 'token-c' }, 'req-3'); + expect(await driverFactory('default')).toBe(first); + + // Sustained past the grace window: the driver is given up, and the caller + // sees the factory's own error rather than a connection it refused to build. + clock.advance(31 * 1000); + await request({ token: 'token-d' }, 'req-4'); + + await expect(driverFactory('default')).rejects.toThrow('credential is unusable'); + + await new Promise(process.nextTick); + expect((first).release).toHaveBeenCalled(); + expect(core.builtDrivers).toHaveLength(1); + + const released = logged('Releasing driver after repeated staleness check failures'); + + expect(released).toHaveLength(1); + expect(released[0]).toMatchObject({ dataSource: 'default', failureCount: 3 }); + + // And once the credential is usable again, the next request rebuilds. + shouldFail = false; + await request({ token: 'token-e' }, 'req-5'); + + const rebuilt = await driverFactory('default'); + + expect(rebuilt.builtFrom).toMatchObject({ password: 'token-e' }); + }); + + test('a recovered factory resets the refusal count', async () => { + let shouldFail = false; + const { core, driverFactory, request } = await createCore({ + driverFactory: (ctx: any) => { + if (shouldFail) { + throw new Error('secret store unreachable'); + } + + return { type: 'postgres', password: ctx.securityContext.token }; + }, + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + // Two refusals, then a probe that resolves — which clears the count, so the + // two refusals after it cannot reach the bound between them. + for (const token of ['token-b', 'token-c']) { + shouldFail = true; + // eslint-disable-next-line no-await-in-loop + await request({ token }, `req-${token}`); + // eslint-disable-next-line no-await-in-loop + expect(await driverFactory('default')).toBe(first); + clock.advance(31 * 1000); + } + + shouldFail = false; + await request({ token: 'token-a' }, 'req-recovered'); + expect(await driverFactory('default')).toBe(first); + + shouldFail = true; + clock.advance(31 * 1000); + await request({ token: 'token-d' }, 'req-d'); + + expect(await driverFactory('default')).toBe(first); + expect(core.builtDrivers).toHaveLength(1); + }); }); diff --git a/packages/cubejs-server-core/test/unit/driver-config-expiry.test.ts b/packages/cubejs-server-core/test/unit/driver-config-expiry.test.ts new file mode 100644 index 0000000000000..a1a8215b374fd --- /dev/null +++ b/packages/cubejs-server-core/test/unit/driver-config-expiry.test.ts @@ -0,0 +1,129 @@ +import { CreateOptions, CubejsServerCore } from '../../src'; +import { parseDriverExpiry, withoutDriverExpiry } from '../../src/core/driver-config-expiry'; + +describe('parseDriverExpiry', () => { + const iso = '2026-08-17T22:29:31.136Z'; + const epochMs = Date.parse(iso); + + test('reads an ISO 8601 string, which is how a credential usually carries it', () => { + expect(parseDriverExpiry(iso)).toBe(epochMs); + expect(parseDriverExpiry('2026-08-17T22:29:31.136+00:00')).toBe(epochMs); + expect(parseDriverExpiry(` ${iso} `)).toBe(epochMs); + }); + + test('reads a Date', () => { + expect(parseDriverExpiry(new Date(epochMs))).toBe(epochMs); + }); + + test('reads epoch milliseconds and epoch seconds alike', () => { + expect(parseDriverExpiry(epochMs)).toBe(epochMs); + // What `time.time() + 3600` in a Python config produces. Read as seconds it + // is 2026; read as milliseconds it would be 1970, and every driver built + // from it would be born expired. + expect(parseDriverExpiry(Math.floor(epochMs / 1000))).toBe(Math.floor(epochMs / 1000) * 1000); + }); + + test('reads a stringified timestamp, which Date.parse would take for a year', () => { + expect(parseDriverExpiry(String(epochMs))).toBe(epochMs); + expect(parseDriverExpiry(String(Math.floor(epochMs / 1000)))).toBe( + Math.floor(epochMs / 1000) * 1000, + ); + }); + + test('drops anything that is not a moment in time', () => { + // Dropped rather than thrown: the lifetime is an optimisation over + // comparing configurations, and failing a deployment's queries over a + // malformed one would be worse than the behaviour it had before the field + // existed. + expect(parseDriverExpiry(undefined)).toBeUndefined(); + expect(parseDriverExpiry(null)).toBeUndefined(); + expect(parseDriverExpiry('')).toBeUndefined(); + expect(parseDriverExpiry(' ')).toBeUndefined(); + expect(parseDriverExpiry('whenever')).toBeUndefined(); + expect(parseDriverExpiry(new Date('nonsense'))).toBeUndefined(); + expect(parseDriverExpiry(0)).toBeUndefined(); + expect(parseDriverExpiry(-1)).toBeUndefined(); + expect(parseDriverExpiry(NaN)).toBeUndefined(); + expect(parseDriverExpiry(Infinity)).toBeUndefined(); + expect(parseDriverExpiry(true)).toBeUndefined(); + expect(parseDriverExpiry({ expiresAt: iso })).toBeUndefined(); + expect(parseDriverExpiry([iso])).toBeUndefined(); + }); +}); + +describe('withoutDriverExpiry', () => { + test('strips the lifetime without mutating the input', () => { + const config = { type: 'postgres', password: 'secret', expiresAt: 1 }; + + expect(withoutDriverExpiry(config)).toEqual({ type: 'postgres', password: 'secret' }); + expect(config.expiresAt).toBe(1); + }); + + test('returns the same object when there is nothing to strip', () => { + const config = { type: 'postgres' }; + + expect(withoutDriverExpiry(config)).toBe(config); + }); +}); + +/** + * Every key of a `DriverConfig` other than `type` is passed to the driver's own + * constructor, so a lifetime left in would arrive as a connection option — and, + * for a configuration that names nothing else, would also displace the pool + * defaults that an otherwise-empty configuration is meant to get. + */ +describe('driver construction', () => { + class ExposedCore extends CubejsServerCore { + public build(val: any, context: any) { + return this.createDriverFromFactoryResult(val, context); + } + } + + let created: jest.SpyInstance; + + beforeAll(() => { + process.env.CUBEJS_API_SECRET = 'api-secret'; + }); + + beforeEach(() => { + created = jest.spyOn(CubejsServerCore, 'createDriver').mockReturnValue({}); + }); + + afterEach(() => { + created.mockRestore(); + }); + + function core() { + return new ExposedCore({ + driverFactory: () => ({ type: 'postgres' }), + logger: jest.fn(), + }); + } + + test('does not pass the lifetime to the driver', async () => { + await core().build( + { type: 'postgres', password: 'secret', expiresAt: '2026-08-17T22:29:31.136Z' }, + { dataSource: 'default' }, + ); + + expect(created).toHaveBeenCalledWith('postgres', expect.not.objectContaining({ + expiresAt: expect.anything(), + })); + expect(created).toHaveBeenCalledWith('postgres', expect.objectContaining({ + password: 'secret', + dataSource: 'default', + })); + }); + + test('a configuration carrying only a lifetime still gets the pool defaults', async () => { + await core().build( + { type: 'postgres', expiresAt: '2026-08-17T22:29:31.136Z' }, + { dataSource: 'default' }, + ); + + const [, opts] = created.mock.calls[0]; + + expect(opts).not.toHaveProperty('expiresAt'); + expect(opts).toHaveProperty('maxPoolSize'); + }); +}); From 69cc64180dbbd104903a91e6418c59d98881e8df Mon Sep 17 00:00:00 2001 From: MikeNitsenko Date: Wed, 19 Aug 2026 00:51:51 +0500 Subject: [PATCH 10/16] docs: state the connection's lifetime in the per-user OAuth recipe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recipe told you to key the orchestrator ID on the username because Cube rebuilds the connection when the configuration changes. That covers a rotation; it does not cover a refresh that starts failing, which leaves the same credential in place while the connection dies. Both samples now return `expiresAt` alongside the token — and only alongside the user's own token, since giving the service-account fallback the user's deadline would rebuild that connection forever. Drop the version number from the rebuild note. It named a release that has since shipped without this, which is worse than naming none: the operator most likely to read it is on that exact version, wondering why rotations only land on redeploy. The behavioural test the note already carries is the durable half. Say what the pre-rebuild fallback actually needs. Keying the orchestrator ID on the credential's expiry picks up rotations, but a credential that stops rotating freezes that ID too, and the dead connection stays pinned for the life of the process — so add a coarse clock once the expiry has passed. Document `expiresAt` in the `driver_factory` reference, including that it is not passed to the driver and not part of the configuration's identity. Co-Authored-By: Claude Opus 5 --- .../connect-to-data/oauth-authentication.mdx | 92 +++++++++++++++---- .../reference/configuration/config.mdx | 18 ++++ 2 files changed, 92 insertions(+), 18 deletions(-) diff --git a/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx b/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx index 993ce445c05af..99d8fc3d50210 100644 --- a/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx +++ b/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx @@ -45,12 +45,17 @@ pre-aggregation cache — on every rotation. -Rebuilding the connection when the configuration changes requires Cube -v1.7.20 or later — if a rotated token is only picked up when you redeploy, -your version predates it. On earlier versions the connection resolved by the -first request is kept until the deployment restarts, so including the token -itself in the orchestrator ID is the only way to pick up a rotation without -restarting, at the cost of a fresh orchestrator per rotation. +Rebuilding the connection needs a Cube version that supports it. If a rotated +token is only picked up when you redeploy, yours predates it: the connection +resolved by the first request is kept until the deployment restarts. + +On those versions, putting the credential's **expiry** in the orchestrator ID +is the only way to pick up a rotation without restarting — at the cost of a +fresh orchestrator per rotation. Add a coarse clock to it once that expiry has +passed, for example `int(time.time() // 300)`, because a credential whose +refresh is failing upstream stops rotating altogether: an ID that changes only +with the credential would pin the dead connection for as long as the process +runs. @@ -215,11 +220,17 @@ def _parse_expiry(value): return parsed.timestamp() -def _access_token(ctx: dict): - """The user's OAuth token, or None to fall back to the service account.""" +def _credentials(ctx: dict) -> dict: + """The user's credentials for this data source, or an empty dict.""" # For other data sources, swap "databricks" for "snowflake", etc. cube_cloud = (ctx.get("securityContext") or {}).get("cubeCloud") or {} - creds = (cube_cloud.get("userCredentials") or {}).get("databricks") or {} + + return (cube_cloud.get("userCredentials") or {}).get("databricks") or {} + + +def _access_token(ctx: dict): + """The user's OAuth token, or None to fall back to the service account.""" + creds = _credentials(ctx) access_token = creds.get("accessToken") if not access_token: @@ -243,12 +254,29 @@ def _access_token(ctx: dict): @config("driver_factory") def driver_factory(ctx: dict) -> dict: + access_token = _access_token(ctx) + creds = _credentials(ctx) + # Cube rebuilds this connection whenever the returned configuration # changes, so returning a rotated token here is enough to replace it. + # + # `expiresAt` covers what a comparison cannot see. A credential whose + # refresh is failing upstream stops rotating: it resolves to an identical + # configuration indefinitely while the connection built from it is already + # dead. Stating the deadline replaces that connection anyway. + # + # Only for the user's own token — the service-account credential below has + # no deadline of its own, and giving it the user's would rebuild the + # fallback connection forever. return { "type": "databricks-jdbc", "url": os.environ["CUBEJS_DB_DATABRICKS_URL"], - "token": _access_token(ctx) or os.environ["CUBEJS_DB_DATABRICKS_TOKEN"], + "token": access_token or os.environ["CUBEJS_DB_DATABRICKS_TOKEN"], + **( + {"expiresAt": creds.get("accessTokenExpiresAt")} + if access_token + else {} + ), "acceptPolicy": True, "catalog": os.environ.get("CUBEJS_DB_DATABRICKS_CATALOG"), } @@ -310,14 +338,28 @@ function accessToken(securityContext) { module.exports = { // Cube rebuilds this connection whenever the returned configuration changes, // so returning a rotated token here is enough to replace it. - driverFactory: ({ securityContext }) => ({ - type: "databricks-jdbc", - url: process.env.CUBEJS_DB_DATABRICKS_URL, - token: - accessToken(securityContext) ?? process.env.CUBEJS_DB_DATABRICKS_TOKEN, - acceptPolicy: true, - catalog: process.env.CUBEJS_DB_DATABRICKS_CATALOG, - }), + // + // `expiresAt` covers what a comparison cannot see. A credential whose refresh + // is failing upstream stops rotating: it resolves to an identical + // configuration indefinitely while the connection built from it is already + // dead. Stating the deadline replaces that connection anyway. + // + // Only for the user's own token — the service-account credential below has no + // deadline of its own, and giving it the user's would rebuild the fallback + // connection forever. + driverFactory: ({ securityContext }) => { + const token = accessToken(securityContext); + const creds = securityContext?.cubeCloud?.userCredentials?.databricks; + + return { + type: "databricks-jdbc", + url: process.env.CUBEJS_DB_DATABRICKS_URL, + token: token ?? process.env.CUBEJS_DB_DATABRICKS_TOKEN, + ...(token ? { expiresAt: creds?.accessTokenExpiresAt } : {}), + acceptPolicy: true, + catalog: process.env.CUBEJS_DB_DATABRICKS_CATALOG, + }; + }, // One orchestrator per user: separate DB connections, execution queues and // pre-aggregation caches. Deliberately not keyed on the token — see the @@ -354,6 +396,12 @@ module.exports = { replacement and drains the old pool, so in-flight queries finish on the connection they started on. +5. **A stated lifetime replaces it even when nothing changed** — `expiresAt` + is enforced on its own, so a credential that stops rotating rather than + rotating to something new does not pin a dead connection. It is excluded + from the comparison in step 4, so a factory may recompute the deadline on + every call without that reading as a new connection. + ## Operational notes - **Replacements are rate-limited.** A data source is replaced at most once @@ -375,6 +423,14 @@ module.exports = { check.** If it has no access at all, liveness checks and any query that falls back to it fail with an opaque authorization error from the driver rather than something diagnosable. +- **A credential that stops refreshing is the case to plan for.** Rotation is + the easy half: the configuration changes and the connection is replaced. A + refresh that starts failing upstream leaves the same credential in place + until it expires and beyond, which no comparison can distinguish from a + credential nobody has touched. Return `expiresAt` and the connection is + replaced on its own deadline; a factory that raises instead of falling back + is given up after it has refused for long enough, so the error reaches the + caller rather than 401s from a pool nobody rebuilds. - **Falling back is silent.** A missing or near-expired token sends the query to the service account instead of failing, so results reflect the service account's permissions rather than the user's. If that is not diff --git a/docs-mintlify/reference/configuration/config.mdx b/docs-mintlify/reference/configuration/config.mdx index db729738c9849..ad10e186d5ecf 100644 --- a/docs-mintlify/reference/configuration/config.mdx +++ b/docs-mintlify/reference/configuration/config.mdx @@ -442,6 +442,24 @@ in the drivers' [source code][link-github-cube-drivers]. +The optional `expiresAt` element states when the connection this configuration +describes stops being usable, as epoch milliseconds (or seconds), an ISO 8601 +string, or a date. It is not passed to the driver. + +A driver is resolved once and then cached, and replaced when the configuration +`driver_factory` returns changes. A connection built from a credential that +*stops* rotating — an expired token whose refresh is failing — is the case that +leaves behind: the factory keeps resolving an identical configuration while the +connection is already dead. `expiresAt` states the lifetime outright, so the +connection is replaced when it elapses whether or not anything else changed. + +It is deliberately excluded from the comparison, so a factory may return a +deadline recomputed on every call without that reading as a new connection. + + + + + A custom `driver_factory` takes precedence over the [`CUBEJS_PRE_AGGREGATIONS_*` environment variables][ref-preagg-data-source], for every data source rather than only the ones it handles. If both are configured, the `driver_factory` connection is used for From 07a25cd3becde1b46c7e63ff94305bb612701e25 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 20:19:17 +0000 Subject: [PATCH 11/16] fix(server-core): bound driver replacement to sustained, fixable causes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the latest review round on the driver rebuild path. A probe failure is evidence about whatever driverFactory had to reach, not about the cached connection. Giving a working pool up after 30s turned a secret store restart into a query outage — the one path here that ended up worse than before the change. The grace window is now five minutes, and the recipe tells a factory that reaches an external dependency to catch its own failures. That window is also now rolling. firstFailureAt was stamped once and the record only ever cleared by a probe that resolved, so three unrelated flakes days apart accumulated into a "sustained refusal" that tore down a pool. Tracking lastFailureAt starts a fresh window once the previous failure has aged out. The give-up path replaced a driver without recording it, so it bypassed both brakes on pool churn: no suppression window, and never counted toward the rebuild threshold. Both replacement paths now go through one helper, with the reason distinguishing them in the log. A configuration whose expiresAt has already passed is no longer honoured. Rebuilding cannot move a deadline the factory keeps re-asserting, so the driver was replaced once per suppression window for the life of the process; it is now kept, with a warning naming the field. Also: expiresAt cannot be a Python datetime — it raises at the native config bridge — so config.mdx states the accepted forms per language. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015R8SandPBdggffPL7WfwB7 --- .../connect-to-data/oauth-authentication.mdx | 9 + .../reference/configuration/config.mdx | 10 +- .../cubejs-server-core/src/core/server.ts | 222 +++++++++++++----- .../unit/driver-cache-invalidation.test.ts | 127 +++++++++- 4 files changed, 299 insertions(+), 69 deletions(-) diff --git a/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx b/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx index 99d8fc3d50210..290a9527aa805 100644 --- a/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx +++ b/docs-mintlify/admin/connect-to-data/oauth-authentication.mdx @@ -431,6 +431,15 @@ module.exports = { replaced on its own deadline; a factory that raises instead of falling back is given up after it has refused for long enough, so the error reaches the caller rather than 401s from a pool nobody rebuilds. +- **A `driver_factory` that reaches an external dependency should catch its + own failures.** Sustained refusal is read as the factory declining to serve + the connection: after several minutes of raising on every check, the pooled + connection is released and the error surfaces to the caller. That is the + intended behaviour for a credential that has genuinely stopped working, but + nothing here can tell it apart from a secret store that is briefly + unreachable. If yours fetches from Vault, a token endpoint or anything else + that can blink, catch the error and return the last known-good credential + (or the service account) rather than letting it propagate. - **Falling back is silent.** A missing or near-expired token sends the query to the service account instead of failing, so results reflect the service account's permissions rather than the user's. If that is not diff --git a/docs-mintlify/reference/configuration/config.mdx b/docs-mintlify/reference/configuration/config.mdx index ad10e186d5ecf..edd0d2346eb08 100644 --- a/docs-mintlify/reference/configuration/config.mdx +++ b/docs-mintlify/reference/configuration/config.mdx @@ -443,8 +443,14 @@ in the drivers' [source code][link-github-cube-drivers]. The optional `expiresAt` element states when the connection this configuration -describes stops being usable, as epoch milliseconds (or seconds), an ISO 8601 -string, or a date. It is not passed to the driver. +describes stops being usable, as epoch milliseconds (or seconds) or an ISO 8601 +string — and in JavaScript, a `Date`. In Python, pass `dt.timestamp()` or +`dt.isoformat()`: a `datetime` cannot cross the config bridge and raises +`Unable to represent PyDateTime type as CLR from Python`. It is not passed to +the driver. + +A deadline that has already passed is ignored, with a warning: replacing a +driver cannot move a deadline the factory keeps re-asserting. A driver is resolved once and then cached, and replaced when the configuration `driver_factory` returns changes. A connection built from a credential that diff --git a/packages/cubejs-server-core/src/core/server.ts b/packages/cubejs-server-core/src/core/server.ts index 3793a7945ba54..784abffe798cf 100644 --- a/packages/cubejs-server-core/src/core/server.ts +++ b/packages/cubejs-server-core/src/core/server.ts @@ -126,14 +126,23 @@ const DRIVER_REBUILD_MIN_INTERVAL_MS = 30 * 1000; const MAX_CONSECUTIVE_PROBE_FAILURES = 3; /** - * How long those failures must span before the driver is given up. + * How long those failures must span before the driver is given up, and how long + * one of them stays on the record. * * The count alone is not a duration: under load three concurrent probes can * fail inside the same blink of a dependency. Requiring both keeps a burst from * tearing down a working pool while still bounding how long a refusal can be * ignored. + * + * Minutes rather than seconds because a probe failure is not evidence about the + * cached connection — it is evidence about whatever the factory had to reach to + * answer. A secret store restarting, a token endpoint returning 503, a DNS blip + * inside the factory: in every one of those the cached credential is untouched + * and still valid, and giving the pool up makes a dependency's outage into a + * query outage. A credential that has genuinely stopped working is not urgent + * to the second, so the bar is set where a dependency can restart under it. */ -const PROBE_FAILURE_GRACE_MS = 30 * 1000; +const PROBE_FAILURE_GRACE_MS = 5 * 60 * 1000; /** * What a cached driver was built from. `null` on either fingerprint means @@ -146,10 +155,20 @@ type DriverOrigin = { expiresAt: number | undefined; }; -/** Consecutive probe failures for one alias set, and when they started. */ +/** + * Probe failures for one alias set inside one rolling window: how many, when + * the window opened, and when it was last extended. + * + * `lastFailureAt` is what makes the window rolling. Probes are only issued when + * the security context fingerprint changes, so in a quiet deployment two of + * them can be hours apart with nothing in between to clear the count — and + * three unrelated flakes on three different days are not a sustained refusal, + * however they look to a counter that only ever goes up. + */ type DriverProbeFailures = { count: number; firstFailureAt: number; + lastFailureAt: number; }; /** A `driverFactory` result together with the context that produced it. */ @@ -158,9 +177,19 @@ type DriverFactoryResult = { securityContextFingerprint: string | null; }; -/** Why a cached driver is being replaced, for the operator reading the log. */ +/** Why a cached driver was found stale, for the operator reading the log. */ type DriverStalenessReason = 'configuration change' | 'lifetime elapsed'; +/** + * Every reason a cached driver is replaced. A refusal is not a staleness + * verdict — the factory never produced a configuration to compare — but it + * tears down the same connection pool, so it is counted and rate-limited + * alongside the verdicts rather than slipping past both brakes. + */ +type DriverReplacementReason = + | DriverStalenessReason + | 'repeated staleness check failures'; + /** * The verdict on a cached driver. `factoryResult` is present only when the * probe already resolved one, so the rebuild does not call the factory twice; @@ -840,6 +869,60 @@ export class CubejsServerCore { */ const rebuildKey = aliasedKeys[0]; + /** + * Count a replacement against the alias set's rebuild history, open a + * suppression window on it, and report it. + * + * Both paths that tear a pool down come through here — a configuration + * the factory changed, and a factory that will no longer produce one. + * They cost the same thing, so they are bounded by the same state: a + * replacement that skipped this would rebuild straight past the interval + * that exists to stop pool churn, and never reach the diagnostic that + * names it. + */ + const recordDriverRebuild = (reason: DriverReplacementReason, warning: string) => { + // Re-read rather than reusing what was captured before the staleness + // probe awaited: reaching here means no concurrent rebuild landed, but + // the count is the one piece of state that would silently lose an + // increment if that ever stopped being true. + const state = driverRebuilds[rebuildKey] + || { count: 0, lastRebuildAt: 0, suppressionReported: false }; + + state.count += 1; + state.lastRebuildAt = Date.now(); + state.suppressionReported = false; + driverRebuilds[rebuildKey] = state; + + // Carries `warning` so it survives the default log level: a plain-params + // message matches no allowlist in `prodLogger`/`devLogger` and is + // dropped below `trace`. Tearing down a connection pool is an event an + // operator needs to be able to correlate against, and the threshold + // message below arrives too late to reconstruct the first rebuilds. + this.logger('Rebuilding driver', { + dataSource, + preAggregations, + rebuildCount: state.count, + reason, + warning, + }); + + // A credential rotation rebuilds a handful of times a day. Rebuilding + // this often means the orchestrator id does not partition by whatever + // the factory reads, so contexts that need different connections keep + // displacing each other's driver — or that the factory is not resolving + // reliably enough to keep any connection. + if (state.count === DRIVER_REBUILD_WARN_THRESHOLD) { + this.logger('Driver rebuilt repeatedly', { + dataSource, + rebuildCount: state.count, + warning: 'Driver keeps being replaced for one orchestrator. ' + + 'contextToOrchestratorId likely does not distinguish the contexts ' + + 'driverFactory returns different connections for, or driverFactory ' + + 'is not resolving a configuration reliably.', + }); + } + }; + // Already resolved by the staleness check below, so the factory is not // asked twice for the same rebuild. let resolvedFactoryResult: DriverFactoryResult | undefined; @@ -921,13 +1004,22 @@ export class CubejsServerCore { return cached; } - const failures = driverProbeFailures[rebuildKey] - || { count: 0, firstFailureAt: Date.now() }; + const now = Date.now(); + const previousFailures = driverProbeFailures[rebuildKey]; + + // A rolling window, not a running total. Probes are only issued when + // the context changes, so a record that is never re-based would add + // up occasional flakes weeks apart and read them as one outage. + const failures = previousFailures + && now - previousFailures.lastFailureAt < PROBE_FAILURE_GRACE_MS + ? previousFailures + : { count: 0, firstFailureAt: now, lastFailureAt: now }; failures.count += 1; + failures.lastFailureAt = now; driverProbeFailures[rebuildKey] = failures; - const failingForMs = Date.now() - failures.firstFailureAt; + const failingForMs = now - failures.firstFailureAt; // Transient, as far as anything here can tell. Reuse, exactly as // before this bound existed. @@ -938,17 +1030,15 @@ export class CubejsServerCore { return cached; } - this.logger('Releasing driver after repeated staleness check failures', { - dataSource, - preAggregations, - failureCount: failures.count, - warning: 'driverFactory has failed every staleness check for ' - + `${Math.round(failingForMs / 1000)}s. Releasing the connection it ` - + 'built rather than serving queries on a configuration it will no ' - + 'longer produce; the next request calls the factory itself, so a ' - + 'factory that fails closed on an unusable credential surfaces its ' - + 'own error.', - }); + recordDriverRebuild( + 'repeated staleness check failures', + `driverFactory has failed every staleness check for ${ + Math.round(failingForMs / 1000) + }s. Releasing the connection it built rather than serving queries on ` + + 'a configuration it will no longer produce; the next request calls ' + + 'the factory itself, so a factory that fails closed on an unusable ' + + 'credential surfaces its own error.', + ); delete driverProbeFailures[rebuildKey]; replaceCachedDriver(cached); @@ -959,48 +1049,10 @@ export class CubejsServerCore { // Opens a fresh suppression window, so the next configuration change // for this alias set waits it out rather than tearing down the pool // this rebuild is about to stand up. - // - // Re-read rather than reusing what was captured before the staleness - // probe awaited: reaching here means no concurrent rebuild landed, but - // the count is the one piece of state that would silently lose an - // increment if that ever stopped being true. - const state = driverRebuilds[rebuildKey] - || { count: 0, lastRebuildAt: 0, suppressionReported: false }; - - state.count += 1; - state.lastRebuildAt = Date.now(); - state.suppressionReported = false; - driverRebuilds[rebuildKey] = state; - - const rebuildCount = state.count; - - // Carries `warning` so it survives the default log level: a - // plain-params message matches no allowlist in - // `prodLogger`/`devLogger` and is dropped below `trace`. Tearing down - // a connection pool is an event an operator needs to be able to - // correlate against, and the threshold message below arrives too late - // to reconstruct the first rebuilds. - this.logger('Rebuilding driver', { - dataSource, - preAggregations, - rebuildCount, - reason: staleness.reason, - warning: `Replacing the connection — ${staleness.reason}.`, - }); - - // A credential rotation rebuilds a handful of times a day. Rebuilding - // this often means the orchestrator id does not partition by whatever - // the factory reads, so contexts that need different connections keep - // displacing each other's driver. - if (rebuildCount === DRIVER_REBUILD_WARN_THRESHOLD) { - this.logger('Driver rebuilt repeatedly', { - dataSource, - rebuildCount, - warning: 'Driver configuration keeps changing for one orchestrator. ' - + 'contextToOrchestratorId likely does not distinguish the contexts ' - + 'driverFactory returns different connections for.', - }); - } + recordDriverRebuild( + staleness.reason, + `Replacing the connection — ${staleness.reason}.`, + ); delete driverProbeFailures[rebuildKey]; replaceCachedDriver(cached); @@ -1049,7 +1101,7 @@ export class CubejsServerCore { ? driverConfigFingerprint(factoryConfig) : null; origin.expiresAt = factoryConfig - ? parseDriverExpiry(factoryConfig.expiresAt) + ? this.resolveBuiltDriverExpiry(factoryConfig, dataSource) : undefined; driver = await this.createDriverFromFactoryResult( @@ -1365,6 +1417,48 @@ export class CubejsServerCore { } } + /** + * The lifetime to hold a driver to, given the configuration it was just built + * from — and `undefined` where that configuration named a deadline that had + * already passed. + * + * Rebuilding cannot fix a deadline the factory keeps re-asserting. Honouring + * one would find the new driver stale the moment its suppression window + * closed, tear down a pool it had just stood up, and resolve the same elapsed + * deadline again, for the life of the process. A driver built from an expired + * configuration is no worse than the one it replaced, so the connection is + * kept and the lifetime dropped — the operator gets a warning naming the + * field rather than churn that never resolves. + * + * The documented recipe does not reach this: its `accessToken()` withholds a + * token that is already near expiry, so the configuration changes to the + * service account, which names no lifetime, and converges. A factory passing + * the provider's `accessTokenExpiresAt` straight through does reach it. + */ + protected resolveBuiltDriverExpiry( + config: DriverConfig, + dataSource: string, + ): number | undefined { + const expiresAt = parseDriverExpiry(config.expiresAt); + + if (expiresAt === undefined || Date.now() < expiresAt) { + return expiresAt; + } + + this.logger('Driver configuration expired on arrival', { + dataSource, + expiresAt: new Date(expiresAt).toISOString(), + warning: 'driverFactory returned a configuration whose expiresAt has ' + + 'already passed. Using the connection anyway and ignoring the ' + + 'lifetime: replacing a driver cannot move a deadline the factory ' + + 'keeps re-asserting, and honouring it would rebuild the pool for the ' + + 'life of the process. expiresAt must state when the credential being ' + + 'returned stops being usable, in the future.', + }); + + return undefined; + } + /** * Decide whether a cached driver still reflects what `driverFactory` would * resolve for the current request context. @@ -1471,9 +1565,11 @@ export class CubejsServerCore { // is excluded from the fingerprint, so a credential re-issued with the // same value and a later expiry compares equal. Carrying the new deadline // over is what keeps that from rebuilding on the old one, once per window, - // forever. + // forever. Guarded like the build path, because a factory re-asserting an + // elapsed deadline would otherwise reinstate it here on the next context + // change, reopening the loop that guard exists to close. if (config) { - origin.expiresAt = parseDriverExpiry(config.expiresAt); + origin.expiresAt = this.resolveBuiltDriverExpiry(config, context.dataSource); } return { stale: false }; diff --git a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts index 2e69a0955394c..78b38a2917e96 100644 --- a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts +++ b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts @@ -737,12 +737,14 @@ describe('driver cache invalidation', () => { await request({ token: 'token-b' }, 'req-2'); expect(await driverFactory('default')).toBe(first); + clock.advance(4 * 60 * 1000); await request({ token: 'token-c' }, 'req-3'); expect(await driverFactory('default')).toBe(first); - // Sustained past the grace window: the driver is given up, and the caller + // Sustained past the grace window — and with no gap long enough to have + // aged the earlier refusals out. The driver is given up, and the caller // sees the factory's own error rather than a connection it refused to build. - clock.advance(31 * 1000); + clock.advance(4 * 60 * 1000); await request({ token: 'token-d' }, 'req-4'); await expect(driverFactory('default')).rejects.toThrow('credential is unusable'); @@ -751,10 +753,11 @@ describe('driver cache invalidation', () => { expect((first).release).toHaveBeenCalled(); expect(core.builtDrivers).toHaveLength(1); - const released = logged('Releasing driver after repeated staleness check failures'); + const released = logged('Rebuilding driver') + .filter((params: any) => params.reason === 'repeated staleness check failures'); expect(released).toHaveLength(1); - expect(released[0]).toMatchObject({ dataSource: 'default', failureCount: 3 }); + expect(released[0]).toMatchObject({ dataSource: 'default', rebuildCount: 1 }); // And once the credential is usable again, the next request rebuilds. shouldFail = false; @@ -801,4 +804,120 @@ describe('driver cache invalidation', () => { expect(await driverFactory('default')).toBe(first); expect(core.builtDrivers).toHaveLength(1); }); + + // Probes only run when the security context changes, so in a quiet deployment + // they can be hours apart. A counter that only ever went up would read three + // unrelated flakes on three different days as one sustained outage and tear + // down a working pool for it. + test('refusals spread beyond the window do not accumulate', async () => { + let shouldFail = false; + const { core, driverFactory, request, logged } = await createCore({ + driverFactory: (ctx: any) => { + if (shouldFail) { + throw new Error('secret store unreachable'); + } + + return { type: 'postgres', password: ctx.securityContext.token }; + }, + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + shouldFail = true; + + // Well past the grace window between each, so every refusal opens a fresh + // window rather than extending the one before it. Six of them, twice the + // bound, and the driver is still there. + for (const token of ['token-b', 'token-c', 'token-d', 'token-e', 'token-f', 'token-g']) { + clock.advance(6 * 60 * 1000); + // eslint-disable-next-line no-await-in-loop + await request({ token }, `req-${token}`); + // eslint-disable-next-line no-await-in-loop + expect(await driverFactory('default')).toBe(first); + } + + expect(core.builtDrivers).toHaveLength(1); + expect((first).release).not.toHaveBeenCalled(); + expect(logged('Rebuilding driver')).toHaveLength(0); + }); + + // Giving a driver up tears down the same pool a configuration change does, so + // it has to answer to the same rate limit. Bypassing it let a factory that + // fails probes but succeeds when called directly churn a pool every window, + // with nothing in the logs that read as churn. + test('rate-limits repeated give-ups', async () => { + let shouldFailProbe = false; + const { core, driverFactory, request, logged } = await createCore({ + driverFactory: (ctx: any) => { + if (shouldFailProbe) { + throw new Error('secret store unreachable'); + } + + return { type: 'postgres', password: ctx.securityContext.token }; + }, + }, { token: 'token-a' }); + + await driverFactory('default'); + + // Three refusals spanning the grace window give the driver up. The build + // that follows succeeds, because it calls the factory directly. + shouldFailProbe = true; + + for (const token of ['token-b', 'token-c']) { + // eslint-disable-next-line no-await-in-loop + await request({ token }, `req-${token}`); + // eslint-disable-next-line no-await-in-loop + await driverFactory('default'); + clock.advance(4 * 60 * 1000); + } + + await request({ token: 'token-d' }, 'req-d'); + shouldFailProbe = false; + + const rebuilt = await driverFactory('default'); + + expect(logged('Rebuilding driver')).toHaveLength(1); + expect(core.builtDrivers).toHaveLength(2); + + // The give-up opened a suppression window like any other replacement, so + // the next changed context reuses rather than tearing the new pool down. + await request({ token: 'token-e' }, 'req-e'); + + expect(await driverFactory('default')).toBe(rebuilt); + expect(core.builtDrivers).toHaveLength(2); + expect(logged('Driver rebuild suppressed')).toHaveLength(1); + }); + + // Replacing a driver cannot move a deadline the factory keeps re-asserting. + // Honouring one would find the new driver stale the moment its suppression + // window closed, for the life of the process. + test('ignores a lifetime that has already elapsed', async () => { + const { core, driverFactory, request, logged } = await createCore({ + driverFactory: (ctx: any) => ({ + type: 'postgres', + password: ctx.securityContext.token, + // A factory passing the provider's expiry straight through, on a + // credential whose refresh has already stopped. + expiresAt: Date.now() - 60 * 1000, + }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + expect(logged('Driver configuration expired on arrival')).toHaveLength(1); + + // Past any suppression window: an honoured deadline would rebuild here, and + // again on every window after it. + clock.advancePastRebuildInterval(); + await request({ token: 'token-a' }, 'req-2'); + + expect(await driverFactory('default')).toBe(first); + + clock.advancePastRebuildInterval(); + await request({ token: 'token-a' }, 'req-3'); + + expect(await driverFactory('default')).toBe(first); + expect(core.builtDrivers).toHaveLength(1); + expect(logged('Rebuilding driver')).toHaveLength(0); + }); }); From dc9e97b8f3186a6667e8efba4ef9ebc360421d38 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 20:28:33 +0000 Subject: [PATCH 12/16] fix(server-core): bound the ignored-lifetime warning, and the lifetime itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two low findings from the re-review of the elapsed-expiresAt guard. The warning was emitted per call, and the carry-over that resolves a deadline runs on every security context change with no rate limit of its own — so the pool churn the guard removed came back as log volume on the hot path for the same misconfiguration. Reported once per driver now, via the origin the alias set shares. The guard also caught only a deadline that had already passed, not one too short to be honoured. A lifetime shorter than the interval replacements are rate-limited to leaves the driver stale the moment its suppression window closes, which is the same loop with a different cause — a 60s STS credential would tear the pool down every 30s for the life of the process. Both are now treated as no lifetime, with the warning distinguishing them. Dropping a short lifetime strands nothing: a credential rotating that fast is picked up by its configuration changing, which is compared on every probe. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015R8SandPBdggffPL7WfwB7 --- .../reference/configuration/config.mdx | 6 +- .../cubejs-server-core/src/core/server.ts | 76 +++++++++++++----- .../unit/driver-cache-invalidation.test.ts | 80 ++++++++++++++++++- 3 files changed, 140 insertions(+), 22 deletions(-) diff --git a/docs-mintlify/reference/configuration/config.mdx b/docs-mintlify/reference/configuration/config.mdx index edd0d2346eb08..a2e2f98cd794c 100644 --- a/docs-mintlify/reference/configuration/config.mdx +++ b/docs-mintlify/reference/configuration/config.mdx @@ -450,7 +450,11 @@ string — and in JavaScript, a `Date`. In Python, pass `dt.timestamp()` or the driver. A deadline that has already passed is ignored, with a warning: replacing a -driver cannot move a deadline the factory keeps re-asserting. +driver cannot move a deadline the factory keeps re-asserting. So is one less +than 30 seconds away — replacements are rate-limited to that interval, so a +shorter lifetime could only ever be honoured by replacing the connection on +every window. A credential rotating that fast is picked up by its configuration +changing instead. A driver is resolved once and then cached, and replaced when the configuration `driver_factory` returns changes. A connection built from a credential that diff --git a/packages/cubejs-server-core/src/core/server.ts b/packages/cubejs-server-core/src/core/server.ts index 784abffe798cf..4ff29a10a0a93 100644 --- a/packages/cubejs-server-core/src/core/server.ts +++ b/packages/cubejs-server-core/src/core/server.ts @@ -153,6 +153,13 @@ type DriverOrigin = { securityContextFingerprint: string | null; configFingerprint: string | null; expiresAt: number | undefined; + /** + * Whether this driver's unusable lifetime has been reported. The carry-over + * that resolves it runs on every security context change, with no rate limit + * of its own, so without this the warning is emitted per request — trading + * the pool churn this guard removes for log volume on the hot path. + */ + lifetimeIgnoredReported: boolean; }; /** @@ -1076,6 +1083,7 @@ export class CubejsServerCore { securityContextFingerprint: null, configFingerprint: null, expiresAt: undefined, + lifetimeIgnoredReported: false, }; aliasedKeys.forEach((key) => { @@ -1101,7 +1109,7 @@ export class CubejsServerCore { ? driverConfigFingerprint(factoryConfig) : null; origin.expiresAt = factoryConfig - ? this.resolveBuiltDriverExpiry(factoryConfig, dataSource) + ? this.resolveBuiltDriverExpiry(factoryConfig, dataSource, origin) : undefined; driver = await this.createDriverFromFactoryResult( @@ -1424,37 +1432,65 @@ export class CubejsServerCore { * * Rebuilding cannot fix a deadline the factory keeps re-asserting. Honouring * one would find the new driver stale the moment its suppression window - * closed, tear down a pool it had just stood up, and resolve the same elapsed - * deadline again, for the life of the process. A driver built from an expired - * configuration is no worse than the one it replaced, so the connection is - * kept and the lifetime dropped — the operator gets a warning naming the - * field rather than churn that never resolves. + * closed, tear down a pool it had just stood up, and resolve the same + * unusable deadline again, for the life of the process. A driver built from + * such a configuration is no worse than the one it replaced, so the + * connection is kept and the lifetime dropped — the operator gets a warning + * naming the field rather than churn that never resolves. + * + * Two deadlines are unusable, and they produce the same loop. One has already + * passed. The other is shorter than the interval replacements are rate-limited + * to: the driver is stale again the moment its suppression window closes, so + * the rate limiter can never let this mechanism honour it. Dropping it strands + * nothing — a credential that short is rotating, and rotation changes the + * configuration, which is caught by comparison rather than by lifetime. * - * The documented recipe does not reach this: its `accessToken()` withholds a + * The documented recipe does not reach either: its `accessToken()` withholds a * token that is already near expiry, so the configuration changes to the * service account, which names no lifetime, and converges. A factory passing - * the provider's `accessTokenExpiresAt` straight through does reach it. + * the provider's `accessTokenExpiresAt` straight through does reach them. */ protected resolveBuiltDriverExpiry( config: DriverConfig, dataSource: string, + origin: DriverOrigin, ): number | undefined { const expiresAt = parseDriverExpiry(config.expiresAt); - if (expiresAt === undefined || Date.now() < expiresAt) { + if (expiresAt === undefined) { + return undefined; + } + + const remainingMs = expiresAt - Date.now(); + + if (remainingMs >= DRIVER_REBUILD_MIN_INTERVAL_MS) { return expiresAt; } - this.logger('Driver configuration expired on arrival', { - dataSource, - expiresAt: new Date(expiresAt).toISOString(), - warning: 'driverFactory returned a configuration whose expiresAt has ' - + 'already passed. Using the connection anyway and ignoring the ' - + 'lifetime: replacing a driver cannot move a deadline the factory ' - + 'keeps re-asserting, and honouring it would rebuild the pool for the ' - + 'life of the process. expiresAt must state when the credential being ' - + 'returned stops being usable, in the future.', - }); + // Once per driver, not once per call: the carry-over path resolves this on + // every security context change, and the operator needs the field named + // once, not on every query that arrives with a fresh JWT. + if (!origin.lifetimeIgnoredReported) { + origin.lifetimeIgnoredReported = true; + + this.logger('Driver lifetime ignored', { + dataSource, + expiresAt: new Date(expiresAt).toISOString(), + warning: remainingMs < 0 + ? 'driverFactory returned a configuration whose expiresAt has already ' + + 'passed. Using the connection anyway and ignoring the lifetime: ' + + 'replacing a driver cannot move a deadline the factory keeps ' + + 're-asserting, and honouring it would rebuild the pool for the life ' + + 'of the process. expiresAt must state when the credential being ' + + 'returned stops being usable, in the future.' + : 'driverFactory returned a configuration whose expiresAt is less than ' + + `${DRIVER_REBUILD_MIN_INTERVAL_MS / 1000}s away, which is shorter ` + + 'than the interval replacements are rate-limited to. Honouring it ' + + 'would replace the connection once per window for the life of the ' + + 'process, so the lifetime is ignored; a credential rotating that ' + + 'fast is picked up by its configuration changing instead.', + }); + } return undefined; } @@ -1569,7 +1605,7 @@ export class CubejsServerCore { // elapsed deadline would otherwise reinstate it here on the next context // change, reopening the loop that guard exists to close. if (config) { - origin.expiresAt = this.resolveBuiltDriverExpiry(config, context.dataSource); + origin.expiresAt = this.resolveBuiltDriverExpiry(config, context.dataSource, origin); } return { stale: false }; diff --git a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts index 78b38a2917e96..ed4d22eb18e47 100644 --- a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts +++ b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts @@ -904,7 +904,7 @@ describe('driver cache invalidation', () => { const first = await driverFactory('default'); - expect(logged('Driver configuration expired on arrival')).toHaveLength(1); + expect(logged('Driver lifetime ignored')).toHaveLength(1); // Past any suppression window: an honoured deadline would rebuild here, and // again on every window after it. @@ -920,4 +920,82 @@ describe('driver cache invalidation', () => { expect(core.builtDrivers).toHaveLength(1); expect(logged('Rebuilding driver')).toHaveLength(0); }); + + // The deadline is resolved again on every probe, and probes run whenever the + // security context changes. Reporting per call would trade the churn the + // guard removes for a warning on every query that arrives with a fresh JWT. + test('reports an ignored lifetime once, not once per probe', async () => { + const { core, driverFactory, request, logged } = await createCore({ + // Ignores the context: the configuration never changes, only the elapsed + // deadline it keeps re-asserting. + driverFactory: () => ({ + type: 'postgres', + password: 'service-account', + expiresAt: Date.now() - 60 * 1000, + }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + // Each of these changes the fingerprint, so each one probes and each one + // carries the same unusable deadline over. + for (const token of ['token-b', 'token-c', 'token-d', 'token-e']) { + // eslint-disable-next-line no-await-in-loop + await request({ token }, `req-${token}`); + // eslint-disable-next-line no-await-in-loop + expect(await driverFactory('default')).toBe(first); + } + + expect(logged('Driver lifetime ignored')).toHaveLength(1); + expect(core.builtDrivers).toHaveLength(1); + }); + + // A lifetime shorter than the replacement interval is the same loop as an + // elapsed one: stale the moment the suppression window closes, every window, + // for the life of the process. + test('ignores a lifetime shorter than the replacement interval', async () => { + const { core, driverFactory, request, logged } = await createCore({ + driverFactory: () => ({ + type: 'postgres', + password: 'sts-credential', + // Genuinely in the future, and still too short for the rate limiter to + // ever let this mechanism act on it. + expiresAt: Date.now() + 10 * 1000, + }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + expect(logged('Driver lifetime ignored')).toHaveLength(1); + + // Past the deadline and past any suppression window, twice over. + for (const requestId of ['req-2', 'req-3']) { + clock.advancePastRebuildInterval(); + // eslint-disable-next-line no-await-in-loop + await request({ token: 'token-a' }, requestId); + // eslint-disable-next-line no-await-in-loop + expect(await driverFactory('default')).toBe(first); + } + + expect(core.builtDrivers).toHaveLength(1); + expect(logged('Rebuilding driver')).toHaveLength(0); + }); + + // The other half of the lifetime contract: a deadline the rate limiter can + // honour is still honoured, so widening the guard did not disable the feature. + test('honours a lifetime longer than the replacement interval', async () => { + let expiresAt = Date.now() + 60 * 60 * 1000; + const { core, driverFactory, request } = await createCore({ + driverFactory: () => ({ type: 'postgres', password: 'static', expiresAt }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + clock.advance(61 * 60 * 1000); + expiresAt = Date.now() + 60 * 60 * 1000; + await request({ token: 'token-a' }, 'req-2'); + + expect(await driverFactory('default')).not.toBe(first); + expect(core.builtDrivers).toHaveLength(2); + }); }); From e0998859ee3e70d9cf4e6014ae9914eda30127be Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 20:35:47 +0000 Subject: [PATCH 13/16] fix(server-core): judge a driver lifetime when stated, not as it ages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lower bound added in dc9e97b ran on every re-read, but the carry-over that re-resolves a deadline runs on every security context change — so `remainingMs` there measured how much of an accepted lifetime was left, not anything the factory had stated. A legitimate one-hour deadline was therefore discarded as soon as a probe landed in its final 30 seconds, with a warning claiming the factory had returned a sub-interval one, and the driver was left with no deadline at all for the stretch the lifetime exists to cover. A deadline equal to the one already installed is returned untouched: it was judged when it arrived, and time passing is not the factory re-asserting it. An exactly-elapsed deadline now reports as elapsed rather than as too short. Separately, retention is now its own constant. Reusing the grace window for both how long refusals must span and how long one stays on the record made the bound unreachable in a deployment that probes less often than the window: it reset to one every time and never gave up a dead credential. Thirty minutes of retention against a five-minute span lets a slow but sustained refusal reach the bound while still forgetting genuinely isolated flakes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015R8SandPBdggffPL7WfwB7 --- .../reference/configuration/config.mdx | 11 +-- .../cubejs-server-core/src/core/server.ts | 47 ++++++++--- .../unit/driver-cache-invalidation.test.ts | 78 ++++++++++++++++++- 3 files changed, 118 insertions(+), 18 deletions(-) diff --git a/docs-mintlify/reference/configuration/config.mdx b/docs-mintlify/reference/configuration/config.mdx index a2e2f98cd794c..68be24f52284f 100644 --- a/docs-mintlify/reference/configuration/config.mdx +++ b/docs-mintlify/reference/configuration/config.mdx @@ -450,11 +450,12 @@ string — and in JavaScript, a `Date`. In Python, pass `dt.timestamp()` or the driver. A deadline that has already passed is ignored, with a warning: replacing a -driver cannot move a deadline the factory keeps re-asserting. So is one less -than 30 seconds away — replacements are rate-limited to that interval, so a -shorter lifetime could only ever be honoured by replacing the connection on -every window. A credential rotating that fast is picked up by its configuration -changing instead. +driver cannot move a deadline the factory keeps re-asserting. So is one that +falls inside the interval replacements are rate-limited to, since a lifetime +that short could only ever be honoured by replacing the connection on every +window. A credential rotating that fast is picked up by its configuration +changing instead. Both are judged when `driver_factory` states the deadline, so +a lifetime that was accepted is held to whatever it said. A driver is resolved once and then cached, and replaced when the configuration `driver_factory` returns changes. A connection built from a credential that diff --git a/packages/cubejs-server-core/src/core/server.ts b/packages/cubejs-server-core/src/core/server.ts index 4ff29a10a0a93..1048cf14a62ee 100644 --- a/packages/cubejs-server-core/src/core/server.ts +++ b/packages/cubejs-server-core/src/core/server.ts @@ -126,8 +126,7 @@ const DRIVER_REBUILD_MIN_INTERVAL_MS = 30 * 1000; const MAX_CONSECUTIVE_PROBE_FAILURES = 3; /** - * How long those failures must span before the driver is given up, and how long - * one of them stays on the record. + * How long those failures must span before the driver is given up. * * The count alone is not a duration: under load three concurrent probes can * fail inside the same blink of a dependency. Requiring both keeps a burst from @@ -144,6 +143,23 @@ const MAX_CONSECUTIVE_PROBE_FAILURES = 3; */ const PROBE_FAILURE_GRACE_MS = 5 * 60 * 1000; +/** + * How long one refusal stays on the record before it is forgotten. + * + * Deliberately separate from the grace window, because the two pull opposite + * ways. The grace window wants to be long, so a dependency can restart under + * it. Retention wants to be long enough that a *sparse* deployment can still + * reach the bound: probes are only issued when the security context fingerprint + * changes, so a few-user deployment may probe once every several minutes, and + * if a refusal expired at the grace window such a deployment would reset to one + * every time and never give up a credential however permanently dead it was. + * + * Longer than the grace window, then, but far short of the days-apart flakes + * that made a never-expiring record wrong: a refusal half an hour stale is not + * evidence about the one happening now. + */ +const PROBE_FAILURE_RETENTION_MS = 30 * 60 * 1000; + /** * What a cached driver was built from. `null` on either fingerprint means * "cannot tell whether it changed", which is always read as "assume it did @@ -166,11 +182,12 @@ type DriverOrigin = { * Probe failures for one alias set inside one rolling window: how many, when * the window opened, and when it was last extended. * - * `lastFailureAt` is what makes the window rolling. Probes are only issued when - * the security context fingerprint changes, so in a quiet deployment two of - * them can be hours apart with nothing in between to clear the count — and - * three unrelated flakes on three different days are not a sustained refusal, - * however they look to a counter that only ever goes up. + * `lastFailureAt` is what makes the window rolling, against + * `PROBE_FAILURE_RETENTION_MS`. Probes are only issued when the security + * context fingerprint changes, so in a quiet deployment two of them can be + * hours apart with nothing in between to clear the count — and three unrelated + * flakes on three different days are not a sustained refusal, however they look + * to a counter that only ever goes up. */ type DriverProbeFailures = { count: number; @@ -1017,8 +1034,10 @@ export class CubejsServerCore { // A rolling window, not a running total. Probes are only issued when // the context changes, so a record that is never re-based would add // up occasional flakes weeks apart and read them as one outage. + // Retention rather than the grace window, so that a deployment + // probing less often than the grace window can still reach the bound. const failures = previousFailures - && now - previousFailures.lastFailureAt < PROBE_FAILURE_GRACE_MS + && now - previousFailures.lastFailureAt < PROBE_FAILURE_RETENTION_MS ? previousFailures : { count: 0, firstFailureAt: now, lastFailureAt: now }; @@ -1461,6 +1480,16 @@ export class CubejsServerCore { return undefined; } + // Already judged when it was installed. This also runs on every probe that + // carries an unchanged configuration over, where what is left of the + // deadline is a measure of time passing rather than of anything the factory + // stated. Measuring it there would drop a perfectly good deadline once it + // entered its final window — and leave the driver with no lifetime at all, + // in precisely the stretch the lifetime exists to cover. + if (expiresAt === origin.expiresAt) { + return expiresAt; + } + const remainingMs = expiresAt - Date.now(); if (remainingMs >= DRIVER_REBUILD_MIN_INTERVAL_MS) { @@ -1476,7 +1505,7 @@ export class CubejsServerCore { this.logger('Driver lifetime ignored', { dataSource, expiresAt: new Date(expiresAt).toISOString(), - warning: remainingMs < 0 + warning: remainingMs <= 0 ? 'driverFactory returned a configuration whose expiresAt has already ' + 'passed. Using the connection anyway and ignoring the lifetime: ' + 'replacing a driver cannot move a deadline the factory keeps ' diff --git a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts index ed4d22eb18e47..d38498aa64fe9 100644 --- a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts +++ b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts @@ -825,11 +825,11 @@ describe('driver cache invalidation', () => { shouldFail = true; - // Well past the grace window between each, so every refusal opens a fresh - // window rather than extending the one before it. Six of them, twice the - // bound, and the driver is still there. + // Past retention between each, so every refusal opens a fresh window rather + // than extending the one before it. Six of them, twice the bound, and the + // driver is still there. for (const token of ['token-b', 'token-c', 'token-d', 'token-e', 'token-f', 'token-g']) { - clock.advance(6 * 60 * 1000); + clock.advance(31 * 60 * 1000); // eslint-disable-next-line no-await-in-loop await request({ token }, `req-${token}`); // eslint-disable-next-line no-await-in-loop @@ -888,6 +888,46 @@ describe('driver cache invalidation', () => { expect(logged('Driver rebuild suppressed')).toHaveLength(1); }); + // The other half of that contract. Probes only fire when the security context + // changes, so a few-user deployment may probe far slower than the grace + // window — and a dead credential there has to be given up eventually, which + // is what retention being longer than the grace window buys. + test('gives up a refusal that is sustained but slow', async () => { + let shouldFail = false; + const { driverFactory, request, logged } = await createCore({ + driverFactory: (ctx: any) => { + if (shouldFail) { + throw new Error('credential is unusable'); + } + + return { type: 'postgres', password: ctx.securityContext.token }; + }, + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + shouldFail = true; + + // Ten minutes apart: slower than the grace window, well inside retention. + for (const token of ['token-b', 'token-c']) { + // eslint-disable-next-line no-await-in-loop + await request({ token }, `req-${token}`); + // eslint-disable-next-line no-await-in-loop + expect(await driverFactory('default')).toBe(first); + clock.advance(10 * 60 * 1000); + } + + await request({ token: 'token-d' }, 'req-d'); + + await expect(driverFactory('default')).rejects.toThrow('credential is unusable'); + + await new Promise(process.nextTick); + expect((first).release).toHaveBeenCalled(); + expect(logged('Rebuilding driver') + .filter((params: any) => params.reason === 'repeated staleness check failures')) + .toHaveLength(1); + }); + // Replacing a driver cannot move a deadline the factory keeps re-asserting. // Honouring one would find the new driver stale the moment its suppression // window closed, for the life of the process. @@ -981,6 +1021,36 @@ describe('driver cache invalidation', () => { expect(logged('Rebuilding driver')).toHaveLength(0); }); + // The lower bound judges what the factory states, not how much of an accepted + // deadline is left. Re-judging on the carry-over would drop a good deadline as + // it entered its final window, switching the lifetime off in exactly the + // stretch it exists to cover. + test('keeps a deadline that a probe lands inside the final window of', async () => { + const expiresAt = Date.now() + 60 * 60 * 1000; + const { core, driverFactory, request, logged } = await createCore({ + // Ignores the context, so the configuration compares equal and the probe + // takes the carry-over path. + driverFactory: () => ({ type: 'postgres', password: 'static', expiresAt }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + // A re-issued JWT ten seconds before the deadline: inside the replacement + // interval, but this deadline was judged an hour ago and accepted. + clock.advance(60 * 60 * 1000 - 10 * 1000); + await request({ token: 'token-b' }, 'req-2'); + + expect(await driverFactory('default')).toBe(first); + expect(logged('Driver lifetime ignored')).toHaveLength(0); + + // And the deadline it kept still fires. + clock.advance(11 * 1000); + await request({ token: 'token-c' }, 'req-3'); + + expect(await driverFactory('default')).not.toBe(first); + expect(core.builtDrivers).toHaveLength(2); + }); + // The other half of the lifetime contract: a deadline the rate limiter can // honour is still honoured, so widening the guard did not disable the feature. test('honours a lifetime longer than the replacement interval', async () => { From aea10a55265d4630ec4571eb6f84ed4dca94d568 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 20:42:20 +0000 Subject: [PATCH 14/16] fix(server-core): keep an accepted lifetime, and count refusal incidents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two residual edges of the previous round. The stated-not-aged rule stopped the bound consuming a deadline it had already accepted, but only where the factory re-stated that deadline exactly. Since expiresAt is excluded from the configuration fingerprint, an unchanged credential can arrive with a moved one — and if that new deadline could not be honoured, the driver was left with no deadline at all, which is the failure the rule exists to prevent, reached through a narrower door. The drop path now falls back to whatever was accepted. An installed deadline is still in the future there, because an elapsed one is caught before the factory is asked. Splitting retention out of the grace window also made the give-up bound reachable across unrelated incidents: a burst of concurrent refusals during one 60s outage, plus a single refusal minutes later, satisfied both the count and the span. The count is now of incidents rather than of refusals — probes that fail together on one blink of a dependency count once — which keeps the sparse deployment the split was for without letting two brief outages drain a pool. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015R8SandPBdggffPL7WfwB7 --- .../cubejs-server-core/src/core/server.ts | 37 +++++++++- .../unit/driver-cache-invalidation.test.ts | 68 +++++++++++++++++++ 2 files changed, 103 insertions(+), 2 deletions(-) diff --git a/packages/cubejs-server-core/src/core/server.ts b/packages/cubejs-server-core/src/core/server.ts index 1048cf14a62ee..cbb4f555f2899 100644 --- a/packages/cubejs-server-core/src/core/server.ts +++ b/packages/cubejs-server-core/src/core/server.ts @@ -157,9 +157,29 @@ const PROBE_FAILURE_GRACE_MS = 5 * 60 * 1000; * Longer than the grace window, then, but far short of the days-apart flakes * that made a never-expiring record wrong: a refusal half an hour stale is not * evidence about the one happening now. + * + * Retention this long would, on its own, let two unrelated blinks half an hour + * apart reach the bound between them. What keeps that from happening is that + * the count is of incidents rather than of refusals — see + * `PROBE_FAILURE_COALESCE_MS`. */ const PROBE_FAILURE_RETENTION_MS = 30 * 60 * 1000; +/** + * How close together two refusals have to be to count as one. + * + * Retention outliving the grace window is what makes the bound reachable in a + * sparse deployment, but on its own it also makes it reachable across unrelated + * incidents: concurrent probes all fail on one blink of a dependency, and a + * burst of three plus a single refusal six minutes later would otherwise + * satisfy both conditions and drain a working pool for what was two brief + * outages. + * + * Counting incidents rather than refusals removes that without giving the + * sparse case back: a burst is one, and the bound still wants three. + */ +const PROBE_FAILURE_COALESCE_MS = 2 * 1000; + /** * What a cached driver was built from. `null` on either fingerprint means * "cannot tell whether it changed", which is always read as "assume it did @@ -1041,7 +1061,15 @@ export class CubejsServerCore { ? previousFailures : { count: 0, firstFailureAt: now, lastFailureAt: now }; - failures.count += 1; + // Requests that arrived together and failed on the same blink of a + // dependency are one refusal, not one each. + if ( + failures.count === 0 || + now - failures.lastFailureAt >= PROBE_FAILURE_COALESCE_MS + ) { + failures.count += 1; + } + failures.lastFailureAt = now; driverProbeFailures[rebuildKey] = failures; @@ -1521,7 +1549,12 @@ export class CubejsServerCore { }); } - return undefined; + // Keep whatever was accepted, if anything. The newly stated deadline cannot + // be honoured, but one this driver is already held to can: an installed + // deadline is still in the future here, because an elapsed one returns + // `stale` from the lifetime check before the factory is ever asked. At the + // build path this is `undefined`, so nothing changes there. + return origin.expiresAt; } /** diff --git a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts index d38498aa64fe9..df6b238c900fb 100644 --- a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts +++ b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts @@ -928,6 +928,44 @@ describe('driver cache invalidation', () => { .toHaveLength(1); }); + // Retention outliving the grace window must not make the bound reachable + // across unrelated incidents. Concurrent probes all fail on one blink of a + // dependency, so a burst is one refusal — otherwise two brief outages half an + // hour apart would drain a working pool. + test('counts a burst of refusals as one incident', async () => { + let shouldFail = false; + const { core, driverFactory, request } = await createCore({ + driverFactory: (ctx: any) => { + if (shouldFail) { + throw new Error('secret store unreachable'); + } + + return { type: 'postgres', password: ctx.securityContext.token }; + }, + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + shouldFail = true; + + // Three refusals at the same instant: one dependency blink under load. + for (const token of ['token-b', 'token-c', 'token-d']) { + // eslint-disable-next-line no-await-in-loop + await request({ token }, `req-${token}`); + // eslint-disable-next-line no-await-in-loop + expect(await driverFactory('default')).toBe(first); + } + + // A second, unrelated blink six minutes later. Two incidents, not four + // refusals, so the bound is not reached and the pool survives. + clock.advance(6 * 60 * 1000); + await request({ token: 'token-e' }, 'req-e'); + + expect(await driverFactory('default')).toBe(first); + expect((first).release).not.toHaveBeenCalled(); + expect(core.builtDrivers).toHaveLength(1); + }); + // Replacing a driver cannot move a deadline the factory keeps re-asserting. // Honouring one would find the new driver stale the moment its suppression // window closed, for the life of the process. @@ -1051,6 +1089,36 @@ describe('driver cache invalidation', () => { expect(core.builtDrivers).toHaveLength(2); }); + // A deadline is excluded from the fingerprint, so an unchanged credential can + // arrive with a moved one. If that new deadline cannot be honoured, the one + // this driver was already held to still can — dropping to no deadline at all + // is the failure the stated-not-aged rule exists to prevent. + test('keeps the accepted deadline when a newly stated one cannot be honoured', async () => { + const accepted = Date.now() + 60 * 60 * 1000; + let expiresAt = accepted; + const { core, driverFactory, request } = await createCore({ + driverFactory: () => ({ type: 'postgres', password: 'static', expiresAt }), + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + // Ten seconds before the accepted deadline, the factory re-states the same + // credential with a slightly different deadline — inside the interval, so + // unhonourable as a new lifetime. + clock.advance(60 * 60 * 1000 - 10 * 1000); + expiresAt = accepted + 5 * 1000; + await request({ token: 'token-b' }, 'req-2'); + + expect(await driverFactory('default')).toBe(first); + + // The accepted deadline still fires. + clock.advance(11 * 1000); + await request({ token: 'token-c' }, 'req-3'); + + expect(await driverFactory('default')).not.toBe(first); + expect(core.builtDrivers).toHaveLength(2); + }); + // The other half of the lifetime contract: a deadline the rate limiter can // honour is still honoured, so widening the guard did not disable the feature. test('honours a lifetime longer than the replacement interval', async () => { From 44fd85bda061a737de7ce5f4383ed9686be6fc1d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 20:48:12 +0000 Subject: [PATCH 15/16] fix(server-core): bound a refusal incident by its duration, not only its count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Counting incidents rather than refusals closed the two-blinks case but opened the opposite one. The coalescing gap is measured against the last refusal seen, so a stream arriving faster than the window is one incident that never ends: `count` stayed at 1 however long the credential was dead. Any deployment serving more than about one request every two seconds across distinct users has that shape, so the give-up stopped deploying under exactly the load where a dead pool costs most. Advancing the gap only on a counted refusal would have been the wrong fix — a single 60s outage under load would then count as thirty incidents, which is the case coalescing was added to prevent. What distinguishes them is how long one incident has been running: a refusal unbroken for the grace window is sustained by any reading, while two 60s blinks are not. Both shapes now give the driver up, and neither can be reached by the other's traffic profile. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015R8SandPBdggffPL7WfwB7 --- .../cubejs-server-core/src/core/server.ts | 37 ++++++++++++--- .../unit/driver-cache-invalidation.test.ts | 46 +++++++++++++++++++ 2 files changed, 76 insertions(+), 7 deletions(-) diff --git a/packages/cubejs-server-core/src/core/server.ts b/packages/cubejs-server-core/src/core/server.ts index cbb4f555f2899..89d999e6bb0ea 100644 --- a/packages/cubejs-server-core/src/core/server.ts +++ b/packages/cubejs-server-core/src/core/server.ts @@ -176,7 +176,10 @@ const PROBE_FAILURE_RETENTION_MS = 30 * 60 * 1000; * outages. * * Counting incidents rather than refusals removes that without giving the - * sparse case back: a burst is one, and the bound still wants three. + * sparse case back: a burst is one, and the bound still wants three. An + * incident is bounded by its own duration as well as by the count, so a + * refusal stream arriving faster than this window is caught by having lasted + * rather than by being counted. */ const PROBE_FAILURE_COALESCE_MS = 2 * 1000; @@ -213,6 +216,13 @@ type DriverProbeFailures = { count: number; firstFailureAt: number; lastFailureAt: number; + /** + * When the incident being counted began — the first refusal of the current + * unbroken run, rather than of the window. An incident that has itself lasted + * the grace window is no longer a blink, which is what keeps a continuous + * stream of refusals from coalescing into one uncountable incident forever. + */ + incidentStartedAt: number; }; /** A `driverFactory` result together with the context that produced it. */ @@ -1059,15 +1069,21 @@ export class CubejsServerCore { const failures = previousFailures && now - previousFailures.lastFailureAt < PROBE_FAILURE_RETENTION_MS ? previousFailures - : { count: 0, firstFailureAt: now, lastFailureAt: now }; + : { + count: 0, firstFailureAt: now, lastFailureAt: now, incidentStartedAt: now, + }; // Requests that arrived together and failed on the same blink of a - // dependency are one refusal, not one each. + // dependency are one refusal, not one each. The gap is measured + // against the last refusal seen rather than the last one counted, so + // that a continuous stream stays one incident — which is only sound + // because an incident is also bounded by its own duration below. if ( failures.count === 0 || now - failures.lastFailureAt >= PROBE_FAILURE_COALESCE_MS ) { failures.count += 1; + failures.incidentStartedAt = now; } failures.lastFailureAt = now; @@ -1075,12 +1091,19 @@ export class CubejsServerCore { const failingForMs = now - failures.firstFailureAt; + // Two shapes of sustained refusal, because either alone leaves a + // traffic profile uncovered. Repeated incidents catch a deployment + // whose probes are sparse enough that each refusal stands alone; one + // unbroken incident catches a busy deployment, where refusals arrive + // faster than the coalescing window and would otherwise count once + // however long the credential stayed dead. + const sustainedIncident = now - failures.incidentStartedAt >= PROBE_FAILURE_GRACE_MS; + const repeatedIncidents = failures.count >= MAX_CONSECUTIVE_PROBE_FAILURES + && failingForMs >= PROBE_FAILURE_GRACE_MS; + // Transient, as far as anything here can tell. Reuse, exactly as // before this bound existed. - if ( - failures.count < MAX_CONSECUTIVE_PROBE_FAILURES || - failingForMs < PROBE_FAILURE_GRACE_MS - ) { + if (!sustainedIncident && !repeatedIncidents) { return cached; } diff --git a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts index df6b238c900fb..6dee19ebba2e4 100644 --- a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts +++ b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts @@ -966,6 +966,52 @@ describe('driver cache invalidation', () => { expect(core.builtDrivers).toHaveLength(1); }); + // The other end of the traffic range from the burst case. Under steady load + // refusals arrive faster than the coalescing window, so they are all one + // incident — and an incident that never ends must be caught by having lasted, + // or a permanently dead credential is never given up where it costs most. + test('gives up an unbroken refusal stream under steady traffic', async () => { + let shouldFail = false; + const { driverFactory, request } = await createCore({ + driverFactory: (ctx: any) => { + if (shouldFail) { + throw new Error('credential is unusable'); + } + + return { type: 'postgres', password: ctx.securityContext.token }; + }, + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + shouldFail = true; + + const startedAt = Date.now(); + let gaveUpAfterMs: number | undefined; + + // A refusal every 1.5s — inside the coalescing window, so nothing here ever + // starts a second incident. + for (let i = 0; i < 400 && gaveUpAfterMs === undefined; i++) { + clock.advance(1500); + // eslint-disable-next-line no-await-in-loop + await request({ token: `token-${i}` }, `req-${i}`); + + try { + // eslint-disable-next-line no-await-in-loop + await driverFactory('default'); + } catch (error) { + gaveUpAfterMs = Date.now() - startedAt; + } + } + + // Given up once the incident had itself run the grace window, not before. + expect(gaveUpAfterMs).toBeGreaterThanOrEqual(5 * 60 * 1000); + expect(gaveUpAfterMs).toBeLessThan(6 * 60 * 1000); + + await new Promise(process.nextTick); + expect((first).release).toHaveBeenCalled(); + }); + // Replacing a driver cannot move a deadline the factory keeps re-asserting. // Honouring one would find the new driver stale the moment its suppression // window closed, for the life of the process. From 6ba21b00d7d82ea6928c7b93adfbdccbc4bff1d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 20:53:16 +0000 Subject: [PATCH 16/16] docs(server-core): describe both routes to giving a driver up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The give-up grew a second, independent route when an incident gained a duration bound, and two comments were left asserting the old shape. "Requiring both keeps a burst from tearing down a working pool" is now false — a burst that lasts the grace window is sufficient on its own, deliberately — and it is exactly the line a maintainer would read before concluding a single outage cannot reach the bound. MAX_CONSECUTIVE_PROBE_FAILURES is renamed to MAX_PROBE_FAILURE_INCIDENTS: the unit stopped being failed checks when refusals began coalescing, and the count stopped being the only route when duration was added. Also pins the other side of that boundary: an incident shorter than the grace window is reused, which nothing covered — the burst test runs on a frozen clock, so it could not distinguish a zero-length incident from one under the bound. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015R8SandPBdggffPL7WfwB7 --- .../cubejs-server-core/src/core/server.ts | 27 ++++++++++------ .../unit/driver-cache-invalidation.test.ts | 32 +++++++++++++++++++ 2 files changed, 50 insertions(+), 9 deletions(-) diff --git a/packages/cubejs-server-core/src/core/server.ts b/packages/cubejs-server-core/src/core/server.ts index 89d999e6bb0ea..57b7b68938056 100644 --- a/packages/cubejs-server-core/src/core/server.ts +++ b/packages/cubejs-server-core/src/core/server.ts @@ -112,8 +112,10 @@ const MAX_DRIVER_REBUILD_ATTEMPTS = 3; const DRIVER_REBUILD_MIN_INTERVAL_MS = 30 * 1000; /** - * Consecutive staleness checks that could not resolve a configuration before - * the cached driver is given up rather than reused. + * Separate refusal incidents, spanning the grace window, before the cached + * driver is given up rather than reused. One of the two routes to a give-up — + * see `PROBE_FAILURE_GRACE_MS` for the other, and `PROBE_FAILURE_COALESCE_MS` + * for why the unit is incidents rather than failed checks. * * A probe failure is the factory declining to produce a connection for this * context. One is transient — a secret store blinking, a timeout — and reusing @@ -123,15 +125,22 @@ const DRIVER_REBUILD_MIN_INTERVAL_MS = 30 * 1000; * is how an expired credential goes on serving errors from a pool nobody * rebuilds. */ -const MAX_CONSECUTIVE_PROBE_FAILURES = 3; +const MAX_PROBE_FAILURE_INCIDENTS = 3; /** - * How long those failures must span before the driver is given up. + * How long refusal has to go on before the driver is given up — both how long + * repeated incidents must span, and how long a single unbroken one must run. * - * The count alone is not a duration: under load three concurrent probes can - * fail inside the same blink of a dependency. Requiring both keeps a burst from - * tearing down a working pool while still bounding how long a refusal can be - * ignored. + * Those are the two routes, and each covers a traffic profile the other cannot + * reach. Where probes are sparse every refusal stands alone, so the count is + * what accumulates. Where they are dense they coalesce into one incident that + * never ends, and only its duration distinguishes it from a blink. + * + * Note what this means, because it is the thing to check before assuming + * otherwise: one continuous dependency outage *is* enough, if it lasts. That is + * deliberate — it is the same call this bound made when the window was widened + * to minutes, and the recipe tells a `driver_factory` reaching an external + * dependency to catch its own failures rather than propagate them. * * Minutes rather than seconds because a probe failure is not evidence about the * cached connection — it is evidence about whatever the factory had to reach to @@ -1098,7 +1107,7 @@ export class CubejsServerCore { // faster than the coalescing window and would otherwise count once // however long the credential stayed dead. const sustainedIncident = now - failures.incidentStartedAt >= PROBE_FAILURE_GRACE_MS; - const repeatedIncidents = failures.count >= MAX_CONSECUTIVE_PROBE_FAILURES + const repeatedIncidents = failures.count >= MAX_PROBE_FAILURE_INCIDENTS && failingForMs >= PROBE_FAILURE_GRACE_MS; // Transient, as far as anything here can tell. Reuse, exactly as diff --git a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts index 6dee19ebba2e4..c1881210fdb6d 100644 --- a/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts +++ b/packages/cubejs-server-core/test/unit/driver-cache-invalidation.test.ts @@ -1012,6 +1012,38 @@ describe('driver cache invalidation', () => { expect((first).release).toHaveBeenCalled(); }); + // The other side of that boundary. An incident bounded by its duration has to + // be reused right up until the duration is reached, or a single dependency + // outage shorter than the grace window drains the pool after all. + test('reuses through an incident shorter than the grace window', async () => { + let shouldFail = false; + const { core, driverFactory, request } = await createCore({ + driverFactory: (ctx: any) => { + if (shouldFail) { + throw new Error('secret store unreachable'); + } + + return { type: 'postgres', password: ctx.securityContext.token }; + }, + }, { token: 'token-a' }); + + const first = await driverFactory('default'); + + shouldFail = true; + + // Four minutes of refusals at 1.5s — one unbroken incident, under the bound. + for (let i = 0; i < 160; i++) { + clock.advance(1500); + // eslint-disable-next-line no-await-in-loop + await request({ token: `token-${i}` }, `req-${i}`); + // eslint-disable-next-line no-await-in-loop + expect(await driverFactory('default')).toBe(first); + } + + expect((first).release).not.toHaveBeenCalled(); + expect(core.builtDrivers).toHaveLength(1); + }); + // Replacing a driver cannot move a deadline the factory keeps re-asserting. // Honouring one would find the new driver stale the moment its suppression // window closed, for the life of the process.