fix(query-orchestrator): collect QueryOrchestrator.test.js and fix the two bugs it had stopped catching

[igorlukanin]

packages/cubejs-query-orchestrator/test/unit/QueryOrchestrator.test.js — 51 tests over pre-aggregation partitioning, lambda partitions, streaming and index handling — has not run since October 2025.

The package adopted jest.base-ts.config.js in #10037, which sets testMatch: ['<rootDir>/test/**/*.test.ts']. .ts only, so the .js suite silently stopped being collected, and yarn unit (and therefore yarn lerna run unit) reported success having run only the .ts suites. The file kept being edited since — most recently in #11050 — by people reasonably assuming it ran.

Confirm on master with npx jest --listTests in the package: the .js file is absent.

What had broken behind it

Collecting the suite gives 47 passed, 4 failed. All four are pre-existing and reproduce deterministically, and they turned out to be two real bugs, not stale expectations. Both landed after collection stopped, which is exactly why nothing caught them.

1. A cached result could be served for a different query (QueryCache)

Fixes 2 of the 4 (index is part of query key, lambda partitions).

#10489 added an isSameRequest short-circuit so that a must-revalidate query whose refreshKey moves mid-flight returns the cached result and refreshes in the background, instead of looping on continue-wait. It decides "is this my own continue-wait cycle?" from requestId alone.

But the result-cache key (QueryCache.queryCacheKey) is [query, values, preAggregations.map(p => p.loadSql)]. It deliberately omits indexesSql and the matched time-dimension range — the very things that change which physical pre-aggregation table gets resolved. So two queries in one request flow that differ only outside that key collide on one cache entry, and the second is served the first's result, including its embedded table names. The branch's fetchNew() is fire-and-forget, so the correct result is computed, cached, and its return value dropped.

This is not staleness within a refresh interval: the pre-aggregation layer resolves and executes the correct freshly-built table, and the cache then returns a result computed against a different one. Reachable whenever a rollup gains or loses an index, or the same query is re-issued with a different matched time-dimension range, under must-revalidate with a fast-moving refreshKey.

Fix: stamp the entry with a queryHash and require it to match. requestId establishes the request flow; it says nothing about which query — the hash supplies the half of the predicate the branch already intended. #10489's behaviour is preserved exactly, since a genuine continue-wait retry re-runs the same query and still takes the branch; when the query differs, control falls through to the pre-existing renewal branch, which under waitForRenew blocks and returns the correct result. Entries written before the field existed carry no hash and are accepted, so deploying this cannot trigger a revalidation storm.

#10489's own regression tests pass either way, because they hand-construct entries and never exercise two different resolved queries under one requestId — that gap is why this shipped. Added same request + different query: must block on fetchNew to close it.

2. A lost wakeup in the in-memory queue driver (LocalQueueDriver)

Fixes the other 2 (range partitions, empty partitions with externalRefresh), which threw ContinueWaitError.

Requests coalesce onto one queue key, but a request that joins an already-active key takes the added = 0 path in addToQueue and registers no interest of its own. It only calls getResultBlocking later — by which point the first waiter has consumed the result and deleted the promise, and setResultAndRemoveQuery has deleted queryDef. Both maps are empty, so the straggler hits the early-null branch and gets ContinueWaitError instead of a result that was computed successfully.

Fix: retain the completed result briefly (15s, capped at 1000 entries) in a map kept separate from resultPromises, and read from it on that early-null path. The separation matters: resultPromises doubles as "a query is in flight on this key", so holding a resolved promise there would hand the next query on the same key an instantly-resolved stale result — there's a regression test for exactly that. A cancelled or orphaned query clears its retained result, so a cancel can't serve one either.

Scope: CubeStoreQueueDriver is not affected — it keys RESULT_BLOCKING on the server-side queueId, and a duplicate addToQueue returns the existing job's id, so a late waiter still blocks on the real job. Production defaults to CubeStore (detectQueueAndCacheDriver), and ContinueWaitError is normal handled control flow that the client retries. So the practical impact is dev-mode / CUBEJS_CACHE_AND_QUEUE_DRIVER=memory, where a high-partition-count pre-aggregation causes avoidable continue-wait churn — a latency defect, not a correctness one. Fixing it also removes a dev/prod behavioural divergence.

Keeping it collected

test/unit/TestCollection.test.ts reads jest.config's testMatch, walks test/ for *.test.[tj]s, and asserts nothing on disk is uncollected — the actual invariant, which generalises past this one file. A count assertion would rot on every added test, and a --listTests | grep shell step is easy to render vacuously green. It also fails loudly on an unsupported glob construct and on a walk that finds nothing, rather than passing vacuously.

Notes

• The testMatch widening is in the package config, not the shared jest.base-ts.config.js. Widening the shared base would pull .js suites into every package extending it, a much larger blast radius than this change needs.
• The package's build tsconfig.json already had allowJs: true and include: ["src", "test"], so tsc has been compiling this file all along; only jest wasn't running it. That's why it kept type-checking clean while never executing.
• cubejs-api-gateway has a different shape of the same class of problem: its unit script is jest ... dist/test, so CI targets built output exclusively and its two source .js tests never run (and don't parse). Left alone here; tracked separately.

Testing

• test/unit in this package: 7 suites, 124 tests, all green (from 6 suites / 120 tests with 6 failing).
• QueryOrchestrator.test.js 51/51, verified stable over 5 consecutive runs.
• Every fix mutation-checked: disabling either half of the cache fix, or either half of the queue fix, restores exactly the failures it addresses; removing the cancel-cleanup or reverting to the retained-promise design each turns its own regression test red.
• tsc -p tsconfig.json clean; eslint src/* test/* clean (0 errors, and 2 fewer warnings than master).

[claude]

Claude finished @igorlukanin's task in 4m 47s —— View job

---

Review: solid, well-diagnosed fix — 0 blocking, 2 medium, 3 low (5 inline comments)

Full review

• Gather context / read diff
• Review QueryCache.ts cache-key fix
• Review LocalQueueDriverConnection.ts retained-result fix
• Review jest config + TestCollection.test.ts
• Review new tests
• Post review

Verdict

The diagnosis holds up under reading. I traced both fixes independently:

The cache fix is real and correctly targeted. cachedQueryResult substitutes pre-aggregation table names into query (QueryCache.ts:221) before calling cacheQueryResult, while queryCacheKey (QueryCache.ts:396) hashes the unsubstituted queryBody.query. So the resolved SQL — the thing that names the physical table — genuinely can differ for one redisKey, and getCacheHash([query, values]) is the right discriminator. On the main path renewQuery builds renewalKey from queryRedisKey([query, values]), so isKeyMismatch would already have caught the collision; #10489's isSameRequest branch short-circuits ahead of it, which is exactly why the guard has to live where it does. The hash is stable cross-process (getCacheHash only mixes in processUid for persistent keys, and [query, values] carries no persistent prop), so the field round-trips through a shared cache driver correctly. The absent-hash acceptance clause is the right call for deploy safety.

The queue fix is real and the resultPromises separation is the right design — the third test (does not hand a retained result to the next query on the same key) pins exactly the failure mode the obvious implementation would have introduced. getQueryAndRemove is the single funnel for cancel/orphan/stall (cancelQuery delegates to it), so one delete there covers all cleanup paths.

TestCollection.test.ts is the right shape of guard — asserting the invariant rather than a count, with two anti-vacuity checks. Nice.

I could not run the suites: node_modules is absent in this checkout and installing the monorepo wasn't feasible in the job. Everything below is from reading; the PR's own test claims are unverified by me.

Findings

#	Sev	Where	Issue
1	Medium	TestCollection.test.ts:34-39	**/ translated as one or more segments; micromatch matches zero. A future test/Foo.test.ts is collected by jest but reported uncollected → spurious CI failure.
2	Medium	QueryCache.abstract.ts:345	No test exercises a matching queryHash. Every same-request test uses hash-less entries, so a write/read hash divergence would silently disable #10489's optimisation forever with a green suite.
3	Low	LocalQueueDriverConnection.ts:65-66	RETAINED_RESULT_MAX caps entry count, but payloads are full query result sets — worst case is 1000 × result size held for 15s where previously it was freed on consume. 1000/15s looks oversized for a millisecond-scale race.
4	Low	LocalQueueDriverConnection.ts:162-184	TTL is swept only on write, so a quiet queue keeps entries past 15s; the comment reads as a hard lifetime.
5	Low	LocalQueueDriverConnection.ts:186	takeRetainedResult peeks rather than takes (deliberately — a test depends on it); name misleads. Plus getResult() doesn't consult retained results — worth confirming that's intentional.

Also raised as a question, not a defect (inline on QueryCache.ts:1029-1034): hashing the resolved SQL means a pre-aggregation rebuild counts as "a different query". Since a rebuild is the usual reason refreshKey moves, a continue-wait retry on a pre-agg-backed query may often miss the short-circuit and fall through to blocking renewal — possibly re-raising the ContinueWaitError churn #10489 removed. Correctness is unaffected either way; if it's deliberate, one sentence in the comment would save the next reader the trace. Same lines carry a trivial perf nit (md5 of the full SQL computed on every cache hit, even with no requestId).

Praise

• Both root causes are explained at the level of why the existing tests couldn't catch this, and each fix ships a test closing that specific gap. The same request + different query test and the does not hand a retained result to the next query test are both aimed at the mutation that would silently undo the fix.
• Widening testMatch in the package config rather than jest.base-ts.config.js is the right blast-radius call, and the CLAUDE.md note plus the api-gateway follow-up in the description mean the next person doesn't have to rediscover any of it.
• Keeping the retained-result map separate from resultPromises, with the reason written down at the constant, is the kind of comment that survives a refactor.

No security concerns: no new input paths, no user-controlled data reaching a sink; the added md5 is a cache discriminator, not a security primitive.

· branch `igor/core-734-queryorchestrator-test-not-collected`

[claude]

Claude finished @igorlukanin's task in 2m 46s —— View job

---

Follow-up review: all 5 prior findings addressed — 0 blocking, 1 low (1 inline comment)

Full review

• Diff 4f7f17e against my prior review
• Verify the micromatch fix in TestCollection.test.ts
• Verify the queryHash round-trip coverage
• Verify the retained-result bounds and sweep changes
• Check existing threads for dedup / resolution
• Post findings

Verdict

4f7f17e addresses every point from the previous pass, and in each case it took the stronger option rather than the cheap one. Re-traced all five:

1. Glob matching (was Medium) — resolved by deleting the hand-rolled translator and calling micromatch.isMatch against the real testMatch. Better than my suggested regex patch: it can't drift from jest's semantics at all, and the added test.each fixture rows pin test/Smoke.test.ts → true, which is exactly the case the old translator got wrong. micromatch added as an explicit devDependency rather than leaned on transitively — correct.

2. Missing positive queryHash case (was Medium) — resolved via the round-trip route I'd flagged as the better option, not the hardcoded hash. The harness now treats undefined entry as "cold cache", lets fetchNew() write the entry through the real path (QueryCache.ts:926), reads it back off the driver, and re-seeds it stale. I verified the write is genuinely awaited before cacheQueryResult returns on the waitForRenew path, so no race; renewalKeyHashed correctly stops the harness double-hashing an already-hashed renewalKey; and extractRequestUUID maps both round-trip-span-1/-2 to round-trip, so the branch is reached for the right reason. If the two hash sites ever diverge, this test goes red — which was the whole ask.

3. Resolved-SQL hashing question — answered in the comment at QueryCache.ts:1032-1038: a rebuilt pre-aggregation counts as a different query on purpose, falling through to the renewal branch and blocking on fresh data. That's a defensible call (correctness over churn) and now costs the next reader nothing.

4. md5 on every cache hit — fixed by making isSameQuery a thunk and reordering it after both requestId checks. Truthiness semantics of isSameRequest unchanged.

5. Retained-result memory (was Low) — 1000 / 15s → 100 / 5s, with the MAX × result size ceiling spelled out at the constant. 5s still comfortably covers the race, which resolves in the same tick as addToQueue; a miss degrades to the pre-existing ContinueWaitError, so shrinking the window is safe.

6. Naming + getResult() (was Low) — takeRetainedResult → readRetainedResult with the reason documented, and getResult() now carries an explicit "deliberately does not consult retainedResults" comment.

All five of my prior threads were already resolved by the author, so there was nothing stale to clean up and nothing to re-affirm.

Findings

#	Sev	Where	Issue
1	Low	TestCollection.test.ts:37	Native path separators on both sides of micromatch.isMatch; on Windows \ is an escape char, so every file reports uncollected → spurious red on the one guard that must not cry wolf. Linux CI unaffected.

One non-finding, for the record: the sweepRetainedResults doc says a queue going quiet "does not hold that result past its window", but the sweep still only fires on a retain or a completion — and if the queue is genuinely idle, neither happens again, so the last entry does outlive 5s. The extra sweep call is still a real improvement (it prunes on any subsequent completion, which retainResult alone wouldn't), the state is bounded at ≤100 entries, and nothing depends on hard expiry — so this is a wording nit, not worth a thread.

I could not run the suites: node_modules is absent in this checkout and installing the monorepo wasn't feasible in the job. Everything above is from reading; the PR's test claims remain unverified by me.

Praise

• Each fix went for the version that can't silently rot: config-derived glob matching over a restated pattern, a round-tripped hash over a hardcoded one. That's the harder path and the right one.
• The test.each rows added alongside the micromatch switch are effectively a regression test for the previous review comment — the exact input that used to be wrong is now pinned.
• Every "why" that came out of the review discussion landed in a code comment rather than only in the PR thread, including the two deliberate-omission notes (getResult(), read-not-take). The next reader won't have to re-derive any of it.

· branch `igor/core-734-queryorchestrator-test-not-collected`

[claude]

Claude finished @igorlukanin's task in 1m 6s —— View job

---

Follow-up review of a085a74: the last open finding is closed — 0 blocking, 0 new findings

Full review

• Diff a085a74 against the prior pass
• Verify the separator normalization in TestCollection.test.ts
• Verify the sweepRetainedResults doc correction
• Re-read the full source diff for anything the two follow-ups disturbed
• Check existing threads for dedup / stale resolution
• Post findings

Verdict

a085a74 is two small, correct changes and I have nothing new to raise.

Separator normalization (TestCollection.test.ts:40-49) — resolved, and in a stronger form than I suggested. Rather than just normalizing with path.sep inline, the separator became a parameter (isCollectedIn(sep, file)), with isCollected bound to path.sep for the real walk and a dedicated test injecting '\\'. That makes the Windows path executable on Linux CI, which my one-liner didn't — the fix and its coverage land together. I traced both directions:

• On Linux: PACKAGE_ROOT has forward slashes, so the injected-separator test's ${PACKAGE_ROOT}\test\unit\QueryOrchestrator.test.js normalizes to a clean POSIX path while the pattern passes through untouched → true, and the .abstract.ts row → false. The assertion is meaningful, not incidentally green.
• On Windows: PACKAGE_ROOT carries \, so both the file and the <rootDir>-substituted pattern get normalized — that second half is the part an inline toPosix(file) alone would have missed, since the pattern is where the backslashes actually come from. Drive-letter : is not special to picomatch, so C:/…/test/**/*.test.ts matches fine.

sweepRetainedResults doc (LocalQueueDriverConnection.ts:174-180) — the previous wording claimed a queue going quiet "does not hold that result past its window", which wasn't literally true; the new text says a result is pruned by the next queue activity and that a fully idle queue does keep its last entry past the window, bounded by the cap. That's exactly the behaviour of the code. I'd called this a wording nit not worth a thread, so fixing it anyway is above the bar.

Re-read the rest of the source diff to confirm nothing else moved: retainResult / readRetainedResult / getQueryAndRemove cleanup / the setResultAndRemoveQuery sweep call are unchanged, sweepRetainedResults mutates while iterating a snapshotted Object.keys (safe), and the QueryCache lazy-isSameQuery thunk still sits behind both requestId checks so the truthiness of isSameRequest is unchanged.

All six prior threads — my five from the first pass plus the separator one — are already resolved by the author, so there was nothing stale to clean up and nothing to re-affirm.

Findings

None. Both of my remaining points are addressed and I found no new issues in this commit.

I could not run the suites: node_modules is absent in this checkout and installing the monorepo wasn't feasible in the job. Everything above is from reading; the PR's test claims (7 suites / 124 tests green, mutation checks) remain unverified by me.

Praise

• Parameterizing the separator instead of hard-coding path.sep turns a platform-specific bug into something Linux CI actually exercises. That's the difference between fixing a review comment and making it un-regressable.
• The doc correction went the honest direction — stating the limitation plainly and why it's bounded — rather than quietly deleting the sentence that was wrong.
• Across all three commits, every review point ended up as a code comment explaining the why, not just a patched line. This branch will read well to whoever touches it next.

· branch `igor/core-734-queryorchestrator-test-not-collected`

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.21%. Comparing base (a6fbbb4) to head (a085a74).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
...tor/src/orchestrator/LocalQueueDriverConnection.ts	74.07%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11481      +/-   ##
==========================================
+ Coverage   59.10%   61.21%   +2.10%     
==========================================
  Files         223      223              
  Lines       17891    17918      +27     
  Branches     3636     3640       +4     
==========================================
+ Hits        10575    10968     +393     
+ Misses       6795     6427     -368     
- Partials      521      523       +2     

Flag	Coverage Δ
cube-backend	61.21% <75.00%> (+2.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.