Repository navigation
fix(core): bound error graph conversion - #36070
Merged
Merged
Conversation
nathanwhit
marked this pull request as ready for review
July 15, 2026 21:38
Cycle termination re-emits the repeated error one final time with its
cause and aggregate members dropped, instead of substituting a
"[Circular]" placeholder node.
The terminal formatter derives its "<ref *n>" / "[Circular *n]" labels
from that repeat: find_recursive_cause matches a cause against its
earlier occurrence by value, which is why is_same_error deliberately
ignores cause. A placeholder matches nothing, so the labels disappeared
and error_cause_recursive{,_tail,_aggregate} failed.
Aggregate members now honor the same rule via the path-local seen set,
which is what bounds the self-referential aggregate that previously
recursed until the stack overflowed. The depth cap, node budget, and
getter containment are unchanged.
bartlomieju
approved these changes
Jul 16, 2026
bartlomieju
left a comment
bartlomieju
left a comment
Member
There was a problem hiding this comment.
Nice fix. The self-referencing AggregateError overflow is well diagnosed: aggregate members restarted conversion with a fresh cycle set, and now cycle state is path-local and threaded into both cause and aggregate recursion. Confirmed the aggregate path was the only entry restarting state, and the layered bounds (32-deep + 256-node budget in conversion, independent 64-deep cap in the formatter) plus TryCatch containment of throwing getters all look correct. Test coverage is thorough (cycles, crossed cycles, deep chains, shared-graph fan-out, throwing getters).
Only non-blocking note: legitimate cause chains deeper than 32 now render [Error details truncated]. Real chains are almost always shallow so this is fine, just a heads-up that it's a user-visible change for pathological cases.
bartlomieju
pushed a commit
that referenced
this pull request
Jul 23, 2026
## Summary - preserve cycle tracking while converting nested error causes and aggregate members - cap error conversion by both path depth and a shared node budget - contain failed aggregate property reads and bound recursive terminal formatting - add regressions for cycles, deep chains, shared graphs, and throwing getters ## Bug Deno converts JavaScript exceptions into a native `JsError` tree before formatting them. Aggregate members previously restarted conversion with a fresh cycle set, so a self-reference could recurse indefinitely. Deep cause chains also had no native recursion bound, and compact graphs containing the same child on multiple branches could cause conversion work to grow much faster than the input graph. The converter now retains path-local cycle state, limits each path to 32 levels, and shares a 256-node budget across the complete conversion. It emits a single truncation marker when either limit is reached. Reads of `AggregateError.errors` and its elements are contained so a throwing getter produces an unavailable-details marker instead of disrupting conversion. The terminal formatter and its cause-chain prepasses have their own depth bound for manually constructed `JsError` values. ## Validation - `./tools/format.js` - `cargo fmt --check --package deno_core --package deno_runtime` - 6 focused `deno_core` error-conversion tests - `cargo test -p deno_runtime test_format_js_error_truncates_deep_cause_chain --lib` - `cargo check -p deno_core -p deno_runtime` - `cargo build --bin deno` - behavior smokes for a cyclic aggregate, a throwing `errors` getter, and a 100-level cause chain
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
2 participants
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.
Summary
Bug
Deno converts JavaScript exceptions into a native
JsErrortree before formatting them. Aggregate members previously restarted conversion with a fresh cycle set, so a self-reference could recurse indefinitely. Deep cause chains also had no native recursion bound, and compact graphs containing the same child on multiple branches could cause conversion work to grow much faster than the input graph.The converter now retains path-local cycle state, limits each path to 32 levels, and shares a 256-node budget across the complete conversion. It emits a single truncation marker when either limit is reached. Reads of
AggregateError.errorsand its elements are contained so a throwing getter produces an unavailable-details marker instead of disrupting conversion. The terminal formatter and its cause-chain prepasses have their own depth bound for manually constructedJsErrorvalues.Validation
./tools/format.jscargo fmt --check --package deno_core --package deno_runtimedeno_coreerror-conversion testscargo test -p deno_runtime test_format_js_error_truncates_deep_cause_chain --libcargo check -p deno_core -p deno_runtimecargo build --bin denoerrorsgetter, and a 100-level cause chain