fix(rowan): avoid stack overflow when dropping deep trees - #11368
fix(rowan): avoid stack overflow when dropping deep trees#11368Austin1serb wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 64e2f7d The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Organic activityNo automation signals detected in the analyzed events. This is an automated analysis by AgentScan |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 8 remain after this review. Walkthrough
Merge Risk: ⚪ Minimal · up to The change replaces recursive cleanup with iterative cleanup and adds coverage for deep and branching structures; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merging this PR will degrade performance by 5.27%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
|
I originally fixed this by adding a custom cleanup path that handled the final ThinArc release and memory cleanup. In the second commit I simplified it. It still cleans up the tree iteratively, but now it only removes child nodes from uniquely owned parents and lets the existing Arc code handle the actual memory cleanup. The simpler version removes quite a bit of low-level memory handling, but CodSpeed went from about a 3.5% slowdown on the first version to about 5.3% on the second. I don’t want to keep changing the it and rerunning the whole CI. |
|
Have you looked at upstream instead? Maybe it's been fixed there already |
I did check the current rust-analyzer/rowan master and it reproduces there as well. |
|
I did a bit more research and found that serde_json documents the same Drop problem with deeply nested values. By default it limits nesting depth, but if that limit is disabled it warns that Drop itself can overflow the stack. The docs show a carefully_drop_nested_arrays example that pulls children into a Vec and cleans them up iteratively, similar to the approach in this PR. https://docs.rs/serde_json/latest/serde_json/struct.Deserializer.html#method.disable_recursion_limit |
Summary
Fixes #6830
Biome can successfully read the deeply nested JavaScript file from the issue, but then crashes while cleaning it up.
The cleanup was recursive, so every nested level left another unfinished function call on the stack. With deeply nested input, those calls kept piling up until the stack was exhausted and Biome crashed.
I had Codex run the reproduction against older versions to see if this was a recent regression. The same behavior goes back years, so it does not appear to come from a specific recent PR or release.
This changes cleanup to use a loop instead of recursion. The loop keeps track of the remaining cleanup work itself and handles it one item at a time, so deeply nested input no longer makes the stack grow.
This PR was created with AI assistance. The solution was reviewed and validated by me.
Test Plan
I added tests for very deep input, branching structures, and shared data.
The original reproduction from the issue now completes successfully with stack sizes down to 512 KiB.
The tests also verify that cleanup still happens correctly and that the fix does not simply avoid the crash by leaking memory.
I also ran the affected parser tests, just f, and just l.
Docs
Not applicable.