fix(cubestore-driver): don't fail queries on write EPIPE, report over-limit messages readably - #11490
fix(cubestore-driver): don't fail queries on write EPIPE, report over-limit messages readably#11490paveltiunov wants to merge 4 commits into
write EPIPE, report over-limit messages readably#11490Conversation
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
|
Claude finished @paveltiunov's task in 3m 52s —— View job Review: solid fix for the
|
| # | 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_SIZEisn't indocs-mintlify, but neither areCUBEJS_CUBESTORE_MAX_CONNECT_RETRIESorCUBEJS_CUBESTORE_NO_HEART_BEAT_TIMEOUT, so this is consistent with how the sibling knobs are treated. The JSDoc onenv.ts:1880cross-referencing the Cube Store counterpart is the more useful documentation here anyway. - Security: nothing of concern. The new env var is
asIntPositive,maxPayloadis 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.lengthcheck is O(1). TheresendCountcap is a real improvement over the previous unbounded re-execution of expensive queries. - Package wiring:
unit: jest --coveragematchesdist/test/**via the existingjest.config.js, so it depends onyarn buildhaving run first — consistent with how the rest of the monorepo'sunitscripts behave underlerna run.filesinpackage.jsonstill only shipsdist/srcanddist/codegen, so the compiled tests don't get published. - Not verified locally: this checkout has no
node_modules, so I could not runjest,tsc, oreslint— the findings above are from reading the code, not from a failing run.
- 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 finished @paveltiunov's task in 3m 29s —— View job Re-review of
|
| # | 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:1588 — 64 << 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 whatfatalRoundsdoes 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 ConnectionErrorremains safe:ConnectionErroris caught nowhere outside this package (cubejs-postgres-driverhas an unrelated class of the same name).- Security: nothing of concern.
asIntPositiveon the new var,maxPayloadnow explicit rather than an implicit library default, no user input in error strings. - Performance: no new per-query work; the
resendCountcap replaces what was effectively unbounded re-execution of expensive queries. - Docs:
CUBEJS_CUBESTORE_MAX_MESSAGE_SIZEisn't indocs-mintlify, consistent with its siblingsCUBEJS_CUBESTORE_MAX_CONNECT_RETRIES/CUBEJS_CUBESTORE_NO_HEART_BEAT_TIMEOUT. The env.ts JSDoc is the more useful reference here. - Packaging:
filesstill ships onlydist/srcanddist/codegen, so the compiled tests aren't published.unit: jest --coveragematchesdist/test/**perjest.config.jsand therefore needsyarn buildfirst — consistent with the rest of the monorepo. - Not verified locally: this checkout has no
node_modules, sojest,tscandeslintcould not be run. Findings are from reading the code.
|
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 Client vs Cube Store limits are independent — documented rather than changed. Defaulting the outgoing check to 64 MB would break anyone who raised A message registered on an already-closed socket never settles — fixed.
Test gaps — both closed. There is now a test asserting a resolved result for a real Unrelated to the diff: the Generated by Claude Code |
…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
|
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 Worth flagging how the test for this went, because the first attempt was wrong. I initially observed the driver's #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 Generated by Claude Code |
|
Claude finished @paveltiunov's task in 1m 53s —— View job Re-review of
|
| # | 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:
nextWebSocketcloses mid-batch, fatal. All keys are insentMessages, sopending.length > 1→ each getsfatalRounds += 1and 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;sendAsyncearly-returns on non-OPEN, so those are no-ops. The'close'handler never deletes from the old socket'ssentMessages, sonextWebSocket.sentMessages[key] === sentMessagecan 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 anyawait, so the identity check at:272correctly skips it. resendCountexhausted. Rejected in the registration pass and never pushed toresent, so it isn't written.- Key ordering.
messageIds are integer-like keys, soObject.keysiterates 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 ConnectionErroris safe —ConnectionErroris caught nowhere outside this package (cubejs-postgres-driverhas an unrelated class of the same name).- Security: nothing of concern.
asIntPositiveon the new env var,maxPayloadnow explicit rather than an implicit library default, no user input in error strings. - Performance: no new per-query work; the
resendCountcap replaces what was effectively unbounded re-execution of expensive queries. - Docs:
CUBEJS_CUBESTORE_MAX_MESSAGE_SIZEisn't indocs-mintlify, consistent with its siblingsCUBEJS_CUBESTORE_MAX_CONNECT_RETRIES/CUBEJS_CUBESTORE_NO_HEART_BEAT_TIMEOUT; theenv.tsJSDoc is the more useful reference. - Packaging:
filesstill ships onlydist/srcanddist/codegen, so compiled tests aren't published.unit: jest --coveragematchesdist/test/**and therefore needsyarn buildfirst — consistent with the rest of the monorepo. - Not verified locally: this checkout has no
node_modules, sojest,tscandeslintcould not be run. The above is from reading the code; the interleaving analysis is reasoning, not an executed run.
Codecov Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
All 83 failures in that job are the same error, from The whole suite failed at driver authentication, so nothing under test ran. The log has no mention of Cube Store, It also can't be caused by the diff: 608f6ab only touches 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, Generated by Claude Code |
|
One assertion failed out of 31, in 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 Why I don't think it's the diff:
I can't reproduce it locally to prove flakiness: this environment has no Cube Store binary (the Two things did resolve on a9948ad, both previously flagged here: Generated by Claude Code |
|
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: Two consequences:
Nothing about the fixes changes; the retry loop is still worth replacing with a readable error. Related, for anyone who hits the original Generated by Claude Code |
Check List
Description of Changes Made
Fixes two ways a healthy query could fail with an unhelpful error:
1.
write EPIPEfailed queries that were never deliveredWebSocketConnectionalready knows how to survive Cube Store going away: the'close'handler resends everything still pending insentMessagesover a freshly established connection, reusing the samemessageIdandconnectionIdso that Cube Store can de-duplicate it (messages_stateinrust/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,readyStatewas stillOPEN, the write went into a dead socket, and thesendcallback gotEPIPE— at which pointsendMessagedeleted the message and rejected it. The reconnect still happened and still resent the query, but the promise was already rejected, so the user sawwrite EPIPEfor 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'tOPEN, 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_RETRIESso 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 (
wsmaxPayload, 100 MB by default, never set explicitly before) makeswstear 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: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:MessageTooLargeErroris distinguished from a retryableConnectionErrorprecisely because resending cannot help. The limit becomes explicit and configurable through the newCUBEJS_CUBESTORE_MAX_MESSAGE_SIZE, defaulting to the same 100 MBwsused 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 unrelatedwrite EPIPE. A peer closing with 1009 is reported the same way.3. Two ways a query could never settle at all (found in review)
'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.Also from review: an oversized response no longer fails unrelated queries multiplexed on the same connection.
wsdrops 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 withEPIPE(the exacterrorBufferpath 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
EPIPEones 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;tscandeslintare clean. Wired into CI through aunitscript on the package, whichyarn lerna run unitpicks up.Notes for the reviewer
CUBESTORE_TRANSPORT_MAX_FRAME_SIZE: in tungstenite 0.20max_frame_size/max_message_sizeare 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'smaxPayloadis the only limit that applies here.CUBEJS_CUBESTORE_MAX_MESSAGE_SIZE(100 MB) and Cube Store'sCUBESTORE_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.rust/cubestore/cubestore/src/http/mod.rslogs 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 ontokio-tungstenite, so the capacity error can be downcast fromwarp::Error::source()and answered withMessage::close_with(1009, …), and the same change could advertise the server's limit on the upgrade response next tox-cubestore-version. Happy to do it in a follow-up.🤖 Generated with Claude Code
https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej