fix(wire): bound recursive Serialize, de-recurse Drop (#1056)#1082
Merged
Conversation
`bca metrics -O json` on ~1 000 nested functions (11 KB of source) overflowed the thread stack. 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. Issues #700 / #709 converted every AST *traversal* to an explicit work stack; this is the same hazard in the recursive *types*, which those tests never reached. Three recursions were involved: - `wire::FuncSpace::from` / `wire::Ops::from` projected a nested tree with `spaces.iter().map(Self::from).collect()` — one frame per level at roughly 2.3 KB each, aborting at ~900 levels on a 2 MiB thread. Both now walk an explicit work stack. - The derived `Serialize` on `FuncSpace`, `Ops`, and `AstNode`. serde cannot emit a tree without one native frame per level, so the depth is bounded rather than de-recursed: 128 nested spaces, 512 nested AST nodes, past which serialization fails with an ordinary error. This mirrors the 128-level limit serde_json's `Deserializer` already applies to the same documents. - The compiler-generated `Drop` glue, which aborted at ~16 000 levels in a debug build. An explicit `Drop` hoists descendants into a flat work list, so teardown costs no stack depth. The `Drop` impls are a source-level break — fields can no longer be moved out of these types by value. STABILITY.md reserves such breaks for a major bump; this one lands under a minor as a documented exception, because the alternative was leaving a remotely-reachable process abort open until 3.0. Both limits sit far above real source: across the 14 450-file corpus under tests/repositories the deepest AST is 188 levels and the deepest space nesting is 10. Fixes #1056
Carries the #1056 fix, whose `Drop` impls are a documented source-level break, so the workspace moves to a minor rather than a patch bump. Regenerates the lockfiles, the man pages, the vendored grammar version literals in `src/langs.rs`, and the SARIF snapshots that embed the driver version.
The #1056 regression tests drove `analyze` at depth 8 000, which costs ~25s each in a debug build — not from the code under test, but from the quadratic `Node::parent` ancestor walks still tracked in #1062. Analysed fixtures drop to depth 2 000 (still ~20x the depth at which the recursive `From` overflowed a 512 KiB thread), and a hand-built 100 000- level chain takes over the teardown coverage, which the 8 000-level fixture only cleared by 2x. Seven tests now run in 2.2s instead of 78s, and pin the `Drop` de-recursion 25x deeper than before.
Three tests stepped around the recursive `Drop` on `FuncSpace` / `Ops` that #1056 removed: two flattened the chain by hand and one leaked it with `std::mem::forget`. Their comments now assert something false, and stepping around the teardown is strictly weaker than exercising it — the tests cover the iterative `Drop` for free once the workaround is gone. Lesson 47 prescribed those workarounds; annotated rather than rewritten, since the advice still holds for recursive types without such a `Drop`.
dekobon
added
bug
6 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1082 +/- ##
==========================================
+ Coverage 98.04% 98.06% +0.01%
==========================================
Files 267 268 +1
Lines 66363 66613 +250
Branches 65933 66183 +250
==========================================
+ Hits 65063 65321 +258
+ Misses 859 851 -8
Partials 441 441
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #1056.
The bug
bca metrics -O jsonon 1 000 nested functions — 11 KB of source —overflowed the thread stack. That is a
SIGABRT, not a catchable panic:bca-web'sspawn_blockingwrapper turns a panic into one failedrequest, but an abort takes the whole process down with every request in
flight. The 4 MiB body cap does not help when the payload is kilobytes.
Issues #700 / #709 converted every AST traversal to an explicit work
stack. This is the same hazard in the recursive types, which those
issues' three small-stack regression tests never reached — all three
exercise the dump walk and build their fixtures by hand.
What actually overflowed
The issue was filed against
Serialize, supported by a clean-lookingbisection (
bca checkbuilds and drops the identical tree withoutserializing it, and survived). Re-measuring each stage in isolation on a
default 2 MiB thread told a different story:
wire::FuncSpace::from(recursiveFrom)Serialize→ JSONSerialize→ YAMLSerialize→ TOMLSerialize→ CBORDropThe reported abort was the
Fromconversion, notSerialize— itruns to completion before serialization starts, at ~2.3 KB per frame
against the JSON serializer's ~170 B.
SerializeandDroparereachable too, just needing deeper input: ~380 000 nested
fns fitinside the 4 MiB body cap.
The issue's scope-correction note also wrote
Dropoff after a10 000-level chain survived. A cheap frame is not an iterative one — it
aborts at 16 000 in a debug build.
A fourth site the issue did not list:
AstNode/astis worse than/metrics, because AST depth is far cheaper to buythan space depth. 40 000 levels of nested parentheses — an 80 KB
payload — aborted the process in
serde_json::to_stringon a releasebuild. The
dumpwalk itself survived (that is #700's work); theoverflow is the derived
SerializeonAstNode, and then again itsDrop.The fix
Fromis iterative.wire::FuncSpace::fromandwire::Ops::fromshare a
map_treehelper driving an explicit work stack. Verifiedconverting a 1 000 000-level chain on a 512 KiB thread.
Serializeis bounded, not de-recursed.serdeoffers noiterative escape —
serialize_fieldmust run the child'sSerializeto completion before it returns — so the depth is capped and a deeper
tree fails with an ordinary serializer error naming the type and the
limit. New public constants
wire::MAX_SPACE_SERIALIZE_DEPTH(128) andMAX_AST_SERIALIZE_DEPTH(512). The space limit mirrors the 128-levelrecursion limit
serde_json'sDeserializeralready applies to thesame documents.
Dropis iterative onFuncSpace,Ops,AstNode, and the twowiremirrors. Verified at 1 000 000 levels.Picking the limits
Grounded against the corpus under
tests/repositories(14 450 files —TensorFlow, DeepSpeech, serde, …): deepest AST 188 levels, deepest
FuncSpacenesting 10. The limits are ~2.7x and ~13x those. On theread side a
FuncSpacedocument cannot be parsed back past ~61 levelsanyway (two JSON nesting levels per space against
serde_json's 128), sothe emit limit is the more permissive of the two.
Adding
impl Dropforbids moving fields out by value:let m = space.metrics;is nowE0509, fixed with a.clone()or aborrow. 13 sites inside this repository needed it.
STABILITY.mdreserves source-level breaks for a major bump. This onelands under a minor as an explicit, documented exception — recorded
under (breaking) in
CHANGELOG.mdand inSTABILITY.md— becausethe alternative was leaving a remotely-reachable process abort open until
3.0. There is no way to break theDropchain without animpl Dropon the type. Approved by the maintainer before landing; please confirm
you are still happy with that call.
The workspace moves to 2.1.0 accordingly.
Verification
Seven regression tests, each confirmed to fail against the pre-fix
code by perturbing production back to its former shape — five abort the
test binary with
SIGABRT, exactly the reported symptom. Per the note inthe issue, they drive
analyze/ buildFuncSpacechains rather thanreusing the dump fixtures, so they scale with space nesting rather than
AST depth.
CLI, on the original reproducer:
Live
bca-web, both hostile payloads, same process:make pre-commitgreen.Commits
3fd01c70CHANGELOG/STABILITY/ book updates7b8f3fd68634dd5cNode::parentwalks still tracked in #1062 at depth 8 000. Analysed fixtures dropped to 2 000, a hand-built 100 000-level chain took over the teardown coverage — 2.2 s now, and pinsDrop25x deeperd827ee7bmem::forgetleak) whose comments asserted something false; those tests now cover the iterativeDropfor freee8841073Known remaining recursion
Clone,PartialEq, andDebugare still recursive on these types(
Cloneaborts around 700 levels). None is on a production path — theCLI, web, and Python crates all borrow, and nothing compares or
{:?}-formats a whole tree — so none is reachable today, andhand-writing them would trade a live-bug fix for a field-drift hazard the
derives currently rule out. Worth its own issue if a tree-cloning or
tree-comparing API is ever added.