Skip to content

fix(cubestore-driver): don't fail queries on write EPIPE, report over-limit messages readably - #11490

Open
paveltiunov wants to merge 4 commits into
masterfrom
claude/cubestore-epipe-connection-error-rium57
Open

fix(cubestore-driver): don't fail queries on write EPIPE, report over-limit messages readably#11490
paveltiunov wants to merge 4 commits into
masterfrom
claude/cubestore-epipe-connection-error-rium57

Conversation

@paveltiunov

@paveltiunov paveltiunov commented Aug 6, 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

Description of Changes Made

Fixes two ways a healthy query could fail with an unhelpful error:

Internal Error: Error during processing PostgreSQL message: DataFusionError: Arrow error:
External error: Database Execution Error: ConnectionError: CubeStore connection error: write EPIPE
    at /node_modules/@cubejs-backend/cubestore-driver/src/WebSocketConnection.ts:193:20

1. write EPIPE failed queries that were never delivered

WebSocketConnection already knows how to survive Cube Store going away: the 'close' handler resends everything still pending in sentMessages over a freshly established connection, reusing the same messageId and connectionId so that Cube Store can de-duplicate it (messages_state in rust/cubestore/cubestore/src/http/mod.rs — a resend attaches to the still-running execution, or gets the cached result). But when Cube Store dropped a connection the driver hadn't noticed yet, readyState was still OPEN, the write went into a dead socket, and the send callback got EPIPE — at which point sendMessage deleted the message and rejected it. The reconnect still happened and still resent the query, but the promise was already rejected, so the user saw write EPIPE for a query that never reached Cube Store. sendAsync (used by the resend loop itself) had the same flaw, plus it never settled when the socket wasn't OPEN, stalling the loop.

A failed write now leaves the message registered and terminates the broken socket, so the existing reconnect-and-resend path delivers it. Resends are bounded per message by CUBEJS_CUBESTORE_MAX_CONNECT_RETRIES so nothing retries forever, and a write error on an already closed socket that no resend will pick up still rejects as before, so a query can't hang.

2. An over-limit message retried instead of saying it was too big

A result bigger than the connection accepts (ws maxPayload, 100 MB by default, never set explicitly before) makes ws tear the connection down with close code 1009. Combined with the resend logic above, the query was retried on a fresh connection, produced the same oversized response, and repeated until the retry budget ran out. Measured against the test mock with the budget set to 3:

after 4.0s -> ConnectionError: CubeStore connection lost: message wasn't delivered after 3 retries
query executions seen by Cube Store: 4

That mock re-executes every message. Real Cube Store de-duplicates a resend by (connection_id, message_id), but handing back a cached result also drops the entry, so the loop alternates between re-sending the cached oversized result and genuinely re-executing — at the default budget roughly ten executions and twenty connection teardowns, ending in a message that says nothing about size. Now the first failure is reported and not retried:

MessageTooLargeError: Cube Store response size exceeds the maximum message size of 100 MB.
Reduce the amount of data the query returns, e.g. by adding filters or a limit,
or raise CUBEJS_CUBESTORE_MAX_MESSAGE_SIZE.

MessageTooLargeError is distinguished from a retryable ConnectionError precisely because resending cannot help. The limit becomes explicit and configurable through the new CUBEJS_CUBESTORE_MAX_MESSAGE_SIZE, defaulting to the same 100 MB ws used implicitly, so behaviour is unchanged for anyone not hitting it. It applies to outgoing messages too: a query over the limit is rejected before being sent, since Cube Store would close the connection on it and that surfaces as an unrelated write EPIPE. A peer closing with 1009 is reported the same way.

3. Two ways a query could never settle at all (found in review)

  • A message registered on a socket whose 'close' had already fired was never written and was not in the snapshot the resend loop iterates — no resolve, no reject, and there is no timeout in this class. openSocket() now establishes a fresh connection instead, and rejects rather than hanging if that fails.
  • The resend loop registered each message on the new socket interleaved with awaited writes, so a socket closing mid-batch saw only part of it and could strand the rest. The batch is now registered in one synchronous pass before any of it is written.

Also from review: an oversized response no longer fails unrelated queries multiplexed on the same connection. ws drops the frame before its message id is read, so only a message that was alone in flight can be attributed; otherwise every message gets one more round, which answers the innocent ones and leaves the offender alone where the error can be attributed accurately. Anything still in flight after that round is failed regardless, so this cannot degenerate into a resend loop.

Tests

packages/cubejs-cubestore-driver/test/ runs the driver against a Cube Store mock that speaks the real WebSocket + flatbuffers protocol over real TCP sockets, and breaks the connection in the ways that produce these errors — a buffered write failed with EPIPE (the exact errorBuffer path from the report), a non-writable socket, a close mid-query, an over-limit response, an over-limit request, a 1009 close, and a small query in flight alongside an over-limit response. A successful result round trip is asserted too, both fresh and after a mid-query reconnect, with only the native result decoder stubbed.

Each test was checked against the unfixed source: the EPIPE ones fail with exactly the reported error, the size ones with the retry-exhausted error above, and the batch-registration one with ["2"] instead of ["2","3","4"]. 11 tests pass; tsc and eslint are clean. Wired into CI through a unit script on the package, which yarn lerna run unit picks up.

Notes for the reviewer

  • The error text deliberately does not mention CUBESTORE_TRANSPORT_MAX_FRAME_SIZE: in tungstenite 0.20 max_frame_size/max_message_size are only checked on the read path, so Cube Store puts no limit on what it sends and that knob does nothing for response size. The client's maxPayload is the only limit that applies here.
  • CUBEJS_CUBESTORE_MAX_MESSAGE_SIZE (100 MB) and Cube Store's CUBESTORE_TRANSPORT_MAX_MESSAGE_SIZE (64 MB) are independent, so the outgoing check only catches what is over the client's own limit; a query between the two is refused by Cube Store, not by this check. Defaulting the request direction to 64 MB would break anyone who raised the server-side value, and the driver cannot learn the real one today.
  • The request direction is only half covered. When a query exceeds Cube Store's own incoming limit, tungstenite returns a capacity error and rust/cubestore/cubestore/src/http/mod.rs logs it and breaks the loop without sending a close frame, so the client sees a bare disconnect and falls back to resending. The 1009 handling added here is correct WebSocket behaviour but will not fire against current Cube Store. Closing that gap needs a Rust change — Cube Store already depends on tokio-tungstenite, so the capacity error can be downcast from warp::Error::source() and answered with Message::close_with(1009, …), and the same change could advertise the server's limit on the upgrade response next to x-cubestore-version. Happy to do it in a follow-up.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej

claude added 2 commits August 4, 2026 16:09
When Cube Store closes a connection (restart, rolling deploy, idle drop),
the driver can still see the socket as OPEN and write into it. That write
fails with `write EPIPE`, and the query was rejected right away with
`ConnectionError: CubeStore connection error: write EPIPE`, surfacing as
an internal error in the SQL API and the REST API.

The connection already knows how to recover: the 'close' handler resends
everything that is still pending over a freshly established connection.
The failed write raced with it and rejected the query before the resend
could deliver it, even though the query had never reached Cube Store.

Keep such a message registered in `sentMessages` and terminate the broken
socket instead of rejecting, so the existing resend path delivers it. The
same applies to writes that fail while resending. Resends are bounded by
`CUBEJS_CUBESTORE_MAX_CONNECT_RETRIES` so a message can't retry forever,
and a write error on an already closed socket that no resend will pick up
still rejects as before.

Adds an e2e test suite that runs the driver against a Cube Store mock over
real sockets and breaks the connection in the ways that produce this error.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej
… the size limit

A query result bigger than the connection accepts (`ws` maxPayload, 100 MB)
tore the connection down without an explanation: the query was then resent
over a new connection, produced the same oversized response, and repeated
until the retry budget ran out, ending in `CubeStore connection lost: message
wasn't delivered after N retries` after N re-executions of an expensive query.
Concurrent queries on the same connection failed with `write EPIPE`.

Report the size limit instead, and don't retry what can't succeed:

  MessageTooLargeError: Cube Store response size exceeds the maximum message
  size of 100 MB. Reduce the amount of data the query returns, e.g. by adding
  filters or a limit, or raise CUBEJS_CUBESTORE_MAX_MESSAGE_SIZE.

The limit is now explicit and configurable through CUBEJS_CUBESTORE_MAX_MESSAGE_SIZE
(default 100 MB, same as the `ws` default it replaces), and it applies to
outgoing messages too: a query larger than the limit is rejected before it is
sent, since Cube Store would close the connection on it and that surfaces as an
unrelated `write EPIPE`. A peer closing the connection with 1009 is reported the
same way.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej
@paveltiunov
paveltiunov requested review from a team as code owners August 6, 2026 00:01
@github-actions github-actions Bot added javascript Pull requests that update Javascript code data source driver labels Aug 6, 2026
@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

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


Review: solid fix for the EPIPE path; one behavioural regression for concurrent queries — 1 medium-high, 1 medium, 3 low (5 inline comments)

Full review
  • Gather context / read diff
  • Review WebSocketConnection.ts changes
  • Review env.ts, errors.ts, tests, package wiring
  • Post review

What works well

The core diagnosis is right and the fix is minimal in the right way. Leaving the message registered in sentMessages on a failed write and calling terminate() to force the 'close' handler reuses the reconnect-and-resend path that already existed instead of building a second one — that's the correct shape. resendCount bounding per message, resendScheduled to distinguish "close is coming" from "close already happened", and fatalError to mark what resending cannot fix are all well-chosen pieces of state, and the comments explaining why each exists are unusually good. The reviewer note about tungstenite 0.20 only checking max_message_size on the read path is exactly the kind of thing that should be written down.

MessageTooLargeError extends ConnectionError is safe — nothing outside this package catches ConnectionError (the Postgres driver has its own unrelated class), so widening it can't accidentally re-enable retries somewhere else.

Findings

# Severity Where Issue
1 Medium-high WebSocketConnection.ts:188-209 An over-limit response rejects every pending message on the multiplexed socket with MessageTooLargeError, non-retryably. Small concurrent queries that were never answered now fail with a size error naming a limit they never approached — and unlike before, they aren't resent.
2 Medium WebSocketConnection.ts:318-327 The outgoing guard uses the client's 100 MB default, but Cube Store's incoming limit defaults to 64 MB (rust/cubestore/cubestore/src/config/mod.rs:1588). A 70 MB request passes the guard and is refused by Cube Store — the exact symptom the guard was added to eliminate.
3 Low WebSocketConnection.ts:338-345 The readyState !== OPEN branch still registers a message that is never written and never settled if 'close' already fired. Pre-existing, but it's the last remaining silent-drop in a PR about silent drops, and resendScheduled now makes it cheap to detect.
4 Low WebSocketConnection.ts:33-35 formatSize rounds to 0.1 MB, so a small configured limit renders as 0 MB / 0.1 MB exceeds 0.1 MB.
5 Low test/websocket-connection.test.ts:133-151 No test for a healthy query in flight alongside an over-limit response (finding 1), and no test asserting a successful result round trip.

Notes

  • Docs: CUBEJS_CUBESTORE_MAX_MESSAGE_SIZE isn't in docs-mintlify, but neither are CUBEJS_CUBESTORE_MAX_CONNECT_RETRIES or CUBEJS_CUBESTORE_NO_HEART_BEAT_TIMEOUT, so this is consistent with how the sibling knobs are treated. The JSDoc on env.ts:1880 cross-referencing the Cube Store counterpart is the more useful documentation here anyway.
  • Security: nothing of concern. The new env var is asIntPositive, maxPayload is now explicit rather than relying on a library default (a small improvement), and no user input reaches the error strings.
  • Performance: no new per-query work; buffer.length check is O(1). The resendCount cap is a real improvement over the previous unbounded re-execution of expensive queries.
  • Package wiring: unit: jest --coverage matches dist/test/** via the existing jest.config.js, so it depends on yarn build having run first — consistent with how the rest of the monorepo's unit scripts behave under lerna run. files in package.json still only ships dist/src and dist/codegen, so the compiled tests don't get published.
  • Not verified locally: this checkout has no node_modules, so I could not run jest, tsc, or eslint — the findings above are from reading the code, not from a failing run.
· branch `claude/cubestore-epipe-connection-error-rium57`

Comment thread packages/cubejs-cubestore-driver/src/WebSocketConnection.ts
Comment thread packages/cubejs-cubestore-driver/src/WebSocketConnection.ts
Comment thread packages/cubejs-cubestore-driver/src/WebSocketConnection.ts
Comment thread packages/cubejs-cubestore-driver/src/WebSocketConnection.ts
Comment thread packages/cubejs-cubestore-driver/test/websocket-connection.test.ts
- Don't fail unrelated queries with a size error. The connection multiplexes
  messages and `ws` drops an oversized frame before its message id is read, so
  a message that was alone in flight is the only one that can be attributed.
  Otherwise every message gets one more round, which answers the innocent ones
  and leaves the offender alone on the connection, where the next round does
  attribute it. Whatever is still in flight after that round is failed
  regardless, so an offender that keeps killing the connection before the
  others are answered can't turn into a re-send loop.

- Don't register a message on a socket whose 'close' already fired: nothing
  would write it and the re-send loop of that socket has already taken its
  snapshot, so the query would never settle. `openSocket()` now establishes a
  fresh connection instead, and rejects rather than hanging if that fails.

- Note that the client and Cube Store message size limits are independent: the
  outgoing check only catches what is over the client's own limit, while a
  query over Cube Store's stricter CUBESTORE_TRANSPORT_MAX_MESSAGE_SIZE is
  reported when Cube Store closes the connection.

- Format sizes below 1 MB in KB or bytes, so a small configured limit doesn't
  read as "0 MB".

Tests: a small query in flight alongside an over-limit response is answered
rather than failed, a successful result round trip is asserted (with only the
native result decoder stubbed), and the buffered-write setup waits for the
frame to reach the write buffer instead of a single loop turn.

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

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

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


Re-review of 608f6ab: all 5 prior findings addressed — 2 low remaining (2 new inline comments, 5 prior threads resolved)

Full review
  • Gather context / read the new commit
  • Verify each of the 5 prior findings against the current code
  • Look for new issues introduced by the fixes
  • Resolve stale threads, post new comments

Prior findings — status

# Prior finding Status
1 Oversized response failed every concurrent query non-retryably Fixed. fatalRounds gives each in-flight message one more round; the offender ends up alone on the connection where it can be attributed, and anything still in flight after that round is failed regardless, so it can't degenerate into a re-send loop. The bound is the right call — clean and documented.
2 Outgoing guard (100 MB) looser than Cube Store's 64 MB incoming limit Addressed as documentation. The env.ts:1880 JSDoc and the sendMessage comment now state that the two limits are independent and that a query between 64 MB and 100 MB is refused by Cube Store, not by this check. Verified against rust/cubestore/cubestore/src/config/mod.rs:158864 << 20 is correct. Not defaulting the request direction to 64 MB is defensible: the client can't know the server's configured value, and guessing low would reject queries a tuned Cube Store would accept.
3 Message registered on an already-closed socket never settles Fixed by openSocket(), which re-enters initWebSocket() for a fresh socket and rejects with ConnectionError rather than hanging. Both sendMessage and the re-send loop go through it. (See the one residual case below.)
4 formatSize rendered small limits as 0 MB Fixed — falls back to KB, then raw bytes.
5 Missing tests for the concurrent case and the happy path Fixed. resolves a query with the result Cube Store sent covers a successful round trip both fresh and after a mid-query reconnect, with only the native decoder stubbed; resends the other queries in flight and attributes the limit to the offender covers finding 1. waitForBufferedWrite replaces the single-setImmediate cork, which removes the flake I flagged.

All five threads resolved.

New findings

# Severity Where Issue
6 Low WebSocketConnection.ts:243-259 The re-send loop registers keys on nextWebSocket interleaved with awaited writes. If nextWebSocket closes mid-batch and its fatal branch rejects the sole registered message and returns without scheduling a re-send, the keys registered afterwards are stranded — same never-settles shape openSocket() was added to close. Registering the whole batch synchronously before writing fixes it.
7 Low websocket-connection.test.ts:196-230 The concurrent-offender test depends on a 50 ms margin between the small query's answer being processed and the oversized frame arriving. Under CI load the small query would hit fatalRounds === 2 and fail with a size error. Explicit ordering in the mock would remove the timing dependency.

Other observations

  • The commentary added in this commit is the strongest part of the change. The 'close'-handler comment explains not just what fatalRounds does but why one extra round converges on the offender and why the second round has to fail everything — that reasoning would be very hard to reconstruct from the code alone.
  • MessageTooLargeError extends ConnectionError remains safe: ConnectionError is caught nowhere outside this package (cubejs-postgres-driver has an unrelated class of the same name).
  • Security: nothing of concern. asIntPositive on the new var, maxPayload now explicit rather than an implicit library default, no user input in error strings.
  • Performance: no new per-query work; the resendCount cap replaces what was effectively unbounded re-execution of expensive queries.
  • Docs: CUBEJS_CUBESTORE_MAX_MESSAGE_SIZE isn't in docs-mintlify, consistent with its siblings CUBEJS_CUBESTORE_MAX_CONNECT_RETRIES / CUBEJS_CUBESTORE_NO_HEART_BEAT_TIMEOUT. The env.ts JSDoc is the more useful reference here.
  • Packaging: files still ships only dist/src and dist/codegen, so the compiled tests aren't published. unit: jest --coverage matches dist/test/** per jest.config.js and therefore needs yarn build first — consistent with the rest of the monorepo.
  • Not verified locally: this checkout has no node_modules, so jest, tsc and eslint could not be run. Findings are from reading the code.
· branch `claude/cubestore-epipe-connection-error-rium57`

Copy link
Copy Markdown
Member Author

Addressed the review in 608f6ab. Summary per thread:

Unrelated queries failed by one oversized response — fixed, with one deviation from the suggestion. "Fatal only when it's the sole pending message, otherwise resend" doesn't converge on its own: if the oversized response keeps arriving before the innocent answers do (the offender is resent first, its response kills the connection again), the same set stays pending every round and it becomes the re-send loop this PR exists to remove. So each message now gets exactly one extra round after an unattributable close, tracked by fatalRounds. In the common case the innocent queries are answered in that round and the offender ends up alone, where the size error is attributed to it accurately. In the pathological case everything still in flight after that round is failed regardless, bounding it at one re-execution. New test resends the other queries in flight and attributes the limit to the offender pins both halves down: the small query is answered on the new connection, the big one gets the MessageTooLargeError.

Client vs Cube Store limits are independent — documented rather than changed. Defaulting the outgoing check to 64 MB would break anyone who raised CUBESTORE_TRANSPORT_MAX_MESSAGE_SIZE server-side, and the driver has no way to learn the server's real limit today. The env-var doc and the guard now state that the check only catches what is over the client's own limit and that a query Cube Store refuses is reported when it closes the connection. The proper fix is for Cube Store to advertise its limit on the upgrade response, next to x-cubestore-version — happy to do that in the follow-up alongside the 1009 close frame.

A message registered on an already-closed socket never settles — fixed. openSocket() rejects a socket that is already CLOSED and establishes a fresh one instead (this.webSocket is dropped by the close handler, so the next attempt builds a new connection), and rejects with a ConnectionError rather than hanging if that doesn't work. Used by both sendMessage and the re-send loop.

formatSize rounding — fixed, falls back to KB and bytes under 1 MB. MiB-labelled-MB is deliberate, left as is.

Test gaps — both closed. There is now a test asserting a resolved result for a real HttpQueryResult round trip, including after a mid-query reconnect; only parseCubestoreResultMessage is stubbed, everything else stays real. The cork() setup now polls writableLength until the frame has actually reached the write buffer instead of relying on one turn of the loop.

Unrelated to the diff: the Upload merged coverage to Codecov job failed on Unable to download artifact(s): Artifact download failed after 5 retries, a runner-side artifact flake. Every test job is green.


Generated by Claude Code

Comment thread packages/cubejs-cubestore-driver/src/WebSocketConnection.ts
Comment thread packages/cubejs-cubestore-driver/test/websocket-connection.test.ts Outdated
…g it

Writing yields, so a socket that closed in the middle of a re-send batch saw
only the messages registered so far. When that set was a single message, the
fatal branch rejected it and returned without scheduling another re-send, and
the messages registered after that landed on a socket nobody would write from
or re-send again: they never settled. Register the batch in one synchronous
pass, then write it, so 'close' always sees all of it. A message answered or
failed while the batch is being written is no longer written.

The test observes the socket at the first re-send write, which is the only
point where the two orders differ: by the time the mock server receives that
message the driver has long finished the batch either way.

Also make the concurrent-offender test order the two responses explicitly
instead of spacing them 50 ms apart, so it doesn't depend on the driver being
scheduled promptly under CI load.

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

Copy link
Copy Markdown
Member Author

Both new findings fixed in a9948ad.

#6, re-send batch interleaving — real, and confirmed by the test below. The batch is now registered in one synchronous pass and written afterwards, so 'close' always sees the whole batch. One addition beyond the suggestion: a message that was answered or rejected while the batch was being written is skipped rather than written, since the fatal branch can delete entries from sentMessages during exactly that window.

Worth flagging how the test for this went, because the first attempt was wrong. I initially observed the driver's sentMessages from the mock server, when the first re-sent message arrived — and it passed against the unfixed code. The server's receipt is an I/O round trip later than the client's write chain, by which point the driver has finished the batch under either ordering, so the assertion couldn't discriminate. The committed version hooks sendAsync (only the re-send loop uses it) and snapshots at the first write, which is the one instant where the two orders differ. Verified against the unfixed source:

✕ registers the whole re-sent batch before writing any of it
    Array [
      "2",
  -   "3",
  -   "4",

#7, the 50 ms margin — fixed. The mock now orders the two responses explicitly: on the second connection the small query's answer is sent, and only then the oversized frame. They share a socket, so TCP ordering guarantees the driver processes the answer before the frame that tears the connection down — no wall-clock margin left in the test.

11 tests pass, lint and tsc clean. On the re-review notes: agreed on leaving the request-direction default alone and on skipping docs-mintlify for the env var, consistent with its siblings.


Generated by Claude Code

@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

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


Re-review of a9948ad: both remaining findings addressed — no new issues, LGTM

Full review
  • Read the new commit
  • Verify prior findings 6 & 7
  • Look for new issues introduced by the fix
  • Resolve stale threads, post review

Prior findings — status

# Prior finding Status
6 Re-send loop registered keys on nextWebSocket interleaved with awaited writes, stranding the tail of the batch if that socket closed mid-batch Fixed. WebSocketConnection.ts:246-266 builds the resent list and registers every key in one synchronous pass; :268-275 writes afterwards. A 'close' firing during any of those awaits now sees the whole batch, so the fatal branch either fails all of it or re-sends all of it — no key can be left on a dead socket.
7 Concurrent-offender test depended on a 50 ms margin Fixed. The mock now defers the oversized frame until the small query has been answered on that same connection (deferredBig / answeredSmall, websocket-connection.test.ts:258-287). No wall-clock dependency left.

Both threads resolved.

Verification of the new logic

Walked the interleavings that the up-front registration changes:

  • nextWebSocket closes mid-batch, fatal. All keys are in sentMessages, so pending.length > 1 → each gets fatalRounds += 1 and another round rather than a false attribution to whichever key happened to be registered first. This is strictly better than before, not just non-stranding.
  • Resumed writes after the socket died. The old loop keeps iterating resent; sendAsync early-returns on non-OPEN, so those are no-ops. The 'close' handler never deletes from the old socket's sentMessages, so nextWebSocket.sentMessages[key] === sentMessage can still be true after a third socket picked the message up — but the write is a no-op, so there's no double-send.
  • Answered mid-batch. The 'message' handler deletes the key synchronously before any await, so the identity check at :272 correctly skips it.
  • resendCount exhausted. Rejected in the registration pass and never pushed to resent, so it isn't written.
  • Key ordering. messageIds are integer-like keys, so Object.keys iterates ascending — ['2','3','4'] in the new test is deterministic, and on the second connection the big query (id 1) is re-sent before the small one (id 2), which is what makes the deferred-response ordering in the test work.

Nothing new to raise. Two things I'd note as deliberate rather than defects: ObservableConnection is white-box (it wraps sendAsync on the socket and relies on the re-send loop being its only caller — stated in the doc comment), and a skipped message still had resendCount incremented for a write that never happened. Neither is worth changing.

Carried over from earlier rounds

  • MessageTooLargeError extends ConnectionError is safe — ConnectionError is caught nowhere outside this package (cubejs-postgres-driver has an unrelated class of the same name).
  • Security: nothing of concern. asIntPositive on the new env var, maxPayload now explicit rather than an implicit library default, no user input in error strings.
  • Performance: no new per-query work; the resendCount cap replaces what was effectively unbounded re-execution of expensive queries.
  • Docs: CUBEJS_CUBESTORE_MAX_MESSAGE_SIZE isn't in docs-mintlify, consistent with its siblings CUBEJS_CUBESTORE_MAX_CONNECT_RETRIES / CUBEJS_CUBESTORE_NO_HEART_BEAT_TIMEOUT; the env.ts JSDoc is the more useful reference.
  • Packaging: files still ships only dist/src and dist/codegen, so compiled tests aren't published. unit: jest --coverage matches dist/test/** and therefore needs yarn build first — consistent with the rest of the monorepo.
  • Not verified locally: this checkout has no node_modules, so jest, tsc and eslint could not be run. The above is from reading the code; the interleaving analysis is reasoning, not an executed run.
· branch `claude/cubestore-epipe-connection-error-rium57`

@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.26316% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.85%. Comparing base (3728d62) to head (a9948ad).
⚠️ Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
...cubejs-cubestore-driver/src/WebSocketConnection.ts 80.55% 13 Missing and 1 partial ⚠️
packages/cubejs-backend-shared/src/env.ts 0.00% 1 Missing ⚠️

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

HEAD has 1 upload less than BASE
Flag BASE (3728d62) HEAD (a9948ad)
cubesql 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #11490       +/-   ##
===========================================
- Coverage   79.45%   57.85%   -21.61%     
===========================================
  Files         480      231      -249     
  Lines       98810    18527    -80283     
  Branches     3636     3799      +163     
===========================================
- Hits        78511    10719    -67792     
+ Misses      19778     7285    -12493     
- Partials      521      523        +2     
Flag Coverage Δ
cube-backend 57.85% <80.26%> (-1.26%) ⬇️
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

tests (mssql, false) failed on 608f6ab (job). Not fixing it — it's the test container, not this branch.

All 83 failures in that job are the same error, from tedious while opening the connection to SQL Server:

ConnectionError: Login failed for user 'sa'.
    at connectListener (node_modules/mssql/lib/tedious/connection-pool.js:85:17)
    at Connection.onConnect (node_modules/tedious/src/connection.ts:1784:9)

The whole suite failed at driver authentication, so nothing under test ran. The log has no mention of Cube Store, EPIPE, or MessageTooLarge. (The ConnectionError name is tedious', unrelated to the class in this diff.)

It also can't be caused by the diff: 608f6ab only touches cubejs-cubestore-driver and one JSDoc block in env.ts, none of which is involved in authenticating to SQL Server, and the same job passed on e8b2c90 with the same driver code. A fresh run is in flight for a9948ad; if tests (mssql, false) fails there for the same reason, I'll re-run the job rather than change code.

Separately, the Codecov report is informational: patch coverage 80.26%, with the uncovered lines being the defensive branches (re-send budget exhausted, write error on an already-closed socket, openSocket giving up). The headline project drop is an artifact of the cubesql coverage upload missing from HEAD — the same runner-side artifact flake that failed the coverage job earlier — not of this change.


Generated by Claude Code

Copy link
Copy Markdown
Member Author

integration-cubestore (24.x) failed on 608f6ab (job). Not pushing a fix — my read is a wall-clock race in the test, not a regression, but this one is close enough to the diff that the reasoning is worth stating.

One assertion failed out of 31, in QueryQueueCubeStore Queue Driver › orphaned:

expect(cancelledQuery).toBe('114')
Expected: "114"
Received: "113"
    at test/unit/QueryQueue.abstract.ts:274:30

That test queues four 800–900 ms queries staggered 50/75/100 ms apart, then checks 500 ms later which one the orphaned-query cancellation caught. Getting 113 instead of 114 means the checkpoint landed while a different query was the most recently cancelled — the assertion is decided entirely by wall-clock spacing between queries queued 25 ms apart.

Why I don't think it's the diff:

  • 28 of 29 executed tests in that suite passed, including the sibling orphaned with custom ttl, plus stream handler, priority, sequence and the queue round trips. A broken Cube Store transport would not fail exactly one timing assertion and nothing else.
  • On the hot path this commit adds one await hop (sendMessage now goes through openSocket()) and two fields on the sentMessages entry. That's microtask-level; the assertion turns on tens of milliseconds.
  • The other changed paths — fatalRounds, the re-send batch, formatSize — only execute on a connection failure. Nothing in the log shows one: no EPIPE, no reconnect, no MessageTooLarge.
  • The same job passed on e8b2c90 with this driver code.

I can't reproduce it locally to prove flakiness: this environment has no Cube Store binary (the @cubejs-backend/cubestore post-install is skipped here) and no Docker, so integration:cubestore can't run. The decisive check is the run in flight for a9948ad, which is waiting on build-cubestore. If integration-cubestore fails there the same way, I'll treat it as mine and dig in rather than re-run.

Two things did resolve on a9948ad, both previously flagged here: tests (mssql, false) passed, confirming the Login failed for user 'sa' failure was the container; and Upload merged coverage to Codecov passed, so the artifact-download flake cleared without needing a re-run. unit (24.x, 3.13) and lint are green on a9948ad.


Generated by Claude Code

Copy link
Copy Markdown
Member Author

Correction to a cost claim in the description, after reading the Cube Store side properly.

I wrote that the retry loop meant "21 executions of an expensive query". That was measured against the mock in these tests, which re-executes every message. Real Cube Store does not: http/mod.rs keys in-flight work by (connection_id, message_id) in messages_state, and the driver sends a stable connectionId per WebSocketConnection and preserves messageId across resends. So a resend that arrives while the query is still Processing subscribes the new socket to the running execution, and one that arrives after it is Complete gets the cached result. There is even a janitor that keeps finished results around for exactly this (CUBESTORE_DROP_WS_COMPLETE_MESSAGES_AFTER, 10 min; processing entries 1 h), logging "Keeping N orphaned complete results to be retrieved by reconnecting socket".

Two consequences:

  1. The resend path in fix 1 is cheaper and safer than I claimed — a query that was never delivered, or was delivered but lost its answer, is de-duplicated server-side rather than re-run. The reconnect-and-resend design this PR repairs is the client half of a resume protocol Cube Store already implements.
  2. For the oversized-response loop the cost is lower but still real: handing over the cached result removes the entry, so rounds alternate between re-sending the cached oversized result and genuinely re-executing — roughly ten executions and twenty connection teardowns at the default budget, not twenty-one executions. I've corrected the description.

Nothing about the fixes changes; the retry loop is still worth replacing with a readable error.

Related, for anyone who hits the original write EPIPE: the driver-side error can't say why Cube Store went away, only that it was already gone when the driver wrote. In the WebSocket loop the connection is dropped by a read error from web_socket.next() (logged Websocket error: … — this covers an inbound message over CUBESTORE_TRANSPORT_MAX_MESSAGE_SIZE), a flatbuffer deserialization failure (Websocket message read error), a full query channel under burst control (Websocket channel error), or a non-binary frame. HttpCommand::CloseConnection is the one deliberate server-initiated close, and the only CubeError::wrong_connection that produces it is in the test mock, so it does not fire in production. There is no server-side idle timeout in that loop and the driver pings every 5s, so idleness alone shouldn't close it — which leaves process restart, deploy, OOM, or an intermediary reset as the likely cause when no Cube Store log line accompanies it.


Generated by Claude Code

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

Labels

data source driver javascript Pull requests that update Javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants