fix(server-core): release orchestrators evicted from the LRU

[paveltiunov]

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-823

Description of Changes Made

Two halves of one leak: nothing asked the driver to close, and asking it to close did not close it.

OrchestratorStorage never released an evicted orchestrator. It keeps OrchestratorApi instances in an LRUCache with no dispose handler, so an entry pushed out by max — or replaced, deleted, expired — was dropped without release() ever being called. Only releaseConnections() at shutdown released anything.

Nothing else closes that socket: the driver pings it every 5s, so neither side times it out, and that live timer keeps the whole orchestrator reachable from a GC root. A collected socket would not be closed anyway — V8 runs no destructors and neither ws nor net registers a finalizer. In Node a socket is closed explicitly or never.

disposeAfter now covers every removal reason — disposeAfter rather than dispose because release() is async and calls into the drivers, while dispose runs synchronously inside set()/delete(). The scheduled releases are tracked in pendingReleases so releaseConnections() can await them; clear() only schedules them, so without that shutdown could return with the connections still open. A failing release is swallowed there so one orchestrator cannot fail shutdown, and logged by OrchestratorApi.release() before rethrowing, since that is otherwise the only signal an operator would have that this fix is not working in their deployment.

WebSocketConnection.close() did not close the connection. It called webSocket.close() and stopped there, so the 'close' handler re-sent whatever was in flight over a fresh connection and the heartbeat on that one kept the socket alive again; a query arriving after the close re-opened it just as readily. Releasing the driver therefore freed nothing, which is why the dispose hook alone would not have been enough.

A single closed flag makes the close terminal. initWebSocket() refuses once it is set, which covers both a late query and the re-send path, whose catch then fails the messages that were in flight instead of re-opening for them. Messages already in flight are still given the chance to be answered — an eviction can land mid-query and Cube Store may already be working on the answer — so the socket goes away once the last of them settles, or immediately if there are none.

That wait is bounded, and the bound lives in the existing heartbeat interval rather than a timer of its own: that interval already owns "this socket has waited long enough" and is already the thing keeping the socket reachable. It cannot be left to the ordinary no-heartbeat check, because Cube Store keeps answering the pings of a connection whose query is simply never completed — so without an explicit bound a single wedged message would keep the socket, and the interval, for the life of the process. Pinging continues while a closed connection drains, so one draining legitimately is not dropped for inactivity. When the bound fires, whatever is left is rejected naming the count: a stuck message costs one query.

Two smaller things in the same file:

• The heartbeat interval is stopped through teardown() wherever the socket dies, not only in the 'close' handler. It is the timer, not the socket, that keeps the graph reachable.
• The 'error' handler returns early when the connection is closed — there is nothing to retry towards, and it stops arming a pointless timer per evicted connection — but rejects readyPromise first, since on a socket that never opened that is the only thing left which can settle it. The retry itself uses .then(resolve, reject) rather than resolve(promise): on a socket whose readyPromise has settled, resolve returns without adopting what it was given, so a rejection handed to it was never observed, which is fatal under Node's default --unhandled-rejections=throw. The re-send also bails out when everything it was scheduled for has settled in the meantime.

Measured in production before the fix

~450 new sockets/minute per API pod (429 on a pod 43s after restart, 4,566 at 10 min, 5,198 at 16 min; a pod serving no traffic held 1), 65,551 established sockets on the Cube Store router they pointed at, and that region's ingress OOM-killed 38–46 times in six hours. Reproduced on deployments pinned to v1.7.2 and v1.7.7, so it is not version-specific.

Behaviour change worth a reviewer's attention

close() is terminal, so a caller still holding a released driver gets ConnectionError: Cube Store connection is closed rather than a silently re-opened socket. That is the trade that makes the leak impossible, and it is what an eviction now costs: release() has no drain above the socket, so the next Cube Store round trip of a query still executing on an evicted orchestrator fails. Reachable through the cold-start race in getOrchestratorApi() (has() at server.ts:575, then several awaits before set(), so two concurrent requests for a cold id both set() and the loser is released while in use), through RefreshScheduler holding one api across a run, and through any request outliving the arrival of max distinct ids. Discussion and options are on this thread.

Tests

packages/cubejs-server-core/test/unit/OrchestratorStorage.test.ts (new, 7 tests): eviction, replacement and same-instance re-set; releaseConnections() waits for the releases clear() schedules; it resolves and clears even when one release rejects; a failing release is logged; a finished release is forgotten rather than accumulating in the set.

packages/cubejs-cubestore-driver/test/websocket-connection.test.ts (+7 tests, in the existing suite that drives a mock Cube Store over real TCP): the socket closes when nothing is in flight; the connection stays OPEN until an in-flight query is answered and closes once drained; a query issued after close does not establish a new connection; a socket that dies while draining does not re-open; a message Cube Store never answers is given up on rather than holding the socket; a query still connecting when the close lands is rejected rather than hung; and a socket error on a closed connection leaves no unhandled rejection.

Each was verified to fail against the commit it guards — the connecting-close one by hanging to the jest timeout, the unhandled-rejection one via process.on('unhandledRejection'). All 15 pre-existing tests in that suite still pass, which is the check on the teardown and retry-path changes not regressing the re-send behaviour.

cd packages/cubejs-cubestore-driver && yarn unit: 22 passed.
cd packages/cubejs-server-core && yarn unit: 108 passed, 1 failed — Refresh Scheduler › Exponential backoff, which fails identically on master with this change reverted, so it is pre-existing and unrelated.

Follow-ups, not in this PR

• server.ts constructs new OrchestratorStorage() with no arguments, so the LRU always uses the default max: 100 and silently ignores compilerCacheSize / maxCompilerCacheKeepAlive / updateCompilerCacheKeepAlive. Two things belong with that change rather than ahead of it: ttlAutopurge is unset, so an expired entry is only reaped when something touches the cache, and new OrchestratorStorage({}) throws because max is then undefined. Both are latent while the storage is built with no arguments.
• getOrchestratorApi() is not de-duplicated by id, and getQueryOrchestrator() (RefreshScheduler.ts:238, 621, 812, 831) reaches the queues and cache driver without going through the api at all.
• release() calls the external driver factory unconditionally, and CubeStoreDriver.testConnection() runs SELECT 1, so releasing an orchestrator that never built that driver does a connect + query + close. Guarding it was tried and reverted as not worth the complexity — the wrapper it needs drew two defects in review. How often it applies depends on the config: under cacheAndQueueDriver: 'cubestore' the queue and cache go through the same factory (QueryOrchestrator.ts:90), so any orchestrator that served a query has already built the driver and the memo makes the release build a no-op; under memory with Cube Store still external, an orchestrator that served only non-pre-aggregated queries never touches the factory and every eviction does pay the round trip.

---

Generated by Claude Code

[claude]

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

---

Review: sound fix for a real leak — 1 high, 3 medium, 4 low. Details below.

Full review (5 inline comments posted)

• Read the diff
• Review OrchestratorApi.ts changes
• Review OrchestratorStorage.ts changes
• Review tests
• Post review

The diagnosis is right and well-evidenced: LRUCache with no dispose hook + a driver that keeps its own socket alive + no finalizers in V8 = one Cube Store connection leaked per eviction. disposeAfter (rather than dispose) is the correct hook — it runs after the mutating operation completes, so a release that re-enters the cache can't corrupt it — and idempotent release() + releaseInBackground() are the right two guards for it. Comments explain why, not what. Tests are genuinely adversarial (verified to fail without the fix). No security or input-handling concerns in this diff.

The findings below are all about the new lifecycle rather than the LRU hook itself.

High

Use-after-release is unguarded, and on the Cube Store path it re-creates the leak — OrchestratorApi.ts:271-285. trackOperation never consults releasePromise, and getOrchestratorApi() (server.ts:573-718) is not de-duplicated by id, so two concurrent requests for a cold orchestratorId both build an api and both set() it. The loser is disposed and released while the request that created it still holds the reference; its next executeQuery re-opens a Cube Store connection on an orchestrator that is no longer in the LRU, so nothing will ever release it again. Cold-id concurrency is exactly what a high-cardinality contextToOrchestratorId produces. Fix this →

Medium

• Unbounded drain can hang shutdown — OrchestratorApi.ts:304-316. whenIdle() has no timeout, and shutdown() awaits it transitively. One operation stuck on a dead socket turns a previously-unconditional shutdown into a hang.
• The idle guarantee doesn't hold for long queries — OrchestratorApi.ts:87-89. pt.timeout rejects with Continue wait while the query keeps running in the queue, so activeOperations returns to 0 between client polls; isJob behaves the same way. Strictly better than today, but weaker than the comment claims.
• RefreshScheduler bypasses tracking entirely. getQueryOrchestrator() is public and used at RefreshScheduler.ts:238, 621, 812, 831 to reach getPreAggregations() and the cache driver directly. Those calls touch drivers without incrementing activeOperations, and the scheduler holds one orchestratorApi across a whole refresh run. Same root cause as the high finding.

Low

• releaseDriver instantiates drivers that never existed. driverFn(dataSource) is the lazy factory, and server.ts:668-700 calls testConnection() inside it. An orchestrator that never touched the external driver now opens a Cube Store connection on eviction just to close it — at the ~450 evictions/min/pod in the PR body, that is real churn against the router that was OOM-ing, and a testConnection() failure surfaces as Orchestrator Release Error on an orchestrator with nothing to release. Couldn't comment inline (unchanged line), but it is OrchestratorApi.ts:332-339.
• expire isn't really covered without ttlAutopurge — OrchestratorStorage.ts:22-30. Latent today since ttl is always undefined, live as soon as the follow-up lands.
• Streams outlive their tracked operation — OrchestratorApi.ts:79. streamQuery is tracked only until the stream is returned.
• releaseConnections() uses Promise.all (OrchestratorStorage.ts:58-61, unchanged). One rejecting release() skips this.storage.clear(), leaving entries in the cache — and since releasePromise now caches the rejection, every retry rejects again, so the storage can never be cleared. Promise.allSettled (or catching per-api the way releaseInBackground does) makes shutdown robust.

Tests

Good coverage of the paths the PR set out to cover, and the fixtures are honest about what they stub. Gaps worth adding, matching the findings above: an operation started after release() resolved; a release whose driver release() rejects, asserting releaseConnections() still clears the storage; and a Continue wait rejection asserting what the drain does (or doesn't) wait for. Note that in both fixtures driverFactory and externalDriverFactory return the same object and seenDataSources is empty, so toHaveBeenCalledTimes(1) only exercises the external branch — a case with a seen data source would make the per-datasource loop meaningful.

No docs changes needed; this is internal lifecycle behaviour.

· branch `claude/cube-store-oom-outage-h9xy56`

[claude]

Claude finished @paveltiunov's task in 2m 20s —— View job

---

Re-review of 9237364: both prior blockers addressed — 1 new medium, 1 new low; 3 earlier threads still open.

Full review (2 inline comments posted, 2 stale threads resolved)

• Read the new commit's diff
• Check which prior findings are addressed
• Resolve stale threads / post new findings

Addressed since the last review

• Use-after-release (was high) — released + the re-arm in trackOperation (OrchestratorApi.ts:292-303) means an operation that runs on an instance the LRU has already dropped triggers a second close instead of leaking the re-opened driver. Thread resolved.
• Unbounded drain (was medium) — drain(timeout) (OrchestratorApi.ts:322-354) with a 10s default, null on the eviction path, and an Orchestrator Release Drain Timeout log line. release(null) from releaseInBackground keeps eviction patient; releaseConnections() → release() gets the bound, which is the right split. Thread resolved.

Both are covered by tests, and the drain test asserts the timeout log rather than just the close — good.

New findings

Medium — a failed release is permanent (OrchestratorApi.ts:369-383). released = true is inside the .then(), so a rejecting driver release() leaves releasePromise holding the rejection and released === false. Every later release() re-returns the rejection, the re-arm never fires for that instance, and releaseConnections()'s Promise.all skips storage.clear() — so shutdown leaves the entries in the cache and the sockets open, with no retry path. Clearing releasePromise in a .catch (plus Promise.allSettled in releaseConnections) fixes both halves. Fix this →

Low — two gaps the re-arm still leaves (OrchestratorApi.ts:292-303). (a) An operation that starts and finishes between the drain resolving and released = true being set doesn't re-arm, though it is the one most likely to have overlapped the close; setting the flag before the Promise.all would fix it. (b) For a holder that keeps one instance across a whole run (RefreshScheduler.ts:620-633), every call on a released instance now costs a full driver close + cleanup() — a per-call open/close cycle instead of a leak. Better failure mode, worth naming in the comment; de-duplicating getOrchestratorApi() by id removes the cause.

Minor, not posted: a timed-out drain() leaves its whenIdle() resolver in drainListeners — harmless, it just fires on the next drain.

Still open from the previous review

Re-affirmed, not re-posted:

• Continue wait / isJob make the "waits for the in-flight query" comment stronger than the code (thread) — unchanged in this commit.
• streamQuery is tracked only until the stream is returned (thread).
• expire isn't reaped without ttlAutopurge (thread).
• Not inline (unchanged lines): getQueryOrchestrator() bypasses trackOperation entirely (RefreshScheduler.ts:238, 621, 812, 831), and releaseDriver instantiates drivers that were never used, opening a Cube Store connection on eviction just to close it (OrchestratorApi.ts:399-406).

Tests

The two new tests are well-targeted — the late-operation one asserts the second close, and the bounded-drain one would hang to the jest timeout without the fix. Still missing, matching the medium above: a rejecting driver release() asserting that releaseConnections() clears the storage and that a retry can still succeed.

I couldn't run the suite in this job (test execution wasn't permitted here), so the above is static review only.

· branch `claude/cube-store-oom-outage-h9xy56`

[claude]

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

---

Re-review of 459cc46: three prior findings addressed — 1 new medium; the "failed release is permanent" medium is still open.

Full review (1 inline comment posted, 3 stale threads resolved)

• Read the new commit's diff
• Check which prior findings are addressed
• Resolve stale threads / post new findings

Addressed since the last review

• streamQuery tracked only until the stream is returned — holdUntil() (OrchestratorApi.ts:330-340) keeps the operation counted until the stream ends, and it is called from inside the outer trackOperation, so the count never dips between the two. Thread resolved.
• expire not reaped without ttlAutopurge — ttlAutopurge is now set whenever a ttl is (OrchestratorStorage.ts:19), with a test that lets an entry expire untouched. The adjacent max: options.compilerCacheSize || 100 fix means new OrchestratorStorage({}) no longer throws. Thread resolved.
• Continue wait / isJob drain early — the activeOperations comment (OrchestratorApi.ts:35-43) now says what the counter actually guarantees and names queue-state gating as the way to close the gap. Narrowing the comment was one of the two options offered; thread resolved.

New finding

Medium — a stream that never emits close/error pins the eviction release forever (OrchestratorApi.ts:96-107). Eviction calls release(null) — an unbounded drain — and this hold only settles on 'close' or 'error'. An abandoned stream (aborted client, a pipeline that errors before this stream is wired in) therefore keeps activeOperations above 0 for the life of the process, so the orchestrator and its Cube Store socket are never released: one leaked stream now costs a whole orchestrator, which is the failure mode the PR exists to remove. It is silent too, since null means the Orchestrator Release Drain Timeout line never fires on that path. Either bound the eviction drain generously instead of passing null, or settle the hold with stream.finished() (covers end/finish/error/close and cleans up its own listeners) rather than two once handlers. Same comment notes that once('error') now makes this the stream's first error listener, so a pre-consumer 'error' is swallowed instead of thrown — likely an improvement, worth being deliberate about.

Still open from the previous review

Re-affirmed, not re-posted:

• A failed release is permanent (thread) — unchanged in this commit. released = true is still only inside the .then() (OrchestratorApi.ts:406-415) and releaseConnections() still uses Promise.all (OrchestratorStorage.ts:66-69), so one rejecting driver release() pins the instance, blocks the re-arm, and leaves storage.clear() unreachable at shutdown. This is the one substantive item left.
• Two gaps the re-arm leaves (thread) — the drain→flag window, and the per-call close/re-open cycle for long-lived holders.
• Not inline (unchanged lines): getQueryOrchestrator() bypasses trackOperation entirely (RefreshScheduler.ts:238, 621, 812, 831), and releaseDriver instantiates drivers that were never used, opening a Cube Store connection on eviction just to close it (OrchestratorApi.ts:426-433).

Tests

Both new tests are well-shaped: the stream test asserts the release stays pending until 'close', and the autopurge test lets the ttl lapse without touching the cache, which is precisely the path that was broken. Still missing, matching the open medium: a rejecting driver release() asserting that releaseConnections() clears the storage and that a retry can succeed. Also uncovered by design in the stream test — a stream that never emits anything, which is the new finding above.

Test execution wasn't permitted in this job, so the above is static review only.

· branch `claude/cube-store-oom-outage-h9xy56`

[claude]

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

---

Re-review of 387b43d: all four prior findings addressed — 1 new low-medium, 1 new low. No blockers left.

Full review (2 inline comments posted, no stale threads to resolve — all prior threads already resolved)

• Read the new commit's diff
• Check which prior findings are addressed
• Resolve stale threads / post new findings

Addressed since the last review

• A failed release is permanent (was the last open medium) — the .catch at OrchestratorApi.ts:459-468 drops the cached rejection so the drivers are retried, released is now set before the close so a failure still leaves the re-arm armed, and releaseConnections() uses Promise.allSettled (OrchestratorStorage.ts:71) so clear() always runs — which disposes the leftovers and retries them in the background. Both halves covered by tests, including the storage-level one asserting the cache is empty after a failing release.
• The drain→flag window — released = true moved above the Promise.all, so an operation that starts and finishes while the close is in flight now re-arms. The test drives exactly that by stalling cleanup().
• The re-arm double-closing — rearmRelease() waits for the in-flight release to settle before starting the next one, rather than racing it.
• Stream hold pinned by a stream that never emits close — stream.finished() (line 121) covers end/finish/error/close and cleans up its own listeners, and eviction now drains for at most evictionDrainTimeout (10 min) instead of null, so even a wedged hold ends in a close with the Orchestrator Release Drain Timeout line. The autoDestroy: false test is a good, precise reproduction of the old hang.

The per-call open/close cost for long-lived holders and the Continue wait / job caveat are both now named in comments rather than implied — that was the right resolution for those.

New findings

Low-medium — the catch can clear a newer release from the cache (OrchestratorApi.ts:465). Now that rearmRelease() nulls releasePromise while the release that set released may still be running, a second release can occupy the slot before the first settles. When the first then rejects, its catch does an unconditional this.releasePromise = null and evicts the newer one — so the next release() runs concurrently with a close that is still in progress, which is the case the idempotency guard exists to rule out. Guarding with if (this.releasePromise === releasePromise) closes it. Fix this →

Low — the drain timer isn't unref()'d (OrchestratorApi.ts:413). It is a 10-minute timer on the eviction path and clearTimeout only runs when the race settles, so a background release whose drain is stuck keeps the event loop alive and can delay process exit long after shutdown(). Same comment notes that a timed-out drain() leaves its whenIdle() resolver in drainListeners — harmless, but it accumulates one entry per timed-out release.

Still open from earlier reviews (unchanged lines, not re-posted)

• getQueryOrchestrator() bypasses trackOperation entirely — RefreshScheduler.ts:238, 621, 812, 831 reach the queues and cache driver directly.
• releaseDriver calls driverFn(dataSource) for the external driver unconditionally, so an orchestrator that never used it opens a Cube Store connection on eviction just to close it (OrchestratorApi.ts:488-495).

Both are pre-existing shapes rather than regressions, and the PR body's getOrchestratorApi() de-duplication follow-up is the natural home for the first.

Tests

Four new tests, each targeting one of the fixed findings, and each would fail against the previous commit — the autoDestroy: false stream test and the stalled-cleanup() overlap test in particular are hard to pass by accident. Remaining gap matching the new finding: a rejecting release that overlaps a re-armed one, asserting only one close runs at a time.

I couldn't run the suite in this job (node_modules isn't installed and installs aren't permitted here), so the above is static review only.

· branch `claude/cube-store-oom-outage-h9xy56`

[paveltiunov]

The two findings that couldn't be posted inline (unchanged lines) — one fixed, one deferred.

releaseDriver instantiating drivers that never existed — fixed in e5c3c7d. Confirmed against server.ts:667-700: the external factory memoizes, and on first call it builds the driver and runs testConnection(). So an orchestrator that never touched Cube Store was opening a connection on eviction purely to close it — new churn this branch introduces, against the router the PR exists to relieve. externalDriverFactory is now wrapped in the OrchestratorApi constructor, before QueryOrchestrator destructures it off options, so the release can tell whether anything built the driver; the flag resets on each close, so only a rebuilt driver is closed again. The data-source loop was already guarded this way by seenDataSources. Test release › does not build the external driver just to close it fails against 387b43df.

That also closed the test gap in the same finding: both fixtures now register a seen data source and hold a separate external driver, so the per-datasource loop is actually exercised and the two closes are counted apart.

getQueryOrchestrator() bypassing trackOperation — deferred, recorded in CORE-823. Real, and the same root cause as the use-after-release finding, but closing it means either changing the RefreshScheduler call sites (RefreshScheduler.ts:238, 621, 812, 831) or interposing on the orchestrator itself — a wider change than a leak fix should carry, and the drain is best-effort by design (the activeOperations comment now says so). The re-arm in rearmRelease() bounds the damage: a scheduler call on a released instance triggers another close rather than a permanent leak. De-duplicating getOrchestratorApi() by id is the follow-up that removes the cause for both.

Also noted, not changed: a timed-out drain() leaving its resolver in drainListeners is harmless — it fires on the next drain and the array is swapped out wholesale, so it cannot accumulate per release.

Local verification on e5c3c7d4: tsc and eslint clean; 20/20 Orchestrator tests; full package suite 120 passed / 1 failed (Refresh Scheduler › Exponential backoff, pre-existing on master — reproduced by stashing src/).

---

Generated by Claude Code

[claude]

Claude finished @paveltiunov's task in 2m 10s —— View job

---

Re-review of e5c3c7d: the external-driver finding is addressed — 1 new low-medium, 1 new low; 2 earlier threads still open.

Full review (2 inline comments posted, no stale threads to resolve)

• Read the new commit's diff
• Check which prior findings are addressed
• Resolve stale threads / post new findings

Addressed since the last review

• releaseDriver instantiated a driver that never existed — the factory is now wrapped in the constructor before QueryOrchestrator captures it off options (OrchestratorApi.ts:86-96), which is the right place: testConnection/testOrchestratorConnections (lines 269, 275) and the orchestrator's own pre-aggregation path all go through the wrapper, so the flag sees every builder. An orchestrator that never touched Cube Store no longer opens a connection on eviction just to close it. The two new tests are precise — one asserts the factory isn't called at all, the other builds through options.externalDriverFactory first and asserts a single close — and both fixtures now register a seen data source with a distinct external driver, so the per-datasource loop is genuinely exercised and the two closes are counted apart. That was the test-quality gap flagged in the first review; it's closed.

New findings

Low-medium — the release re-sets the flag it just reset (OrchestratorApi.ts:482). releaseDriver(this.options.externalDriverFactory) calls the wrapper, so externalDriverCreated flips back to true immediately after line 478 clears it. The invariant in the commit message holds only for an orchestrator that never built the driver; once one has, every later release (the rearmRelease() path, or releaseConnections() on an instance already released on eviction) closes it again — and since server.ts's externalPreAggregationsDriverPromise is never cleared on release, that is a repeat release() on the same closed driver, not a fresh one. Releasing through the captured unwrapped factory fixes it.

Low — a failed build counts as built, and the wrapper drops its argument (OrchestratorApi.ts:91-95). The flag is set before awaiting, so a factory that creates the driver and then fails testConnection() — which server.ts handles by releasing the driver and nulling the memo, leaving nothing open — still marks it created, and the eviction release then rebuilds the connection just to close it: the exact churn this commit removes. Setting it on fulfilment avoids that. Separately the wrapper is () => externalDriverFactory(), discarding the dataSource that releaseDriver passes; inert with today's factory, and TypeScript won't flag the narrower signature. Same comment notes the wrapper mutates the caller's options object rather than a copy.

Still open from earlier reviews

Re-affirmed, not re-posted (both still live at the same lines):

• The catch clears releasePromise without an identity check, so it can evict a newer in-flight release from the cache (thread, OrchestratorApi.ts:491).
• The drain timer isn't unref()'d, and it is now a 10-minute timer on the eviction path (thread, OrchestratorApi.ts:434).

And, unchanged lines so not inline: getQueryOrchestrator() bypasses trackOperation entirely (RefreshScheduler.ts:238, 621, 812, 831), covered by the getOrchestratorApi() de-duplication follow-up recorded in the PR body.

Tests

Remaining gaps, matching the two findings above: a second release() after a close that did release the external driver, asserting the driver isn't closed twice; and a factory that rejects, asserting the next release doesn't call it again. Neither is a blocker.

I couldn't run the suite in this job (node_modules isn't installed and installs aren't permitted here), so the above is static review only.

· branch `claude/cube-store-oom-outage-h9xy56`

[claude]

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

---

Re-review of c24449e: both prior findings addressed, no new ones. The two external-driver threads from e5c3c7d are the only things still open.

Full review (0 new inline comments, 2 stale threads resolved)

• Read the new commit's diff
• Check which prior findings are addressed
• Resolve stale threads / post new findings

Addressed since the last review

• The catch could evict a newer release from the slot — release() now captures the promise in a local and the catch clears this.releasePromise only while it still identity-equals that local (OrchestratorApi.ts:492-500). Traced the interleaving the commit message describes: A in flight → rearmRelease() nulls the slot → B assigned → A rejects → guard sees B and leaves it → the rearm's own deferred releaseInBackground() resolves to B rather than starting a third close. The claim that rearmRelease() needs no such check holds — it reads releasePromise and reassigns it with no await between, so nothing can take the slot mid-way. Thread resolved.
• The drain timer wasn't unref()'d, and a timed-out drain leaked its listener — timer.unref() at line 421, and the timeout branch now filters its own onIdle out of drainListeners. Folding whenIdle() into drain() to get a handle on the resolver is the right simplification; drain() was its only caller, and the activeOperations === 0 early return already covers the case whenIdle() was short-circuiting. Thread resolved.

The new test is precise: it drives the exact three-release interleaving, asserts the slot still holds the second release after the first rejects, and would fail on e5c3c7d where the unconditional null dropped it. toHaveBeenCalledTimes(2) at the end pins that no third close ran.

Nothing new found in this commit. The one behaviour worth naming but not worth a change: when rearmRelease()'s deferred releaseInBackground() finds a release already in the slot, it inherits that release's drain bound — so an eviction-triggered re-arm can end up bounded by the 10s shutdown timeout rather than evictionDrainTimeout. Benign, and arguably what you want during shutdown.

Still open from the previous review

Both from e5c3c7d, unchanged in this commit, so re-affirmed rather than re-posted:

• release() closes through this.options.externalDriverFactory, which is the wrapper — so externalDriverCreated flips back to true immediately after line 483 resets it, and the next release closes an already-closed external driver (thread, OrchestratorApi.ts:487).
• The wrapper sets the flag before awaiting, so a build that fails testConnection() counts as created and the eviction release rebuilds the connection just to close it; and it calls externalDriverFactory() without threading dataSource through (thread, OrchestratorApi.ts:91-95).

And, as recorded in the PR body: getQueryOrchestrator() bypasses trackOperation entirely (RefreshScheduler.ts:238, 621, 812, 831), deferred to the getOrchestratorApi() de-duplication follow-up in CORE-823.

Tests

The remaining gaps match the two open threads: a second release() after a close that did release the external driver, asserting it isn't closed twice; and a factory that rejects, asserting the next release doesn't call it again. Neither blocks.

node_modules isn't installed in this job and installs aren't permitted, so the above is static review only — I couldn't run the suite.

· branch `claude/cube-store-oom-outage-h9xy56`

[claude]

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

---

Re-review of 32925d2: both wrapper defects fixed, no new findings. Nothing open but the deferred RefreshScheduler item.

Full review (0 new inline comments, no stale threads — all prior threads already resolved)

• Read the new commit's diff
• Check which prior findings are addressed
• Resolve stale threads / post new findings

Addressed since the last review

• The release re-set the flag it had just reset — untrackedExternalDriverFactory captures the factory as provided, before the constructor installs the wrapper, and release() closes through that (OrchestratorApi.ts:99, :502). The reset at line 498 now survives the close, so an orchestrator that built the driver once no longer closes it on every later release. Traced the paths that can set the flag again — QueryOrchestrator captures options.externalDriverFactory at construction (QueryOrchestrator.ts:86-122), and PreAggregations/PreAggregationLoader/PreAggregationLoadCache all read it from there, so every real builder still goes through the wrapper and only the release bypasses it. That's the split you want.
• A failed build counted as built — the wrapper is async and marks on fulfilment (OrchestratorApi.ts:106-112), so the testConnection() failure path in server.ts:668-702 — which releases the driver and nulls the memo, leaving nothing open — no longer makes the eviction release rebuild a connection just to close it.
• The wrapper dropping dataSource — moot, and correctly so: OrchestratorApiOptions.externalDriverFactory is DriverFactory (() => Promise<BaseDriver> | BaseDriver), not DriverFactoryByDataSource, so there is no argument to thread. Typing the new field DriverFactory and passing it to releaseDriver(driverFn?: DriverFactoryByDataSource) is fine — a zero-arg function is assignable to the one-arg signature. Making the wrapper async is also safe: every consumer (QueryOrchestrator.ts:92, PreAggregationLoader.ts:251/740/822/934, PreAggregationLoadCache.ts:98/132) awaits it, and the only other use is a truthiness check.

Both new tests are precise and would fail on e5c3c7d: the first drives a full release, then a late operation, and asserts the data-source driver closes twice while the external one closes once — exactly the asymmetry the fix creates; the second rejects the factory once and asserts it isn't called again.

Nothing new found in this commit. One behaviour worth naming but not worth a change: server.ts never clears externalPreAggregationsDriverPromise on release, so if something does touch the external factory after a close, the memo hands back the already-closed driver, the flag flips, and the re-armed release calls release() on it a second time. That is the pre-existing memo shape (the data-source driverPromise memo and seenDataSources behave the same way), and it costs a redundant close rather than a leak — the case this commit removes is the far more common one where nothing touched it at all.

Still open

Only the item already deferred in the PR body: getQueryOrchestrator() bypasses trackOperation entirely (RefreshScheduler.ts:238, 621, 812, 831), to be closed by de-duplicating getOrchestratorApi() by id under CORE-823. Every inline thread on this PR is resolved.

Tests

The two gaps I flagged on e5c3c7d are both closed by the tests in this commit. No further gaps identified.

node_modules isn't installed in this job and installs aren't permitted, so the above is static review only — I couldn't run the suite or tsc.

· branch `claude/cube-store-oom-outage-h9xy56`

[paveltiunov]

Upload merged coverage to Codecov is failing on this PR and is not this PR's — it is a pre-existing fragility in push.yml. Proposed one-line patch below; I am not pushing it here.

I gave a wrong explanation for this earlier in the thread and want to correct it: I said the Codecov failures were downstream of the npm-registry 504 window that killed the driver jobs, i.e. no coverage artifacts to merge. That was wrong. It has now failed on the current head 32925d28, where 30 of the 31 jobs in the Build run succeeded — unit, unit-core and all 19 integration (...) jobs. upload-coverage declares needs: [unit, integration], so it only ran because every job it depends on passed. It is the sole failure in the run.

What actually fails. Not the Codecov upload — the artifact download before it:

##[error]Unable to download artifact(s): Unable to download and extract artifact:
Artifact download failed after 5 retries.

preceded by ~20 successful downloads. The step (push.yml:793-796) is:

      - name: Download all coverage artifacts
        uses: actions/download-artifact@v4
        with:
          path: all-coverage

With no name and no pattern, download-artifact@v4 fetches every artifact in the run, so all-coverage/ receives the cubestored-x86_64-unknown-linux-gnu-release binary, native-linux-x64-glibc-3.13.node, the *.dockerbuild build records and any cypress screenshots — none of which the next step reads, since it merges only *.lcov. One large unrelated download exhausting its 5 retries fails the job.

Proposed patch. The coverage artifacts are coverage-unit (push.yml:151) and coverage-integration-<db>-<planner> (:540), so restricting the download to what the merge step actually consumes is one line:

        with:
          pattern: coverage-*
          path: all-coverage

That drops the large binaries from the transfer entirely, which both removes the failure mode and makes the job faster.

Why I am not pushing it here. There is no existing fix to port — this would be a new .github/workflows/push.yml change affecting every PR in the repo, which belongs in its own CI-owned PR rather than inside a server-core leak fix. Happy to open that separately if you'd like.

One thing to flag: if upload-coverage is a required check, this PR cannot go green on its own — it has failed on all four heads (647030f3, 387b43df, c24449ea, 32925d28). The workflow patch or an override would be needed.

Re-running the failed job once now, since a transient artifact download is exactly the kind of thing a re-run settles. I will keep watching until the PR is green or merged.

---

Generated by Claude Code

[paveltiunov]

Re-run spent, and it failed identically — so this is deterministic, not a flake, and my "a re-run should settle it" in the previous comment was wrong. The re-run also let me pin the exact artifact, which corrects the mechanism I guessed at there.

actions/download-artifact logs Found 20 artifact(s) and 20 Starting download lines, but only 19 Artifact download completed successfully. Matching the expected digests from its own manifest against the completions, the one that never finishes is:

cube-js~cube~ORG2R2.dockerbuild  (ID 9639670152, 118735 bytes,
                                  sha256:2d9ca8c2d5b8f862c0a84a725ba8e5e4d47b1e8f98c4a64c162c48d3d64481fd)

That is the Docker buildx build-record artifact, uploaded automatically by docker/build-push-action in the Build & Test :dev for Debian job. Not coverage, not a *.lcov, and nothing the merge step could consume.

Two corrections to what I wrote before:

• It is not a size or timeout problem. The two genuinely large artifacts both download fine in the same step — cubestored-x86_64-unknown-linux-gnu-release at 49 MB and native-linux-x64-glibc-3.13.node at 23 MB. The one that fails is 118 KB. .dockerbuild records are a non-standard payload rather than a plain zip of files, which is the likely reason extraction fails where the ordinary artifacts succeed.
• It will not clear itself. Both attempts failed on the same artifact, so every run of this job fails as long as a .dockerbuild record exists in the run — which is whenever the Debian docker job runs.

This makes the patch from my previous comment not just a tidy-up but the actual fix, because pattern: coverage-* excludes precisely the artifact that fails:

      - name: Download all coverage artifacts
        uses: actions/download-artifact@v4
        with:
          pattern: coverage-*
          path: all-coverage

Still not pushing it here — it is a repo-wide push.yml change, unrelated to this PR's server-core diff, and it wants CI ownership rather than a drive-by. Say the word and I will open it as its own PR.

Everything else on 32925d28 is green: unit, unit-core, all 19 integration (…) jobs, build, lint, build-cubestore, both native builds and all three CodeQL analyses. upload-coverage is the only red check, and by its own needs: [unit, integration] it only ran because the rest passed.

---

Generated by Claude Code

[claude]

Claude finished @paveltiunov's task in 5m 46s —— View job

---

Re-review of the rewritten branch: much simpler fix, but the drain went with it — 1 high, 1 medium, 4 low.

Full review (3 inline comments posted; no stale threads — every prior thread is already resolved)

• Read the new diff (the branch was rewritten: OrchestratorApi.ts is untouched now)
• Review WebSocketConnection.ts
• Review OrchestratorStorage.ts
• Review tests
• Post review

The branch is now two commits and 315 lines instead of the previous lifecycle machinery, and the relocation is the right instinct: the leak's proximate cause is that WebSocketConnection.close() didn't close anything — the 'close' handler re-sent whatever was pending over a fresh socket whose heartbeat kept the whole graph reachable — so fixing it at the driver rather than bolting a drain onto OrchestratorApi is a much smaller surface. The established guard in the 'error' handler is a genuine second bug caught along the way: an error on an already-established socket used to raise a replacement connection that nobody had asked for. Refusing in initWebSocket() (rather than at each call site) is the right chokepoint — it covers the re-send path and post-release queries with one check — and the driver tests drive a real mock Cube Store server rather than mocking ws, so they assert the socket state Cube Store actually sees.

What the rewrite drops, though, is the drain, and that changes what an eviction costs.

High

Eviction now fails in-flight work instead of leaking it — OrchestratorStorage.ts:20. release() is master's version again: disposeAfter closes the drivers immediately, and close() is now terminal, so the next Cube Store round trip of a query still executing on the evicted api throws ConnectionError: Cube Store connection is closed. The socket-level drain only covers messages already written to the wire, not the next one. The cold-start double-set race in getOrchestratorApi() (server.ts:572-577, no de-duplication by id) makes this reachable on two concurrent first requests for a new tenant; RefreshScheduler holding one api across a whole run and any request outliving max distinct ids make it reachable in steady state. Details and the options in the thread.

Medium

release() builds the external driver just to close it. OrchestratorApi.ts:245 calls releaseDriver(this.options.externalDriverFactory) unconditionally, and server.ts:667-701 creates the driver inside that factory and runs testConnection() on it. So an orchestrator that never touched Cube Store now opens a Cube Store connection on eviction purely to close it — new churn, introduced by this branch, against the router the PR exists to relieve, at the ~450 evictions/min/pod the description measures. The data-source loop is already guarded this way by seenDataSources; the external one needs the same. Not inline: unchanged line.

Low

• expire is not actually covered. The comment at OrchestratorStorage.ts:14 lists all four removal reasons, but with ttl set and ttlAutopurge unset, lru-cache only reaps an expired entry when something touches it — an orchestrator for a tenant that goes quiet, which is precisely what maxCompilerCacheKeepAlive is for, sits expired-but-undisposed holding its socket. Latent while server.ts:131 constructs new OrchestratorStorage() with no arguments, live the moment the follow-up wires the options through. Adjacent: new OrchestratorStorage({}) throws from lru-cache, since max: options.compilerCacheSize is undefined when an options object is passed without it — the default parameter only applies when the argument is omitted entirely.
• A failed release is silent — OrchestratorStorage.ts:26-34. Both the eviction path and shutdown now swallow it with no log.
• The drain never ends if a message is never answered — WebSocketConnection.ts:609. Same comment notes that closeIfDrained() inspects this.webSocket rather than the socket the message arrived on.
• currentConnectionTry is incremented on a path that no longer retries. In the established branch of the 'error' handler (WebSocketConnection.ts:193-209) the counter is bumped and then the handler returns without attempting a connection. It is only reset by a 'pong' on the current socket, so until a replacement connects it inflates retryWaitTime() — delaying the re-send that carries the pending messages — and eats the maxConnectRetries budget that a genuinely cold connect will need. Harmless in the common case, but it is now counting something other than connection tries.

Tests

The four close tests are well chosen and each maps to a distinct half of the fix — drained close, mid-query close, post-close query, and death while draining — and asserting server.connections.length is what makes "did not re-open" a real assertion rather than a proxy. The OrchestratorStorage suite covers eviction/replacement/identity-set/failed-release/pendingReleases cleanup, and stubbing the api down to release() is the right seam.

Gaps, matching the findings above: nothing asserts what happens to an operation still running on an evicted orchestrator (the high finding — a test here would have to decide what the intended behaviour is, which is the useful part); nothing covers an orchestrator that never built the external driver, asserting the factory isn't called on release; and nothing covers a close() whose in-flight message is never answered, which today never terminates.

node_modules isn't installed in this job and installs aren't permitted, so the above is static review only — I couldn't run the suite or tsc.

· branch `claude/cube-store-oom-outage-h9xy56`

[paveltiunov]

Reworked to the approach from the parallel PR — force-pushed as a7e21f39, and @keydunov's approval is reset as a result, sorry. The earlier direction is gone; the PR body is rewritten to match.

What changed, and why the earlier direction was the wrong one:

I had built the release lifecycle up inside OrchestratorApi — operation counting, a bounded drain, a re-arm after a late operation, a stream hold, external-driver tracking — roughly 360 lines of machinery, most of it added in response to review findings on the previous round. That was treating the symptom. WebSocketConnection.close() never actually closed anything: the 'close' handler re-sent whatever was in flight over a fresh connection and its heartbeat kept the socket alive, so releasing a driver freed nothing and a late query re-opened it anyway. All the drain and re-arm logic existed to manage a close that did not close.

The replacement is a single closed flag on the connection, and OrchestratorStorage shrinks to a disposeAfter hook plus a pendingReleases set so releaseConnections() can await the releases clear() schedules. OrchestratorApi is back to its original state — no diff against master. Net change is 315 insertions across 4 files, down from 818.

Several findings from the previous rounds stop existing rather than being fixed, which is the clearest sign the shape was wrong:

• The failed-release-pins-the-instance bug, the newer-release-evicted-from-the-cache bug, the drain→flag window, the unref'd 10-minute timer and the accumulating drain listeners were all defects in the machinery, and go with it.
• The use-after-release hole the first review flagged is now closed by the connection refusing to re-open, instead of by a re-arm that closed and re-opened the drivers on every later call.
• The releaseDriver churn finding — an orchestrator that never touched Cube Store opening a connection on eviction just to close it — is gone too, along with the wrapper I had added for it and its two bugs.

Still standing from the earlier reviews, unchanged: getQueryOrchestrator() bypasses any tracking (RefreshScheduler.ts:238, 621, 812, 831), and getOrchestratorApi() is not de-duplicated by id. Neither is load-bearing now — a released connection refuses rather than leaks — so both stay in CORE-823 as follow-ups.

One behaviour worth calling out for review: close() is terminal, so a caller still holding a released driver gets ConnectionError: Cube Store connection is closed rather than a silently re-opened socket. That is the intended trade — it is the failure mode that makes the leak impossible — but it is a behaviour change, and the third new driver test pins it.

The upload-coverage check is expected to stay red for the reason in the two comments above (the .dockerbuild artifact in actions/download-artifact); unrelated to this rework.

---

Generated by Claude Code

[claude]

Nothing drains the orchestrator before this release, and close() is now terminal — so an eviction mid-request now fails that request instead of leaking.

This rewrite drops the trackOperation/drain machinery, so OrchestratorApi.release() is master's version: it closes the data-source drivers and the external driver the moment disposeAfter fires, with no regard for what is still running. Paired with the new terminal close(), the two changes compose into a behaviour change that neither has on its own:

• Before this PR: eviction did nothing at all. An in-flight query on the evicted api kept working; the cost was the leaked socket.
• After: eviction releases immediately. WebSocketConnection only drains messages already written to the socket; anything after that — the next Cube Store queue round trip of a query still being executed, a Continue wait poll that resumes, a pre-aggregation build — hits initWebSocket(), sees this.closed, and throws ConnectionError: Cube Store connection is closed. Permanently, by design.

The same applies to the data-source side: server.ts's driverPromise memo is not cleared by release(), so the held api keeps handing back a driver whose pool has been ended.

Who still holds an evicted api:

1. The cold-start race. getOrchestratorApi() (server.ts:572-577) is not de-duplicated by id, so two concurrent requests for a new orchestratorId both build an api and both set() it; the first is disposed and released while the request that created it is still using it. Two concurrent first requests for a new tenant is not an edge case in a high-cardinality contextToOrchestratorId deployment — it is the common case.
2. RefreshScheduler holds one orchestratorApi for a whole refresh run (RefreshScheduler.ts:620-633).
3. Any request that outlives the arrival of max distinct ids.

At the eviction rate the PR body measures (~450/min/pod), (3) alone is a steady trickle of failed queries. The leak is real and worth closing, but trading it for hard request failures needs either a drain (what the earlier iteration of this branch had), or de-duplicating getOrchestratorApi() by id plus a grace period before the release, or at minimum a decision recorded here that failing those requests is acceptable.

Fix this →

[paveltiunov]

The mechanism is real and I verified both halves rather than taking them on trust, so recording that here — but the remedy is not mine to choose, so this thread stays open pending the author's decision.

Confirmed:

• The cold-start race is real. getOrchestratorApi() checks has() at server.ts:575 and then awaits contextToOrchestratorId, orchestratorOptions and getOrchestratorInitializedOptions before set() at the end, so two concurrent requests for a cold id both build an api and both set() it. The loser is disposed and released while the request that created it still holds it.
• The data-source side is real too. driverPromise is a closure-local memo per getOrchestratorApi() call and release() does not clear it, so a held api keeps handing back a driver whose pool has been ended.
• The composition is the new part. Before this PR eviction released nothing, so neither of the above had consequences; the cost was the socket. Now eviction releases immediately and close() is terminal, so the next Cube Store round trip of a query still executing gets ConnectionError: Cube Store connection is closed.

On severity, the numbers from the PR body make case (3) concrete rather than theoretical: ~450 orchestrator creations/minute/pod against max: 100 means the whole cache turns over roughly every 13 seconds, and Cube queries routinely run longer than that. So on the tenant that motivated this fix, shipping as-is trades leaked sockets for a steady trickle of failed queries. That is worth being explicit about rather than filed as a nit.

What I have not done is re-add the drain. The trackOperation/drain machinery was removed deliberately by the author in favour of this approach, and re-introducing it on a review finding would reverse that decision rather than implement it. Three options are on the table and the choice is the author's:

1. A minimal active-operation counter with a bounded wait in release() — roughly 30 lines, not the ~360 the earlier iteration grew into (no re-arm, no stream hold, no external-driver wrapper).
2. De-duplicate getOrchestratorApi() by id, which removes cause (1) outright, and accept the residual from (2) and (3).
3. Ship as-is and record that failing those requests is acceptable, presumably alongside a larger max.

Flagged to the author with (1) as my recommendation, since it is the difference between fixing the leak and fixing it without failing queries. Will implement whichever is chosen and report back here.

---

Generated by Claude Code

[claude]

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

---

Re-review of fee13c0: both prior lows fixed, but dropping established opens an unhandled-rejection path — 1 high; the eviction/drain thread is still open.

Full review (1 inline comment posted, no stale threads to resolve)

• Read the two new commits' diffs
• Check which prior findings are addressed
• Re-check the established removal and the new close timer
• Post findings

Addressed since the last review

• Unbounded drain — close() now arms a bound (WebSocketConnection.ts:595-620), rejects whatever is left naming the count, tears down the heartbeat and terminates; closeIfDrained() clears it when the drain wins. noHeartBeatTimeout (30s) is a sensible reuse, the timer is unref()'d, and the test drives it with CUBEJS_CUBESTORE_NO_HEART_BEAT_TIMEOUT=1 against a server that never answers — precisely the case that previously held the socket forever.
• closeIfDrained() read this.webSocket — it now takes the socket the answer arrived on and compares it against the current one, so a late answer on a superseded socket can no longer be read as the current connection having drained.
• A failed release was silent — OrchestratorApi.release() logs Orchestrator Release Error through its own logger before rethrowing (OrchestratorApi.ts:243-259), which is the right home for it: the storage keeps swallowing so shutdown can't fail, and the api still says what happened. The test builds a real OrchestratorApi with a stubbed orchestrator and asserts the log fires via the eviction path, not by calling release() directly — good seam.

New finding

High — dropping the established flag makes a closed connection's socket error an unhandled rejection (WebSocketConnection.ts:199). initWebSocket() rejects whenever closed is set, and the 'error' handler hands that rejected promise to resolve — which, on an already-settled readyPromise, returns without adopting it, so nothing ever observes the rejection. Under Node's default --unhandled-rejections=throw that is a fatal ConnectionError: Cube Store connection is closed. With CUBEJS_CUBESTORE_MAX_CONNECT_RETRIES defaulting to 20 the retry branch is effectively always taken, and the trigger is ordinary: an evicted connection draining when the peer resets it, or a close() that lands while the socket is still CONNECTING. The does not re-open when the socket dies while draining test walks the whole sequence; its retry timer just fires around when the test ends. initWebSocket().then(resolve, reject), or returning early when this.closed, closes it.

The commit message's justification for dropping the flag holds for a live connection — the reconnect lands in this.webSocket and is reaped by a later close — but the closed case has no reconnect to reap, only an ownerless rejection. Note the PR body still claims the 'error' handler no longer reconnects once established; that was reverted in c3ec6b2, so the description needs a line either way.

Still open from the previous review

• Eviction now fails in-flight work rather than leaking it (thread, OrchestratorStorage.ts:20) — unchanged in these two commits. release() still closes immediately with no drain above the socket, and close() is terminal, so the next Cube Store round trip of a query running on an evicted api throws. Re-affirmed rather than re-posted; it wants either a decision recorded or getOrchestratorApi() de-duplicated by id.
• Also as recorded in the PR body: getQueryOrchestrator() bypasses any tracking (RefreshScheduler.ts:238, 621, 812, 831), deferred under CORE-823.

Tests

The two new tests are each precise about the thing they fix — the never-answered-message one asserts both the rejection wording and that the socket goes away, and the release-logging one goes through eviction rather than poking release(). Gaps matching the findings: nothing asserts that a socket error on a closed connection produces no unhandled rejection (the high finding above — a test would need process.on('unhandledRejection') or jest's --unhandled-rejections=strict), and nothing yet covers what an in-flight request sees when its orchestrator is evicted. In createRealApi the data-source factory and externalDriverFactory return the same object and seenDataSources is empty, so only the external branch is exercised — fine for a logging test, worth knowing.

node_modules isn't installed in this job and installs aren't permitted, so the above is static review only — I couldn't run the suite or tsc.

· branch [`claude/cube-store-oom-outage-h9xy56`](https://github.com/cube-js/cube/tree/claude/cube-store-oom-outage-h9xy56)

[paveltiunov]

Pushed c22a475d. Covering the two findings from the review summary that had no inline thread, plus a change of mechanism on the drain bound.

Medium, release() builds the external driver just to close it — fixed. Confirmed against server.ts:667-701: the factory creates the driver and runs testConnection() inside it, so an orchestrator that never touched Cube Store was opening a connection on eviction purely to close it. It was harmless while release only happened at shutdown; releasing on every eviction is what makes it churn, against the router this branch exists to relieve. The factory is wrapped in the constructor before QueryOrchestrator captures it, marked on fulfilment (a build that failed testConnection() left nothing open), and the release closes through the unwrapped factory so the flag isn't re-set on the way out. Two tests, one of which fails against the previous commit.

The drain bound moved into the heartbeat interval, replacing the setTimeout from c3ec6b2d, on the author's call — and it is the better home: that interval is already the timer that owns "this socket has waited long enough" and already the thing keeping the socket reachable, so there is one timer to reason about and no stray one that can outlive what it was bounding. Pinging continues while a closed connection drains, so one draining legitimately is not dropped for inactivity, and the closed branch returns before the ordinary no-heartbeat check, which cannot bound this wait for the reason in the thread above.

Low, currentConnectionTry bumped on a path that no longer retries — moot. That was the established branch, which is gone as of c3ec6b2d, so the counter is followed by the retry logic again exactly as on master.

Low, expire / new OrchestratorStorage({}) — left alone deliberately. Both are accurate: ttlAutopurge is unset so an expired entry is only reaped when something touches the cache, and max: options.compilerCacheSize is undefined when an options object omits it. Both are latent while server.ts:131 constructs the storage with no arguments, and both belong with the follow-up that wires those options through — which is where the behaviour change they imply can be reasoned about as one. Raised with the author rather than changed here; the one thing I would not leave is the comment claiming all four removal reasons are covered, which is the author's to adjust.

Still open and unchanged: the high finding on the thread above, which needs a decision rather than a patch.

---

Generated by Claude Code

[claude]

Claude finished @paveltiunov's task in 4m 31s —— View job

---

Re-review of f5183c8 + c22a475: the external-driver churn is addressed — 1 new low-medium; 2 earlier threads still open.

Full review (1 inline comment posted, no stale threads to resolve)

• Read the new commits' diffs (f5183c8, c22a475)
• Check which prior findings are addressed
• Resolve stale threads / post new findings

Addressed since the last review

• release() built the external driver just to close it (c22a475, was medium) — the factory is wrapped in the constructor before QueryOrchestrator captures it off options, so every real builder goes through the flag, and the release closes through untrackedExternalDriverFactory so the wrapper can't set the flag again on the way out. That is the same shape as the last iteration of this branch, and the two defects that shape had then are both pre-fixed here: the unwrapped release, and marking on fulfilment rather than on entry. Checked that a second release can't double-close: releaseConnections() only calls clear() (one dispose per entry), and resetInstanceState()'s trailing clear() runs on an already-empty cache.
• The drain bound moved into the heartbeat (f5183c8) — this reads as a genuine simplification rather than a shuffle. The ping interval was already the per-socket timer that keeps the graph reachable, so folding the bound into it removes the possibility of a closeTimer outliving the socket it was bounding, and abandonClose() factored out of the timer callback is easier to follow than the old inline closure. Traced the paths that clear the interval while messages are still pending — the 'error' handler's teardown() and the 'close' handler — and both end in the re-send catch rejecting the pending messages via initWebSocket() throwing, so no path is left with pending messages, no interval, and no bound. abandonClose() deletes and rejects before terminate(), so the 'close' handler that follows sees nothing pending and doesn't schedule a re-send.

New finding

Low-medium — marking on fulfilment opens the opposite race (OrchestratorApi.ts:58-64). Between the wrapper entering the factory and its promise resolving, externalDriverCreated is still false, so an eviction landing in that window skips the external branch and returns while the build completes underneath it — a live Cube Store connection on an orchestrator the LRU has already dropped, with nothing left to close it. The window spans a network round trip, since server.ts:667-701 runs testConnection() inside the factory. Setting the flag on entry and resetting it in a catch keeps both properties (a failed build still doesn't count as built) and turns the overlap into a close of the driver the build produced. Details and the diff in the thread.

Still open from earlier reviews

Both unchanged in these two commits, so re-affirmed rather than re-posted:

• A socket error on a closed connection is an unhandled rejection (thread, WebSocketConnection.ts:218) — resolve(this.initWebSocket()) still hands a rejected promise to an already-settled resolve. Note f5183c8 makes this more reachable, not less: a closed connection now stays alive under its own interval for up to noHeartBeatTimeout, which is the window in which the peer resetting it walks straight into this path.
• Eviction fails in-flight work rather than leaking it (thread, OrchestratorStorage.ts:20) — still awaiting the author's decision between a minimal drain, de-duplicating getOrchestratorApi() by id, or recording that the failures are acceptable.

And, as recorded in the PR body: getQueryOrchestrator() bypasses any tracking (RefreshScheduler.ts:238, 621, 812, 831), deferred under CORE-823.

Tests

The two new OrchestratorApi tests are the right pair — one asserts the factory isn't called at all, the other builds through options.externalDriverFactory first and asserts a single close — and moving createRealApi onto a seen data source rather than an external driver makes the logging test exercise the per-datasource loop, which is the branch that was previously untouched. Gaps: nothing covers a release() that overlaps a pending first build (the finding above), and nothing covers a build that rejects.

One note on f5183c8 and the existing suite: gives up on a message Cube Store never answers sets CUBEJS_CUBESTORE_NO_HEART_BEAT_TIMEOUT=1, but the bound is now only evaluated on the 5s ping tick, so that test's real latency is ~5s rather than ~1s. Well within JEST_TIMEOUT, and the assertion is unchanged — just worth knowing that the knob no longer sets the resolution.

node_modules isn't installed in this job and installs aren't permitted, so the above is static review only — I couldn't run the suite or tsc.

· branch [`claude/cube-store-oom-outage-h9xy56`](https://github.com/cube-js/cube/tree/claude/cube-store-oom-outage-h9xy56)

[claude]

Claude finished @paveltiunov's task in 2m 32s —— View job

---

Re-review of 52abc59: the unhandled rejection is fixed, but the early return added with it turns a rejection into a hang — 1 medium; 2 earlier threads still open.

Full review (1 inline comment posted, 1 stale thread resolved)

• Read the new commit's diff
• Check which prior findings are addressed
• Resolve stale threads / post new findings

Addressed since the last review

• The ownerless rejection on the retry path — this.initWebSocket().then(resolve, reject) (WebSocketConnection.ts:236) attaches a handler, so a rejection is observed whether or not readyPromise has settled; on a settled one both are no-ops, on an unsettled one it behaves exactly as the old adoption did. That closes the fatal-under---unhandled-rejections=throw case. The test is honest about emitting the 'error' rather than provoking it, and says why — ws reports a dying socket against the mock as 'close' — which is the right call: the handler's contract is the subject, and process.on('unhandledRejection') is the only way to assert it. Thread resolved.

New finding

Medium — the early return skips every settle path (WebSocketConnection.ts:216-223). readyPromise is settled only by 'open', this handler's reject, or the retry the early return now skips — teardown() has already cleared the interval and the 'close' handler doesn't touch it. So a socket still CONNECTING when close() lands (eviction while the first query connects: closeIfDrained() sees no pending messages, calls socket.close(), ws emits WebSocket was closed before the connection was established) leaves the await this.initWebSocket() in sendMessage pending forever. The previous commit rejected there, because resolve(promise) on an unsettled promise adopts. A one-line reject(new ConnectionError('Cube Store connection is closed')) before the return keeps both properties and is a no-op once established.

Same block, smaller: the comment's "whatever was in flight is dealt with by the 'close' handler" holds only because ws emits 'close' after 'error' — the abandonClose bound is gone by then, since teardown() runs two lines above. True in practice, worth stating rather than implying.

Still open from earlier reviews

Both unchanged in this commit, so re-affirmed rather than re-posted:

• Eviction fails in-flight work rather than leaking it (thread, OrchestratorStorage.ts:20) — still awaiting the author's decision between a minimal drain, de-duplicating getOrchestratorApi() by id, or recording that the failures are acceptable.
• Marking externalDriverCreated on fulfilment opens the opposite race (thread, OrchestratorApi.ts:64) — a release overlapping the first build skips a driver that is about to exist.

And, as recorded in the PR body: getQueryOrchestrator() bypasses any tracking (RefreshScheduler.ts:238, 621, 812, 831), deferred under CORE-823.

Tests

The new test is well-targeted and would fail against c22a475 with exactly the fatal rejection. The gap it leaves is the finding above: nothing covers a close() landing while the socket is still CONNECTING, which is now a hang rather than a rejection and so wouldn't be caught by an unhandledRejection assertion either — it needs an explicit expect(...).rejects on the in-flight query.

node_modules isn't installed in this job and installs aren't permitted, so the above is static review only — I couldn't run the suite or tsc.

· branch [`claude/cube-store-oom-outage-h9xy56`](https://github.com/cube-js/cube/tree/claude/cube-store-oom-outage-h9xy56)

[claude]

Claude finished @paveltiunov's task in 2m 58s —— View job

---

Re-review of 8db1939: the revert is a defensible simplification and removes a finding rather than fixing it — no new findings. Two earlier threads still open.

Full review (0 new inline comments, 1 stale thread resolved)

• Read the new commit's diff
• Check which prior findings are affected
• Resolve stale threads / post new findings

What changed

8db1939 reverts c22a475 — the externalDriverCreated flag, the constructor wrapper and untrackedExternalDriverFactory are gone, along with the two release tests they came with. OrchestratorApi.ts is back to master plus the release-error logging. The whole branch is now 494 insertions across 5 files, and OrchestratorApi's entire diff is a try/catch around the existing Promise.all.

I checked the reasoning in the commit message rather than taking it: with cacheAndQueueDriver: 'cubestore' the queue/cache driver is built through cubeStoreDriverFactory, which calls externalDriverFactory() (QueryOrchestrator.ts:90-101), so any orchestrator that has served a query has already built the external driver and the memo makes the release a no-op build. The guard only saved the connect + SELECT 1 + close for an orchestrator created and never used, and it cost a wrapper that drew two defects in review. Trading it away is the right call, and the commit message records the accepted cost explicitly — which is what I'd ask for.

Also verified the revert left nothing dangling: no references to externalDriverCreated or untrackedExternalDriverFactory remain, the DriverFactory import is dropped with its only use, and createRealApi in the storage test never configured an externalDriverFactory, so the logging test is unaffected.

New findings

None.

One qualification on the commit message, worth knowing rather than changing: the "already been built" argument holds for cacheAndQueueDriver: 'cubestore'. Under memory with Cube Store still configured as external, an orchestrator that served only non-pre-aggregated queries never touches the factory, so every eviction does pay the connect + SELECT 1 + close. That config is a minority and its eviction rate is nothing like the measured one, so it doesn't change the conclusion.

Still open

Both unchanged in this commit, so re-affirmed rather than re-posted:

• The early return in the 'error' handler doesn't settle readyPromise (thread, WebSocketConnection.ts:216-223) — a socket closed while still CONNECTING leaves the awaiting sendMessage pending forever, where the previous commit rejected it. This is the one substantive item left.
• Eviction fails in-flight work rather than leaking it (thread, OrchestratorStorage.ts:20) — still awaiting the author's decision between a minimal drain, de-duplicating getOrchestratorApi() by id, or recording that the failures are acceptable.

And as recorded in the PR body: getQueryOrchestrator() bypasses any tracking (RefreshScheduler.ts:238, 621, 812, 831), deferred under CORE-823; and expire / new OrchestratorStorage({}) deferred to the options-wiring follow-up.

Tests

Removing the two release tests alongside the code they covered is correct — they asserted the guard, and the guard is gone. OrchestratorApi.test.ts is back to its single pre-existing test. The remaining test gap is the one attached to the open WebSocketConnection thread: a close() landing while the first query is still connecting, asserting that query rejects rather than hanging.

node_modules isn't installed in this job and installs aren't permitted, so the above is static review only — I couldn't run the suite or tsc.

· branch [`claude/cube-store-oom-outage-h9xy56`](https://github.com/cube-js/cube/tree/claude/cube-store-oom-outage-h9xy56)

[claude]

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

---

Re-review of 88a2e8a: the CONNECTING hang is fixed correctly — no new findings. One earlier thread still open.

Full review (0 new inline comments, no stale threads to resolve — only the eviction/drain thread is unresolved, and it still applies)

• Read the new commit's diff
• Check which prior findings are addressed
• Trace the readyPromise settle paths and unhandled-rejection risk
• Resolve stale threads / post new findings

Addressed since the last review

• The early return skipped every settle path (WebSocketConnection.ts:233) — reject(new ConnectionError('Cube Store connection is closed')) before the return restores the rejection that resolve(promise)-by-adoption used to produce, and is a no-op once 'open' has settled the promise. I traced the surrounding claims rather than taking them:
  • readyPromise is settled only by 'open', the MAX_PAYLOAD branch, this handler's reject, or the retry — and the closed branch skips the retry, so this really was the last thing that could settle it.
  • The reject cannot itself become ownerless: every initWebSocket() result is returned to a caller that awaits it (sendMessage:430, the re-send at :345, the retry's .then(resolve, reject)), and this.webSocket is assigned synchronously in the same tick the promise is constructed, so there is no window where a socket exists with no consumer for its readyPromise.
  • The CONNECTING path the test drives is reachable exactly as described: closeIfDrained() finds sentMessages empty (a message is registered only after the await), nulls this.webSocket, and calls socket.close(); ws aborts the handshake with an 'error'. The subsequent 'close' sees nothing pending, so no re-send is scheduled and nothing double-settles.
• The corrected comment is right, and worth having in the file — what was written to the socket is rejected by the 'close' handler's re-send catch (via initWebSocket() throwing), not by the drain bound, which teardown() two lines above has already disarmed. That was implied before and is now stated.

The test is the right shape: it asserts the query rejects rather than asserting on internals, which is the only way to catch a hang, and it would time out against 52abc59.

New findings

None. Two things I checked and deliberately did not raise as findings:

• this.currentConnectionTry += 1 still runs above the closed branch, on a path that no longer retries. Dead but harmless — the connection is terminal, and the counter is never read again for it.
• close() re-stamps closedAt on every call, so a second close() on a draining connection restarts the noHeartBeatTimeout bound. Reachable when one CubeStoreDriver instance backs both a data source and the external driver, since release() closes both — but it costs at most one extra bound period on an already-terminal connection.

Still open

• Eviction fails in-flight work rather than leaking it (thread, OrchestratorStorage.ts:20) — unchanged, and still the one item awaiting a decision rather than a patch: a minimal drain, de-duplicating getOrchestratorApi() by id, or recording that the failures are acceptable.
• As recorded in the PR body: getQueryOrchestrator() bypasses any tracking (RefreshScheduler.ts:238, 621, 812, 831), and expire / new OrchestratorStorage({}), both deferred under CORE-823.

Tests

The gap attached to the previous review is closed by this commit's test. No new gaps identified; the six close tests now cover drained close, mid-query drain, post-close query, death while draining, a never-answered message, an error on a closed connection, and a close during CONNECTING.

node_modules isn't installed in this job and installs aren't permitted, so the above is static review only — I couldn't run the suite or tsc.

· branch [`claude/cube-store-oom-outage-h9xy56`](https://github.com/cube-js/cube/tree/claude/cube-store-oom-outage-h9xy56)

[claude]

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

---

Re-review of 5159581: both field changes are correct and self-contained — no new findings. Only the eviction/drain thread is still open.

Full review (0 new inline comments, no stale threads to resolve)

• Read the new commit's diff
• Verify the two field changes against their call sites
• Check unresolved threads
• Post review

What changed

Both were things I raised as "checked and deliberately not a finding" on 88a2e8a, so this is tidy-up rather than a fix — and in both cases the field now means what its name says, which is cheaper than the comment that would otherwise have to explain it.

currentConnectionTry moved below the closed branch (WebSocketConnection.ts:236-239). Traced the three exits above it: the MAX_PAYLOAD branch already returned before the old increment, so it is unaffected; the closed branch now returns without spending budget, which is right since it attempts no connection; the ordinary path is unchanged. retryWaitTime() (:415) is 1000 * (currentConnectionTry + 1) and the increment still precedes both it and the < maxConnectRetries test in the same tick, so the retry pacing and the failure threshold are bit-for-bit what they were on the live path. The only observable difference is that an evicted connection's error no longer inflates the counter or eats the budget a genuinely cold connect needs — and since the counter is reset only by a 'pong' on the current socket, that inflation could previously persist.

closedAt stamped once (:629-637). The reachability claim in the commit message holds: OrchestratorApi.release() closes the data-source and external drivers separately, and one CubeStoreDriver can back both, so close() twice on a draining connection is a real sequence — and re-stamping restarted the noHeartBeatTimeout bound evaluated at :149, which is the one thing standing between a wedged message and a socket kept for the life of the process. Guarding the whole assignment on !this.closed rather than just the timestamp is fine: closed is write-once-true and closeIfDrained() still runs unconditionally after the block, so a second close() that arrives after the drain finished still gets its chance to close the socket.

Checked the surrounding invariants haven't shifted: closeIfDrained() is still the only path that nulls this.webSocket on a clean drain, abandonClose() is still only reachable from the interval, and no test asserts on either field, so nothing in the suite is pinned to the old semantics.

New findings

None.

Still open

• Eviction fails in-flight work rather than leaking it (thread, OrchestratorStorage.ts:20) — the only unresolved thread on the PR, and still a decision rather than a patch: a minimal drain, de-duplicating getOrchestratorApi() by id, or recording that the failures are acceptable. The PR body now documents the trade explicitly, which covers the third option if that is the call.
• As recorded in the PR body: getQueryOrchestrator() bypasses any tracking (RefreshScheduler.ts:238, 621, 812, 831); expire / new OrchestratorStorage({}); and release() building the external driver just to close it — all deferred under CORE-823.

Tests

No test changes in this commit, and none are needed: neither field is observable through the public surface, and the behaviour they guard (the drain bound firing once, the retry budget) is already covered by gives up on a message Cube Store never answers and the pre-existing retry tests. The suite's coverage of the close path is unchanged at seven cases.

node_modules isn't installed in this job and installs aren't permitted, so the above is static review only — I couldn't run the suite or tsc.

· branch [`claude/cube-store-oom-outage-h9xy56`](https://github.com/cube-js/cube/tree/claude/cube-store-oom-outage-h9xy56)

[codecov]

Codecov Report

❌ Patch coverage is 87.50000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.49%. Comparing base (8785332) to head (5159581).
⚠️ Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
...cubejs-cubestore-driver/src/WebSocketConnection.ts	86.04%	5 Missing and 1 partial ⚠️
...ges/cubejs-server-core/src/core/OrchestratorApi.ts	83.33%	0 Missing and 1 partial ⚠️

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

HEAD has 1 upload less than BASE

Flag	BASE (8785332)	HEAD (5159581)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11661       +/-   ##
===========================================
- Coverage   84.52%   58.49%   -26.04%     
===========================================
  Files         261      237       -24     
  Lines       85440    18902    -66538     
  Branches        0     3840     +3840     
===========================================
- Hits        72221    11056    -61165     
+ Misses      13219     7282     -5937     
- Partials        0      564      +564     

Flag	Coverage Δ
cube-backend	58.49% <87.50%> (?)
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.

[paveltiunov]

Correction on Upload merged coverage to Codecov: I called it deterministic twice, and it is not. It passes on the current head.

On 51595811 the job succeeded (19:22:26, run 33106503057), and codecov/patch / codecov/project reported normally. Two comments above I said it "will fail on every run as long as a .dockerbuild record exists in the run" and that it "will not clear itself". That was wrong, and it is the kind of wrong that matters — it argued for a workflow change on the premise that nothing else would fix it.

The mechanism I described still holds, but it is a race, not a certainty. upload-coverage needs only [unit, integration], while the .dockerbuild build record is uploaded by Build & Test :dev for Debian without pushing, which is in neither. So whether the download step meets that artifact depends on which finishes first:

• On this run, the Debian job started at 19:06:22 and was still in progress when upload-coverage ran and succeeded at 19:22 — no build record existed yet, so there was nothing for actions/download-artifact to choke on.
• On the runs where it failed, that job had already completed and uploaded its record.

That fits every observation, including why it failed on four consecutive heads and then passed: those heads were slower to reach upload-coverage relative to the Debian build, not different in kind.

What this changes: pattern: coverage-* is still the right fix — it removes the race entirely and stops shipping ~72 MB of binaries into a step that reads only *.lcov — but it is a flake-elimination and a speed-up, not a prerequisite for this PR going green. Treat my earlier "this PR cannot reach green on its own" as withdrawn.

Also cleared on this head: tests (mssql, true|false) and both integration (…, mssql, …) jobs pass. The Login failed for user 'sa' failures were environmental, as I said, and have gone away on their own.

Current state of 51595811: every completed check green, zero failures, with the remaining tests (…) matrix still running.

---

Generated by Claude Code