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

[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

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 socket, which is exactly the event the existing resend loop already handles — no second retry path. The loop itself is unchanged except that it registers a batch before writing it, so a socket dying mid-batch can't strand the rest. A message registered on a socket whose 'close' has already fired re-establishes the connection instead of waiting for a resend that will never come, since there is no timeout in this class.

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 above, the query was retried on a fresh connection, produced the same oversized response, and repeated. 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, and the error it eventually surfaces says nothing about size. Now the 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.

The connection multiplexes messages and ws drops the oversized frame before its message id is read, so the error can only be pinned on a message that was alone in flight. Anything else gets one more round, which answers the innocent queries and usually leaves the offender alone on the connection, where the next round does name it. Whatever is still in flight after that round is failed regardless, so an offender whose response keeps arriving ahead of the other answers can't be re-sent forever; only consecutive unattributable failures count towards that, so an ordinary disconnect in between doesn't spend a query's extra round. Outside the over-limit case the resends stay bounded by the connection-level retry only, as before this PR.

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, a small query in flight alongside an over-limit response, an over-limit response that always beats the other answer and so can never be attributed, and a slow query spanning two size incidents with an ordinary disconnect between them.

A successful result round trip is asserted too, both fresh and after a mid-query reconnect, with only the native result decoder stubbed.

Every test was checked against the source it fixes: 10 of the 12 fail against master, the EPIPE ones with exactly the reported error. The two that only a later commit fixes were checked against that commit rather than master — the unattributable-loop one hung to the jest timeout against 5219a9d, and the extra-round one failed with MessageTooLargeError on the innocent query against e7a5bd5. 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]

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`

[claude]

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: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 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`

[paveltiunov]

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

[paveltiunov]

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

❌ Patch coverage is 82.45614% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.96%. Comparing base (3728d62) to head (f5a22fc).
⚠️ Report is 20 commits behind head on master.

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

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

HEAD has 1 upload less than BASE

Flag	BASE (3728d62)	HEAD (f5a22fc)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11490       +/-   ##
===========================================
- Coverage   79.45%   57.96%   -21.50%     
===========================================
  Files         480      231      -249     
  Lines       98810    18561    -80249     
  Branches     3636     3793      +157     
===========================================
- Hits        78511    10759    -67752     
+ Misses      19778     7280    -12498     
- Partials      521      522        +1     

Flag	Coverage Δ
cube-backend	57.96% <82.45%> (-1.15%)	⬇️
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]

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

[paveltiunov]

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

[paveltiunov]

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

[claude]

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

---

Re-review of 5219a9d: the simplification is mostly right, but dropping fatalRounds reintroduces a non-terminating re-send loop — 1 medium-high, 2 low

Full review

• Gather context / read the new commit
• Verify prior findings still hold under the simplified code
• Look for regressions introduced by the revert
• Resolve stale threads, post review

On the refactor as a whole

The direction is right and most of what was cut deserved cutting. handleSendError collapsing into a bare terminate() is a genuine improvement: the write-failure path had grown three states (resendScheduled, resendCount, a messageId threaded through sendAsync purely for error attribution) to express "let the existing 'close' handler deal with it", and now it just says that. -182 lines with the reported EPIPE failures still fixed and still covered by tests is a good trade, and the 10 remaining tests are the ones that carry the actual behaviour.

Two of the three pieces that came back with the revert matter, though, and one of them is the case you yourself argued doesn't converge.

Findings

#	Severity	Where	Issue
8	Medium-high	WebSocketConnection.ts:202-216	With pending.length === 1 as the only attribution condition and no round counter, an over-limit response arriving while a second query is in flight re-sends forever. Nothing bounds it — currentConnectionTry is never incremented on the MAX_PAYLOAD_EXCEEDED_CODE path and is reset by every pong, so retryWaitTime() stays at 1 s. The offending query's promise never settles.
9	Low	WebSocketConnection.ts:221-225	Re-send loop registers keys on nextWebSocket interleaved with awaited writes again (prior finding #6, fixed in a9948ad, reverted here). A socket dying mid-batch through the fatal single-pending branch strands the keys registered afterwards — never settles.
10	Low	WebSocketConnection.ts:304-309	The CLOSED branch now rejects rather than re-establishing. It settles, which is the important half, but it fails a query that was never written and that one re-entry into initWebSocket() would have delivered.

Finding 9 couldn't be posted inline — those lines are identical to master after the revert, so they're outside the diff GitHub will anchor to. Details:

nextWebSocket.sentMessages[key] = … happens after await …sendAsync(…) yields for the previous key. If nextWebSocket dies mid-batch with exactly one key registered so far, its 'close' handler takes the fatalError && pending.length === 1 branch, rejects that one and returns without scheduling a re-send. The keys registered afterwards land on a socket whose 'close' has already been handled: sendAsync early-returns on non-OPEN without calling send, no re-send loop will iterate them, and there is no timeout in this class. The non-fatal path survives only incidentally — the re-send setTimeout reads Object.keys a second later, by which point the batch happens to be complete. Registering the whole batch in one synchronous pass before writing any of it fixes it, as in a9948ad.

On finding 8 specifically

The sequence:

1. big + small in flight; big's response exceeds maxPayload → error (WS_ERR_UNSUPPORTED_MESSAGE_LENGTH) → fatalError set, socket torn down.
2. pending.length === 2, so the fatal branch is skipped and both are re-sent.
3. Object.keys iterates message ids ascending, so big goes first; Cube Store re-attaches it to the running execution or returns the cached result, and that oversized frame is on the wire ahead of small's answer. ws reads it first and destroys the connection before small is answered.
4. Pending set unchanged → step 2.

The new test doesn't catch this because its mock constructs the converging order: deferredBig / answeredSmall (websocket-connection.test.ts:216-239) hold the oversized frame until small has been answered on that connection. Reverse those two sends on connection ≥ 1 and it should hang to the jest timeout.

This is the same objection you raised when you introduced fatalRounds:

"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 same set stays pending every round and it becomes the re-send loop this PR exists to remove.

That still holds. A single per-message counter restores the bound without any of the other machinery — one extra round each, then fail whatever is still in flight. Suggested diff is in the inline comment.

Description drift

Two claims in the PR body no longer describe the code and should be corrected along with the fix:

• "Resends are bounded per message by CUBEJS_CUBESTORE_MAX_CONNECT_RETRIES so nothing retries forever" — resendCount is gone; a connection that establishes successfully and dies with pending messages re-sends without limit. That's the pre-PR behaviour restored, defensible on its own, but the sentence claims otherwise.
• "Anything still in flight after that round is failed regardless, so this cannot degenerate into a resend loop" — this is precisely what finding 8 removes.

The commit message's "Bounding the re-sends is left to the connection-level retry, as before" is also optimistic: currentConnectionTry resets on every pong, so a repeatedly-established-then-dying connection has no effective ceiling.

Unaffected by this commit

• The EPIPE fix itself — terminate() on write failure, message left registered, 'close' re-sends — is intact and covered by resends a query when the write fails with EPIPE and resends every query that was in flight when the write failed.
• Single-message over-limit response, over-limit request, and 1009 close are all still correctly non-retryable, with tests.
• MessageTooLargeError extends ConnectionError remains safe — ConnectionError is caught nowhere outside this package.
• Security: nothing of concern. asIntPositive on the new env var, maxPayload explicit rather than an implicit library default, no user input in error strings.
• Performance: no new per-query work. Note that removing resendCount also removes the only cap on re-executing an expensive query, which is a step back from the previous commit even outside the over-limit case.
• formatSize KB/bytes fallback, the env-var JSDoc on the two independent limits, and the happy-path round-trip test are all unchanged and still address findings 2, 4 and 5.
• 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 and loop analysis is reasoning, not an executed run.

All 7 prior threads were already resolved; nothing to resolve this round.

· branch `claude/cubestore-epipe-connection-error-rium57`

[claude]

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

---

Re-review of e7a5bd5: all three prior findings fixed — 2 low remaining, otherwise LGTM

Full review

• Read the new commit
• Verify prior findings 8, 9, 10
• Look for new issues introduced by the fix
• Resolve stale threads, post review

Prior findings — status

#	Prior finding	Status
8	pending.length === 1 as the only attribution rule → non-terminating re-send loop	Fixed. fatalRounds is back on SentMessage (:51) and the fatal branch (:213-232) gives every message in flight one extra round, then fails whatever is left. Walked the loop: round 1 sets both to 1 and re-sends, round 2 takes them to 2 and rejects both. Bounded at one extra execution. The new test gives up when an over-limit response keeps killing the connection constructs the diverging order (oversized frame sent as soon as both queries have arrived on that connection, on every connection) rather than the converging one, so it does discriminate — it's the case that hung before.
9	Re-send loop registered keys interleaved with awaited writes	Fixed. :245-247 registers the whole batch in one synchronous pass, :249-252 writes afterwards. A 'close' during any write now sees the complete batch, so the fatal branch takes the pending.length > 1 path rather than falsely attributing to whichever key was registered first.
10	CLOSED branch rejected a query that was never written	Fixed. :337 re-enters sendMessage, which builds a fresh connection since 'close' dropped this.webSocket.

Both open threads resolved.

Verification of the new logic

• Dropped identity check in the write loop. a9948ad skipped a key whose entry had changed on nextWebSocket; that guard is gone here. It turns out to be unnecessary: a socket loses entries only via 'message' (which happens strictly after that key's own write) or via 'close' (which makes it non-OPEN, and sendAsync early-returns on non-OPEN). So no key is written twice to a live socket, and a stale write to a dead one is a no-op. Removing it is fine.
• webSocket.sentMessages[key].buffer after the batch is copied. The fatal branch of a later socket only mutates that socket's own map, so the old map still holds the entry — no undefined deref.
• Early return when the fatal branch empties the map still nulls this.webSocket on the way out (:225-231), so it doesn't leak a dead socket.
• error handler ordering on MAX_PAYLOAD_EXCEEDED_CODE is unchanged and still correct: this.webSocket = null before the (no-op, post-open) reject, so the next initWebSocket() builds fresh.

Findings

#	Severity	Where	Issue
11	Low	WebSocketConnection.ts:217	fatalRounds is cumulative over a message's lifetime, never reset. A long-running query that survived one unattributable close, then rode out a couple of ordinary disconnects, is rejected with MessageTooLargeError in the first round of a later, unrelated size incident — the extra round it was owed never happens. Resetting it on a non-fatal close keeps convergence (the pathological loop is fatal every round) and makes the counter mean what the comment says.
12	Low	WebSocketConnection.ts:337	The CLOSED re-entry has no counter of its own — a peer that accepts the upgrade and closes cleanly never increments currentConnectionTry, so the chain is unbounded. Not a busy loop (each turn costs a TCP connect), and consistent with the other retry paths here; noted rather than raised.

Carried over

• The EPIPE fix — terminate() on write failure, message left registered, 'close' re-sends — is untouched and still covered.
• Single-message over-limit response, over-limit request, and the 1009 close remain correctly non-retryable, with tests.
• MessageTooLargeError extends ConnectionError is safe; ConnectionError is caught nowhere outside this package.
• Security: nothing of concern. asIntPositive on the new env var, maxPayload explicit rather than an implicit library default, no user input in error strings.
• Performance: no new per-query work. Ordinary (non-fatal) re-sends remain unbounded by design — pre-PR behaviour, and Cube Store de-duplicates them server-side, so they don't re-execute.
• Docs: CUBEJS_CUBESTORE_MAX_MESSAGE_SIZE isn't in docs-mintlify, consistent with its siblings; the env.ts JSDoc is the useful reference.
• The PR description now matches the code again on both bounding claims.
• 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 and convergence analysis is reasoning, not an executed run.

· branch `claude/cubestore-epipe-connection-error-rium57`

[paveltiunov]

All three findings fixed in e7a5bd5.

#8 confirmed by reproducing it rather than by reading, since you couldn't run anything: the new test gives up when an over-limit response keeps killing the connection has the mock hold the oversized frame until both queries are in flight and never answer the small one, so the pending set can't shrink. Against 5219a9d it hung to the 60 s jest timeout; with the round counter back it settles in about a second. Your sequence was right, including that nothing bounded it — currentConnectionTry isn't touched on the over-limit path and pong resets it.

#9 fixed by registering the batch in one pass before writing any of it. Worth noting it was only reachable because of the fatal branch this PR adds — on master the interleaving is harmless, since the re-send setTimeout re-reads sentMessages a second later — so it's this PR's regression to fix, not a pre-existing one.

#10 now re-enters sendMessage, as suggested.

11 tests pass, tsc and eslint clean. The PR body no longer claims a per-message re-send budget: outside the over-limit case the bound is the connection-level retry only, which is the pre-PR behaviour.

---

Generated by Claude Code

[claude]

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

---

Re-review of f5a22fc: both prior findings addressed — no new issues, LGTM

Full review

• Read the new commit
• Verify prior findings 11 & 12
• Look for regressions introduced by the reset
• Resolve stale threads, post review

Prior findings — status

#	Prior finding	Status
11	fatalRounds cumulative over a message's lifetime, so an innocent long-running query could be failed in the first round of an unrelated size incident	Fixed. WebSocketConnection.ts:232-242 resets fatalRounds for every pending message on a close that carries no fatalError, so only consecutive unattributable failures count. The bound survives because the pathological loop is fatal on every round — nothing ever reaches the else.
12	CLOSED re-entry: unbounded recursion, and messageId reuse read as a fresh send	Addressed as far as it needed to be. The messageId reuse is now documented at :345-347. The unbounded-chain half I raised as an observation rather than a defect and it's unchanged — consistent with every other retry path in the class, and each turn costs a real TCP connect, so it isn't a busy loop. Fine to leave.

Both threads resolved.

Verification of the reset

• Convergence still holds. In the divergent case (gives up when an over-limit response keeps killing the connection) the mock sends the oversized frame on every connection, so fatalError is set every round, the else never runs, and both messages hit fatalRounds > 1 on round two. The bound is untouched.
• pending is still complete in the else. No deletions happen on that path, so every key indexes a live SentMessage — no undefined deref.
• Reset scope. It applies to the offender too, which is unavoidable: the whole premise is that the offender can't be identified. See the note below.
• New test discriminates. Walked it: connection 0 → oversized (both to fatalRounds 1, re-sent), connection 1 → terminate() (ordinary close, both reset to 0), connection 2 → oversized (both back to 1, re-sent), connection 3 → long answered then oversized, so big is alone and correctly named. Without the reset, connection 2 takes both to 2 and rejects long with MessageTooLargeError — exactly the misattribution the commit removes. The answer-then-oversize ordering on connection 3 shares a socket, so TCP ordering makes it deterministic; no wall-clock margin.
• Types. fatalRounds stays non-optional on SentMessage and is initialised at :330; the re-send loop copies the same object across sockets, so the counter follows the message as intended.

One observation, not a finding

Alternating failures are the one shape the reset doesn't bound: if an ordinary disconnect lands between every two size incidents, the counter is cleared each time and the offender is re-sent indefinitely. It needs a connection that is both flapping for unrelated reasons and serving an oversized response, and in that state the driver is already re-connecting on its own — pre-PR behaviour, and strictly better than the previous cumulative counter, which fixed the loop by breaking the common case. Worth knowing rather than worth changing.

Carried over from earlier rounds

• The EPIPE fix — terminate() on write failure, message left registered, 'close' re-sends — is untouched and covered by resends a query when the write fails with EPIPE and resends every query that was in flight when the write failed.
• Single-message over-limit response, over-limit request, and the 1009 close remain correctly non-retryable, each with a test.
• The re-send batch is still registered in one synchronous pass before any write (:249-262), so a socket dying mid-batch can't strand its tail.
• 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 explicit rather than an implicit library default, no user input in error strings.
• Performance: no new per-query work; the reset is an O(pending) pass on a path that only runs when a connection dies.
• 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 useful reference.
• Packaging: files ships only dist/src and dist/codegen, so compiled tests aren't published. unit: jest --coverage matches dist/test/** and 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 convergence and interleaving analysis is reasoning, not an executed run.

· branch `claude/cubestore-epipe-connection-error-rium57`

[paveltiunov]

#11 is real and fixed in f5a22fc. Reproduced it rather than reasoning about it: gives a query its extra round back once an ordinary disconnect intervenes runs a size incident, then an ordinary disconnect, then a second size incident, with the same slow query in flight throughout. Without the reset that query rejects with MessageTooLargeError instead of being answered — exactly the misattribution you described. fatalRounds now resets on any close that isn't fatal, so only consecutive unattributable failures count; the pathological loop is fatal every round, so it still converges (gives up when an over-limit response keeps killing the connection still passes).

#12 left as is, agreed it's an observation — every retry path in this class is unbounded the same way, and bounding just this one would be arbitrary. Added the note that messageId is deliberately reused so Cube Store de-duplicates the re-entry.

12 tests pass, tsc and eslint clean.

---

Generated by Claude Code

[paveltiunov]

CI on f5a22fc is green except for one job, tests (mssql, true), which is a SQL Server container failure rather than anything in this diff:

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)

All 86 runnable tests in mssql-full.test.js fail with that same error, i.e. nothing ever connected. The decisive part: tests (mssql, false) passed on this same commit — identical code, same suite, one passed and one failed, so the difference is the container, not the change.

The two failures on the previous head are gone on f5a22fc and were environment flakes too, for the record:

• integration-cubestore (24.x) — was one assertion, QueryQueueCubeStore › orphaned, "114" vs "113" at QueryQueue.abstract.ts:274, a wall-clock race in query-orchestrator's shared queue test with the other 28 passing. Green on f5a22fc, which also exercises this driver end to end.
• integration-smoke (24.x, 3.13, false) — was Port 6875 not bound after 10000ms in beforeAll: the Materialize testcontainer never started, so Cube never booted. Both smoke variants green on f5a22fc.

I can't re-run the mssql job myself — rerun-failed-jobs returns 403 Resource not accessible by integration — so it needs a maintainer re-run. Everything else is green: unit, unit-core, build, build-cubestore, lint, CodeQL, codecov, and the rest of the driver matrix. (tests (redshift, ...) is cancelled by design on secrets-gated runs.)

---

Generated by Claude Code