test_runner: add StackCheck to pretty_format to stop SIGSEGV on deeply nested diff/snapshot values#34885
test_runner: add StackCheck to pretty_format to stop SIGSEGV on deeply nested diff/snapshot values#34885robobun wants to merge 5 commits into
Conversation
…ow on deep values A failing toEqual diff or toMatchSnapshot over a ~20k-deep (non-circular) array or object would recurse Formatter::print_as until the native stack was exhausted and the test runner SIGSEGV'd mid-run with no summary. The sibling formatter in ConsoleObject.rs already carries a StackCheck guard for the same recursion; this one never had it. Add the field (initialized via StackCheck::init() so the bound is armed), check is_safe_to_recurse() before descending into Array/Object/Map/Set, and set failed=true when the limit is reached so the matcher still reports a normal failure with a truncated diff instead of crashing the process.
|
Status: diff is green, review LGTM. CI red on unrelated lanes; needs a maintainer to merge. Reproduced with stock bun (4 subprocess tests SIGSEGV with stderr cut off at the test-file header). With the fix applied, The CI: Overlap with #34884: that PR applies the same |
|
Updated 1:35 AM PT - Jul 21st, 2026
❌ @robobun, your commit faf9c95 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34885That installs a local version of the PR into your bun-34885 --bun |
WalkthroughChangesPretty-format recursion safety
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/test/pretty-format-overflow.test.ts`:
- Around line 55-58: Condense the regression comment above the deeply nested
pretty-format test from four lines to three or fewer while preserving its
explanation of the SIGSEGV risk and why the test runs in a subprocess.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3ee84e4-99e4-4809-82d1-5b7c21039515
📒 Files selected for processing (2)
src/runtime/test_runner/pretty_format.rstest/js/bun/test/pretty-format-overflow.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/bun/test/pretty-format-overflow.test.ts (1)
79-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrengthen the deep-format assertions.
1 fail/1 passand the generic matcher header only prove that the child completed; a formatter that emitted an empty or incorrect representation could still satisfy these checks. Assert the generated snapshot contents and stable truncated outer representation in the array/object diffs.As per coding guidelines, every assertion must be able to fail and assert the strongest invariant.
Also applies to: 103-106, 127-130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/bun/test/pretty-format-overflow.test.ts` around lines 79 - 82, Strengthen the stderr assertions in the deep-format test cases around the existing process execution checks, including the corresponding blocks at the other referenced locations. Assert the expected snapshot contents and stable truncated outer array/object representations in each diff, rather than relying only on the generic matcher header and pass/fail counts; preserve the existing exit-code assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/js/bun/test/pretty-format-overflow.test.ts`:
- Around line 79-82: Strengthen the stderr assertions in the deep-format test
cases around the existing process execution checks, including the corresponding
blocks at the other referenced locations. Assert the expected snapshot contents
and stable truncated outer array/object representations in each diff, rather
than relying only on the generic matcher header and pass/fail counts; preserve
the existing exit-code assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8319f17-6a1d-4f22-8a98-3ba9e7f885a2
📒 Files selected for processing (1)
test/js/bun/test/pretty-format-overflow.test.ts
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Re: strengthening the deep-format assertions (CodeRabbit), pushed 38df8b3 adding Re: suggested linked issues, neither is this bug:
Leaving both out of the PR body. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Tag::JSX recurses print_as -> format -> print_as::<JSX> via props.children but is not in can_have_circular_references(), so a ~20k-deep React element chain still exhausted the native stack with the check gated there. Move the is_safe_to_recurse() check to the top of print_as so every self-recursive tag is covered, and add a subprocess test for the JSX shape.
There was a problem hiding this comment.
Both prior findings addressed — the JSX-chain gap is fixed in bf068fb (check hoisted above can_have_circular_references(), with a fourth subprocess test), and the snapshot-truncation determinism concern has a reasoned deferral. Leaving the final call on that trade-off (silent non-deterministic truncation vs. a follow-up depth cap) to a maintainer.
What was reviewed:
- Confirmed all
pretty_format::Formatterconstruction routes throughFormatter::new()(pretty_format.rs:169, 214), sostack_checkis always armed;DiffFormattersites reach it viaJestPrettyFormat::format. StackCheck::is_safe_to_recurse()is an inlined pointer comparison — safe to run on everyprint_asentry.- New tests follow harness conventions (
tempDir,describe.concurrent, concurrent pipe drain, stderr-before-exitCode assertions) and cover Array/Object/JSX/snapshot shapes.
Extended reasoning...
Overview
The PR adds a StackCheck field to pretty_format::Formatter and guards print_as entry with is_safe_to_recurse(), preventing native stack exhaustion (SIGSEGV) when formatting ~20k-deep acyclic values in toEqual diffs and toMatchSnapshot serialization. Production change is ~15 lines in one file; tests add four subprocess cases in the existing pretty-format-overflow.test.ts.
Prior feedback
I left two inline findings on the earlier revision:
- 🔴 The check was gated on
can_have_circular_references(), soTag::JSXchains still crashed. Fixed in bf068fb — the check is now above the gate and a deep-React-element subprocess test was added. - 🟡 On the snapshot path,
fmt.failedis never read, so a truncated serialization is persisted with a trip-depth that varies by build/platform. The author gave a reasoned deferral: throwing or returningErrfrom insideprint_ascreates re-entrant exception state on thediff_format.rspath (which discards the result withlet _ =), and a deterministic depth cap changes serialization for all deep values. Strictly better than the pre-PR SIGSEGV, and I marked it non-blocking originally.
Security risks
None. This is a defensive recursion bound in test-runner output formatting.
Level of scrutiny
Low-to-moderate. The mechanism mirrors the existing ConsoleObject.rs guard exactly, and StackCheck is an established in-tree helper. I verified every Formatter construction site goes through new() and that is_safe_to_recurse() is a cheap inlined check suitable for per-entry use.
Why defer rather than approve
The one open item is a product/design call the author explicitly left for a maintainer: whether persisting a non-deterministic truncated snapshot (at extreme depth) is acceptable interim behavior, or whether the depth-cap follow-up should land alongside this. The author's reasoning against the in-PR alternatives is sound, but "happy to follow up separately if wanted" is a question for a human to answer, not me.
|
Overlaps with #34884: both add the same |
There was a problem hiding this comment.
LGTM — the JSX-chain gap from the earlier review is closed (check now sits above the can_have_circular_references() gate, with a subprocess test for the 20k-deep React element shape), and the snapshot-truncation trade-off is a reasoned "strict improvement over SIGSEGV" call I already marked non-blocking. Note the overlap with #34884; whichever lands second needs the trivial rebase.
Extended reasoning...
Overview
Adds a StackCheck field to pretty_format::Formatter, initialized in Formatter::new(), and checks is_safe_to_recurse() at the top of print_as — before the can_have_circular_references() gate — so every self-recursive tag (Array/Object/Map/Set/JSX) bails out by setting self.failed = true instead of exhausting the native stack. Mirrors the identical guard already present in ConsoleObject.rs. Four new subprocess tests cover deep arrays, deep objects, deep JSX chains (toEqual diff path), and deep arrays via toMatchSnapshot.
Follow-up from the earlier review
The earlier 🔴 (stack check gated behind can_have_circular_references(), so Tag::JSX bypassed it) was fixed in bf068fb by hoisting the check above the gate; a fourth subprocess test for the 20k-deep React element chain was added and confirmed to fail on stock 1.3.14. Verified at pretty_format.rs:1161 that the check now runs unconditionally on print_as entry. The earlier 🟡 (snapshot path never reads fmt.failed, so a tripped guard persists a platform-dependent truncated serialization) was acknowledged with a reasoned trade-off — throwing or returning Err would leave a pending exception on the DiffFormatter Display path, and a fixed depth cap changes serialization for all deep values. I already flagged it non-blocking; not re-litigating.
Security risks
None. This is a defensive recursion guard in the test runner's diff/snapshot formatter; no untrusted-input parsing, auth, crypto, or network paths are touched.
Level of scrutiny
Low-to-medium. ~11 lines of Rust adding a field + a top-of-function guard that copies an established in-tree pattern. Formatter has no other struct-literal constructors (a missing field would fail to compile), so new() covers every entry. Tests follow harness conventions: describe.concurrent, tempDir, both pipes drained via Promise.all, stderr asserted before exitCode, bunEnv spread where modified.
Other factors
Fail-before/pass-after was demonstrated on both debug+ASAN and release. expect.test.js (415 tests) passes unchanged. The overlap with #34884 (same StackCheck field, plus a console_object::Formatter::new() seat) is already noted in the thread and is a coordination concern rather than a correctness one.
1 participant
What
bun testSIGSEGVs (exit 139, no test summary) when a failingtoEqual/toBediff or atoMatchSnapshotserializer walks a ~20k-deep value. At 8k depth it still prints a correct1 failwith a diff; past that the native stack is exhausted insidepretty_format::Formatter::print_asand the whole runner dies mid-run.Why
src/runtime/test_runner/pretty_format.rs'sFormatteronly carries an identity-keyedvisitedmap for cycle detection; it has noStackCheckand no depth limit, so a deep acyclic structure recursesprint_asuntil the process crashes. The sibling formatter insrc/jsc/ConsoleObject.rsalready guards the same recursion withStackCheck::is_safe_to_recurse().Fix
Add a
stack_check: StackCheckfield topretty_format::Formatter, initialized viaStackCheck::init()inFormatter::new()so the bound is armed by default. Before descending into anycan_have_circular_references()tag (Array/Object/Map/Set) inprint_as, checkis_safe_to_recurse(); when it fails, setfailed = trueand return. The matcher still reports a normal failure with a truncated diff, and the snapshot serializer writes a truncated serialization, instead of the process dying.Verification
New tests cover deep arrays, deep objects, and the snapshot path, each in a subprocess so a regression fails the suite rather than segfaulting the outer runner.
expect.test.js(415 tests) and the snapshot suites pass unchanged.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file