Skip to content

fix(server-core): build one orchestrator api per id, not one per concurrent caller - #11834

Open
paveltiunov wants to merge 6 commits into
masterfrom
pavel-claude/busy-lamport-wpo548
Open

fix(server-core): build one orchestrator api per id, not one per concurrent caller#11834
paveltiunov wants to merge 6 commits into
masterfrom
pavel-claude/busy-lamport-wpo548

Conversation

@paveltiunov

@paveltiunov paveltiunov commented Sep 9, 2026

Copy link
Copy Markdown
Member

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

CORE-884 — a regression from CORE-823 / #11661.

Description of Changes Made

CubejsServerCore.getOrchestratorApi() is asynchronous and writes the orchestrator cache only at the end of the build, so concurrent callers of a cold id all miss the cache together and each cache an api of their own. Since #11661 a replaced OrchestratorStorage entry is released (disposeAfter, reason set), and WebSocketConnection.close() is terminal — so every one of those writes closed the Cube Store web socket of the api the previous caller had just been handed, and that caller's next query failed with

ConnectionError: Cube Store connection is closed
    at WebSocketConnection.initWebSocket (.../cubestore-driver/src/WebSocketConnection.ts:120:13)
    at WebSocketConnection.sendMessage (.../WebSocketConnection.ts:433:31)
    at CubeStoreCacheDriver.get (.../CubeStoreCacheDriver.ts:68:18)
    at CubeQueryCache.cacheQueryResult (.../QueryCache.ts:1110:15)

Concurrency of two is the everyday case rather than a rarity: one /v1/load with total: true runs its data query and its count query through Promise.all, each fetching the api for itself (gateway.tsgetSqlResponseInternal), so a single request is enough to race with itself.

The build is now memoized by orchestrator id while it runs. The memo is registered before the first await inside the build, so nothing can interleave between the cache miss and it, and it is dropped once the build settles — a success is in the cache by then, and a failure must not become the cached answer for that id. resetInstanceState() clears the memo alongside the orchestrator storage.

Evidence from the deployment that reported it

v1.7.29, both API pods up for 4.5h with no restarts, so the id keeps going cold through LRU eviction (OrchestratorStorage max 100) on a multi-vhost deployment.

  • Every failing request carries total: true.
  • 22:39:00.148–.158 UTC: five REST requests, one security context (same jti/vhost/iat), all five failed and all at ~2.04s in — ten concurrent builds, one surviving api.
  • 22:16:48.894 UTC: a single request, alone on the deployment for 35 seconds, failed the same way — its own two halves raced.

Tests

New packages/cubejs-server-core/test/unit/getOrchestratorApi.test.ts, 8 cases. Six of them fail against the parent commit; the one that matters most is the link from "an api was released" to "a Cube Store connection was closed":

✕ concurrent callers of one id share a single orchestrator api      Expected: 1  Received: 5
✕ no orchestrator handed to a caller is released behind its back    Expected 0 calls  Received 4
✕ the two queries of one total:true request get the same api
✕ a later caller reuses the cached api rather than building another
✕ a failed build is not left behind to fail every later request     Expected: 1  Received: 3
✕ the Cube Store driver of a live caller is not closed under it     Expected []  Received [0, 1]

Two more were added from review, each verified by mutation rather than assumed:

  • callers of different ids get an api each — every other case pins contextToOrchestratorId to a constant, so a memo keyed too loosely would satisfy all of them. Keying the memo on a constant fails this one.
  • a build that outlives a reset does not drop the build that replaced it — pins the identity check in the finally, which resetInstanceState() clearing the memo makes load-bearing. Reducing it to a plain delete fails this one in 77ms.

No e2e spec: the bug is a microtask-ordering race inside server-core with no user-drivable surface that reproduces it deterministically.

Suites run locally: cubejs-server-core unit (8/8 new; the 4 pre-existing failures in RefreshScheduler/OptsHandler are identical with this file removed entirely, so they are not from this change) and cubejs-cubestore-driver (22/22, so the terminal-close semantics from #11661 are untouched). The new file also exits clean under jest --detectOpenHandles without --forceExit.

🤖 Generated with Claude Code

https://claude.ai/code/session_014gBgEB5wBDFxWrtBhk7G7X

…urrent caller

`getOrchestratorApi()` is asynchronous and writes the orchestrator cache only at
the end, so concurrent callers of a cold id all miss the cache and each cache an
api of their own. Since #11661 a replaced cache entry is released, and releasing
an api closes its Cube Store web socket for good -- so every one of those writes
tore the connection out from under the caller that had just been handed the
previous api, which then failed with `ConnectionError: Cube Store connection is
closed`.

Concurrency of two is the everyday case rather than a rarity: one `/v1/load`
with `total: true` fetches the api once for its data query and once for its
count query (`gateway.ts` -> `getSqlResponseInternal`), so a single request is
enough to race with itself. On the deployment this was reported from, every
failing request carried `total: true`, including one that was alone on the
deployment for 35 seconds; a burst of five arriving in the same millisecond
produced ten builds and failed all five.

The build is now memoized by id while it runs, so a burst shares one api and the
entry is written once. The memo is registered before the first `await` inside
the build, so nothing can interleave between the cache miss and it. It is
dropped once the build settles: a success is in the cache by then, and a failure
must not become the cached answer for that id.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014gBgEB5wBDFxWrtBhk7G7X
@paveltiunov
paveltiunov requested a review from a team as a code owner September 9, 2026 23:58
@github-actions github-actions Bot added the javascript Pull requests that update Javascript code label Sep 9, 2026
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 3m 30s —— View job


Approach is right and the fix is correct — 0 blocking, 1 medium (teardown doesn't clear the memo), 4 low (dead guard + over-long comments + test timer leak). 5 inline comments posted.

Full review

Todos

  • Read the diff (server.ts, new getOrchestratorApi.test.ts)
  • Verify the memoization is actually race-free
  • Check interaction with OrchestratorStorage disposeAfter / releaseConnections / resetInstanceState
  • Review test coverage and hygiene
  • Check comment quality against repo guidelines
  • Post inline comments and summary

The fix itself

The core reasoning holds up. buildOrchestratorApi is async, so calling it runs synchronously to its first await; getOrchestratorApi has no await between the orchestratorStorage.has() miss (line 590), the memo miss (line 594), and the set (line 613). That whole window is one synchronous turn, so the claim in the comment — nothing can interleave — is true, and it's the part that's easy to get wrong. Dropping the memo in .finally rather than only on success is also right: a rejected build must not become the cached answer, and by the time it runs a successful build is already in orchestratorStorage.

Two details I checked and found fine:

  • pending is referenced inside its own .finally callback. Safe — the callback is a microtask, so the const binding is initialized before it can run. Subtle but correct.
  • Promise.prototype.finally typechecks: root tsconfig.base.json is lib: ["es2023"].

The diagnosis is convincing too. /v1/load with total: true fetching the api twice through Promise.all makes concurrency-of-one-request the common case, not a burst-only edge, which matches the reported symptom of single isolated requests failing.

Findings

# Severity Where Issue
1 medium server.ts:613 resetInstanceState() / shutdown() clear orchestratorStorage but not buildingOrchestratorApis
2 low server.ts:599-610 === pending guard protects against a state that cannot occur, and the comment asserts the impossible race as fact
3 low test:36-44 Cores are torn down with releaseConnections(), never shutdown() — the constructor's refresh timer leaks and can perturb the release spy
4 low server.ts:133-145 13-line field JSDoc; the invariant is one sentence
5 low test:4-18, 27-32 14-line JSDoc on a helper, describing the bug rather than the helper

#1 is the one worth acting on before merge, or at least deciding about deliberately. A build in flight across a reset both lands its pre-reset api in the freshly cleared storage and — new with this PR — gets served from the memo to every post-reset caller until it settles. The first half is pre-existing; the memo widens it.

#2 is not a bug, just dead insurance with a misleading justification. buildingOrchestratorApis has exactly two writers, and the delete is the only one that can run before this check, so the entry is either pending or absent. Keep the check if you like, but the comment shouldn't describe a race that the code structure rules out.

#3 is worth a look because it can undermine the suite's own assertions. refreshWorkerMode falls through to NODE_ENV !== 'production', which is true under Jest, and isReadyForQueryProcessing() is satisfied by the apiSecret + driverFactory the fixture passes — so each of the six cores starts a real interval that outlives its test. handleScheduledRefreshInterval calls back into getOrchestratorApi, which is exactly what expect(release).not.toHaveBeenCalled() is watching. Slow CI, not a fast laptop, is where that shows up.

Tests

Coverage of the actual fix is good, and better than most concurrency-bug tests: the six cases separate sharing (one api per id), safety (nothing released behind a caller's back), the real-world trigger (total: true's two halves), cache reuse, failure not being memoized, and — the one that earns its keep — the end-to-end link from "an api was released" to "a Cube Store driver was closed." That last test is what makes this a regression test for the reported incident rather than for an implementation detail.

Two gaps I'd consider:

Agreed that an e2e spec isn't warranted here — this is microtask ordering with no deterministic user-drivable surface.

Security / performance

Nothing to flag. No input handling or auth logic is touched. The memo is bounded by concurrent cold ids and each entry is removed when its build settles, so it can't grow unbounded; if anything it reduces work by collapsing duplicate builds.

Note on verification

node_modules isn't installed in this checkout and the package's Jest config runs against dist/, so I reviewed statically and did not execute the suite. The environment-dependent claims above (refreshWorkerMode default, lib: es2023, the two-writer analysis of the memo) were each traced through the source rather than run.

· branch `pavel-claude/busy-lamport-wpo548`

Comment thread packages/cubejs-server-core/src/core/server.ts
Comment thread packages/cubejs-server-core/src/core/server.ts
Comment thread packages/cubejs-server-core/test/unit/getOrchestratorApi.test.ts Outdated
Comment thread packages/cubejs-server-core/src/core/server.ts
Comment thread packages/cubejs-server-core/test/unit/getOrchestratorApi.test.ts Outdated
Review follow-ups.

`resetInstanceState()` clears the orchestrator storage and reloads the env, but
a build still in flight kept handing its pre-reset api -- built from the
pre-reset context -- to every caller arriving until it settled. It now clears
the memo alongside the storage. That does not stop the in-flight build from
setting its api into the freshly cleared storage, which is how master behaved
too; what it stops is the memo widening that from "the caller that started the
build" to "everyone who arrives during it".

The test's teardown moves into `afterEach`, so a test that fails before its last
line no longer leaves the scheduled refresh timer running. `releaseConnections()`
rather than `shutdown()`: it is the one that cancels that interval, while
`shutdown()` only clears the compiler cache and releases connections. Also
dropped `externalDbType: 'cubestore'` from the driver test -- the external
factory is what the test needs, and the dbType additionally sent dev mode
through the `CubeStoreHandler` branch for no benefit.

Trimmed the comments to what the test names cannot say.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014gBgEB5wBDFxWrtBhk7G7X
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 1m 37s —— View job


Re-reviewed after b84140a — 0 blocking, 0 medium, 3 low. The reset leak and the leaked refresh timers are both properly fixed; 2 prior comment-length threads still stand, 1 new nit posted.

Full re-review

Todos

  • Diff the new commit against the previously reviewed state
  • Verify the resetInstanceState() fix
  • Verify the test teardown actually cancels the refresh timer
  • Re-check the memoization for anything the refactor changed
  • Resolve stale threads, dedupe against still-standing ones

What changed since the last review

b84140a addresses three of the five prior findings.

Finding #1 (medium — memo not cleared on teardown): fixed. this.buildingOrchestratorApis.clear() now sits next to this.orchestratorStorage.clear() in resetInstanceState() (server.ts:577), so a build in flight across a reset can no longer be handed to post-reset callers. The three-line comment above it states the consequence, which is the right thing to preserve there.

The residual half I mentioned — the in-flight build still set()s its pre-reset api into the freshly cleared storage, and shutdown()/releaseConnections() don't clear the memo either — I'm not re-raising. The storage write is pre-existing behaviour unrelated to this PR, and post-shutdown() callers aren't a real scenario. Fixing it properly means releasing the in-flight api rather than forgetting the promise, which is a different change than this one. Thread resolved.

Finding #3 (low — leaked refresh timers): fixed, and the teardown choice is correct. I checked the claim in the new afterEach comment against the source: releaseConnections() (server.ts:947-959) does await this.scheduledRefreshTimerInterval.cancel(), while shutdown() (line 998) does not touch the interval at all — it only clears the compiler cache, releases the gateway, and drains storage. So releaseConnections() is the right teardown here and my earlier suggestion to use shutdown() instead was wrong; the pattern in index.test.ts works only because those tests construct cores that don't start the timer. Moving it into afterEach also means a test failing before its last line no longer leaks, which is the more valuable half. Thread resolved.

Finding #5 (low — 14-line helper JSDoc): fixed. The block is gone, flushReleases is down to one line, and the surviving fact (a replaced entry gets released) is stated once. Thread resolved. One placement nit posted inline — it's now anchored to const cores, which isn't what it's about.

Still standing

Both re-affirmed rather than re-posted; the unresolved threads carry the detail.

# Severity Where Issue
2 low server.ts:610-618 The === pending guard's comment justifies it with a race the code structure rules out — the entry is either pending or absent when that finally runs. Keeping the check as insurance is fine; the comment shouldn't assert the impossible case as the reason.
4 low server.ts:133-145 13-line field JSDoc for a one-sentence invariant; the incident narrative is already in the commit message.
new low test:4-8 Four-line comment about Cube Store connections now sits above const cores.

The fix, re-verified

Unchanged and still correct. getOrchestratorApi has no await between the storage miss (line 592), the memo miss (line 598), and the set (line 617), and buildOrchestratorApi is async so it runs synchronously to its first await — the whole window is one turn. Dropping the memo in .finally rather than only on success is right: a rejected build must not become the cached answer, and a successful one is in orchestratorStorage by then. pending referenced inside its own .finally is safe (microtask, so the binding is initialized).

I also confirmed the extraction is safe to do: no subclass in the repo overrides getOrchestratorApi, and all nine other call sites (RefreshScheduler.ts, server.ts:487) go through the public method, so they all pick up the memo rather than bypassing it.

Tests

The six cases still cover the fix well — sharing, nothing released behind a caller's back, the total: true two-halves trigger, cache reuse, failure not memoized, and the connection-close link that makes this a regression test for the actual incident.

One gap remains: contextToOrchestratorId is pinned to a constant in every test, so nothing catches a fix that over-shares across ids. A two-id case asserting two distinct apis would close it. Fix this →

Security / performance

Nothing to flag. No input handling or auth logic touched. The memo is bounded by concurrent cold ids, each entry removed when its build settles, and cleared on reset — it collapses duplicate builds rather than adding work.

Note on verification

node_modules isn't installed in this checkout and the package's Jest config runs against dist/, so this was a static review; I did not execute the suite. The two claims that mattered most this round — that releaseConnections() cancels the refresh interval and shutdown() does not, and that nothing overrides getOrchestratorApi — were traced through the source directly.

· branch `pavel-claude/busy-lamport-wpo548`

Comment thread packages/cubejs-server-core/test/unit/getOrchestratorApi.test.ts Outdated
…s really for

Review follow-ups on comment length, and a correction.

The reviewer was right that the identity check in the `finally` guarded a state
that could not occur: with `set` reachable only after a falsy `get`, and `delete`
the only other writer, the entry could only ever be this build's own promise.

That stopped being true one commit ago. `resetInstanceState()` now clears the map
as well, so a build it drops mid-flight settles to find the entry owned by a
later caller's build, and an unguarded `delete` would take that build's memo with
it -- putting its callers back to building an api each, which is the whole defect
this PR is about. Reachable only when the dropped build fails: one that succeeds
fills the cache on its way out, and callers read the cache before the memo. So
the check stays and its comment now names the writer that makes it load-bearing,
with a test that fails without it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014gBgEB5wBDFxWrtBhk7G7X
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 1m 33s —— View job


Re-reviewed after 03f116e — 0 blocking, 0 medium, 3 low. Both comment-length threads are properly closed, and the guard now has a real justification backed by a test. 2 nits posted, 1 prior thread still open.

Full re-review

Todos

  • Diff 03f116e against the previously reviewed state
  • Verify the new reachability claim for the identity guard
  • Trace the new reset-crossing test for ordering soundness
  • Re-check comment length against the repo guideline
  • Resolve stale threads, dedupe against still-standing ones

What changed since the last review

03f116e closes the two remaining comment findings — and, more interestingly, it turns out my finding #2 was wrong in a way that mattered.

Finding #4 (13-line field JSDoc): fixed. Down to three lines stating the invariant and its consequence.

Finding #2 (the === pending guard's stated reason): fixed, and my analysis of it was incomplete. I claimed the map had exactly two writers, so the guard was dead insurance. It now has three — resetInstanceState() calls buildingOrchestratorApis.clear() (server.ts:572), which b84140a added in response to my finding #1. That makes the interleaving real:

  1. Build A is in flight for id X; the memo holds A.
  2. resetInstanceState() clears the memo.
  3. Caller B misses both cache and memo, starts build B, memo[X] = B.
  4. Build A settles and reaches its finally.

Without the identity check, step 4 deletes B's entry, and every caller arriving while B is still building goes back to building an api of its own — the original bug, re-entered through the reset. So the .finally(() => delete) I suggested in that thread would have been a regression once #1 was fixed. The new comment (server.ts:600-603) names this scenario correctly. Both threads resolved.

New test: a build that outlives a reset does not drop the build that replaced it. I traced the ordering, since a test for a microtask race is only worth as much as its ordering guarantees:

  • gates.push(resolve) runs in the Promise executor, so it is synchronous with entering the mock — waitFor(() => gates.length === 1) is a sound "the build has reached its gate" signal, not a sleep.
  • await expect(acrossReset).rejects.toThrow(...) is enough to guarantee the finally has already run: getOrchestratorApi returns the finally-chained promise, so the callback executes before that promise rejects. The later caller on line 162 therefore genuinely probes the post-finally state rather than racing it.
  • Against a plain delete, later builds a third api: builds becomes 3 and (await later)[0] !== (await afterReset)[0]. Both assertions fail, so the test discriminates.

The guard on build < 2 in the mock is a good call — an unheld third build makes the failure mode "assertion reports the duplicate" instead of "test times out", which is the difference between a useful regression test and an annoying one.

The reachability claim in the test's comment also checks out: a build that succeeds writes orchestratorStorage before its finally, and getOrchestratorApi reads the cache before the memo, so a succeeding build can't reach the "entry belongs to someone else" state in a way any caller observes.

Findings

# Severity Where Issue
new low test:122-126 5-line comment; the last sentence is the load-bearing one, the first two restate the test name
new low test:38-46 waitFor re-spells the setImmediate promise flushReleases already is
6 low test:4-8 (unresolved thread) the Cube Store note is still anchored to const cores

Remaining gaps

Neither is blocking; noting them so they're a deliberate choice.

  • No two-id test. contextToOrchestratorId is pinned to a constant in all seven cases, so a fix that over-shared across ids would pass the whole suite. Fix this →
  • The succeeding half of the reset crossing. If build A succeeds after a reset, it writes its pre-reset api into the cleared storage, callers get handed it, and build B's later write replaces and releases it — the reported failure, reached through a reset. This is pre-existing (both builds wrote storage before this PR too) and I said last round I wouldn't re-raise it; a real fix means releasing the in-flight api rather than forgetting its promise. Flagging only because the new test now sits directly adjacent to it.

Security / performance

Nothing to flag. No input handling or auth logic touched. The memo is bounded by concurrent cold ids, each entry removed when its build settles, and cleared on reset.

Note on verification

node_modules isn't installed in this checkout and the package's Jest config runs against dist/, so this was a static review — I did not execute the suite. The claim that decided this round (the third writer that makes the identity guard load-bearing) was traced through resetInstanceState() in the source and cross-checked against the new test's control flow.

· branch `pavel-claude/busy-lamport-wpo548`

Comment thread packages/cubejs-server-core/test/unit/getOrchestratorApi.test.ts Outdated
Comment thread packages/cubejs-server-core/test/unit/getOrchestratorApi.test.ts
Review follow-ups.

Every other case in the file pins `contextToOrchestratorId` to a constant, so a
memo keyed too loosely -- or not keyed at all -- would satisfy all of them. The
new case gives each caller an id of its own and expects an api each; keying the
memo on a constant fails it, and none of the others notice.

The note about a replaced entry being released moves above the imports, where it
reads as the file's premise rather than as an explanation of `const cores`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014gBgEB5wBDFxWrtBhk7G7X
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Claude Code is working…

I'll analyze this and get back to you.

View job run

…y cannot say

Review nits. The first two sentences restated the test name and the
`resetInstanceState()` call in it; what survives is the reason build 0 has to
throw, which nothing else in the body explains. `waitFor` now drains through
`flushReleases` rather than re-spelling the same `setImmediate` promise.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014gBgEB5wBDFxWrtBhk7G7X
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 1m 39s —— View job


Re-reviewed after 2414fb0 + dab76ac — 0 blocking, 0 medium, 1 low. Both remaining gaps from the last round are closed; every prior thread is resolved. 1 nit posted.

Full re-review

Todos

  • Diff the two new commits against the previously reviewed state
  • Verify the new per-id test actually discriminates
  • Re-check the comment trim in dab76ac
  • Re-verify the fix end to end
  • Resolve stale threads, dedupe against still-standing ones

What changed since the last review

Four things, and they close everything I had left open.

The two-id gap is closed (2414fb0). This was the one substantive gap I'd carried across two rounds: contextToOrchestratorId was pinned to a constant in all seven cases, so a memo keyed too loosely — or a getOrchestratorApi that ignored the id entirely — would have passed the whole suite. callers of different ids get an api each (test:89-100) closes it, and it discriminates properly: contextToOrchestratorId flows straight through from options (server.ts:230, no wrapping), callConcurrently already hands each caller a distinct requestId, so three ids in means three distinct apis out or the assertion fires. The expect(release).not.toHaveBeenCalled() on top also catches the other failure shape — three ids sharing one OrchestratorStorage slot and releasing each other.

waitFor reuses flushReleases (2414fb0) and the reset case's comment is down to its load-bearing half (dab76ac) — both taken as suggested. The file-level note is now genuinely file-level, above the imports, instead of anchored to const cores.

Findings

# Severity Where Issue
new low test:14-16 "One id for every caller" now reads as one id per caller four lines above a test that does exactly that

That's the only thing I found this round.

The fix, re-verified

Unchanged since 03f116e and still correct.

  • No await between the storage miss (server.ts:587), the memo miss (server.ts:592), and the set (server.ts:611); buildOrchestratorApi is async, so it runs synchronously to its first await. The whole window is one turn, which is what the comment at line 596 claims.
  • .finally rather than success-only: a rejected build must not become the cached answer, and a successful one is in orchestratorStorage (line 752) by then.
  • The identity guard is load-bearing, not insurance — resetInstanceState() is the third writer (server.ts:572), and the reset-crossing test drives exactly that interleaving.
  • pending referenced inside its own .finally is safe: the callback is a microtask, so the binding is initialized.

I also re-confirmed the release-spy assertions aren't vacuous, since the whole suite leans on them. disposeAfter invokes api.release() as the set unwinds — the call is synchronous even though the release it starts is not — so expect(release).not.toHaveBeenCalled() is reading a spy that would already have been hit had a replacement happened. The one test that needs the async half (the Cube Store driver of a live caller is not closed under it) is the one that awaits flushReleases(), correctly.

Tests

Eight cases now, and the set covers the fix from every angle that matters: sharing within an id, separation across ids, nothing released behind a caller's back, the total: true two-halves trigger that made this an everyday failure, cache reuse, a failure not being memoized, the reset-crossing interleaving that makes the identity guard necessary, and the connection-close link that ties the whole thing back to the reported ConnectionError. That last one is what makes this a regression test for the incident rather than for an implementation detail.

No further gaps I'd hold the PR for. The succeeding half of the reset crossing (an in-flight build landing its pre-reset api in freshly cleared storage) remains untouched, as agreed two rounds ago — it predates this PR, and fixing it means releasing the in-flight api rather than forgetting its promise.

Security / performance

Nothing to flag. No input handling or auth logic touched. The memo is bounded by concurrent cold ids, each entry removed when its build settles, and cleared on reset — it collapses duplicate builds rather than adding work.

Prior threads

All eight are resolved, seven by the author's commits and one by mine. Nothing re-affirmed, nothing skipped as a duplicate.

Note on verification

node_modules isn't installed in this checkout (no eslint, and the package's Jest config runs against dist/), so this was a static review — I did not execute the suite or the linter. The claim that decided this round, that the new test's contextToOrchestratorId override reaches getOrchestratorApi unwrapped, was traced through the constructor at server.ts:230.

· branch `pavel-claude/busy-lamport-wpo548`

Comment thread packages/cubejs-server-core/test/unit/getOrchestratorApi.test.ts Outdated
"One id for every caller" parses as one id per caller, which is the opposite of
what the fixture does -- and it now sits a few lines above a test named "callers
of different ids get an api each", where that misreading has somewhere to land.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014gBgEB5wBDFxWrtBhk7G7X
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @paveltiunov's task in 54s —— View job


Re-reviewed after f609972 — 0 blocking, 0 medium, 0 low. All three nits from the last round were taken and the two-id gap is closed; nothing new to raise, no open threads remain.

Full re-review

Todos

  • Diff 2414fb0, dab76ac, f609972 against the previously reviewed state
  • Review the new two-id test for discriminating power
  • Re-check the memo logic and comments end to end
  • Resolve stale threads, dedupe against still-standing ones

What changed since the last review

Three test-only commits; server.ts is untouched since 03f116e.

2414fb0 — the file-level note moved, and the two-id gap closed. The Cube Store fact now sits above the imports, so const cores is self-explanatory again. More importantly this adds callers of different ids get an api each, which was the one real coverage gap I'd flagged twice: every other case pins contextToOrchestratorId to a constant, so a memo keyed on a constant — or not keyed at all — satisfied the whole suite. I checked it discriminates: with a constant key all three callers share one api and new Set(apis).size is 1 against an expected 3. The expect(release).not.toHaveBeenCalled() on the same test is meaningful too — three distinct ids never replace each other in an OrchestratorStorage with max 100, so any release there would mean over-sharing.

dab76ac — the reset case's comment trimmed to the sentence the body can't say, and waitFor now drains through flushReleases. Both taken verbatim.

f609972 — "one shared id for all callers". The phrase no longer reads as the opposite of what it means now that a test named callers of different ids get an api each sits four lines below it.

The fix, re-verified

Unchanged and still correct. getOrchestratorApi (server.ts:583) resolves the id, misses the storage, misses the memo, and sets — with no await anywhere in that window, and buildOrchestratorApi being async runs synchronously to its first await, so the whole thing is one turn. The .finally drops the memo on both outcomes (a success is in the cache by then; a failure must not become the cached answer), and the identity check is load-bearing because resetInstanceState() is a third writer — that's the case a build that outlives a reset does not drop the build that replaced it drives.

Tests

Eight cases, and the set now separates every dimension that matters: sharing within an id, not sharing across ids, nothing released behind a caller's back, the total: true two-halves trigger, cache reuse, failure not memoized, the reset-crossing identity guard, and the end-to-end link from "an api was released" to "a Cube Store driver was closed" that makes this a regression test for the reported incident rather than for an implementation detail. Each of the two added-from-review cases was verified by mutation rather than assumed, which is the right standard for a concurrency suite.

The one residual I noted last round and said I wouldn't re-raise still stands as known and deliberate: a build in flight across a reset still writes its pre-reset api into the freshly cleared storage. That's how master behaves too, and fixing it means releasing the in-flight api rather than forgetting its promise — a different change than this regression fix.

Threads

All nine of my prior threads are already resolved; nothing to resolve or dedupe this round.

Security / performance

Nothing to flag. No input handling or auth logic touched. The memo is bounded by concurrent cold ids, each entry removed when its build settles, and cleared on reset — it collapses duplicate builds rather than adding work.

Note on verification

node_modules isn't installed in this checkout and the package's Jest config runs against dist/, so this was a static review; I did not execute the suite. The claim that mattered this round — that the new two-id test actually discriminates against a constant-keyed memo — was traced through contextToOrchestratorId resolution at server.ts:584 and the fixture's per-requestId override.

· branch `pavel-claude/busy-lamport-wpo548`

Copy link
Copy Markdown
Member Author

Drivers tests: the 5 databricks-jdbc jobs are red, and it isn't this PR's.

Failing on run 34420899436 (head 03f116e):

  • tests (databricks-jdbc, true)
  • tests (databricks-jdbc-export-bucket-azure, true)
  • tests (databricks-jdbc-export-bucket-gcs, true)
  • tests (databricks-jdbc-export-bucket-gcs-prefix, true)
  • tests (databricks-jdbc-export-bucket-s3-prefix, true)

Why it isn't this PR's. The root error is the Java bridge failing to load, identically in all five:

error: Internal Error: Error: Cannot find module '../build/jvm_dll_path.json'
Require stack:
- /cubejs/node_modules/java/src-node/nodeJavaBridge.js
- /cubejs/node_modules/java/index.js
- /cubejs/packages/cubejs-jdbc-driver/index.js
- /cubejs/packages/cubejs-databricks-jdbc-driver/dist/src/DatabricksDriver.js

jvm_dll_path.json is a build artifact of the java npm package, produced when its native addon is built at install time. It is missing, so every query against the driver fails at connect — 106 of 145 tests in each job, not a subset.

Three things put it outside this diff:

  1. This PR changes two files, both TypeScript in packages/cubejs-server-core — no Java, no JDBC, no driver, no package.json, no lockfile, no Dockerfile.
  2. In the same run all 29 other driver jobs passed — snowflake (all 8 export-bucket variants), clickhouse, postgres, mysql, mssql, bigquery, athena, oracle, pinot, crate, questdb. The only jobs red are exactly the ones that load the java bridge. A server-core change cannot break the JDBC bridge alone and leave 29 other drivers green.
  3. server-core appears in the require stack only because it is what resolves the driver — the throw is two frames below it, inside node_modules/java.

No fix to port. Nothing in the repo or in open PRs references jvm_dll_path, so there's no existing change I could pull into this branch to make it green.

Re-run: not spending a manual one — the three pushes since have already re-queued these jobs on newer heads (9193, 9194, 9195 on the current head f609972). I'll report what they land on; if they come back green this was transient in the install step, and if they stay red it's a java-package/image issue that needs its own fix rather than one in this PR.


Generated by Claude Code

@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.24%. Comparing base (185c1a9) to head (f609972).
⚠️ Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (185c1a9) and HEAD (f609972). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (185c1a9) HEAD (f609972)
cubesql 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #11834       +/-   ##
===========================================
- Coverage   80.30%   60.24%   -20.06%     
===========================================
  Files         500      239      -261     
  Lines      106354    19213    -87141     
  Branches     3886     3888        +2     
===========================================
- Hits        85405    11575    -73830     
+ Misses      20399     7089    -13310     
+ Partials      550      549        -1     
Flag Coverage Δ
cube-backend 60.24% <100.00%> (+0.07%) ⬆️
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.

Copy link
Copy Markdown
Member Author

Following up on the CI comment above — and the answer turns out to settle it for every red job here.

server.ts has been byte-identical since b84140a; 03f116e changed only comments in it, and 2414fb0 / dab76ac / f609972 are test-file-only. So any failure caused by the code would have to reproduce across all of them. Here is what the runs actually did:

run head Drivers tests
9191 b84140a 47/47 green — every databricks-jdbc job and both bigquery-export-bucket-gcs variants passed
9192 03f116e 7 × databricks-jdbc red, jvm_dll_path.json missing
9194 dab76ac bigquery-export-bucket-gcs, false red

The same code is green in 9191 and red in 9192/9194, so the code isn't what changed. What did change is that I pushed six times in about half an hour, and drivers-tests.yml doesn't cancel superseded runs — five Drivers runs were in flight against the same fixtures at once.

That also explains the BigQuery failure specifically, which is a different one from the JDBC bridge and worth naming. All 5 of its failures are:

Cube pre-aggregations build failed: failure: Not found:
  Table cube-open-source:dev_pre_aggregations.ec__s_a_external_ixfnquip_hd3si5hr_1la3vhu

111 tests passed and all 96 snapshots matched in that same job, so the driver connected and queried fine — only the pre-aggregation-backed tests failed, and only by not finding tables that had just been built. cube-open-source:dev_pre_aggregations is one shared dataset across all concurrent runs, and pre-aggregation cleanup drops tables it has no touch record for, so a sibling run reaps the tables this one just wrote. Concurrency against a shared dataset, induced by my push rate rather than by the diff.

What I'm doing about each:

  • databricks-jdbc — nothing to push; green on b84140a with this code, and no fix exists to port. Superseded runs will settle.
  • bigquery-export-bucket-gcs — nothing to push, same reason. It needs the concurrent runs to drain, which happens on its own now that I've stopped pushing.
  • integration-smoke (24.x, 3.13) on the current head — separate cause again: Failed to connect to Reaper, testcontainers' Ryuk sidecar, thrown from QuestDBRunner.startContainer() inside beforeAll. The suite died before any test body ran, so I've re-run it (run 34421545041) — that is my one re-run spent.

No code change is warranted for any of the three. I'll report where the current head lands.


Generated by Claude Code

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants