fix: report query depth instead of failing or crashing on it by waralexrom · Pull Request #11815 · cube-js/cube

waralexrom · 2026-09-09T11:07:17Z

Problem

A deeply encoded query fails in three different ways, and none of them says that depth is the reason.

Grouped query over a rollup — Error decoding expr as protobuf: … recursion limit reached. The same query ungrouped succeeds.
25 chained multi-stage stages — the request dies with Bad Gateway, grouped and ungrouped alike. No error surfaces anywhere.
A measure with a deeply nested expression — Error: Internal: RecursionLimitExceeded.

Nothing at compile or plan time says a depth budget exists, and none of the three messages names depth. Flattening the encoding cures all three.

Cause

All three reproduce, and they are two different animals.

(1) and (3) are counters, not stack exhaustion.

The SQL parser ran at sqlparser's default recursion limit of 50; CubeStoreParser never called with_recursion_limit. Reproduces at nesting depth 47.
From<ParserError> for CubeError formatted the error with {:?}. ParserError::RecursionLimitExceeded carries no message of its own, so the variant name became the message, classified Internal — hence the exact Internal: RecursionLimitExceeded from the report.
Plan serialization hit prost's compile-time decode limit of 100 protobuf levels. datafusion-proto nests two message levels per logical-plan node, capping a plan at about 48 nodes. Cube Store inlines a CTE body at every reference, so one chained stage arrives as Projection + SubqueryAlias = 2 nodes — roughly 24 stages, which is where the report lands. Reproduced byte-identically at plan depth 54.
The encode→decode roundtrip happens even with zero select workers: the select path calls to_serialized_plan() and then execute_router_plan decodes it again.
Why ungrouped passes: an ungrouped query is gated out of pre-aggregation matching, so it goes to the source warehouse and never reaches Cube Store — no protobuf roundtrip, no cap.

Both counters were far more conservative than the stack behind them. Measured on a release build, 2 MiB of thread stack decodes a plan 364 levels deep, so prost gave up with roughly 87% of the stack still unused.

(2) is stack exhaustion, with no counter involved. buildSqlAndParams is a synchronous native call, and multi-stage planning descends once per stage. A long enough chain exhausts the calling thread's stack and the process aborts — fatal runtime error: stack overflow is not a panic and not catchable, on either side of the native boundary, so a dead worker and a Bad Gateway is all the caller sees.

What changed

Cube Store — the counters become budgets that report themselves.

The parser takes 128 nesting levels, under CUBESTORE_SQL_PARSER_RECURSION_LIMIT.
prost's cap is lifted (no-recursion-limit) and replaced by an explicit check on plan depth before the roundtrip, under CUBESTORE_MAX_QUERY_PLAN_DEPTH (150). Past it the query is refused with the depth reached, the budget and the knob named, as a user error. Depth is one measure over everything the encoding nests — plan nodes, expression nodes and the plans of subqueries carried in expressions — added along a path, and the walk keeps its own stack rather than recursing.
Streaming sources parse under the same budget; they used to build their own parser at sqlparser's default of 50, which made the error name a limit and a knob that had no bearing on them.
RecursionLimitExceeded now names nesting depth and stays a user error.
cubestore-main was leaving its tokio threads at the 2 MiB platform default while select workers sized theirs explicitly; it now asks for 8 MiB, under CUBESTORE_MAIN_STACK_SIZE. Both budgets are stack budgets — neither the parser nor the protobuf decoder grows its stack — so this is what makes raising them real rather than nominal.

Tesseract — the crash becomes an error. Planning measures the longest chain of multi-stage members reachable from each queried member and refuses anything past CUBEJS_MAX_MULTI_STAGE_DEPTH (32), naming the depth, the member and the knob. The walk is iterative and memoized per symbol, so it survives the graphs it exists to refuse and terminates on a cyclic one. Only members that are actually planned as a stage count: a reference — a view member proxying one, say — inherits multi_stage from what it resolves to while planning collapses it, and calculation chains recurse too but cost a fraction of a stage per level.

How it was verified

Generated models with depth as a parameter, so every claim above is a measurement.

All three failure modes reproduced first, mode 1 byte-identically.
Each test was run against the unfixed code. Parser: Internal: RecursionLimitExceeded. Serialization: the ticket's decode error, and without the plan-depth check a 301-node plan aborts the process during encode, not only decode. Planner: a 40-stage chain planned happily instead of being refused, and a 32-stage chain re-exported by a view was refused at 33.
A 125-level nested expression was checked to slip past a budget that counts only plan nodes (it reads as depth 2) and to abort the process at 400 levels — the case that made expression nesting part of the same budget.
Release-build ceilings, per shape: plan decode 364 levels on 2 MiB and 604 on 4 MiB; parsing 300 levels of nested expressions on 2 MiB, but only 60–80 of nested subqueries, which cost ~33 KiB a level and are the shape that sets the parser budget; multi-stage planning a few hundred minimal stages.
test_four_entrypoints_in_one_query was ignored for exactly this protobuf limit. It is enabled here and passes against a live Cube Store, with the rollup answer checked against the same query over Postgres.
Suites: cubesqlplanner --lib --features integration-cubestore 1377 passed / 0 failed; cubestore --lib 348 passed with 2 failures that are the known environmental ones (an external CSV answering 403, and one that passes in isolation and only fails under parallel load).

Risks

CUBESTORE_MAIN_STACK_SIZE = 8 MiB applies to tokio's blocking threads too, so with max_blocking_threads at its default the reserved address space grows. Stacks commit lazily, so this is virtual, not resident — but an environment with ulimit -v set would notice.
Lifting prost's recursion limit removes a bound on decode depth. The plan-depth check is what replaces it, and it must stay ahead of the decode: it is the only thing between a deep plan and a stack overflow. The plan bytes are cluster-internal, but a caller who can send SQL can shape a deep plan, which is why the budget is checked rather than trusted.
The multi-stage budget is not a proof of safety. How much stack a stage needs depends on what it plans and how much there is depends on the caller, so 32 leaves a wide margin for a minimal stage and a narrower one for a heavy stage on a small stack. Raising the default should follow a measurement, not a guess.
Chains deep enough to exhaust the stack while the model is still being compiled — before planning starts, so before the guard runs — still abort without an error. Closing that needs a depth counter inside symbol resolution, which is a larger change and is not attempted here.
CUBEJS_MAX_MULTI_STAGE_DEPTH is read directly in Rust and is not registered in packages/cubejs-backend-shared/src/env.ts; there is no config channel into the planner for it, and the alternative is plumbing it through BaseQueryOptions from JS.
Queries between the old caps and the new budgets now execute where they used to be refused. That is the point, but it is new load: plans up to 150 levels of nesting reach execution for the first time.
Deploy workers before routers. A router on this build accepts plans roughly 3x deeper than stock prost decodes, so during a partial rollout such a plan sent to a worker that still has the old cap fails there with the opaque recursion limit reached this PR set out to remove. It is transient and self-resolving, but it is the wrong order to roll out in.

🤖 Generated with Claude Code

claude · 2026-09-09T11:07:34Z

Claude finished @waralexrom's task in 5m 25s —— View job

Review complete — no blockers; 1 medium, 5 low findings posted inline

Full review

Gather context / read diff
Review Cube Store changes (parser, serialized_plan, stack size, streaming)
Review Tesseract depth guard
Review tests
Post findings

Overall

This is unusually well-diagnosed work. Three distinct failure modes, correctly separated into "counters that were far tighter than the stack behind them" and "no counter at all", with the fix matched to each. The measurements in the description are the kind of thing that makes a depth budget defensible rather than arbitrary, and enabling test_four_entrypoints_in_one_query — which was ignored for exactly this protobuf limit — is a good proof that the change does what it says.

A few things I specifically liked:

Both depth walks keep an explicit stack and memoize, so the guard survives the graphs it exists to refuse. The measured: Vec<Rc<MemberSymbol>> in depth_guard.rs:68 keeping symbols alive so pointer keys can't be recycled is a detail that is easy to get wrong and is right here.
The cycle handling in multi_stage_depth terminates rather than hanging, which matters because a cyclic model would otherwise turn a guard into a new hang.
Errors are classified User and name the depth reached, the budget, and the knob. That is the actual deliverable and it is done consistently across all three sites.
c967e2b (a reference member is not a stage) and the view-re-export test are the right correction — without it every view over a multi-stage chain would have been charged double.

Findings

#	Severity	Where	Issue
1	Medium	`serialized_plan.rs:79-87`	The guard's own subquery recursion is unbounded and its memo isn't shared across subquery boundaries. It is now the only thing before an unbounded prost decode, but it measures before it can report.
2	Low-Med	`serialized_plan.rs:1541-1549`	The roundtrip test runs on 32 MiB, so the load-bearing claim ("150 fits the 4 MiB select-worker stack") is never exercised. `sql/parser.rs` uses the production stack size for the same reason — the two helpers reach opposite conclusions from the same argument.
3	Low-Med	`serialized_plan.rs:143-148`	The error tells operators to raise `CUBESTORE_MAX_QUERY_PLAN_DEPTH` without mentioning that it is only meaningful relative to `CUBESTORE_SELECT_WORKER_STACK_SIZE`.
4	Low	`Cargo.toml:113`	`no-recursion-limit` unifies across the whole build graph, and the decode side has no replacement guard. Acknowledged in the risk list; noted for where the residual exposure actually sits.
5	Low	`depth_guard.rs:39-56`	Memo is rebuilt per root, so a wide query is O(roots × graph). Separately: the guard is stage-only, so a long plain-calculation chain still aborts the process with no error — narrower than "refuses instead of overflowing" suggests.
6	Low	`depth_guard.rs:1-24` and peers	Doc comments run well past the repo's ~3-line guidance and read as PR body pasted into source. The reasoning is real; the measured ceilings are what will rot silently.

Checked and fine

kafka.rs:439/476 still build a default-limit Parser, but both are inside #[cfg(test)], so the "streaming sources parse under the same budget" claim holds for production paths.
all_used_symbols() covers filter symbols, so a deep chain reachable only through a filter is guarded.
multi_stage_depth's diamond and repeated-push cases resolve correctly under the LIFO ordering; the on_path set is only consulted for genuine ancestors.
logical_plan_depth's pointer keys are safe — inputs() borrows from Arcs that outlive the walk.
env_parse_lenient is pub in config, so the cubestored.rs import is fine.

None of these findings should block. #1 is the one I'd want addressed before merge, since it is the guard undermining its own guarantee on the exact input class it was written for.

· branch `cubestore-multi-stage-depth-limits`

claude · 2026-09-09T11:11:38Z

+            // A subquery is a whole plan hanging off the expression that carries it. This is
+            // the one recursive step here, taken once per level of subquery nesting.
+            match expr {
+                Expr::ScalarSubquery(subquery)
+                | Expr::Exists(Exists { subquery, .. })
+                | Expr::InSubquery(InSubquery { subquery, .. }) => {
+                    deepest_subquery = deepest_subquery
+                        .max(depth + logical_plan_depth(subquery.subquery.as_ref()));
+                }


The guard's own walk recurses without a bound, on the one input shape it exists to refuse.

logical_plan_depth is carefully iterative for plan inputs and expression trees, but subqueries reach it through this call, so nesting depth d of ScalarSubquery/Exists/InSubquery costs d native frames of logical_plan_depth + expression_depths + the apply_expressions closure — and each frame allocates a HashMap and a Vec. Since no-recursion-limit removed prost's bound, this function is now the only thing standing between a deep plan and a stack overflow, and it measures before it can report. A plan nested past what the walk survives aborts the process inside the guard — exactly the failure mode the PR removes elsewhere.

Two things would close it:

Carry the depth into the recursive call and bail out as soon as it exceeds max_query_plan_depth(), rather than computing the full depth and comparing afterwards. That makes the walk's own recursion bounded by the budget by construction.

Thread one depth_below memo through the subquery recursion instead of building a fresh one per call. Today the same Arc'd subquery plan reached from k expressions is re-walked k times, and nested sharing multiplies (2^d in the pathological case).

In practice the parser limit (128) bounds subquery nesting from the SQL path, so this is a defense-in-depth gap rather than a live crash — but the whole point of the change is that the depth budget is the thing that must not be trusted to some other layer.

Fix this →

claude · 2026-09-09T11:11:48Z

+    /// Serializing and deserializing both recurse per plan node and cost far more stack in a
+    /// debug build than in the release build that ships, hence the explicit size.
+    fn on_a_deep_enough_stack<T: Send + 'static>(f: impl FnOnce() -> T + Send + 'static) -> T {
+        std::thread::Builder::new()
+            .stack_size(32 * 1024 * 1024)
+            .spawn(f)
+            .unwrap()
+            .join()
+            .expect("serialization must not exhaust the stack")


32 MiB here means the load-bearing claim about the budget is never tested.

The doc on DEFAULT_MAX_QUERY_PLAN_DEPTH (lines 45–48) says the budget is really a stack budget and that 150 stays well inside the 4 MiB select worker gets. That is the property the whole design rests on — but plan_at_the_depth_limit_survives_the_serialization_roundtrip runs the roundtrip on 8× that stack, so the test passing tells us nothing about whether a depth-150 plan decodes on a worker.

Contrast parse_on_a_main_sized_stack in sql/parser.rs, which deliberately uses the production 8 MiB for exactly this reason — the rationale there ("the budget is only usable on a stack that fits it") applies here too, and the two helpers reach opposite conclusions from the same argument.

If a debug build genuinely can't decode 150 levels on 4 MiB, that is worth knowing and stating; a #[cfg(not(debug_assertions))] variant pinned to 4 * 1024 * 1024 would at least make CI's release runs enforce it.

claude · 2026-09-09T11:11:55Z

+        return Err(CubeError::user(format!(
+            "Query plan is nested too deeply to execute: {} levels against a limit of {}. \
+             Reduce the nesting the query asks for -- chained stages, nested subqueries and \
+             expressions all count -- or raise CUBESTORE_MAX_QUERY_PLAN_DEPTH.",
+            depth, limit
+        )));


The message invites the operator to raise CUBESTORE_MAX_QUERY_PLAN_DEPTH, but the budget is only meaningful relative to CUBESTORE_SELECT_WORKER_STACK_SIZE (4 MiB default, and itself operator-settable). Someone who follows this advice to 600 on a node with the worker stack lowered to 1 MiB trades a reportable error for the unreportable stack overflow this PR is removing.

Worth naming the coupling — either in the message ("…and raise CUBESTORE_SELECT_WORKER_STACK_SIZE with it") or by deriving the ceiling from the configured worker stack rather than hardcoding 150 independently of it.

claude · 2026-09-09T11:12:02Z

+fn max_query_plan_depth() -> usize {
+    static MAX_DEPTH: OnceLock<usize> = OnceLock::new();
+    *MAX_DEPTH.get_or_init(|| {
+        let depth = env_parse_lenient(
+            "CUBESTORE_MAX_QUERY_PLAN_DEPTH",
+            DEFAULT_MAX_QUERY_PLAN_DEPTH,
+        );
+        if depth == 0 {
+            DEFAULT_MAX_QUERY_PLAN_DEPTH
+        } else {
+            depth
+        }
+    })
+}


0 silently becomes the default. That is a defensible choice, but 0 is the value an operator most plausibly sets when they mean "no limit", and they get the tightest setting instead, with no warning — unlike env_parse_lenient, which at least logs when it discards a malformed value.

The same three-line pattern appears verbatim in sql/parser.rs::sql_parser_recursion_limit. A shared env_parse_positive(name, default) that logs the rejection would cover both and keep the two knobs behaving alike.

claude · 2026-09-09T11:12:12Z

+pub fn check_multi_stage_depth(roots: &[Rc<MemberSymbol>]) -> Result<(), CubeError> {
+    let limit = max_multi_stage_depth();
+    for root in roots {
+        let depth = multi_stage_depth(root);
+        if depth > limit {
+            return Err(CubeError::user(format!(
+                "Member '{}' chains {} multi-stage members deep, against a limit of {}. Each \
+                 one is planned as a separate stage, and this many cannot be planned. Collapse \
+                 the intermediate stages into fewer members, or raise \
+                 CUBEJS_MAX_MULTI_STAGE_DEPTH.",
+                root.full_name(),
+                depth,
+                limit
+            )));
+        }
+    }
+    Ok(())
+}


Two notes on this loop.

The memo is per-root. multi_stage_depth builds a fresh HashMap for every entry in roots, and all_used_symbols() returns every measure, dimension and filter symbol in the request. For a wide query over a shared member graph that is O(roots × graph) — the memoization inside the walk deliberately avoids re-expanding a shared symbol, and then the outer loop re-expands the whole graph anyway. Hoisting depth_below / measured out of multi_stage_depth and threading them across roots keeps the walk linear in the graph.

The guard is stage-only, and the crash it prevents isn't. a_plain_member_chain_is_not_a_multi_stage_chain pins that a 40-member calculation chain plans fine, and the module doc explains why (a calculation level costs a fraction of a stage). That's correct as a budget decision, but it means the process-abort failure mode is only narrowed, not closed: a long enough plain chain still exhausts the stack with no error, and unlike the multi-stage case nothing counts it. The PR's risk list mentions compile-time overflow but not this. Worth stating explicitly, since "refuses the chain instead of overflowing" reads as a stronger guarantee than what ships.

claude · 2026-09-09T11:12:23Z

+//! Depth guard for chained multi-stage members.
+//!
+//! Every multi-stage member on a dependency path becomes its own CTE, and `MultiStageQueryPlanner`
+//! plans them by descending from a member into its children. Planning runs synchronously on the
+//! caller's thread, so a long enough chain exhausts that thread's stack and the process aborts —
+//! a crash no error handler sees, on either side of the native boundary.
+//!
+//! The guard turns that into a reportable error. It measures only multi-stage members that are
+//! planned as a stage: calculation chains recurse as well, but cost a fraction of a stage per
+//! level, and a reference -- a view member proxying one, say -- inherits `multi_stage` from what
+//! it resolves to while planning collapses it into that member and opens no stage of its own.
+
+use crate::planner::symbols::MemberSymbol;
+use cubenativeutils::CubeError;
+use std::collections::{HashMap, HashSet};
+use std::rc::Rc;
+use std::sync::OnceLock;
+
+/// Multi-stage members a single dependency path may carry.
+///
+/// Far above any hand-written model — real ones chain a handful of stages — and below the depth
+/// at which planning runs out of stack, which a release build reaches around a few hundred
+/// minimal stages, sooner for heavy ones and sooner still on a smaller caller stack.
+const DEFAULT_MAX_MULTI_STAGE_DEPTH: usize = 32;


Per the repo's review guidance an explanatory comment runs to about three lines; this file opens with an 11-line module doc plus a 6-line doc on the constant, and serialized_plan.rs (lines 41–51, 101–106) and sql/parser.rs (the 9-line doc on DEFAULT_SQL_PARSER_RECURSION_LIMIT) are similar.

The content is genuinely load-bearing here — "the budget is a stack budget", "only members planned as a stage count", the measured per-level cost — so this isn't narration to delete. But most of these read as PR description pasted into the source, and the parts that will rot first (measured ceilings, "a release build reaches around a few hundred") will rot silently. Suggest keeping the one sentence per block that a later editor would break something by not knowing, and letting the PR body carry the rest.

For this file that's roughly: stages are planned recursively so a long chain overflows the stack; only members that open a stage are counted, because references collapse and calculations cost a fraction of a level.

claude · 2026-09-09T11:12:37Z

 arc-swap = "1.7.1"
 object_store = "0.11.1"
-prost = "0.13.1"
+prost = { version = "0.13.1", features = ["no-recursion-limit"] }


no-recursion-limit is a Cargo feature, so it unifies across the whole build graph — every crate that decodes protobuf in this workspace loses the 100-level bound, not just datafusion-proto on the plan path. The plan bytes are cluster-internal and the new check covers the encode side, but nothing replaces the bound on the decode side, which is where the stack actually gets consumed and where a worker running a mixed-version rollout is exposed.

The risk section already calls this out and the mitigation (deploy workers first) is the right one; the part worth pinning down in the code is that check_query_plan_depth is only reachable from to_serialized_plan. A worker that receives bytes it did not encode has no guard at all. If SerializedPlan::logical_plan re-ran check_query_plan_depth on the decoded plan it would still be too late to prevent an overflow — so if you want a real bound there, it has to come from a depth-counting decoder or from keeping a (higher, finite) prost limit rather than removing it.

codecov · 2026-09-09T11:20:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.33%. Comparing base (c7dcc5c) to head (70656cf).

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #11815   +/-   ##
=======================================
  Coverage   80.33%   80.33%           
=======================================
  Files         500      500           
  Lines      106397   106397           
  Branches     3881     3881           
=======================================
  Hits        85472    85472           
  Misses      20375    20375           
  Partials      550      550

Flag	Coverage Δ
cube-backend	`60.23% <ø> (ø)`
cubesql	`84.75% <ø> (ø)`

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.

Both budgets a deep query runs into are internal recursion caps that report neither depth nor a way to raise them: - `sqlparser`'s own recursion limit of 50, never configured, surfaced through `From<ParserError>`'s `{:?}` as `Internal: RecursionLimitExceeded`; - prost's compile-time decode limit of 100 protobuf levels, which is two levels per logical-plan node and so caps a plan at about 48 nodes — roughly 24 chained multi-stage stages, since Cube Store inlines a CTE body at every reference. These tests state what the two paths owe the caller: nesting a generated query reaches has to parse, a plan past prost's cap has to survive the serialization roundtrip, and past whatever budget does apply the error has to name the depth reached, the budget, and the knob that raises it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ion caps Deep queries were refused, or aborted the node, by budgets nobody chose and nothing reported. - The SQL parser ran at `sqlparser`'s default of 50 nesting levels. It now takes 128, configurable through `CUBESTORE_SQL_PARSER_RECURSION_LIMIT`. - Plan serialization ran into prost's compile-time decode limit of 100 protobuf levels — two per plan node, so about 48 nodes. That cap is lifted (`no-recursion-limit`) and replaced by an explicit budget on plan depth, `CUBESTORE_MAX_QUERY_PLAN_DEPTH`, checked before the roundtrip. Unlike prost's, this one is reported: a plan past it names the depth reached, the budget and the knob, and it is a user error, because the query is what has to change. - `From<ParserError>` rendered `RecursionLimitExceeded` through `{:?}`, so the one message that did exist became a bare variant name classified as an internal error. It now names nesting depth and stays a user error. Both budgets are really stack budgets: every level of nesting is a recursive descent, and neither the parser nor the protobuf decoder grows its stack. The `cubestore-main` runtime was leaving its threads at the 2 MiB platform default while select workers sized theirs explicitly, so it now asks for 8 MiB, under `CUBESTORE_MAIN_STACK_SIZE`. Measured on a release build: decoding takes a plan 364 levels deep on 2 MiB and 604 on 4 MiB, and parsing takes 200 levels of the costliest shape (nested subqueries, around 33 KiB a level) on 8 MiB. Both defaults sit well inside what 8 MiB allows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Every multi-stage member on a dependency path is planned as its own stage, by descending from a member into its children. Planning is synchronous on the caller's thread, so a long enough chain exhausts that thread's stack and the process aborts — nothing on either side of the native boundary sees an error, and a served query dies without one. These tests state that a chain past the budget is refused with the depth, the member and the knob named, that a chain inside it still plans, and that a reference chain of the same length is not treated as a chain of stages. The four-entrypoint query over Cube Store was ignored for the protobuf decode limit it used to hit; with that limit gone it runs, so it is enabled here and its results are pinned. The harness checks the rollup answer against the same query over Postgres before snapshotting it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…flowing A chain of multi-stage members long enough to exhaust the planning thread's stack aborted the process. `buildSqlAndParams` is a synchronous native call, so that abort is a dead API worker and a request that ends with no error at all rather than something a caller can act on. Planning now measures the longest chain of multi-stage members reachable from each queried member and refuses anything past `CUBEJS_MAX_MULTI_STAGE_DEPTH` (32 by default), naming the depth, the member and the knob. The walk keeps its own stack and memoizes per symbol, so it survives the graphs it is there to refuse and terminates on a cyclic one. Only stages are counted. Reference and calculation chains recurse as well, but cost a fraction of a stage per level, and models built over views reach depths a stage budget would wrongly refuse. The budget is not a proof of safety: how much stack a stage needs depends on what it plans, and how much there is depends on the caller. A release build plans a few hundred minimal stages before the stack runs out, so 32 leaves a wide margin there, and a heavy stage narrows it. Chains deep enough to exhaust the stack while the model is still being compiled, before planning begins, are still not reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…dary The protobuf encoding nests two message levels for an expression node just as it does for a plan node, so a deeply nested expression is as deep an encoding as a chain of stages, on a plan of two nodes. With prost's own decode limit gone, nothing else bounds it. Also pins the boundary itself: a plan of exactly the budget has to be served, which the previous case missed by one node and so left the comparison free to be either strict or not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Lifting prost's decode limit removed the only bound on how deeply a message could nest, and the budget that replaced it counted plan nodes and the plans of subqueries but not expression nesting. A nested arithmetic expression a hundred levels deep is a two-node plan the budget read as trivially shallow, and it decoded with nothing stopping it. Depth is now one measure over all three, because the encoding nests them the same way: a plan node, an expression node and a subquery's root each cost a level, and they add along a path rather than competing for the maximum. The expression walk keeps its own stack too, so only subquery nesting recurses. Two things the budget's own documentation got wrong: deserializing happens in the select worker, whose stack is `CUBESTORE_SELECT_WORKER_STACK_SIZE` and not the 8 MiB the main runtime now asks for, so that is the stack the default is measured against; and parsing is what runs on the main runtime. The parser budget also applies to streaming sources now. They built their own parser at sqlparser's default of 50, so a Kafka source's select statement was refused at a depth the error attributed to a limit and an environment variable that had no bearing on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A view member is a proxy that inherits `multi_stage` from what it resolves to, and it sits a dependency level above it. Planning collapses the proxy before it plans anything, so it opens no stage — but a budget that counts every multi-stage symbol on the path counts it, and the same chain that plans on the cube is refused one member short through the view. Also drops the module note describing the `#[ignore]`d shape and the `--ignored` run that no longer exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A reference inherits `multi_stage` from the member it resolves to, and planning resolves the chain before it does anything, so the reference opens no stage of its own. Counting it charged a view's re-export of a chain an extra level and refused a query that plans on the cube it re-exports. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

claude · 2026-09-10T22:02:02Z

Claude finished @waralexrom's task in 55s —— View job

Re-review after force-push — no new findings; the push was a rebase, all 7 prior threads still apply

Full re-review

List existing review threads
Diff current HEAD against previously reviewed commit
Re-read changed files
Resolve stale threads / post any new findings

What changed in this push

Nothing in the diff. Every blob SHA on the branch is identical to the one I reviewed at c967e2b:

File	Blob
`multi_stage/depth_guard.rs`	`583c02f`
`queryplanner/serialized_plan.rs`	`67b87df`
`cubestore/Cargo.toml`	`7b97022`
`sql/parser.rs`	`6f674f2`
`tests/multi_stage_depth.rs`	`b4d4ace`
`cubestore/src/lib.rs`	`2cc880b`

The eight commits are the same eight, rewritten by a rebase onto a newer master. The only master commit touching rust/cubestore/src/queryplanner or rust/cube/cubesqlplanner since the base is c7dcc5c (fix(schema-compiler) — ClickHouse cast types), which does not interact with the parser, the plan-depth walk, or the multi-stage guard. So there is no new surface to review and no semantic drift to re-check.

Prior threads

Re-affirmed all 7; none resolved, since every one is still live on unchanged lines. In the order I'd act on them:

#	Severity	Where	Still applies
1	Medium	`serialized_plan.rs:87`	`expression_depths` → `logical_plan_depth` is still the one unbounded recursion, and it measures before it can report. The one I'd want closed before merge.
2	Low-Med	`serialized_plan.rs:1549`	Roundtrip test still runs on 32 MiB, so "150 fits the 4 MiB worker stack" is still untested.
3	Low-Med	`serialized_plan.rs:148`	Error still names `CUBESTORE_MAX_QUERY_PLAN_DEPTH` without its coupling to `CUBESTORE_SELECT_WORKER_STACK_SIZE`.
4	Low	`serialized_plan.rs:67`	`0` still silently becomes the tightest setting, with no log, and the pattern is still duplicated in `sql/parser.rs`.
5	Low	`depth_guard.rs:56`	Memo still per-root; guard still stage-only, so a long plain-calculation chain still aborts uncounted.
6	Low	`depth_guard.rs:24`	Doc blocks still well past the repo's ~3-line guidance.
7	Low	`Cargo.toml:113`	`no-recursion-limit` still unifies build-wide with no decode-side replacement; mitigation is still the rollout order.

Verdict unchanged: none of these block, and #1 remains the one worth addressing, since it's the guard undermining its own guarantee on exactly the input class it was written for.

· branch `cubestore-multi-stage-depth-limits`

waralexrom requested a review from a team as a code owner September 9, 2026 11:07

github-actions Bot added cube store Issues relating to Cube Store rust Pull requests that update Rust code labels Sep 9, 2026

vercel Bot deployed to Preview September 9, 2026 11:10 View deployment

claude Bot reviewed Sep 9, 2026

View reviewed changes

waralexrom and others added 8 commits September 10, 2026 23:53

waralexrom force-pushed the cubestore-multi-stage-depth-limits branch from c967e2b to 70656cf Compare September 10, 2026 22:01

vercel Bot deployed to Preview September 10, 2026 22:04 View deployment

Navigation Menu

Uh oh!

fix: report query depth instead of failing or crashing on it - #11815

fix: report query depth instead of failing or crashing on it#11815
waralexrom wants to merge 8 commits into
masterfrom
cubestore-multi-stage-depth-limits

waralexrom commented Sep 9, 2026

Uh oh!

claude Bot commented Sep 9, 2026 •

edited

Loading

Overall

Findings

Checked and fine

Uh oh!

claude Bot Sep 9, 2026

Uh oh!

claude Bot Sep 9, 2026

Uh oh!

claude Bot Sep 9, 2026

Uh oh!

claude Bot Sep 9, 2026

Uh oh!

claude Bot Sep 9, 2026

Uh oh!

claude Bot Sep 9, 2026

Uh oh!

claude Bot Sep 9, 2026

Uh oh!

codecov Bot commented Sep 9, 2026 •

edited

Loading

Uh oh!

claude Bot commented Sep 10, 2026 •

edited

Loading

What changed in this push

Prior threads

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

waralexrom commented Sep 9, 2026

Problem

Cause

What changed

How it was verified

Risks

Uh oh!

claude Bot commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete — no blockers; 1 medium, 5 low findings posted inline

Overall

Findings

Checked and fine

Uh oh!

claude Bot Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

claude Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Re-review after force-push — no new findings; the push was a rebase, all 7 prior threads still apply

What changed in this push

Prior threads

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

claude Bot commented Sep 9, 2026 •

edited

Loading

codecov Bot commented Sep 9, 2026 •

edited

Loading

claude Bot commented Sep 10, 2026 •

edited

Loading