[pull] master from cube-js:master - #691
Merged
Merged
Conversation
…ma-compiler test (#11656) * test(schema-compiler): cover multi-fact derived measure defined on a view Adds a Tesseract test for the AOV basket shape: a ratio measure authored on a view whose numerator (sum of sales amount) and denominator (count distinct of transaction ids, narrowed by filters owned by the line-item cube) live in two fact cubes at different grains, joined only through shared items, locations and dates cubes. Covers the working shape - a multi_stage view measure, whose division is evaluated after each fact has been aggregated to the query grain - and pins the current limits: the same expression without multi_stage, and any multi-fact query carrying a segment, both fail to find a join path, and the legacy planner does not plan multi-fact queries at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLzMjTkAfG9tjDmjVR3EL2 * docs: document view-level measures and multi-fact derived metrics Views can define their own measures and dimensions as long as the SQL only combines members the view already includes. Adds the `measures` and `dimensions` parameters to the view reference, and a short section on the views concept page for the case they exist for: a metric whose parts live in different cubes. Adds a "Combining facts in one measure" section to multi-fact views covering `multi_stage`, which defers the expression until each fact has been aggregated and joined - without it the same measure fails to find a join path across the two facts. Adds an average order value recipe covering both shapes: a plain calculated measure when both parts sit in one cube, and a multi_stage view measure when revenue and transaction count come from two fact tables at different grains. Also corrects the multi-fact filters section: fact-specific filters and segments are not applied per subquery, they fail the query - a measure's own `filters` is what narrows one fact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLzMjTkAfG9tjDmjVR3EL2 * docs: reconcile the two multi-fact filter sections The SQL API "Filtering the join" bullet claimed a predicate on a fact-specific dimension filters only that fact's subquery, which contradicts the corrected "Filters and segments" section. The rewrite does attach the predicate to that fact's member, but the merged query is then planned like any other multi-fact query, so the member still has to be shared by all facts. Says that once, and points the SQL API bullet at it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLzMjTkAfG9tjDmjVR3EL2 * docs: mark cross-cube view measures multi_stage; test the fan-out case The orders/line_items example was wrong: the two cubes join one_to_many, so a plain calculated view measure is evaluated inside the single joined scan and its `sum` runs over the multiplied rows - 400 instead of 300 on the sample data, a silently inflated numerator. `multi_stage` aggregates each side first. Makes the rule one rule on all pages: a view measure combining members of different cubes wants `multi_stage`, whether the cubes join with fan-out (wrong number) or don't join at all (no join path). The guide page now shows the short version and links the reference instead of repeating it. Test changes: - covers both reference forms - `{CUBE.member}` and `{view_name.member}` - and pins that a bare `{member}` is rejected - new fan-out block: the plain measure inlines into the multiplied join, the multi_stage one aggregates each side before dividing - drops the planner-generated `q_N` aliases from the ratio pattern, keeping the assertion on the aggregate columns - compiles the model once per suite, and splits the positive segment case out of the negative test Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLzMjTkAfG9tjDmjVR3EL2 * test: temper the fan-out assertion; match the docs example to it The `[\s\S]*?` span between the sum and its GROUP BY was unanchored, so it would happily cross a `LEFT JOIN "line_items"` and land on a later GROUP BY - passing on exactly the regression it exists to catch. Tempered against `line_items`; checked it still matches the real multi_stage SQL and now rejects a doctored copy whose sum leg carries the join. The docs example now nests `orders.line_items` and says the two cubes join one_to_many, so the view block on its own shows which of the two cases the surrounding prose is describing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLzMjTkAfG9tjDmjVR3EL2 * docs: state the join relationship in prose, not inside the YAML tab A CodeGroup shows one tab at a time, so a note added as a YAML comment is invisible to a reader on the JavaScript tab - and the relationship is exactly what the example needs stated, since `orders.line_items` alone doesn't say whether the join fans out. Moved above the CodeGroup on both pages. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLzMjTkAfG9tjDmjVR3EL2 * docs: trim the duplicated view-measure example on the guide page The block was byte-identical to the one on the view reference, against docs-mintlify/CLAUDE.md's "say it once". The guide page's point is that a view can declare a measure at all, so it now shows just that declaration, elides the cubes block, and links the reference for the full example. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLzMjTkAfG9tjDmjVR3EL2 --------- Co-authored-by: Claude <noreply@anthropic.com>
…#11658) * fix(release): make lerna-publish.sh step 5 non-interactive and clean up staged files Step 5 ended with a dangling line continuation after --create-release=github, so the intended --yes never reached lerna. The release therefore stopped at lerna's "Are you sure you want to create these versions?" prompt and exited 1, which is fatal in any non-interactive/CI shell. The step 4 cleanup used `git restore .`, which only restores the working tree from the index. The cubestore `version` lifecycle hook (sync-cargo-version.js) stages rust/cubestore/Cargo.toml and Cargo.lock, so those stayed staged at the bumped version after any failure — leaving the tree dirty and making the next run abort on working tree validation. Restore the index as well. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kRxi2jizmJQKt9cuv4vmg * fix(release): guard lerna-publish.sh cleanup against pre-existing work Addresses review feedback on the `git restore --staged --worktree .` cleanup. The restore discards staged as well as unstaged changes to tracked files, so on a dirty tree it would destroy work the operator had staged before invoking the script (an index-only change has no reflog to recover from). Gate the script on a clean tree up front, so cleanup can only ever undo what the script itself did. The gate checks tracked files only (--untracked-files=no). `git restore` cannot touch untracked paths, so they are outside the blast radius being guarded, and untracked build output that no .gitignore covers must not block a release. Also pair each cleanup with a tightly scoped `git clean`: lerna writes a CHANGELOG.md for any package that lacks one, and a newly created CHANGELOG.md is untracked, so `git restore` leaves it behind to trip lerna's working tree validation on the next run. Scoping the clean to '*CHANGELOG.md' removes exactly those files and leaves every other untracked path alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kRxi2jizmJQKt9cuv4vmg * fix(release): clean up lerna-publish.sh bump from an EXIT trap Addresses further review feedback. `set -e` aborts the script before the inline step 4 cleanup if step 1's lerna call or step 2's `yarn install` fails, leaving exactly the bumped, partly staged tree these fixes exist to avoid — and a `yarn install` failure is a realistic path, not a theoretical one. Move cleanup into an EXIT trap so it also covers those abort paths, and drop the two duplicated inline blocks. The trap is installed after the pre-flight gate, never before, so a tree the gate rejects is left untouched; step 4 clears the trap because the version commit, tag and release are meant to survive. The handler preserves the original exit status. The gate's advice now also names the discard command, since leftover state from a failed run is the likeliest way to trip it and neither committing nor stashing a half-finished version bump is what the operator wants. Scope the clean pathspecs to 'packages/*/CHANGELOG.md' and 'rust/*/CHANGELOG.md', matching the workspace globs in package.json, so an untracked CHANGELOG.md the operator happens to hold elsewhere in the tree is no longer removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kRxi2jizmJQKt9cuv4vmg * fix(release): cover an interrupted release run in the cleanup trap Review nit. The window the trap protects is two long-running commands, so an operator interrupting the run is a likely way out of it. bash does run an EXIT trap for an untrapped SIGINT (verified on 5.2.21, both signalling the process directly and the process group), but naming INT and TERM costs nothing and states the guarantee instead of resting it on that behaviour. The handler disarms itself before doing anything, so its own `exit` cannot run the cleanup a second time now that a signal reaches it directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kRxi2jizmJQKt9cuv4vmg * fix(release): make an interrupted release run exit non-zero Naming INT and TERM in the trap left the handler exiting with `$?`, which on a signal path is the last *completed* command's status - 0 when the signal lands between commands. An interrupted run therefore reported success: verified exit code 0 after a SIGINT mid-bump, which would let `./lerna-publish.sh && ...` carry on as though the release had happened. Re-raise the signal instead, once the handler has disarmed itself and cleaned up, so the caller sees the run was interrupted. The `exit $status` path is kept for the EXIT trap, where `$?` is the real failure code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kRxi2jizmJQKt9cuv4vmg * fix(release): confirm the release step by default, --yes to skip The release step commits, tags, pushes and creates the GitHub release, so passing --yes unconditionally took away the operator's last look before all of that goes out. Make the prompt the default again and let the caller opt out. ./lerna-publish.sh # asks before releasing ./lerna-publish.sh --yes # unattended ./lerna-publish.sh minor --yes # bump still positional Step 1's bump keeps --yes unconditionally: the cleanup trap throws that bump away, so there is nothing there for an operator to confirm. lerna's prompt cannot be answered where stdin is not a terminal - it fails with a bare exit 1 that says nothing about why, which is how this surfaced in the first place. Check for that up front, before the bump rather than after it, and say what to pass instead. Also quote "$BUMP" at both call sites, now that it comes out of an argument loop rather than straight from $1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kRxi2jizmJQKt9cuv4vmg * fix(release): restore the happy-path cleanup the trap refactor dropped The two `git restore .` sites were not duplicates of each other. The one before the release step is a required step of the happy path: step 1's bump has to come back out before the real `lerna version` runs, because lerna reads the versions in the tree to compute the next ones and refuses to commit over a dirty tree at all. Folding both sites into the exit handler and then disarming it before the release step meant that on a successful run the bump was never undone, so the release ran on a still-bumped tree - it would abort on checkWorkingTree, or bump a second time on top of step 1 if it got past that. Split the restore into restore_bump, call it explicitly where master did, and have the trap call the same function for the runs that never reach it. Restore first, then disarm, so an interrupt during the restore is still covered. The step numbering goes back to master's 1-5 now that the cleanup step is a step again. Reproduced with a stub that reports `git status --porcelain` at the start of the release step: before this it read "M CONTRIBUTING.md M README.md", now empty. The earlier stub matrix could not see it - a stub yarn neither validates the working tree nor recomputes versions, so the "clean tree, full run" row recorded the missing cleanup as expected behaviour. That row now asserts the tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kRxi2jizmJQKt9cuv4vmg * fix(release): let --help work without a .gh-token `. .gh-token` ran before the argument loop, so under `set -e` the usage output I added in the previous commit died on any checkout without the token file - which is every fresh clone, since .gh-token is gitignored: $ ./lerna-publish.sh --help ./lerna-publish.sh: line 4: .gh-token: No such file or directory Source it after the argument and terminal checks instead. Nothing before that point needs GH_TOKEN; only the release step's --create-release=github does, and a run that gets that far still fails the same way if the file is missing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kRxi2jizmJQKt9cuv4vmg --------- Co-authored-by: Claude <noreply@anthropic.com>
…eation (#11657) * docs(ai): describe agent-config reconciliation instead of UI-first creation The multi-agent page claimed spaces and agents must first be created through the UI before they can be configured via YAML. It is the other way around: agents/config.yml is the source of truth, agents cannot be created in the UI and their settings are read-only there, and Cube creates the records from the config when a deployment is reconciled. Replace that callout and document the reconciliation flow that actually ships — Pending Configurations on the Agents and Spaces pages, the Reconcile agent configs button in the Semantic Model IDE, Create All, and what reconciliation does not cover (property, rule and certified-query edits apply straight from the config). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N6JE5RQgcZZ3rXMcgH7Y6Q * docs(ai): qualify what reconciliation covers, and who can be created by hand Review follow-ups on the reconciliation section, each checked against the Cloud code: - An agent's `space` link is made at creation. Changing it in YAML does not move an existing agent — the agent page flags the mismatch, and the relink happens on the next reconcile. The blanket "every other property takes effect without reconciling" claimed otherwise. - `description` is read from the config at request time, so it stays in the list of properties that need no reconcile. - Spaces can be created by hand, unlike agents, but such a space carries no link to a `spaces:` entry, so the entry still reads as pending. - Name where a leftover record is deleted: the Agents or Spaces page, which both offer a delete row action. Prose is unwrapped to one line per paragraph to match the rest of the page. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N6JE5RQgcZZ3rXMcgH7Y6Q * docs(ai): say how to act on an agent whose space no longer matches its config The space-mismatch paragraph said reconciling again moves the agent without saying what to press, and the answer depends on the case: - the newly named space has no record yet — it lists under Pending Configurations, and Create All creates it and relinks the agent in one action. - the space already exists for the deployment — nothing is pending, the panel renders nothing, so there is no Create All at all. Deleting the agent makes its config pending again and the next Create All recreates it in the space the config names. Both paths verified against the reconcile action and the agent settings page, where the space row is a read-only link and the config assignment is locked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N6JE5RQgcZZ3rXMcgH7Y6Q * docs(ai): move the by-hand space caveat to the section that documents it The caveat was in the opening callout, which is where it was least likely to be read: a reader who lands on Space scope sees the "Create space" path without it. Moving it onto that bullet also relieves the densest paragraph on the page, and says what actually goes wrong — the hand-made space has no link to a spaces: entry, so the entry stays pending and reconciling creates a second space. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N6JE5RQgcZZ3rXMcgH7Y6Q * docs(ai): state the invariant for a space mismatch, and what a delete costs "There is no Create All to press" only held when nothing else was pending — the panel renders whenever any entry is, so a reader who added a second agent in the same edit sees the button and would go looking for a UI bug. The invariant is that Create All won't move the mismatched agent; the button's absence is the special case. The delete-and-recreate remedy also needed its cost stated. Traced rather than assumed: chat threads live in ai-engineer's own database with a plain agentId column, no foreign key or cascade onto the console-server agents table, and deleting an agent is a bare row destroy — so nothing is dropped from storage, but the recreated agent is a new record and earlier chats don't carry over to it. The page says that, and not that deleting discards them. Also fold the config file into Step 1's body, and reword the rename warning so the old record "stays behind with everything tied to it" rather than "keeps what it stored", which described storage it doesn't own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N6JE5RQgcZZ3rXMcgH7Y6Q * docs(ai): matching is on the config link, not on any record's name Moving the by-hand-space caveat into Space scope exposed a contradiction the page had been carrying: it promised entries are matched "to what already exists" by name, while the caveat says a hand-made space with the identical name does not satisfy the entry. The caveat is what the code does. Every record reconciliation creates stores the config entry it came from, and matching compares that stored value — a space created by hand has none, so the entry stays pending and reconciling creates a second space. Both places that made the broader claim now scope it to the records Cube created from the config. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N6JE5RQgcZZ3rXMcgH7Y6Q * docs(ai): finish the sweep — a per-deployment space is linked to its entry The previous commit rewrote the two places that described matching as name-against-existing-records and missed a third, pre-existing one: the per-deployment naming paragraph said the space stays linked to the name declared in agents/config.yml. Its conclusion was right but the mechanism was the one the Create-space bullet three lines above depends on being different — a hand-made space with an identical name doesn't match because it has no entry link. Swept the rest of the page: the remaining "linked"/"matching" sentences describe which space an agent resolves to, or how scope is inferred from agents, and are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N6JE5RQgcZZ3rXMcgH7Y6Q * docs(ai): a global space counts as created for every deployment Step 2 said reconciliation compares the config with the records it created "for that deployment", which is wrong for a global space: it is account-level, so an entry reads as reconciled on every deployment even though the space was created while reconciling one of them. That is what the pending check actually asks — whether a record is available to the deployment, not whether it was created for it — and the Space scope warning 30 lines below is too late for a reader forming the model from Step 2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N6JE5RQgcZZ3rXMcgH7Y6Q * docs(ai): a per-deployment entry is pending on every deployment separately "You need reconciliation only when a new name appears in the config" was a config-global rule summarizing a per-deployment check, and the page already documented the case where the two diverge: the same spaces: entry declared in several deployments, scoped per deployment. Reconciling on production creates a record available only to production, so staging still lists that entry as pending with no new name anywhere in the config — which reads as a stale list to anyone taking the rule literally. Also carry Step 2's vocabulary into the mismatch paragraph: a space is "available to" a deployment rather than "exists for" it, which is the accurate phrasing for a global space. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N6JE5RQgcZZ3rXMcgH7Y6Q * docs(ai): scope belongs to the record, and agents recur per deployment Two errors in the rule sentence, both of the kind this PR exists to remove. "A per-deployment entry" attributes scope to the config entry, but nothing in spaces: declares it — scope is chosen in the Create All panel or on the space page, so a reader would go looking in the spaces: reference for a key that doesn't exist. The examples also omitted agents, which are the commonest case: an agent record carries a deploymentId, so on a deployment reconciled for the first time every agents: entry is pending with no new name and no space scoping involved. Restated as what recurs per deployment — an agent, and a space created per deployment — against the global space that counts as created everywhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N6JE5RQgcZZ3rXMcgH7Y6Q * docs(ai): spaces are not hand-creatable either, so stop documenting that path The page presented creating a space by hand as one of two places you pick the scope, with a caveat about the entry staying pending. Manual creation is not a supported path for either kind of record: both come from the config, and the UI creates them when you reconcile. Scope selection now has one documented home — the Create All panel while reconciling, plus re-scoping an existing space on its page. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N6JE5RQgcZZ3rXMcgH7Y6Q * docs(ai): state the supported path without over-claiming what the UI blocks Two problems with the sentence that replaced the by-hand path. "Their settings are read-only there" was asserted of spaces as well as agents, and the Space scope section three sections down tells you to change a space's scope on its page — which the split-a-shared-space instruction depends on. Read-only was only ever verified for agents. "Neither spaces nor agents can be created in the UI" is also stronger than what a reader can see: the Create space control exists, it just produces a space with no link to a spaces: entry. Asserting it away invites the same "the docs are wrong" reaction as the callout this PR removed. Both replaced by the claim that actually holds: declaring in the config and reconciling is the only supported way to create either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N6JE5RQgcZZ3rXMcgH7Y6Q --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…er-limit messages readably (#11490) * fix(cubestore-driver): resend queries when the write to Cube Store fails 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 * fix(cubestore-driver): report a readable error when a message is over 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 * fix(cubestore-driver): address review on message size handling - 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 * fix(cubestore-driver): register the whole re-sent batch before writing 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 * refactor(cubestore-driver): rely on the existing resend loop The fix had grown a second retry path next to the one the 'close' handler already implements: a per-message resend budget, a per-message counter of how many connections died under it, an `openSocket()` that re-established a connection before registering, and a two-pass re-send that registered a batch before writing it. None of that is needed to fix the reported failures. A failed write now just terminates the socket, which is exactly the event the existing re-send loop already handles, and the loop itself goes back to what it was. Bounding the re-sends is left to the connection-level retry, as before; the only case that could genuinely spin -- an over-limit message that no re-send can fix -- is failed rather than re-sent. Attributing an over-limit response is now a single condition: `ws` drops the frame before its message id is read, so the error is reported when the message was alone in flight, and everything else takes another round, which answers the innocent queries and leaves the offender alone to be named next time. Net effect on the change: -182 lines in WebSocketConnection.ts, one test dropped with the machinery it covered. The remaining 10 tests pass and still fail against the unfixed source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej * fix(cubestore-driver): bound the re-sends of an unattributable failure Review of the simplification caught that `pending.length === 1` as the only attribution rule doesn't terminate. An over-limit response that arrives while a second query is in flight is never attributed: both messages are re-sent, Cube Store hands the offender back its cached result ahead of the other answer, and the pending set is unchanged when the connection dies again. Nothing bounds that -- `currentConnectionTry` isn't touched on the over-limit path and every pong resets it -- so it's a 1s loop of re-execution and teardown with neither promise settling. The new test reproduces it: an oversized response that always beats the small query's answer hung to the 60s jest timeout, and now settles in about a second. One counter per message restores the bound: an unattributable failure gives every message in flight one more round, which answers the innocent ones and usually leaves the offender alone to be named next time, and whatever is still in flight after that round is failed regardless. Also from review, both reachable through the fatal branch added here: - The re-send loop registered messages on the new socket interleaved with awaited writes, so a socket dying mid-batch could strand the rest of it with nobody left to write or re-send them. The batch is now registered in one pass before any of it is written. - A message registered on a socket whose 'close' had already fired now re-enters `sendMessage`, which establishes a fresh connection, rather than failing a query that was never written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej * fix(cubestore-driver): count only consecutive unattributable failures `fatalRounds` was cumulative over a message's whole lifetime, so the extra round it grants could be spent long before it was needed: a query that was merely in flight during one over-limit incident, then survived ordinary disconnects for minutes, was rejected with MessageTooLargeError in the first round of a later incident it had nothing to do with -- the misattribution the counter exists to prevent. Resetting it on a close that isn't fatal keeps the bound, since the loop it bounds is fatal every round, and makes the counter mean what its comment says. The new test walks a size incident, an ordinary disconnect and a second size incident with the same slow query in flight throughout; without the reset that query fails with the size error instead of being answered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej * fix(cubestore): answer an over-limit message with close code 1009 An incoming message over CUBESTORE_TRANSPORT_MAX_MESSAGE_SIZE or CUBESTORE_TRANSPORT_MAX_FRAME_SIZE surfaced as a capacity error from the WebSocket stream, which the handler's catch-all logged before breaking out of the loop. That dropped the connection without a close frame, so the client saw a bare disconnect it could only read as Cube Store going away, and retried the query that caused it. The error is raised as soon as the frame header is parsed and before the payload is read, so the frame stream is left desynchronized and the message id is never seen: the connection can't be reused, and an application level error can't be attributed to the query that caused it. The capacity error is now recognized and answered with 1009 ("Message Too Big"), the close code the WebSocket protocol reserves for exactly this, carrying the size and the limit as the reason, so the client can report the size instead of retrying a disconnect. Every other break in the loop is unchanged. The test drives the server with a 4 KB limit over a real socket and asserts the close frame; against the previous code it fails with the reported `Connection reset by peer` instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej * test(cubestore): bind the message size test to its own port master's new ws_process_id_header_test binds 53032, which this test was using too. Tests in the crate share a binary and run in parallel, so the two would race for the address once the branches meet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej * fix(cubestore): answer an over-limit request with an error, not a close The size limit was enforced by the transport, which refuses the frame as soon as its header is parsed and before the payload is read. That leaves the frame stream desynchronized and the message id unread, so the only possible answer was a close frame — taking down every other query multiplexed over that connection along with the oversized one. Enforce the limit a layer up instead, once the message has arrived and its id is known: the client gets an error naming the request that was too big, and the connection stays up for everything else in flight on it. The transport keeps the limit as a backstop, at a factor above the configured value, so a peer still can't make the server buffer without bound; past that backstop the close frame remains the answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej * docs(cubestore): explain why the frame limit gets the size headroom too Review asked why TRANSPORT_SIZE_HEADROOM is applied to the frame limit when only the message size is re-checked in the handler. It has to be: a query arrives as a single frame, so an exact frame limit refuses an over-limit message at the transport before the handler sees it, and at the default configuration — both knobs 64 << 20 — that is every over-limit message, which would leave the readable error unreachable. Frame size cannot be re-checked up here, since reassembly happens below warp. Say so, along with what it means when the two knobs are configured apart: the frame knob bounds a single allocation at the headroom multiple of its value, and the message limit enforces the policy. Also add the blank line between the two size tests and the full stop the client-side error text already has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej * fix(cubestore-driver): give every message its extra round after a fatal close A message that was alone in flight was failed on the spot, on the reasoning that an unattributable oversized response must belong to the only query there was. But a fatal close on a sole in-flight query is not proof the query is at fault -- the same close is what an ordinary disconnect looks like -- and re-sending is what recovers that case. Drop the special case, so every message in flight gets the one extra round the counter already grants: the innocent ones are answered, and a genuine offender spends its round and is reported with its size on the next fatal close. The bound is unchanged, since it was never the one doing the bounding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej * docs: an over-limit Cube Store message is answered, not dropped #11655 documented the transport size limits while this change was in flight, so it describes the behaviour this branch removes: a message above the transport limit closing the connection, surfacing as `write EPIPE`, recorded as a known limitation. It is answered with an error naming the size now, and the connection survives for the other queries multiplexed over it. Also document CUBEJS_CUBESTORE_MAX_MESSAGE_SIZE, which this change adds, next to the Cube Store side limits it is independent of. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej * fix(cubestore): report the configured size limit, not the backstop The close reason and the log line formatted tungstenite's `max_size`, which is what the transport was configured with -- the configured limit times TRANSPORT_SIZE_HEADROOM. So they named a number no operator set, double the real one, while a message between the two is refused by the handler check against the un-doubled value. Divide the headroom back out rather than formatting the message limit directly, so that a CUBESTORE_TRANSPORT_MAX_FRAME_SIZE configured below the message limit reports itself when it is the one that fired. Both size tests now assert the configured number instead of just the prefix, which is the assertion that would have caught this: against the previous code the close reason reads 8192 where the limit is 4096. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej * docs: correct why a sole in-flight message gets a round, and the backstop Two things review caught, both about accuracy of the explanation rather than behaviour. The comment justified the extra round for a sole in-flight message with "the close can just as well be an ordinary disconnect". That is not true inside this branch: `fatalError` is only set when `ws` saw an oversized frame or the peer closed with 1009, and an ordinary disconnect leaves it null and takes the reset path instead. The real ambiguity is that the frame says an oversized message arrived, not which query produced it -- it can belong to a query already rejected on an earlier round whose response was still on the wire. The architecture page promised the connection stays up without saying where that stops. Past twice the configured limit the message is refused before it can be attributed and the connection is closed after all, which is the case an operator reading the page would be looking up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej * fix(cubestore-driver): surface the sizes Cube Store sends with a 1009 close The 'close' handler bound only the code and dropped the reason, so the concrete numbers Cube Store puts there never reached the user: they were told to raise CUBESTORE_TRANSPORT_MAX_MESSAGE_SIZE without being told what it is or how far over they went. On the server side that only exists in the Cube Store log, which is where an API user cannot look. Append the reason when there is one. A peer that closes with 1009 and no reason -- an intermediary, or an older Cube Store -- keeps the generic wording, which is now covered by its own test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej * fix(cubestore): name the limit that refused the message, not always the message one tungstenite raises the same capacity error for a frame over max_frame_size as for a message over max_message_size, and the variant doesn't say which. The reason hardcoded "maximum message size", so with CUBESTORE_TRANSPORT_MAX_FRAME_SIZE below the message limit a request that tripped the frame backstop was reported against a message limit that would have accepted it -- sending an operator to raise a knob that changes nothing. Compare the configured value back against both and name the one that matches. Equal limits, the default, keep naming the message one, where the number is the same either way. The neutral wording of naming both knobs doesn't fit: a close reason is capped at 123 bytes by RFC 6455 and the two variable names alone are most of that. New test configures the frame limit below the message limit and asserts the frame limit is what the close reports; the two existing size tests set them equal, so neither covered this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej * fix(cubestore-driver): let the close reason replace the generic wording The reason was appended after the sentence it supersedes, so the error stated the same thing three times, and the ". " join assumed the peer's string carries no trailing full stop -- true of Cube Store's reason today, but the sibling handler string grew one two commits ago. Use the reason in place of the generic clause when there is one, folding out control characters since peer text lands in an error a user reads, and normalising the trailing full stop rather than assuming its absence. Also name both transport knobs in the advice. Now that Cube Store reports whichever limit refused the message, the reason can say "frame size" while the advice pointed only at CUBESTORE_TRANSPORT_MAX_MESSAGE_SIZE -- the knob that would not have helped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej * test(cubestore-driver): cover a 1009 close that names the frame limit Both 1009 tests used a reason naming the message limit, so neither pinned the case the advice was widened for: a close reason quoting CUBESTORE_TRANSPORT_MAX_FRAME_SIZE must not be followed by advice pointing only at the message knob. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPf8PuEMdZ6RE7BusNGWej --------- Co-authored-by: Claude <noreply@anthropic.com>
github-actions
Bot
added
pr:community
rust
data source driver
cubejs-jdbc-driver
driver:athena
driver:bigquery
driver:clickhouse
driver:crate
driver:databricks
driver:dremio
driver:druid
driver:duckdb
driver:firebolt
driver:hive
driver:materialize
driver:mongodb
driver:mssql
driver:mysql
driver:mysql-aurora-serverless
driver:oracle
driver:pinot
driver:postgres
labels
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )