Skip to content

fix(query-orchestrator): initialize missing data source queue in isPa… - #11617

Merged
ovr merged 1 commit into
cube-js:masterfrom
dochernyshov:fix/pre-aggregation-job-status-queue
Aug 27, 2026
Merged

fix(query-orchestrator): initialize missing data source queue in isPa…#11617
ovr merged 1 commit into
cube-js:masterfrom
dochernyshov:fix/pre-aggregation-job-status-queue

Conversation

@dochernyshov

@dochernyshov dochernyshov commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

…rtitionExist

Polling job status from a replica that never built that data source crashed on this.queue[dataSource].getQueueDriver(). Create the queue via getQueue() first, matching the other queue accessors.

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

Issue Reference this PR resolves
Related to #11313 (queue lookup used only default). That did not cover this crash.
Created a new one #11615

Description of Changes Made (if issue reference is not provided)
POST /cubejs-api/v1/pre-aggregations/jobs { "action": "get" } calls isPartitionExist, which did this.queue[dataSource].getQueueDriver(). this.queue is per process. On a replica that never built that data source the key is missing, so the poll 500s with Cannot read properties of undefined (reading 'getQueueDriver') even when Cube Store already has the result.

@dochernyshov
dochernyshov requested a review from a team as a code owner August 21, 2026 14:45
@github-actions github-actions Bot added javascript Pull requests that update Javascript code pr:community Contribution from Cube.js community members. labels Aug 21, 2026
@dochernyshov
dochernyshov force-pushed the fix/pre-aggregation-job-status-queue branch from 90208f4 to b138d98 Compare August 21, 2026 14:48
…rtitionExist

Polling job status from a replica that never built that data source crashed
on this.queue[dataSource].getQueueDriver(). Create the queue via getQueue()
first, matching the other queue accessors.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ovr
ovr force-pushed the fix/pre-aggregation-job-status-queue branch from b138d98 to de539cf Compare August 27, 2026 11:26
@ovr

ovr commented Aug 27, 2026

Copy link
Copy Markdown
Member

/bot-review

@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

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


Review: looks correct — 0 blocking, 2 low (1 connection-leak nit, 1 test suggestion)

Full review
  • Gather context (diff, PR body)
  • Read PreAggregations.ts around isPartitionExist / getQueue
  • Compare against the other queue accessors
  • Review test quality
  • Post review

Verdict

The fix is correct and minimal. isPartitionExist was the only remaining place indexing this.queue[dataSource] directly; every other consumer (PreAggregations.ts:859, :864) already goes through await this.getQueue(dataSource), which lazily constructs the queue under QueryCache.createQueue. So this brings the odd one out in line with the established pattern rather than adding a special case, and the crash described in #11615 (replica polls job status for a data source it never built → undefined.getQueueDriver()) is genuinely fixed by it. Hoisting queue into a local also removes the second getQueueDriver() lookup, which is a small readability win.

No security or auth surface here — dataSource reaching getQueue is the same value already passed to driverFactory(dataSource, true) a few lines above, so the fix doesn't widen anything.

Findings

Sev File Note
Low src/orchestrator/PreAggregations.ts:471-474 conn isn't released if getResult rejects — pre-existing, but these exact lines are being rewritten. Suggested try/finally inline.
Low test/unit/PreAggregations.test.ts:283-312 Test covers the regression; suggested asserting the queue-creation side effect and adding a missing_partition case.

Notes / non-issues

  • Behavioural side effect: a status poll now instantiates the queue for that data source on the replica (calls options.queueOptions(dataSource), registers the queue object). That's the intended fix and matches how every other accessor behaves; the actual queue-driver connection was already being created on this path by createConnection(), so there's no new connection cost.
  • getQueue double-checked lock (PreAggregations.ts:701-745) isn't atomic — there's an await between the two if (!this.queue[dataSource]) checks, so two concurrent first-callers can each build a queue and one gets orphaned. Pre-existing and out of scope for this PR, but this fix does add a new concurrent entry point (job polling) into that path, so it's slightly more reachable now than before. Worth a follow-up caching the in-flight promise rather than the resolved queue.
  • Docs: none needed — internal behaviour fix, no API change.

Verification

I could not execute yarn jest in this environment (command not in the allowed tool set), so the new test is reviewed statically only — the assertion shape and fixtures look consistent with the rest of the file, and the pre-fix code would have thrown a TypeError on this input, so it is a real regression test. Please confirm CI green before merge.

· branch `fix/pre-aggregation-job-status-queue`

Comment on lines +471 to +474
const queue = await this.getQueue(dataSource);
const conn = await queue.getQueueDriver().createConnection();
const result = await conn.getResult(key);
this.queue[dataSource].getQueueDriver().release(conn);
queue.getQueueDriver().release(conn);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these lines are being rewritten anyway, worth closing the pre-existing connection leak: if conn.getResult(key) rejects (Cube Store hiccup, timeout), release(conn) never runs and the queue-driver connection is leaked. For the CubeStore queue driver that's a pooled connection held for the process lifetime, and this is a polling endpoint, so leaks accumulate per failed poll.

Suggested change
const queue = await this.getQueue(dataSource);
const conn = await queue.getQueueDriver().createConnection();
const result = await conn.getResult(key);
this.queue[dataSource].getQueueDriver().release(conn);
queue.getQueueDriver().release(conn);
const queue = await this.getQueue(dataSource);
const conn = await queue.getQueueDriver().createConnection();
let result;
try {
result = await conn.getResult(key);
} finally {
queue.getQueueDriver().release(conn);
}

Comment on lines +311 to +312
).resolves.toEqual([true, 'done']);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test covers the crash path (before the fix this threw Cannot read properties of undefined), which is the important part. Two small strengtheners, both optional:

  • Assert the side effect explicitly, e.g. expect(Object.keys(preAggregations.getQueues())).toContain('named_data_source') — it documents why the call no longer throws rather than relying on the absence of a TypeError.
  • Add a companion case where the table is absent, asserting [true, 'missing_partition'] for the unknown data source; that's the more common replica-poll shape (job still building) and exercises the result == null branch.

Also, nothing shuts the created queue down at the end of the test. With cacheAndQueueDriver: 'memory' that should be inert (no reconcile is triggered), but if jest ever reports open handles on this file, this is the first place to look.

@ovr
ovr merged commit ab48039 into cube-js:master Aug 27, 2026
86 of 89 checks passed
@ovr

ovr commented Aug 27, 2026

Copy link
Copy Markdown
Member

Thank you, @dochernyshov, for your contribution! 🍰 👍

@dochernyshov

Copy link
Copy Markdown
Contributor Author

@ovr Thanks for taking care of it!
Should we close this issue then or later?
#11615

@ovr

ovr commented Aug 27, 2026

Copy link
Copy Markdown
Member

@dochernyshov, I plan to do an additional fix for this place. I will close #11615 later. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

javascript Pull requests that update Javascript code pr:community Contribution from Cube.js community members.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants