test(query-orchestrator): make the orphaned queue test deterministic

[ovr]

Check List

• Tests have been run in packages where changes have been made if available
• Linter has been run for changed code
• Tests for the changes have been added if not covered yet
• Docs have been added / updated if required

Description of Changes Made

QueryQueue.abstract.ts's orphaned test is a long-standing flake — most recently it failed #11608's integration-cubestore job with Continue wait at QueryQueue.abstract.ts:270, and its magic numbers had already been nudged in #6090, #6094, #6125, #6189 and #9705. The cause is that one 800ms delay value had to satisfy two hostile constraints: with continueWaitTimeout: 1s / executionTimeout: 2s / orphanedTimeout: 2s / concurrency: 1, each awaited executeInQueue must return inside its own 1s window (delay < 1000) while 114 must stay queued past the 2s orphaned timeout behind three sequential queries (3 * delay > 2000), leaving ~200ms of slack per step minus several Cube Store round trips. Timing now comes from explicit per-query orphaned timeouts and an explicit reconcileQueue() poll instead of that ratio — two blockers hold the single worker for ~2.4s with orphanedTimeout: 60 so they are never orphan-eligible themselves, and the orphan candidate carries orphanedTimeout: 1, so it can be added ~1.4s late and the test still holds. expect(delayCount).toBe(2) also proves the orphaned query never executed, which the old version never checked, and fresh keys 121-123 drop the coupling to sequence's 111-114 (ordering stays covered by sequence and negative priority).

Verification

Run against docker cubejs/cubestore:v1.7.23, with 12 busy-loops saturating the box to reproduce CI conditions — under that load the old test reproduced both signatures (Continue wait at line 270, and Expected "114", Received "113").

run	before	after
Cube Store suite, idle, 5x	pass	pass
Cube Store orphaned, under load	5/8 passed	8/8 passed
memory driver suite, under load	—	4/4 passed

yarn unit 68/68 in query-orchestrator, yarn tsc and eslint clean. Test runtime drops from 3675ms to ~2.5s.

Note one thing left untouched: LocalQueueDriverConnection reports actively executing queries as orphaned (state.recent survives retrieveForProcessing), so on the memory driver a reconcile can cancel a query mid-flight, while Cube Store only cancels active queries on heartbeat timeout. That divergence is why the blockers here need orphanedTimeout: 60; changing dev-mode driver behaviour doesn't belong in a test fix.

🤖 Generated with Claude Code

[claude]

Claude finished @ovr's task in 5m 31s —— View job

---

Review: sound direction, 3 comments (0 blocking, 1 medium, 2 low)

Full review — click to expand

• Read the PR diff
• Read QueryQueue.ts reconcile / processQuery / queryTimeout paths
• Read LocalQueueDriverConnection locking + orphan semantics
• Attempt local test run — not done, packages/cubejs-query-orchestrator/node_modules is absent in this checkout and a full monorepo yarn install was out of scope for the review. Findings below are from reading the queue implementation, not from a reproduction. CI's integration-cubestore job is the real signal here.
• Post review

Verdict

The core idea is right and this is a real improvement over what it replaces. Decoupling the orphan trigger from the delay / continueWaitTimeout ratio removes the (667, 1000) squeeze that five previous PRs kept nudging, and driving the trigger from an explicit per-query orphanedTimeout: 1 plus an explicit reconcileQueue() poll is the correct shape — reconcileQueueImpl is genuinely the only thing that cancels orphans (QueryQueue.ts:546-566), so polling it beats hoping one fires.

I verified the mechanics hold:

• orphanedTimeout inside the query payload is honoured — QueryQueue.ts:259 prefers query.orphanedTimeout over the queue-level 2, so the blockers really are orphan-ineligible at 60 and 123 really is eligible at ~1.1 s.
• cancelHandlers.delay fires for orphans regardless of whether the query ever started (processCancel, QueryQueue.ts:1016-1024), so cancelledQuery === '123' is a valid signal for "cancelled while queued".
• Concurrency is enforced inside retrieveForProcessing (LocalQueueDriverConnection.ts:295), so the repeated reconciles in the poll loop can't sneak 123 into execution ahead of 122 — expect(delayCount).toBe(2) is a meaningful assertion, not an accidental one.
• '3' + delayCount ⇒ '32' is consistent with delayCount === 2 at the final executeInQueue.

Two things I'd like tightened, both left as inline comments:

#	Severity	Where	Issue
1	Medium	line 280	Speculative processQuery('122') calls issued by the poll loop can hold the processing lock when 121's completion reconcile fires; the "Skip processing" branch doesn't re-reconcile, and by then the poll loop has exited — so 122 can be stranded in toProcess. Narrow on the memory driver, wider on Cube Store where that branch does network round trips. Suggested a reconcile-until-drained loop.
2	Low	line 279	.catch(e => e) on all three pending promises is never inspected. A TimeoutError on a blocker (only ~800 ms of slack against executionTimeout: 2 for a delay: 1200) still leaves delayCount === 2 and '32', so the test goes green on a queue that timed out. Suggested asserting ContinueWaitError, matching line 501.
3	Low	line 253	cancelledQuery = null belongs in beforeEach next to delayCount / streamCount.

What I liked

• expect(delayCount).toBe(2) is the assertion the old test was missing entirely — it's what turns "something got cancelled" into "the orphan never executed".
• Dropping keys 111-114 in favour of 121-123 kills both the cross-test coupling and the "recover if previous test broken something" preamble, and the PR body correctly identifies that ordering coverage survives in sequence and negative priority.
• The comment at lines 255-257 documenting the memory-driver-vs-Cube-Store divergence in orphan reporting is exactly the kind of thing that stops the next person from "simplifying" orphanedTimeout: 60 back out. Same for calling that divergence out in the PR body rather than silently working around it.
• Security / performance / docs: nothing applicable — test-only change, no inputs, no queries, no public API.

· branch [`fix-flaky-test-11608`](https://github.com/cube-js/cube/tree/fix-flaky-test-11608)

[claude]

There is a narrow window where expect(delayCount).toBe(2) can miss 122 — worth hardening, since removing residual flake is the whole point of the PR.

reconcileQueueImpl still dispatches one process message per call even when the slot is saturated (toProcessLimit = active.length >= this.concurrency ? 1 : ..., QueryQueue.ts:576). So each of the ~10 poll iterations above fires a speculative processQuery('122'). Those take the processing lock, fall into the "Skip processing" branch and free it — and that branch, unlike the success path, does not call reconcileQueue() (QueryQueue.ts:971 vs :974).

If one of those speculative attempts holds the lock at the moment 121 finishes and runs its completion reconcile, the real dispatch of 122 gets retrieveForProcessing → null (LocalQueueDriverConnection.ts:286-290) and returns without re-dispatching. Nothing then picks 122 up: the poll loop has already exited (it stops as soon as cancelledQuery === '123', ~1.1 s — right on top of 121 finishing at ~1.2 s), and awaitProcessing() only drains promises that already exist. Result is either delayCount === 1 here, or 122 gets dispatched by the final executeInQueue('123')'s own reconcile and that call throws Continue wait — the exact CI signature this PR is retiring.

Probability is low on the memory driver (the lock is held across a couple of microtasks) but wider on Cube Store, where getQueryDef in that branch is a network round trip. Cheap fix — reconcile until the queue is actually drained rather than once:

Suggested change

	await awaitProcessing();
	// 121 may still be executing; keep reconciling so 122 is re-dispatched even if a
	// speculative processQuery from the poll loop above briefly held its processing lock.
	const drainDeadline = Date.now() + 3000;
	do {
	await queue.reconcileQueue();
	await awaitProcessing();
	if (delayCount < 2) {
	await delayFn(null, 100);
	}
	} while (delayCount < 2 && Date.now() < drainDeadline);

[claude]

Nit: delayCount and streamCount are reset in beforeEach (lines 102-106) but cancelledQuery is reset here, inside the one test that reads it. Moving it to beforeEach keeps the shared mutable state in one place and stops a stray cancel from a preceding test (e.g. the timeout test's Cancelling query due to timeout path, which also runs cancelHandlers.delay) from leaking into whichever test looks at it next.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.33%. Comparing base (73ec070) to head (76f48bc).
⚠️ Report is 6 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (73ec070) and HEAD (76f48bc). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (73ec070)	HEAD (76f48bc)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11610       +/-   ##
===========================================
- Coverage   84.10%   59.33%   -24.77%     
===========================================
  Files         257      228       -29     
  Lines       81789    18222    -63567     
  Branches        0     3675     +3675     
===========================================
- Hits        68785    10812    -57973     
+ Misses      13004     6863     -6141     
- Partials        0      547      +547     

Flag	Coverage Δ
cube-backend	59.33% <ø> (?)
cubesql	?

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.

[claude]

Claude finished @ovr's task in 2m 21s —— View job

---

Re-review of 76f48bc — suggestion 1 applied and correct; 2 prior threads (1 medium, 1 low) still open, no new findings

Full review — click to expand

• List existing review threads (gh list-review-threads)
• Read the incremental diff (76f48bc) and the full PR diff
• Verify the new ContinueWaitError assertion against the queue/driver implementation
• Re-check the two unresolved threads against current code
• Attempt local test run — not done, packages/cubejs-query-orchestrator/node_modules is absent in this checkout. Findings are from reading the implementation. CI's integration-cubestore job remains the real signal.
• Post review

What changed since last review

Only the error-assertion suggestion was taken (QueryQueue.abstract.ts:279-280):

const outcomes = await Promise.all(pending);
outcomes.forEach((e) => expect(e).toBeInstanceOf(ContinueWaitError));

I traced whether this can itself become a new flake, and it holds in both directions:

• Can a blocker resolve instead of throwing? No. continueWaitTimeout is 1 and both blockers have delay: 1200, so getResultBlocking always loses its race against the 1 s timeout (LocalQueueDriverConnection.ts:124-129) → falsy result → ContinueWaitError at QueryQueue.ts:363-364. CI slowness only pushes the delay further past 1 s, never under it.
• Can 123 reject with something other than ContinueWaitError? This was the one worth checking, because its cancellation (~1.1 s: added at ~100 ms with orphanedTimeout: 1) lands right on top of its own continue-wait deadline. It's safe: getQueryAndRemove only deletes state, it never resolves the pending result promise (LocalQueueDriverConnection.ts:216-227, contrast setResultAndRemoveQuery at :234-251). So a cancelled-while-queued client still falls through the timeout branch to ContinueWaitError rather than parseResult throwing a stored error. The race is benign either way.

Net: the assertion closes the silent-drift hole (a TimeoutError on a blocker no longer passes as green) without adding a timing dependency. Good change.

Still open

Neither of these was addressed in 76f48bc; both threads are unresolved and I'm not re-posting them as new comments.

#	Severity	Where	Status
1	Medium	line 281	Still applies. awaitProcessing() only drains promises that already exist — shutdown() awaits reconcilePromise and returns, it never re-dispatches (QueryQueue.ts:431-439). So if a speculative processQuery('122') from the poll loop holds the processing lock when 121's completion reconcile fires, 122 can be stranded in toProcess and expect(delayCount).toBe(2) fails — or the final executeInQueue('123') picks it up and throws Continue wait, the signature this PR retires. Reconcile-until-drained fix is in the thread.
2	Low	line 253	Still applies. cancelledQuery = null belongs in beforeEach next to delayCount / streamCount (lines 102-106).

Fix both →

Notes

• Nothing else changed in the incremental commit; no new correctness, security, performance or docs concerns. Test-only diff, single file.
• Minor stylistic thought, not worth a thread: outcomes.forEach((e) => expect(e).toBeInstanceOf(...)) won't tell you which of the three failed. expect(outcomes).toEqual([...]) with expect.any(ContinueWaitError) would, but the three keys are easy enough to identify from the surrounding output.

· branch [`fix-flaky-test-11608`](https://github.com/cube-js/cube/tree/fix-flaky-test-11608)