From 221e83e213088a479a0ae7639aa406ab3216b091 Mon Sep 17 00:00:00 2001 From: Pavel Tiunov Date: Tue, 1 Sep 2026 18:35:15 -0700 Subject: [PATCH 1/2] fix(cubesql): stop reporting `Continue wait` as a SQL API error (#11717) * fix(cubesql): stop reporting `Continue wait` as a SQL API error A `Continue wait` coming back from a parallelized plan was logged as a `Cube SQL Error` load event, so Query History showed the queue's "not finished yet" signal as a failed request. #10649 suppressed that event, but only for an error whose message is exactly `Continue wait`. DataFusion's `RepartitionExec` has to hand one error to every output partition and a boxed error is not `Clone`, so `wait_for_task` flattens ours to its `Display` string and re-wraps it as `DataFusionError::Execution`: the typed `CubeError` is gone and the message reads `Execution error: Continue wait`, which an equality check does not match. `target_partitions` defaults to `num_cpus::get()` with `repartition_aggregations` / `repartition_windows` on, so any GROUP BY or window function over a `CubeScan` takes that path. - add `CubeError::is_continue_wait()`, which matches the cause first and falls back to a substring test on the message, and use it at the call sites that each spelled the check differently - exact equality in `handle_sql_query` and `load_data`, substring in `NodeBridgeTransport` - restore the `ContinueWait` cause and the canonical message in the DataFusion and Arrow conversions, so the cause survives the flattening and the prefixes cannot compound through a further wrapping layer - also try `DataFusionError` when downcasting `DataFusionError::External`, which `From` already did - classify the streaming path's errors by cause, as `load_data` does, rather than prefixing the message * fix(cubesql): address review on `Continue wait` detection Follow-up on the review of #11717. The message check is a tail match, not a substring test - the earlier comment and commit message said "substring", which is the looser semantics this deliberately avoids. - `normalize_continue_wait` canonicalizes the message whenever the error is a continue wait, not only when the cause was missing. An error can arrive with the cause already set and a prefixed message - `CubeScanMemoryStream` sets the cause without rewriting the message, and the `CubeError` downcast arm returns it unchanged - and JS compares the message exactly in places (`gateway.ts`'s `err.message === 'Continue wait'`). Fixing it centrally covers the `scan.rs` case too, so that call site stays as it is. - Match the tail of the *first line*, so a message that arrived over the JS bridge with a stack appended still resolves. `errorString` reads `err.error` and `err.message` before falling back to `err.stack`, so a continue wait does not reach that fallback today; this keeps the check robust if it ever does. - Compare as ASCII bytes instead of lowercasing both sides. The needle is pure ASCII and these messages can quote the whole failing query, so `to_lowercase` allocated a copy of it - plus one of the constant - on every call. Three more tests: a typed continue wait carrying a prefixed message, a message with a stack appended, and the awkward inputs the byte-tail compare has to survive (shorter than the needle, and a split landing inside a multi-byte character). * fix(cubesql): require `Continue wait` to start a word Follow-up nit from the review of #11717. The tail match had no word boundary, so a first line ending in a longer word - `discontinue wait` - was read as the queue's signal and would have been swallowed. The phrase now has to start a word. Every real prefix ends in a separator (`Execution error: `), and an unprefixed message has no preceding character at all, so both keep matching. * fix(cubesql): match `Continue wait` as a lowercased substring Reverts the narrowing this branch picked up over the last two commits - the tail match, the word boundary, and the ASCII byte compare - back to the check `NodeBridgeTransport` has used since 2024: message.to_lowercase().contains("continue wait") The message has to survive arbitrary wrapping on both sides: a prefix from whichever layer re-wrapped the error (`Execution error: `, `Database Execution Error: `, and these compound), and a suffix when it arrived over the JS bridge with a stack appended. Anything narrower has to enumerate those shapes, and the shape is what keeps changing - twice now a continue wait has reached query history as a failed request because a check was too specific about it. The cost, written into the doc comment rather than left implicit: a real failure quoting the phrase - an error naming a column `continue wait` - is read as the queue's signal and goes unreported. No such message is known, and the opposite mistake is the one with a track record. Two tests asserted the narrow behaviour and are updated: the quoting cases drop out of `a_real_failure_is_left_alone`, and the boundary test becomes `a_message_without_the_phrase_is_not_a_continue_wait`, keeping the empty, short and non-ASCII inputs that still must not match. * fix(cubesql): do not read a quoted `continue wait` as the queue's signal Review follow-up. The lowercased substring check feeds `normalize_continue_wait`, which runs on every DataFusion and Arrow conversion and *replaces* the message - so a false positive does not merely go unreported, it destroys the original error text and the caller sees a query that appears to poll forever instead of the error naming their mistake. These messages interpolate user-controlled SQL, so the realistic false positive is `No field named 'continue wait'` - and it is always quoted. Skip an occurrence immediately preceded by `'`, `"` or a backtick. That is one character of context rather than an enumeration of wrapper shapes: arbitrary prefixes and appended stacks still match, and the exception is per occurrence, so a message that quotes the phrase and also carries it unquoted is still a continue wait. The doc comment previously priced a false positive as "would go unreported", which was the cost at the original `NodeBridgeTransport` site, not here. Fixed. * fix(cubesql): record the Postgres cost of a misread `Continue wait` Review follow-up, doc and a constant - no behaviour change. The cost paragraph named `normalize_continue_wait` replacing the message but missed the other consumer the widened check newly reaches. `load_data` held an equality check until this branch: a real database error misread there is minted with the `ContinueWait` cause locally, so nothing re-classifies it on the way up - unlike a genuine continue wait, which the transport retries and never surfaces on the Postgres path - and `sql/postgres/error.rs` answers the client `SqlStatementNotYetComplete` (`03000`) instead of their failure. That also corrects the reachability note in the PR description, which argued the `ContinueWait` arm there was unreachable. The argument held for a genuine continue wait and does not cover an error misread into that cause. Also lifts the lowercased needle into a constant, so the substring test no longer allocates a copy of `CONTINUE_WAIT_MESSAGE` on every call, with a test keeping the two spellings in step. * fix(cubesql): match `Continue wait` by message structure, not substring Replaces the lowercased `contains` and its quote guard with a check that follows the structure these messages actually have. Every wrapping layer prepends its own label and a colon (`Execution error: `, `Database Execution Error: `, and these compound), and a message that came over the JS bridge can carry an appended stack, which puts the message on the first line and the frames after it. So the phrase always lands as a whole `:`- or newline-delimited part, however many layers wrapped it: message .split(['\n', ':']) .any(|part| part.trim().eq_ignore_ascii_case(CONTINUE_WAIT_MESSAGE)) That matches every wrapped shape without enumerating them - which is what the substring test bought - while a real failure that only mentions the phrase inside a larger part keeps its message. The quote guard covered just the shapes where the phrase is quoted, so `Execution error: continue wait is not a column` was still read as the queue's signal, and `normalize_continue_wait` replaces the message, meaning that text was gone before anything downstream saw it. The trade now runs the other way: a layer that appends rather than prepends (`Continue wait.`) would stop matching. None does today - the phrase is a wire constant that gets wrapped, not edited - and the doc comment says so. Splitting also drops both allocations the old check made per call: `to_lowercase` on a message that can quote the whole failing query, plus one for the constant. `CONTINUE_WAIT_MESSAGE_LOWER` and the test keeping it in step go with them. Tests: `the_phrase_is_matched_as_a_whole_part` pins the wrapped and stack-appended shapes on the predicate directly, and `a_real_failure_is_left_alone` gains the unquoted mid-part mention the quote guard let through. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XFsWyd58aFEWhLT2CjuiiV * fix(cubesql): name the one wrapper that appends, and pin it with a test Review follow-up on the structural match. The doc comment justified the trade with "No layer produces those today - the phrase is a wire constant that gets wrapped, not edited", and that is not true as written: the `Rewrite` arm of this file's own `Display` renders Rewrite Error: {}. Please check logs for additional information where every other arm is `