refactor(query-orchestrator): LocalQueueDriver - drop processingId

[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

LocalQueueDriver kept six parallel maps keyed by query hash plus a Redis-era processingLocks[hash] = processingId lease that had to be released via freeProcessingLock, whereas Cube Store has no lock token at all — a queue item's status == Active is the lock, set atomically by QUEUE RETRIEVE and released only by QUEUE ACK/QUEUE CANCEL — which is why freeProcessingLock was already a no-op there and processingId was never sent to Cube Store in any command. This collapses those maps into one Cube Store shaped record per key (id, status, priority, created, heartbeat, orphaned, payload, extra) plus a byId index and drops the processing-lock and processingId concepts from QueueDriverConnectionInterface, both drivers and QueryQueue; removing the lock is safe because retrieveForProcessing is inverted from mutate-then-discover to discover-then-mutate, so every failure path leaves the queue untouched and there is nothing left to roll back. As a result the memory driver stops reporting running queries as orphaned (recent used to survive activation, which cancelled isJob: true builds mid-flight), stops activating unknown keys, stops returning duplicate hashes from getQueriesToCancel, and returns the existing item id on dedup instead of the caller's fresh one. The two onlyLocalTest tests existed only to exercise the separate lock and are replaced by eight parity tests that run against both drivers; verified with 74/74 unit tests, clean tsc/lint, and the same abstract suite green against a real Cube Store 1.7.23 (25/25).

Note

This removes freeProcessingLock, getNextProcessingId and the ProcessingId type from QueueDriverConnectionInterface in @cubejs-backend/base-driver. Both implementations are in-repo and updated here; no other caller exists.

Unrelated pre-existing issue found while verifying: yarn integration:cubestore cannot start against published cubejs/cubestore images because beforeAll issues QUEUE CLEAR, which even the v1.7.23 image rejects (the release image lags its tag). I ran the suite via a local harness without that statement.

[claude]

Claude finished @ovr's task in 7m 28s —— View job

---

Review: the lock removal holds up — I verified the new LocalQueueDriver semantics line-by-line against Cube Store's Rust implementation. 0 high, 1 medium, 4 low (5 inline comments).

Full review

Todo

• Gather context (diff vs base, changed files, callers)
• Review LocalQueueDriverConnection rewrite
• Review interface + CubeStoreQueueDriver + QueryQueue changes
• Cross-check claimed parity against CubeStore's Rust queue implementation
• Review test coverage
• Post findings

What I checked

The load-bearing claim is that status == Active is the only lock and that every failure path of retrieveForProcessing mutates nothing, so freeProcessingLock has nothing to undo. I read the Rust side to confirm rather than take it on faith:

• queue_retrieve_by_path (rust/cubestore/cubestore/src/cachestore/cache_rocksstore.rs:1450) does exactly what the new retrieveForProcessing does, in the same order: count pending → collect active → bail with NotEnoughConcurrency if active.len() >= concurrency → bail NotFound if the path is missing → bail LockFailed if the item isn't Pending → set Active, update_heartbeat(), active.push(key), pending -= 1. Even the "insert heartbeat, otherwise created datetime is used for orphaned filtering" comment is mirrored. All four bail-outs return before any batch_pipe write, so [0, null, active, pending, null] really does mean "nothing changed".
• into_queue_retrieve_rows (queue_item.rs:311) confirms the EXTENDED-vs-empty-rows distinction the new CubeStoreQueueDriver comments describe: all four failure variants collapse to one all-null row under EXTENDED, and to zero rows without it — so the RetrieveForProcessingResponse | null union and the if (result) guard in the "unknown key" test are both right.
• filter_to_cancel (cache_rocksstore.rs:671) matches the new isOrphaned/isStalled split precisely: pending → custom orphaned deadline else created + orphanedTimeout; active → heartbeat ?? created vs heartBeatTimeout. So the fix for "running queries reported as orphaned" is genuine parity, not a guess.

Also verified: keyScore/orphanedTime really are unused by both drivers now (the new interface docs are accurate); options.queueId was never read by CubeStoreQueueDriver.addToQueue, so the local driver owning its own id sequence is the right alignment; the new priority sort (priority desc, created, id) reproduces the old keyScore = time + (10000 - priority) * 1E14 ascending order; getQueryAndRemove/cancelQuery/setResultAndRemoveQuery are only ever called with a live queueId or null (PreAggregations.ts:864 passes null, reconcile passes the tuple id), so the deliberate no-key-fallback in resolveItem doesn't strand items; and nothing in-repo implements QueueDriverConnectionInterface besides these two — QueryOrchestrator hard-rejects anything but memory/cubestore, so removing freeProcessingLock/getNextProcessingId/ProcessingId from the published base-driver interface can't break a plugged-in third-party driver at runtime. It is still a public type removal, so it wants a changelog note.

Things the rewrite fixes that are worth keeping in the description: the old heartBeat entry was never deleted by freeProcessingLock (only active was), so an activated-then-released item left a stale heartbeat that later reported a pending item as stalled and removed it. That whole class of bug disappears with the single-record model.

Findings

#	Severity	Where	Issue
1	medium	QueryQueue.abstract.ts:401-407	withConnections skips cleanup when the callback throws, so a failing assertion leaks an active item into a concurrency: 1 shared queue and cascades into timeouts in every later test
2	low	QueryQueue.abstract.ts:450, :473	Assertions compare against the whole queue for the shared prefix, coupling them to leftovers from earlier tests
3	low	LocalQueueDriverConnection.ts:126-132	mergeDef returns the live item.payload when extra is null (the old getQueryStageState deep-cloned), and the return shape varies with whether optimisticQueryUpdate ran
4	low	LocalQueueDriverConnection.ts:252-260	On dedup this returns the original addedToQueueTime while Cube Store returns now; unpinned divergence in a parity-focused refactor. Also AddToQueueOptions.queueId is now read by no driver but still documented as an input
5	low	LocalQueueDriverConnection.ts:341-350	User-visible memory-mode change: an executing query is no longer cancelled at orphanedTimeout, only on a stale heartbeat. Correct (matches Cube Store) but deserves a release note

Details are in the inline comments.

Not issues, for the record

• optimisticQueryUpdate now returning true for a missing item matches QUEUE MERGE_EXTRA, which also always reports success, and QueryQueue ignores the value — ownership is still enforced where it matters, by setResultAndRemoveQuery returning false on a stale queueId.
• updateHeartBeat accepting pending items is harmless because isStalled is only consulted for active ones, and it's an improvement: a node holding a stale id can no longer refresh a different item that took over the same key.
• orphaned: options.orphanedTimeout ? … : null treating 0 as unset mirrors if (options.orphanedTimeout) in the Cube Store QUEUE ADD builder.
• state.resultPromises still grows unboundedly for isJob: true queries that nobody blocks on (setResultAndRemoveQuery creates and resolves a promise, getResultBlocking is the only thing that deletes it). Pre-existing, out of scope, but this PR is the natural place to notice it.

Testing / verification caveat

This checkout has no node_modules, so I could not run yarn tsc, yarn lint, or the unit suite myself — I'm relying on your reported 74/74 + clean tsc/lint, and on reading the Rust source for the Cube Store parity claims. The eight new parity tests are a real improvement over the two onlyLocalTest lock tests they replace (which asserted almost nothing beyond console.log); findings 1 and 2 are about their robustness under failure, not their coverage.

· branch `remove-freeprocessinglock-localqueue`

[claude]

Reviewing PR #11596

• Gather context (diff, changed files)
• Review LocalQueueDriverConnection rewrite
• Review interface + CubeStoreQueueDriver + QueryQueue changes
• Review test coverage
• Reconcile prior review threads
• Post findings

View job run

[claude]

Cleanup is skipped on the exact path where it matters most. keys is only assigned from fn's resolved value, so any failing expect inside the callback throws before the assignment and the finally block iterates an empty array — the queue items stay in the shared queue.queueDriver prefix.

That is the worst case for this suite: concurrency: 1, and e.g. the saturated test leaves saturated-1 in active state. A leaked active item permanently saturates the queue, so every later test that goes through executeInQueue (orphaned, stream handler, …) times out. One clear assertion failure turns into a cascade of unrelated 10s timeouts, and the real cause is buried.

Registering keys eagerly instead of returning them keeps cleanup unconditional:

Suggested change

	await connections[0].getQueryAndRemove(connections[0].redisHash(key), null);
	}
	await connection.freeProcessingLock('race', processingId1, true);
	await connection.freeProcessingLock('race2', processingId4, true);
	connections.forEach(connection => queue.queueDriver.release(connection));
	}
	};
	const keys: QueryKey[] = [];
	const track = (...queryKeys: QueryKey[]) => { keys.push(...queryKeys); };
	try {
	await fn(track, ...connections);
	} finally {
	for (const key of keys) {
	await connections[0].getQueryAndRemove(connections[0].redisHash(key), null);
	}

(each test then calls track(key) right after addQuery instead of return [key].) A try/catch per removal would also help, so one failed cleanup doesn't skip the rest.

[claude]

These two assertions (getActiveQueries() here, getToProcessQueries() on line 473) compare against the entire queue for the shared prefix, so they only hold if no other test left anything behind — and this describe runs after orphaned, orphaned with custom ttl, stream handler, etc. Combined with the cleanup gap in withConnections, a leak from any earlier test in the file fails these for a reason that has nothing to do with the semantics under test.

Scoping the assertion to the keys the test owns keeps the intent and removes the coupling:

Suggested change

	test('retrieveForProcessing leaves the item pending when the queue is saturated', async () => {
	expect((await connection.getActiveQueries()).filter(([k]) => k === hash)).toEqual([[hash, queueId]]);

[claude]

Deliberate-looking divergence from Cube Store worth a comment (or a parity test): on dedup Cube Store's addToQueue returns data.addedToQueueTime, i.e. now, while this returns the original existing.payload.addedToQueueTime. Since added === 0 the value only reaches the Added to queue log via queueSize/addedToQueueTime, and the local answer is arguably the more useful one — but the new addToQueue dedupes by key test asserts only added/queueId, so nothing pins this down either way, and the whole point of the refactor is that the two drivers agree.

Same spot: options.queueId is now ignored (the sequence owns the id, matching Cube Store), which is right, but AddToQueueOptions.queueId is still documented as an input in queue-driver.interface.ts. Since no driver reads it anymore, its doc comment could say it is only used for the skip-queue path.

[claude]

This is a faithful port of filter_to_cancel in rust/cubestore/cubestore/src/cachestore/cache_rocksstore.rs:671 (custom deadline wins, else created + driver timeout, pending only) — nice.

Worth calling out in the PR body / release notes though, because it is user-visible for memory mode: previously recent survived activation, so a query still executing past orphanedTimeout (default 120s) was picked up by getQueriesToCancel, removed and cancelled. Now an active item is only cancelled once its heartbeat goes stale (heartBeatInterval * 4), and processQuery refreshes that every heartBeatInterval, so a long-running dev-mode query runs until executionTimeout instead of being killed at 120s. That is exactly the Cube Store behaviour and it is the bug fix you describe for isJob: true builds, but anyone relying on the old cap in development will see the difference.

[claude]

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

---

Re-review after a77efa9: the QUEUE RETRIEVE throw is correct — I confirmed it in the Rust source. 0 high, 0 medium, 1 new low (+5 prior threads still apply).

Full review

Todo

• Gather context (diff vs base, new commit)
• Review the new commit (throw instead of null on empty QUEUE RETRIEVE)
• Re-review LocalQueueDriverConnection / interface / QueryQueue
• Review test coverage
• Reconcile prior review threads
• Post findings

The new commit checks out

a77efa9 claims zero rows from QUEUE RETRIEVE EXTENDED is impossible against a real Cube Store, so null can be dropped from RetrieveForProcessingResponse. That is exactly what into_queue_retrieve_rows does (rust/cubestore/cubestore/src/cachestore/queue_item.rs:313-357): Success yields one row, and LockFailed | NotEnoughConcurrency | NotFound | ExclusiveAccessFailed yield vec![one all-NULL row] when extended and vec![] only when it is not. The driver hardcodes EXTENDED in the SQL (CubeStoreQueueDriver.ts:298), so the vec![] arm is unreachable — the old "old Cube Store without EXTENDED support" comment was wrong, and a server that predates the keyword fails at parse, not with zero rows.

Blast radius of the new throw is contained: retrieveForProcessing has exactly one caller (QueryQueue.ts:752), inside the try whose catch logs Queue storage error and whose finally releases the connection — the same path any other driver error already took. Previously a null result fell through to Skip processing + freeProcessingLock; now it is a logged error, which is the more honest outcome for a broken counterparty. processQuery is only ever invoked via sendProcessMessageFn, and it swallows everything, so no unhandled rejection.

The destructuring rewrite at QueryQueue.ts:750-757 is safe: let retrieveQueueId; ends in ; so there is no ASI hazard on the following [...] = await ... statement, and activated still degrades correctly when activeKeys is [].

I also re-confirmed the parts the previous round leaned on, plus two I had not checked:

• queue_add (cache_rocksstore.rs:1218-1266) returns (row.id, false) for an existing path and never touches orphaned/priority — so the local driver's early return on dedup, and not refreshing the orphaned deadline (the old state.recent[key] = … on every add did refresh it), is deliberate parity. pending: if added { pending + 1 } else { pending } matches countPending() / pending + 1 exactly.
• getQueryStage (QueryQueue.ts:645-668) reads queryInQueue.startQueryTime, which now lives in item.extra. getQueryStageState routes defs through mergeDef, so Executing query still reports timeElapsed — that path would have silently broken if extra were omitted there.
• getCacheHash returns strings under 256 chars unchanged (utils.ts:17), so resultListKey(item.key) in setResultAndRemoveQuery and resultListKey(queryKeyHash) in getResultBlocking land on the same key. Re-hashing a hash is idempotent.
• orphanedTimeout was already in the driver options (QueryQueue.ts:132); the local driver simply never read it before. And repo-wide, freeProcessingLock, getNextProcessingId, ProcessingId, processingCounter, keyScore, RetrieveForProcessingSuccess/Fail now have zero references outside dist/ — including the bench harness.

Findings

#	Severity	Where	Issue
new	low	queue-driver.interface.ts:65	orphanedTime is now dead in both drivers — dropping it with keyScore avoids a second breaking interface change later. Also setResultAndRemoveQuery is typed Promise<unknown> while its new doc describes a boolean contract

Re-affirmed 5 prior threads that still apply to the current diff (left unresolved rather than re-commented):

Severity	Where	Issue
medium	QueryQueue.abstract.ts withConnections	cleanup skipped when the callback throws, leaking an active item into the concurrency: 1 shared queue
low	QueryQueue.abstract.ts:444, :467	assertions compare against the whole shared-prefix queue
low	LocalQueueDriverConnection.ts:126-132	mergeDef returns the live item.payload when extra is null
low	LocalQueueDriverConnection.ts:249-259	dedup returns the original addedToQueueTime; Cube Store returns now (CubeStoreQueueDriver.ts:110)
low	LocalQueueDriverConnection.ts:340-349	user-visible memory-mode change: an executing query is no longer cancelled at orphanedTimeout

Line numbers shifted by a few in a77efa9, but the code and the fix direction are unchanged in every case.

Still worth a release note

ProcessingId, RetrieveForProcessingSuccess, RetrieveForProcessingFail, freeProcessingLock, getNextProcessingId and the addToQueue arity are all reachable from @cubejs-backend/base-driver's public surface (src/index.ts:5 re-exports the whole module). Nothing in-repo is affected and QueryOrchestrator only accepts memory/cubestore at runtime, so a third-party driver cannot actually be plugged in — but it is still a published type removal, so a CHANGELOG/breaking note is warranted, alongside the memory-mode orphaned-cancellation change.

Verification caveat (unchanged)

This checkout has no node_modules, so yarn tsc, yarn lint and the unit suite could not be run here — I'm relying on your reported 74/74 + clean tsc/lint. Everything above is from reading the TypeScript and the CubeStore Rust source.

· branch `remove-freeprocessinglock-localqueue`

[claude]

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

---

Re-review after e976d20: dropping orphanedTime is behaviour-preserving — verified end to end. 0 high, 0 medium, 2 new low (+5 prior threads still apply).

Full review

Todo

• Gather context (new commit, callers, current file state)
• Verify the orphanedTime removal is behaviour-preserving
• Re-check prior review threads against the current diff
• Post findings

The new commit checks out

e976d20 removes the third and last vestigial addToQueue argument. I traced whether the queue-wide default really survives, since that is the only thing that could regress:

• orphanedTime was already unread by both drivers (_orphanedTime), so deleting the parameter changes nothing about how a deadline is computed. The 'orphanedTimeout' in query ? query.orphanedTimeout : this.orphanedTimeout fallback that went with it fed only orphanedTime, so it was dead too — options.orphanedTimeout = query.orphanedTimeout (unchanged, QueryQueue.ts:256) is still the sole per-item input.
• The queue-wide default is genuinely applied driver-side in both, and symmetrically: LocalQueueDriverConnection.isOrphaned falls back to this.orphanedTimeout * 1000 when item.orphaned is null, and Cube Store omits the ORPHANED clause (CubeStoreQueueDriver.ts:103) so the item's orphaned is None and filter_to_cancel uses created + orphaned_timeout — where that timeout arrives as this.options.orphanedTimeout * 1000 on QUEUE TO_CANCEL / QUEUE ORPHANED (:236, :248). Same units, same source, both drivers.
• this.orphanedTimeout in QueryQueue is still live (:117 default 120, :132 into the driver options), so no dangling field and no unused-var lint.
• Repo-wide there are no remaining references to orphanedTime outside dist/, and addToQueue has exactly one production caller (QueryQueue.ts:258) plus the two implementations and the tests — all five arities now agree, including the benchmark wrapper (which passes through by name).

Findings (new)

#	Severity	Where	Issue
1	low	queue-driver.interface.ts:63	With orphanedTime gone, AddToQueueQuery.orphanedTimeout is the single source of the deadline but is still typed unknown and assigned into a number? field; setResultAndRemoveQuery is still Promise<unknown> against a documented boolean contract
2	low	QueryQueue.abstract.ts:294, :305	orphanedTimeout: time in the query object is a millisecond epoch parked under the same name as the seconds option — now demonstrably dead, since no driver reads query.orphanedTimeout

The previous round's orphanedTime thread is implemented, so I resolved it; its secondary Promise<unknown> nit is carried into finding 1.

Prior threads re-affirmed (left unresolved, not re-commented)

Severity	Where	Issue
medium	QueryQueue.abstract.ts:395-401	withConnections assigns keys from the callback's return value, so a failing expect skips cleanup and leaks an active item into the concurrency: 1 shared queue
low	QueryQueue.abstract.ts:443, :466	whole-queue assertions couple these tests to leftovers from earlier tests (:505/:514 in the orphaned test are the same shape)
low	LocalQueueDriverConnection.ts:131	mergeDef returns the live item.payload when extra is null
low	LocalQueueDriverConnection.ts:256	dedup returns the original addedToQueueTime; Cube Store returns now
low	LocalQueueDriverConnection.ts:342-348	user-visible memory-mode change: an executing query is no longer cancelled at orphanedTimeout

Still worth a release note

addToQueue's arity has now changed three times in this PR (keyScore, then orphanedTime), on top of the ProcessingId / RetrieveForProcessingSuccess / RetrieveForProcessingFail / freeProcessingLock / getNextProcessingId removals — all reachable from @cubejs-backend/base-driver's public surface. Nothing in-repo is affected and QueryOrchestrator only accepts memory/cubestore at runtime, so no third-party driver can actually be plugged in, but it is still a published type removal and deserves a CHANGELOG/breaking note alongside the memory-mode orphaned-cancellation change.

Verification caveat (unchanged)

This checkout has no node_modules, so yarn tsc, yarn lint and the unit suite could not be run here — I'm relying on your reported 74/74 + clean tsc/lint. Everything above is from reading the TypeScript and the CubeStore Rust source.

· branch `remove-freeprocessinglock-localqueue`

[claude]

Dropping orphanedTime is the right call — with it gone, options.orphanedTimeout is the single input for the deadline, so the two remaining unknowns in this file are now the only weak spots in that contract:

• AddToQueueQuery.orphanedTimeout (line 27) is unknown, and QueryQueue.ts:256 assigns it straight into AddToQueueOptions.orphanedTimeout?: number. That assignment only type-checks because the source is unknown-into-any-ish and query is loosely typed; typing it orphanedTimeout?: number would make the seconds contract enforced at the one place a caller can get it wrong. It matters more now than before, since the parameter that used to carry the absolute ms deadline is gone and this field is what everything derives from.
• setResultAndRemoveQuery (line 84) is still declared Promise<unknown> while the doc comment right above it describes a boolean contract and both implementations return Promise<boolean>. Tightening it makes if (!(await ...)) in QueryQueue type-checked instead of relying on unknown being truthy-testable.

Both are nits; neither blocks.

[codecov]

Codecov Report

❌ Patch coverage is 93.06931% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.33%. Comparing base (8b6e94c) to head (63e9bc2).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
...tor/src/orchestrator/LocalQueueDriverConnection.ts	93.61%	4 Missing and 2 partials ⚠️
...-query-orchestrator/src/orchestrator/QueryQueue.ts	85.71%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (8b6e94c) and HEAD (63e9bc2). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (8b6e94c)	HEAD (63e9bc2)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11596       +/-   ##
===========================================
- Coverage   84.06%   59.33%   -24.74%     
===========================================
  Files         257      228       -29     
  Lines       81582    18210    -63372     
  Branches        0     3688     +3688     
===========================================
- Hits        68581    10804    -57777     
+ Misses      13001     6858     -6143     
- Partials        0      548      +548     

Flag	Coverage Δ
cube-backend	59.33% <93.06%> (?)
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 1m 55s —— View job

---

Re-review after 634e255: making AddToQueueQuery.orphanedTimeout optional is safe and the test cleanup is exactly right. 0 high, 0 medium, 0 new low (+5 prior threads still apply, 1 resolved).

Full review

Todo

• Gather context (new commit, current file state)
• Verify the orphanedTimeout optional change is safe
• Re-check prior review threads against the current diff
• Post findings

The new commit checks out

634e255 widens AddToQueueQuery.orphanedTimeout to optional and drops the dead orphanedTimeout: time from the orphaned with custom ttl query object plus the two <any> casts. I checked the three things that could go wrong:

• No production reader breaks. AddToQueueQuery has exactly three references outside dist/ — the declaration, LocalQueueDriverConnection.ts:241 and CubeStoreQueueDriver.ts:72 — and neither driver reads query.orphanedTimeout; the local one stores query verbatim as payload, Cube Store JSON.stringifys it. The only reader is QueryQueue.ts:256, and undefined there is already the normal case (options.orphanedTimeout is number | undefined, and both drivers gate on truthiness: options.orphanedTimeout ? created + … : null and if (options.orphanedTimeout)).
• No construction site relied on the field being required. Widening a field can only break readers, and grep shows none; PreAggregations.ts:565 / PreAggregationLoader.ts:446 set it on the query body, which is any-typed all the way through.
• The test is still testing what it claims. time is now unused and gone, the assertion is still driven by the seconds-valued orphanedTimeout in the options object (2s vs 60s, pausePromise(2500), only key '1' reported), and the two helpers keep orphanedTimeout: 60 / 1 in options. Nothing lost by dropping the casts — the helpers take connection: any anyway, so the cast was only silencing the required-field error on the literal.

The commit message's claim about consistency holds: QueryCache.CacheOptions:139, PreAggregations:238 and PreAggregationPartitionRangeLoader:42 all declare orphanedTimeout?: number, so this declaration was the outlier.

One correction to my previous round

I said typing this field number instead of unknown "would make the seconds contract enforced at the one place a caller can get it wrong." That was overstated: executeInQueue takes query: QueryDef, and QueryDef = any (queue-driver.interface.ts:1), so the assignment at QueryQueue.ts:256 is unchecked regardless of how AddToQueueQuery declares the field. Narrowing unknown → number is still worth doing as documentation of the unit, but it buys no compile-time safety until QueryDef stops being any. The setResultAndRemoveQuery: Promise<unknown> half of that thread is unaffected and still stands — both implementations return Promise<boolean> and the doc comment above line 84 describes a boolean contract.

Threads

Resolved as implemented: the orphanedTimeout: time ms-epoch-in-the-query-object thread — the fix landed exactly as suggested.

Re-affirmed and left unresolved (not re-commented):

Severity	Where	Issue
medium	QueryQueue.abstract.ts:395-401	withConnections assigns keys from the callback's return value, so a failing expect skips cleanup and leaks an active item into the concurrency: 1 shared queue
low	queue-driver.interface.ts:84	setResultAndRemoveQuery typed Promise<unknown> against a documented boolean contract (the AddToQueueQuery half of this thread is now partially addressed — optional, still unknown)
low	QueryQueue.abstract.ts:443, :466	whole-queue assertions couple these tests to leftovers from earlier tests — and orphaned with custom ttl itself has the same shape at :289/:313, which this commit touched without changing
low	LocalQueueDriverConnection.ts:131	mergeDef returns the live item.payload when extra is null
low	LocalQueueDriverConnection.ts:256	dedup returns the original addedToQueueTime; Cube Store returns now
low	LocalQueueDriverConnection.ts:342-348	user-visible memory-mode change: an executing query is no longer cancelled at orphanedTimeout

Still worth a release note

Unchanged from the last round: ProcessingId, RetrieveForProcessingSuccess, RetrieveForProcessingFail, freeProcessingLock, getNextProcessingId and three addToQueue arity changes are all reachable from @cubejs-backend/base-driver's public surface. Nothing in-repo is affected and QueryOrchestrator only accepts memory/cubestore at runtime, so no third-party driver can actually be plugged in — but it is still a published type removal, and it deserves a CHANGELOG/breaking note alongside the memory-mode orphaned-cancellation change. This commit adds a (compatible) widening to that list.

Verification caveat (unchanged)

This checkout has no node_modules, so yarn tsc, yarn lint and the unit suite could not be run here — I'm relying on your reported clean tsc/lint. Everything above is from reading the TypeScript and the CubeStore Rust source.

· branch `remove-freeprocessinglock-localqueue`

[claude]

Reviewing PR #11596

• Gather context (new commit 5065f5c)
• Verify the new commit
• Re-check prior review threads against the current diff
• Post findings

View job run · branch remove-freeprocessinglock-localqueue

[claude]

Claude finished @ovr's task in 1m 28s —— View job

---

Re-review after 5065f5c + 63e9bc2: the mergeDef copy is the right fix; the withConnections comment answers a different question than the one I asked. 0 high, 0 medium, 0 new low (+5 prior threads, 1 now downgraded).

Full review

Todo

• Gather context (two new commits, current file state)
• Verify the mergeDef shallow-copy change
• Re-assess the withConnections cleanup thread against the new comment
• Re-check remaining prior threads
• Post findings

5065f5c — mergeDef returns { ...item.payload }

Exactly the suggested fix, and the return shape is now unconditional: both branches hand back a fresh top-level object, so callers no longer alias queue state and the shape no longer depends on whether optimisticQueryUpdate happened to run. All five call sites (:312 getQueryAndRemove, :377 getQueryStageState, :393 getQueryDef, :430 retrieveForProcessing, and cancelQuery via :312's sibling) are read paths, so nothing depended on identity. Thread resolved.

One residual worth knowing but not worth changing: the old getQueryStageState used R.clone, i.e. a deep copy, so nested payload.query / payload.queryKey are still shared now. That is the same exposure getQueries()'s {...def, queryKey} spread already had, nothing in-repo mutates a def, and Cube Store returns freshly-parsed JSON so it is a copy there by construction. Not worth a deep clone on every getQueryDef.

63e9bc2 — the withConnections comment

The comment is accurate about what it claims: tenantPrefix is crypto.randomBytes(6) (:49), the shared queue is namespaced with it, and the one test that needs its own driver (stalled queries…, :558) generates a second random prefix. So leaked items genuinely cannot bleed into another run sharing a Cube Store.

But that was not the failure mode in the thread — the concern is intra-run. Within this file, queue driver semantics runs after orphaned / stream handler / etc. against the same concurrency: 1 queue, and the tests inside it assert against the whole queue (getActiveQueries()).toEqual([[hash, queueId]]) at :450, getToProcessQueries() at :471, getOrphanedQueries()).toEqual([]) at :511). A failing expect in, say, the saturated test skips cleanup and leaves saturated-1 active, which then fails :511 and :450-style assertions in every later test in the describe. One real failure becomes a screenful of unrelated ones.

That is a diagnosability cost on an already-red run, not a correctness or flakiness problem — the suite is green either way. Given the tradeoff is now explicitly written down, I'm downgrading it from medium to low and leaving the thread open rather than re-litigating; the eager-track variant in that thread is still a two-line change if you want it. The comment would read truer as "a failed assertion can cascade into later assertions in this describe; acceptable because the run is already failing, and the randomized prefix keeps it out of other runs."

Threads

Nothing became inapplicable this round, so nothing new resolved (mergeDef was resolved by you). Re-affirmed and left unresolved, not re-commented:

Severity	Where	Issue
low (was medium)	QueryQueue.abstract.ts:398	withConnections cleanup skipped on a failing expect — now documented, cascade scope is intra-run only
low	QueryQueue.abstract.ts:450, :471	whole-queue assertions couple these tests to leftovers from earlier tests
low	queue-driver.interface.ts:84	setResultAndRemoveQuery typed Promise<unknown> against a documented boolean contract
low	LocalQueueDriverConnection.ts:256	dedup returns the original addedToQueueTime; Cube Store returns now
low	LocalQueueDriverConnection.ts:342-348	user-visible memory-mode change: an executing query is no longer cancelled at orphanedTimeout

Still worth a release note

Unchanged: ProcessingId, RetrieveForProcessingSuccess, RetrieveForProcessingFail, freeProcessingLock, getNextProcessingId and the addToQueue arity changes are all reachable from @cubejs-backend/base-driver's public surface. Nothing in-repo is affected and QueryOrchestrator only accepts memory/cubestore at runtime, so no third-party driver can actually be plugged in — but it is a published type removal, and it deserves a CHANGELOG/breaking note alongside the memory-mode orphaned-cancellation change.

Verification caveat (unchanged)

This checkout has no node_modules, so yarn tsc, yarn lint and the unit suite could not be run here — I'm relying on your reported clean tsc/lint and 74/74. Everything above is from reading the TypeScript and the CubeStore Rust source.

· branch `remove-freeprocessinglock-localqueue`