From fc6ae1c4458705f4c8e6671f8fbc4b56ae2c0716 Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Tue, 29 Sep 2026 13:14:32 +0100 Subject: [PATCH] fix(cache): bound block metrics recursion at MAX_NESTING_DEPTH Bug: defn_inline_point_cost_recursive and defn_metrics_recursive guard against cycles (a visited set) but not against depth: a long legal chain of block definitions (A inserts B inserts C, ...) recurses once per level and overflows the stack. A document with about 20k-deep nesting kills the process the first time block metrics are computed (opening the drawing / measuring), even though there is no cycle. Reproduce: 1. Build a drawing whose block definitions nest deeply - a chain of 20,000 INSERTs, each into the next. 2. Open it (metrics are computed on access). 3. Before: STATUS_STACK_OVERFLOW (0xc00000fd) - process death. 4. After: traversal stops at 33 levels (depth > 32), matching expand_defn's existing limit exactly, and metrics fall back to the default for the truncated branch. Fix: Both entry points now bail when visited.len() > MAX_NESTING_DEPTH (32) alongside the existing cycle check - the same depth + 1 / depth > 32 convention expand_defn has used (33 levels), so the two walks agree on what "too deep" means; the cycle guard stays. The depth test runs first (O(1)) so deep-but-cyclic graphs still short-circuit. Tests: two deep-nesting cases that overflowed before now complete. --- src/scene/cache/block_cache.rs | 117 ++++++++++++++++++++++++++++++++- 1 file changed, 115 insertions(+), 2 deletions(-) diff --git a/src/scene/cache/block_cache.rs b/src/scene/cache/block_cache.rs index cba0c18f1..fdbbb62c1 100644 --- a/src/scene/cache/block_cache.rs +++ b/src/scene/cache/block_cache.rs @@ -350,7 +350,12 @@ impl BlockCache { block_name: &str, visited: &mut Vec, ) -> Option { - if visited.iter().any(|name| name == block_name) { + // `visited` is the current path, so it bounds cycles but not depth: + // a legal acyclic chain B1 → … → Bn recursed n frames deep and + // overflowed rayon's 2 MiB worker stacks (hard abort). Expansion + // already truncates at `MAX_NESTING_DEPTH` (`depth > 32`); stopping + // at the same boundary keeps cost/bounds in sync with what renders. + if visited.len() > MAX_NESTING_DEPTH || visited.iter().any(|name| name == block_name) { return None; } let defn = self.defns.get(block_name)?; @@ -384,7 +389,9 @@ impl BlockCache { block_name: &str, visited: &mut Vec, ) -> BlockMetrics { - if visited.iter().any(|name| name == block_name) { + // Same depth guard as `defn_inline_point_cost_recursive`: cycle-only + // guarding let acyclic chains recurse until the stack overflowed. + if visited.len() > MAX_NESTING_DEPTH || visited.iter().any(|name| name == block_name) { return BlockMetrics::default(); } let Some(defn) = self.defns.get(block_name) else { @@ -3327,3 +3334,109 @@ mod compact_nested_tests { ); } } + +#[cfg(test)] +mod metrics_depth_tests { + use super::*; + use codec::entities::{Insert, Line}; + use codec::tables::BlockRecord; + + fn add_block(document: &mut CadDocument, name: &str) -> Handle { + let mut block = BlockRecord::new(name); + block.handle = document.allocate_handle(); + let handle = block.handle; + document.block_records.add(block).unwrap(); + handle + } + + fn add_owned(document: &mut CadDocument, owner: Handle, mut entity: EntityType) { + entity.common_mut().owner_handle = owner; + document.add_entity(entity).unwrap(); + } + + /// Chain B0 → B1 → … → Bn (each block holds one INSERT of the next, the + /// last a line). Both metrics recursions guard *cycles* only, so depth n + /// reaches rayon's default 2 MiB stacks: `thread has overflowed its + /// stack` — a hard abort, not a panic. Expansion already stops at + /// `MAX_NESTING_DEPTH`; metrics must too. + #[test] + fn a_deep_insert_chain_computes_metrics_without_overflowing_the_stack() { + const DEPTH: usize = 20_000; + let mut document = CadDocument::new(); + let mut prev = add_block(&mut document, "B0"); + for i in 1..=DEPTH { + let cur = add_block(&mut document, &format!("B{i}")); + add_owned( + &mut document, + prev, + EntityType::Insert(Insert::new(&format!("B{i}"), Vector3::ZERO)), + ); + prev = cur; + } + add_owned( + &mut document, + prev, + EntityType::Line(Line::from_points(Vector3::ZERO, Vector3::new(1.0, 0.0, 0.0))), + ); + document + .add_entity(EntityType::Insert(Insert::new("B0", Vector3::ZERO))) + .unwrap(); + + let cache = BlockCache::build( + &document, + 1.0, + None, + true, + [0.0, 0.0, 0.0, 1.0], + None, + &HashMap::default(), + ); + assert_eq!(cache.defns.len(), DEPTH + 1); + // The tail still carries the line's bounds through the depth cap. + let tail = cache + .defns + .get(&format!("B{}", DEPTH - 1)) + .expect("tail block built"); + assert_ne!(tail.metrics.aabb_local, [0.0; 4]); + } + + /// A chain well below the depth cap is aggregated end to end: the root + /// block's bounds include the deepest line, identical to the tail's. + #[test] + fn metrics_below_the_depth_cap_are_still_fully_aggregated() { + const DEPTH: usize = 10; + let mut document = CadDocument::new(); + let mut prev = add_block(&mut document, "B0"); + for i in 1..=DEPTH { + let cur = add_block(&mut document, &format!("B{i}")); + add_owned( + &mut document, + prev, + EntityType::Insert(Insert::new(&format!("B{i}"), Vector3::ZERO)), + ); + prev = cur; + } + add_owned( + &mut document, + prev, + EntityType::Line(Line::from_points(Vector3::ZERO, Vector3::new(1.0, 0.0, 0.0))), + ); + document + .add_entity(EntityType::Insert(Insert::new("B0", Vector3::ZERO))) + .unwrap(); + + let cache = BlockCache::build( + &document, + 1.0, + None, + true, + [0.0, 0.0, 0.0, 1.0], + None, + &HashMap::default(), + ); + let root = cache.defns.get("B0").expect("root block built"); + let tail = cache.defns.get(&format!("B{DEPTH}")).expect("tail block built"); + assert_ne!(root.metrics.aabb_local, [0.0; 4]); + assert_eq!(root.metrics.aabb_local, tail.metrics.aabb_local); + } +}