fix(query-orchestrator): enforce rollup-only mode on the streaming query path

[igorlukanin]

CUBEJS_ROLLUP_ONLY=true is meant to make a pre-aggregation miss fail instead of falling through to the source database. On the streaming path it did nothing: the query ran against the warehouse and returned correct rows, so an operator got no signal that acceleration had been lost.

QueryOrchestrator has two entry points that both load pre-aggregations, but only one applied the guard. fetchQuery checked rollupOnlyMode after deriving usedPreAggregations; streamQuery called the identical loadAllPreAggregationsIfNeeded and then went straight to cachedQueryResult, never checking. That check was the only enforcement site in the codebase, so anything reaching the orchestrator through streamQuery bypassed rollup-only completely — and streamQuery is the streaming / SQL API path, reached from the two persistent: true call sites in ApiGateway.

The bypass is the transport, not the planner. Multi-stage matching already fails closed — Tesseract's all-or-nothing rollback returns empty usages, which fetchQuery rejects — so this affected any query over the streaming path, multi-stage or not.

This matters beyond a missed optimisation: rollup-only is a cost-control and SLA guarantee, and it also disables source-database connection testing. A deployment could therefore be configured as if the warehouse were unreachable while still querying it on every SQL API request.

What changed

The miss check is extracted into one checkRollupOnlyMode method and called from both entry points. In streamQuery it runs after pre-aggregations are loaded but before the stream is created, which is where an error can still be returned to the caller rather than surfacing mid-flight; the error message is unchanged, so a miss now looks the same whichever transport was used.

The guard tests preAggregationsTablesToTempTables.length rather than re-deriving usedPreAggregations. That derivation feeds two response shapes that only fetchQuery needs, and the emptiness condition is equivalent — R.fromPairs can collapse duplicate keys but never turns a non-empty array into zero keys — so the cheaper test is the same boolean, not an approximation of it.

OrchestratorApi.streamQuery now normalises a thrown error the way executeQuery already does. Without it, this failure reached ApiGateway.handleError as a bare Error and was reported as a 500 "Internal Server Error" with a stack, while the identical miss on /v1/load is a query error. The message the SQL client receives was already correct; the server-side log was not.

Behaviour change

A rollup-only deployment whose SQL API queries were silently missing pre-aggregations will now see those queries fail. That is the point of the setting, and it is the same error /v1/load has always raised, so nothing new needs handling on the client. The fix reveals a pre-existing miss rather than creating one — but it is worth calling out for anyone who had come to rely, knowingly or not, on the fall-through.

No API, configuration, or schema changes.

Tests

New unit coverage in packages/cubejs-query-orchestrator/test/unit/QueryOrchestratorRollupOnly.test.ts, over a real QueryOrchestrator:

• a miss rejects on streamQuery, and the source database is never queried
• a miss rejects on fetchQuery
• the error message cannot be mistaken for a "continue wait" — the native transport retries on that substring, so a reworded message would spin instead of failing
• a matched pre-aggregation still streams
• with rollup-only off, a miss still falls through

Each assertion was checked against a deliberately broken implementation: removing the guard from streamQuery, making it throw whenever rollup-only is on, making it ignore the flag, and rewording the message all turn a different test red.

Documentation already described the fixed behaviour — "Cube will only fulfill queries using pre-aggregations", and rollupOnlyMode as "an error will be thrown if a query can't be served from a pre-aggregation" — so no docs change was needed here; the code had drifted from what was written.

[claude]

Claude finished @igorlukanin's task in 3m 11s —— View job

---

Review complete — 1 high, 2 low. The orchestrator-side fix looks right; the OrchestratorApi error wrapper regresses Continue wait.

Full review

• Read PR diff
• Review QueryOrchestrator.ts change
• Review OrchestratorApi.ts error normalisation
• Review test coverage
• Check for stale review threads (none exist)
• Post review

What's good

The core diagnosis and fix are correct. streamQuery and fetchQuery really did have divergent enforcement, and hoisting the check into checkRollupOnlyMode called from both is the right shape. Placing the check in streamQuery after loadAllPreAggregationsIfNeeded but before cachedQueryResult is the correct seam — it's the last point where a rejection can still be a returned error rather than a mid-stream failure.

The reasoning about preAggregationsTablesToTempTables.length === 0 being equivalent to Object.keys(R.fromPairs(...)).length === 0 holds: R.fromPairs can only collapse keys, never produce zero from a non-empty input. Dropping the derivation from the guard is a genuine simplification, and usedPreAggregations is still computed where the two response shapes need it.

Moving the check earlier in fetchQuery (now before the if (values) block rather than after usedPreAggregations) does not change reachability — it was already ahead of both the isJob and the !queryBody.query returns.

High — ContinueWaitError is swallowed on the streaming path

packages/cubejs-server-core/src/core/OrchestratorApi.ts:65-78 — see the inline comment.

The new catch is unconditional, so a ContinueWaitError (thrown by QueryQueue.ts:364 whenever a pre-aggregation build is still in flight — a routine outcome on this path) is rewritten to { error: 'Error: Continue wait' }. That loses the .message that gateway.ts:1995 keys on, so the gateway no longer maps it to a retryable Continue wait: the response becomes a 400 "Orchestrator error", and the string 'Error: Continue wait' fails the exact-match check in scan.rs:898, so the native client doesn't retry either. A SQL API query hitting a building pre-aggregation would fail hard instead of waiting.

Rethrowing ContinueWaitError untouched before the logging/wrapping restores the prior behaviour; ContinueWaitError is already imported in the file. Same fix also stops a non-error being logged as Error querying db, which executeQuery deliberately avoids.

Low — the OrchestratorApi half of the fix has no tests

All five new tests exercise QueryOrchestrator directly. The error-normalisation change in OrchestratorApi.streamQuery — the part that actually alters what the client sees, and where the regression above lives — is uncovered. A small test in cubejs-server-core asserting both shapes (a rollup-only miss arrives as { error: ... }, a ContinueWaitError arrives as Continue wait) would pin this down.

Low — the "continue wait" wording test is weaker than its rationale

QueryOrchestratorRollupOnly.test.ts:170-179 — the comment says the native transport "retries any error whose message contains continue wait", but the Rust side uses eq_ignore_ascii_case("continue wait") (scan.rs:695, scan.rs:898, cubeclient/src/apis/default_api.rs:76), i.e. full-string equality. The assertion (not.stringMatching(/continue wait/i)) is strictly stronger than what's needed, so the test isn't wrong — but the stated reason is, and the next person reading it may draw the wrong conclusion about the transport contract. Worth correcting the comment.

Notes

• Test fixtures and mock driver follow the existing QueryOrchestrator.test.js conventions (per-test redis prefix, cleanup() in afterEach), and the any casts on partial query bodies are explained. Good.
• throw { error: ... } throws a non-Error, losing the stack downstream — but executeQuery already does exactly this, so matching it is the right call here rather than diverging.
• No docs change needed is correct; rollupOnlyMode is already documented as throwing on a miss.
• I could not run yarn test in this environment (command approval unavailable), so the new tests are reviewed by reading only, not by execution.

• [branch](https://github.com/cube-js/cube/tree/igor/core-733-rollup-only-mode-is-not-enforced-on-the-streaming-query-path)

[claude]

This catch-all swallows ContinueWaitError and breaks the SQL API's retry-while-building path.

orchestrator.streamQuery → loadAllPreAggregationsIfNeeded → QueryQueue throws ContinueWaitError (QueryQueue.ts:364) whenever a pre-aggregation build is still in flight — this is a routine outcome on the streaming path, not an error. Today it propagates as an Error with message === 'Continue wait', which is exactly what gateway.ts:1995 keys on:

const e = err.message === 'Continue wait' ? { error: 'Continue wait' } : err;

That normalisation feeds handleError's Continue-wait branch → 200 with error: 'Continue wait', and the native transport retries on it (rust/cubesql/cubesql/src/compile/engine/df/scan.rs:898 — note it's eq_ignore_ascii_case("continue wait"), an exact match, not a substring test).

After this change the rejection is { error: 'Error: Continue wait' }:

• err.message is undefined, so gateway.ts:1995 no longer recognises it;
• handleError falls into the e.error branch → 400 "Orchestrator error";
• the error string is 'Error: Continue wait', which fails the exact-match check in scan.rs, so the client does not retry.

Net effect: a query issued over the SQL API while its pre-aggregation is building now fails hard instead of waiting. It also logs Error querying db for a non-error, whereas executeQuery deliberately logs Continue wait for the same condition.

ContinueWaitError is already imported in this file — suggest letting it through untouched so the existing gateway normalisation keeps working:

Suggested change

	try {
	return await this.orchestrator.streamQuery(query);
	} catch (err) {
	this.logger('Error querying db', {
	query: query.query?.replace(/\s+/g, ' '),
	params: query.values,
	error: ((err as Error).stack || err),
	requestId: query.requestId,
	});
	// Shaped like executeQuery's rejection so the gateway reports a query
	// error rather than falling through to its Internal Server Error branch.
	throw { error: (err as Error).toString() };
	}
	try {
	return await this.orchestrator.streamQuery(query);
	} catch (err) {
	// A pre-aggregation still building is not a query error: let it through
	// unchanged so the gateway maps it to a retryable `Continue wait`.
	if (err instanceof ContinueWaitError) {
	throw err;
	}
	this.logger('Error querying db', {
	query: query.query?.replace(/\s+/g, ' '),
	params: query.values,
	error: ((err as Error).stack || err),
	requestId: query.requestId,
	});
	// Shaped like executeQuery's rejection so the gateway reports a query
	// error rather than falling through to its Internal Server Error branch.
	throw { error: (err as Error).toString() };
	}

Worth a regression test asserting a ContinueWaitError from the orchestrator still reaches the caller as Continue wait — the currently-untested half of this fix is where the behaviour change hides. Fix this →