fix(wire): bound recursive Serialize, de-recurse Drop (#1056)

CHANGELOG.md

@@ -53,9 +53,48 @@ for historical reference.
   workflow runs those scripts against a cheap dev build on any PR that
   touches the release/wheel plumbing, so a future metric rename or
   serialization change reds a PR check instead of a release.
+- `wire::MAX_SPACE_SERIALIZE_DEPTH` (128) and `MAX_AST_SERIALIZE_DEPTH`
+  (512): the nesting depths past which `FuncSpace` / `Ops` and `AstNode`
+  refuse to serialize (#1056). Both are set far clear of real source:
+  across the 14 450-file corpus under `tests/repositories` (TensorFlow,
+  DeepSpeech, serde, …) the deepest AST is 188 levels and the deepest
+  space nesting is 10. The space limit is also more permissive than the
+  read side, where a document caps out near 61 levels — `serde_json`'s
+  own 128-level `Deserializer` limit charges two levels per space.
+
+### Changed
+
+- **(breaking)** `FuncSpace`, `Ops`, `AstNode`, `wire::FuncSpace`, and
+  `wire::Ops` now implement `Drop` (#1056), so fields can no longer be
+  moved out of one by value: `let m = space.metrics;` becomes
+  `let m = space.metrics.clone();` or `let m = &space.metrics;`
+  (`E0509`). The compiler-generated `Drop` glue recursed once per
+  nesting level and aborted the process on a deep tree — reachable
+  through `bca-web`'s 4 MiB body cap — and an explicit `Drop` that
+  hoists descendants into a flat work list is the only way to break
+  that chain. [`STABILITY.md`](./STABILITY.md) reserves source-level
+  shape breaks for a major bump; this one is landed under a minor as a
+  deliberate, documented exception, because the alternative was leaving
+  a reachable remote process abort open until `3.0`. The mechanical fix
+  at each call site is a `.clone()` or a borrow; 13 sites inside this
+  repository needed it.
 
 ### Fixed
 
+- `wire::FuncSpace::from` and `wire::Ops::from` no longer recurse
+  (#1056). Both projected a nested tree with
+  `spaces.iter().map(Self::from).collect()`, one stack frame per nesting
+  level at roughly 2.3 KB each, which aborted the process at ~900 levels
+  on a default 2 MiB thread. They now walk an explicit work stack and
+  convert a 1 000 000-level chain on a 512 KiB thread.
+- Serializing a `FuncSpace`, `Ops`, or `AstNode` deeper than its limit
+  now fails with an ordinary serializer error naming the type and the
+  limit, instead of overflowing the stack (#1056). `serde` cannot emit a
+  tree without one native frame per level — `serialize_field` must run
+  the child's `Serialize` to completion before returning — so the depth
+  is bounded rather than de-recursed, mirroring the 128-level recursion
+  limit `serde_json`'s `Deserializer` already applies to the same
+  documents.
 - The `cognitive` metric's nesting lookup is no longer quadratic in
   nesting depth (#1062). It recovered each node's inherited nesting via
   `node.parent()`, which is `O(depth)` — tree-sitter stores no parent
@@ -150,6 +189,21 @@ for historical reference.
 
 ### Security
 
+- Closed a remotely-triggerable process abort in the recursive
+  `Serialize` and `Drop` paths (#1056). `bca metrics -O json` on ~1 000
+  nested functions (11 KB of source) overflowed the thread stack, and a
+  stack overflow is a `SIGABRT`, not a catchable panic: `bca-web`'s
+  `spawn_blocking` wrapper turns a panic into one failed request, but an
+  abort takes the whole process down with every request in flight. Three
+  recursions were involved, all now bounded — see the **Fixed** and
+  **Changed** entries below. Reachable payloads were small: ~11 KB of
+  nested `fn`s for the serialization overflow, ~80 KB of nested
+  parentheses for the `/ast` one, both far inside the 4 MiB body cap.
+
+  Issues #700 / #709 had converted every AST *traversal* to an explicit
+  work stack; this was the same hazard in the recursive *types*, which
+  those tests did not reach.
+
 - Narrowed a remotely-triggerable CPU-exhaustion vector: a few kilobytes
   of deeply nested source could pin a core for minutes against the
   unauthenticated `bca-web` endpoints, whose parse deadline frees the

Cargo.toml

@@ -25,7 +25,7 @@ exclude = [
 ]
 
 [workspace.package]
-version = "2.0.1"
+version = "2.1.0"
 edition = "2024"
 rust-version = "1.94"
 license = "MPL-2.0"
@@ -76,11 +76,11 @@ tree-sitter-c = "=0.24.2"
 # Rationale and the rejected alternatives live on the umbrella
 # issue (dekobon/big-code-analysis#149); the operational details
 # (publish order, first-bootstrap workflow) live in `RELEASING.md`.
-tree-sitter-ccomment = { package = "bca-tree-sitter-ccomment", path = "./tree-sitter-ccomment", version = "=2.0.1" }
-tree-sitter-mozcpp = { package = "bca-tree-sitter-mozcpp", path = "./tree-sitter-mozcpp", version = "=2.0.1" }
-tree-sitter-mozjs = { package = "bca-tree-sitter-mozjs", path = "./tree-sitter-mozjs", version = "=2.0.1" }
-tree-sitter-preproc = { package = "bca-tree-sitter-preproc", path = "./tree-sitter-preproc", version = "=2.0.1" }
-tree-sitter-tcl = { package = "bca-tree-sitter-tcl", path = "./tree-sitter-tcl", version = "=2.0.1" }
+tree-sitter-ccomment = { package = "bca-tree-sitter-ccomment", path = "./tree-sitter-ccomment", version = "=2.1.0" }
+tree-sitter-mozcpp = { package = "bca-tree-sitter-mozcpp", path = "./tree-sitter-mozcpp", version = "=2.1.0" }
+tree-sitter-mozjs = { package = "bca-tree-sitter-mozjs", path = "./tree-sitter-mozjs", version = "=2.1.0" }
+tree-sitter-preproc = { package = "bca-tree-sitter-preproc", path = "./tree-sitter-preproc", version = "=2.1.0" }
+tree-sitter-tcl = { package = "bca-tree-sitter-tcl", path = "./tree-sitter-tcl", version = "=2.1.0" }
 
 # PyO3 powers the `big-code-analysis-py` cdylib. Pinned to a major
 # series; bumping is a deliberate, separate change because the Bound

STABILITY.md

@@ -145,6 +145,19 @@ section.
     note below). `SpaceKind` additionally derives `Hash` (#552) so
     it can key a `HashMap`/`HashSet`. These derive additions only
     widen the trait set and are additive.
+
+    `FuncSpace` implements `Drop` (as do `Ops`, `AstNode`, and their
+    `wire` counterparts), so its fields cannot be moved out of a value
+    by name — `let m = space.metrics;` is `E0509`; clone or borrow
+    instead. This is the one source-level shape break landed under a
+    minor bump rather than held for `3.0` (#1056): the
+    compiler-generated `Drop` glue recursed once per nesting level and
+    aborted the process on a tree deep enough to reach through
+    `bca-web`'s body cap, and an explicit `Drop` that flattens
+    descendants into a work list is the only way to break that
+    recursion. Treat the `Drop` impls themselves as an implementation
+    detail — the contract is that these trees tear down in constant
+    stack, not the presence of any particular `Drop` body.
   - `FunctionSpan` in `src/function.rs`.
 - **Offender / catalog enums**
   - `Severity` in `src/output/offenders.rs` (re-exported from the
@@ -469,6 +482,21 @@ back with *this library's* serde stack:
   absent from the document stays absent, and `wire::CodeMetrics::selected()`
   rebuilds the `MetricSet` from the present keys.
 
+**Nesting is bounded on the way out, too.** `serde` cannot emit a tree
+without one native stack frame per level, and overflowing that stack
+aborts the process instead of raising a catchable panic. Serialization
+therefore stops at `wire::MAX_SPACE_SERIALIZE_DEPTH` (128) nested
+`FuncSpace` / `Ops` levels and `MAX_AST_SERIALIZE_DEPTH` (512) nested
+`AstNode` levels, failing with an ordinary serializer error naming the
+type and the limit (#1056). The space limit mirrors the 128-level
+recursion limit `serde_json`'s `Deserializer` already applies on the
+way in — which, at two JSON levels per space, caps *reading* a document
+back near 61 levels, so the emit limit is the more generous of the two.
+For scale, the deepest space nesting across the 14 450-file corpus
+under `tests/repositories` is 10 levels, and the deepest AST is 188.
+Both limits may be raised in a minor bump; lowering one is a `3.0`
+break.
+
 The bit-exactness of float magnitudes is a property of *this library's*
 parser configuration, not of JSON text in general: a downstream consumer
 parsing the same document with a stock serde_json (no `float_roundtrip`)
@@ -1167,8 +1195,11 @@ Every release MUST update [`CHANGELOG.md`][changelog]:
   may also carry an **(breaking)** entry *only* for an MSRV bump
   (see [MSRV policy](#msrv-policy)), which is a toolchain break, not
   a source-level shape break. Treat that as the single permitted
-  exception; any other `(breaking)` entry in a minor section is a
-  bug.
+  routine exception; any other `(breaking)` entry in a minor section
+  needs the justification spelled out in the entry itself. Exactly one
+  such entry exists: the `Drop` impls on the result trees in `2.1.0`
+  (#1056), landed early because holding them for `3.0` would have left
+  a remotely-reachable process abort open.
 - **Patch bumps** list every *known* value change and any
   internal-only refactors that have user-visible effects (e.g. a
   default that flips). A patch bump never carries **(breaking)**.

big-code-analysis-book/src/commands/rest.md

@@ -136,12 +136,36 @@ Status codes:
 - `413 Payload Too Large` — the request body exceeded the server limit.
 - `500 Internal Server Error` — metric computation or AST construction
   failed for an otherwise-valid request, or a `/vcs` history walk failed
-  on the server side.
+  on the server side. This also covers a response too deeply nested to
+  serialize (`serialize_failed`) — see
+  [Nesting limits](#nesting-limits) below.
 - `503 Service Unavailable` — the parse pool is saturated by orphaned
   (timed-out) tasks; retry later.
 - `504 Gateway Timeout` — the parse (or history walk) exceeded the
   server's configured deadline.
 
+## Nesting limits {#nesting-limits}
+
+Responses are trees, and no serializer can emit a tree without one
+native stack frame per level. Rather than let a deeply nested response
+overflow the stack — which aborts the whole process, taking every
+in-flight request with it — the server refuses to serialize past a
+fixed depth and answers `500` with the `serialize_failed` token:
+
+| Response | Limit | What counts as a level |
+|---|---|---|
+| `/metrics` | 128 | one nested function space (a function inside a function inside a …) |
+| `/ast` | 512 | one AST node |
+
+Both limits sit far above anything real source reaches. Across the
+14 450-file corpus this project tests against (TensorFlow, DeepSpeech,
+serde, …) the deepest AST is 188 nodes and the deepest function-space
+nesting is 10. The
+`/metrics` limit is also more generous than the read side — a JSON
+document of nested spaces cannot be parsed back past ~61 levels,
+because `serde_json`'s own 128-level recursion limit charges two levels
+per space.
+
 ## Content negotiation {#content-negotiation}
 
 The structured analysis endpoints — `/v1/ast`, `/v1/comment` (JSON

big-code-analysis-cli/Cargo.toml

@@ -20,7 +20,7 @@ ignore = "^0.4"
 # The CLI exposes every language `bca` advertises, so it pins the
 # library's full feature set explicitly. Disabling default features
 # here would silently drop grammars from the CLI build. See #252.
-big-code-analysis = { path = "..", version = "=2.0.1", default-features = false, features = [
+big-code-analysis = { path = "..", version = "=2.1.0", default-features = false, features = [
   "all-languages",
   # End-user binary ships every backend that exists; `vcs` is the
   # umbrella turning on `vcs-git` (issue #328).

big-code-analysis-cli/src/markdown_report.rs

@@ -1483,14 +1483,10 @@ mod tests {
                 assert_eq!(out[0].name, "root.rs");
                 assert_eq!(out[DEPTH].name, format!("f_{}::f_inner", DEPTH - 1));
 
-                // FuncSpace contains `spaces: Vec<FuncSpace>`, so
-                // letting the chained tree drop at scope exit walks
-                // one frame per nested space and would overflow this
-                // tight stack — masking the production-side
-                // assertions above with a Drop-side overflow on test
-                // exit. The OS reclaims the memory at process exit;
-                // this is fine for a test.
-                std::mem::forget(current);
+                // `current` drops at scope exit rather than being
+                // leaked: `FuncSpace`'s `Drop` is iterative as of #1056,
+                // so tearing the chain down costs no stack depth and
+                // cannot mask the production-side assertions above.
             })
             .expect("spawn worker thread with bounded stack");
         handle

big-code-analysis-web/Cargo.toml

@@ -18,7 +18,7 @@ futures = "^0.3"
 # pins the library's full feature set explicitly. Disabling default
 # features here would silently drop grammars from the daemon build.
 # See #252.
-big-code-analysis = { path = "..", version = "=2.0.1", default-features = false, features = [
+big-code-analysis = { path = "..", version = "=2.1.0", default-features = false, features = [
   "all-languages",
   # `POST /vcs` change-history endpoint (issue #328).
   "vcs",