Conversation
WalkthroughThe formatter now retains visited errors during rendering and propagates pending JavaScript exceptions. Regression tests cover cyclic errors in uncaught output, ChangesCircular error formatting
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
@coderabbitai review |
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/regression/issue/circular-error-stack.test.ts`:
- Around line 86-88: In test/regression/issue/circular-error-stack.test.ts,
replace the rationale comment at lines 86-88 with the confirmed GitHub issue URL
comment, and remove the duplicate rationale block at lines 123-127; retain only
the single issue-URL comment required for regression tests.
- Around line 90-97: Drain both configured subprocess pipes concurrently in both
test cases: at test/regression/issue/circular-error-stack.test.ts lines 90-97,
include stdout alongside stderr and proc.exited in the Promise.all call; at
lines 105-116, include stderr alongside stdout and proc.exited. No other changes
are needed.
🪄 Autofix
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: 88ddf1fa-210d-4104-a2e0-20f71eb8947d
📒 Files selected for processing (2)
src/jsc/ConsoleObject.rstest/regression/issue/circular-error-stack.test.ts
|
Checked all five; same printer, different bugs, and this PR was deliberately narrowed to not overlap them.
All five can land independently of this one. |
There was a problem hiding this comment.
LGTM — the fix removes the visited-map removal/re-insertion dance in print_error and adds the same has_exception() propagation already used in print_as_prelude.
What was reviewed:
- Traced the visited-map lifecycle:
print_as_preludeinserts,VisitedRemoveguard removes on exit, and keeping the value in the map duringprint_errorlike_objectcorrectly bounds both the inline-property re-entry (errors: [e]→print_as) and the cause-chain loop atVirtualMachine.rs:6438. - Confirmed the top-level error still prints (the map is only consulted for child re-entries, not the initial render).
- Tests drain both pipes, use
test.concurrent, and the author verified fail-without-fix on both ASAN and release.
Extended reasoning...
Overview
The PR fixes a SIGSEGV in the error printer when an Error is both its own cause and an entry in its own errors array. The change is a net -14 lines in src/jsc/ConsoleObject.rs plus three regression tests.
The production change has two parts:
- Delete the 18-line block in
Formatter::print_errorthat temporarily removed the value from the visited map (via raw pointer +scopeguard::defer!with anunsafere-insertion) before callingprint_errorlike_object. The value now stays in the map, so re-entrant formatting ofcause/errorshits the existing[Circular]guard inprint_as_preludeand the cause-chain loop inprint_error_instance_body. - Add a 3-line
has_exception()check afterprint_errorlike_objectreturns, converting a pending exception intoErr(JsError::Thrown)— the identical pattern already used atConsoleObject.rs:3299inprint_as_prelude.
Security risks
None. This is error-formatting output logic; no auth, crypto, filesystem, or network surface. The change removes an unsafe block rather than adding one.
Level of scrutiny
Medium. The error printer is subtle and this file has several other open PRs targeting adjacent shapes (#35820, #35825, #34892, #34884, #36921), but the author analyzed each and this change is deliberately scoped not to overlap — it fixes only the Formatter::print_error visited-map handling, not the AggregateError branch or StackCheck seating those PRs cover. The removed code was demonstrably wrong (caused a crash), and the replacement is strictly simpler. I traced the map lifecycle through print_as → print_as_prelude (insert) → print_error → print_errorlike_object → property iteration → re-entrant print_as (finds existing → [Circular]) → cause-chain loop at VirtualMachine.rs:6427-6454 (finds existing → [Circular]) → VisitedRemove guard (remove on exit). The top-level error still renders because the map is only consulted for children.
Other factors
- All prior review feedback (my pipe-draining note, CodeRabbit's identical finding, and the comment-cop bot's comment-length flags) was addressed in cf12347/a63cab1/796adb2; every thread is resolved.
- The gate evidence in the description shows all three new tests fail on both ASAN-debug and release without the fix (SIGSEGV / mangled worker message) and pass with it.
- The removed code included a raw-pointer capture inside
scopeguard::defer!with anunsafere-insertion — deleting it is a clear safety improvement. - The added
has_exception()check does not clear the exception, soBun.inspect's stack-overflow rethrow (pinned bybun-inspect.test.ts) is preserved.
|
CI status: 195 of 196 jobs passed on 796adb2. The one red lane (darwin 14 x64 test-bun) is test/cli/test/parallel.test.ts, which the CI helper marks as pre-existing (same failure on main) and which this diff does not touch; it has been reported for main-break triage. The remaining entries in the annotation passed alone or on retry. The previously red bun-inspect.test.ts stack-overflow test passes on this revision. |
796adb2 to
3cdb45c
Compare
|
Updated 5:28 PM PT - Sep 28th, 2026
✅ @robobun, your commit 9638e773baf4a0dc7b1253d00df04e5772caf3b6 passed in 🧪 To try this PR locally: bunx bun-pr 37270That installs a local version of the PR into your bun-37270 --bun |
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. The fix itself is small and looks correct — dropping the visited-map removal so the [Circular] guard fires, plus the has_exception() → Err(Thrown) check that mirrors print_as_prelude a few lines up. Given it adjusts the error-printer's exception-return contract (an earlier revision here broke bun-inspect.test.ts) and there are five open PRs touching adjacent guards in the same printer (#35820, #35825, #34892, #34884, #36921), a maintainer glance at the landing order would still be worthwhile.
What was reviewed:
- Traced
print_as→print_as_prelude→print_error: the value now stays inself.mapacrossprint_errorlike_object, so re-entry forcause/errorshits the existingget_or_put→[Circular]path; top-level name/message rendering doesn't go through the formatter, so it isn't shadowed. - Confirmed the new
has_exception()guard matches the pattern atConsoleObject.rs:3289and leaves the exception pending (noclearException). - Tests: pipes drained concurrently, positive assertions only,
test.concurrentfor the subprocess cases; the vacuousASSERTION FAILEDcheck from the previous revision is gone.
Extended reasoning...
Overview
Two files: src/jsc/ConsoleObject.rs (net −14 lines in Formatter::print_error) and three new tests appended to test/regression/issue/circular-error-stack.test.ts. The source change (1) deletes the block that temporarily removed the current error from the formatter's visited map before calling vm.print_errorlike_object, and (2) adds a self.global_this.has_exception() check after that call to convert a pending exception into Err(JsError::Thrown) instead of Ok(()).
Security risks
None. This is diagnostic-output formatting for already-thrown errors; no auth, crypto, parsing of untrusted input, or resource acquisition. The change removes an unsafe scopeguard::defer! block with a raw-pointer write and replaces it with nothing, which is a net safety improvement.
Level of scrutiny
Medium. The diff is small and the mechanism is well-argued, but the error printer's recursion and exception-propagation contract is subtle: the original removed code carried a rationale comment ("circular check already done in print_as"), an earlier revision of this PR broke bun-inspect.test.ts and had to be narrowed, and five open PRs (#35820/#35825/#34892/#34884/#36921) touch adjacent guards in the same call graph. A maintainer should confirm this is the one to land and that keeping the value in the visited set has no unintended effect on non-cyclic error rendering that the existing suites don't cover.
Other factors
- All prior review feedback (comment-cop, CodeRabbit, my two inline nits on pipe draining and the vacuous
ASSERTION FAILEDassertion) is addressed; every thread is resolved. - CI on 796adb2 was 195/196 with the one red job flagged as pre-existing on main; the latest push (60f95cd) only removes one test assertion.
- The PR's evidence block shows the three new tests failing on unfixed debug/ASAN and release builds and passing with the fix, satisfying the fails-for-the-right-reason bar.
- The added exception check follows the exact pattern already used at
ConsoleObject.rs:3289-3290inprint_as_prelude, and per the PR description the pending exception is intentionally not cleared soBun.inspectstill rethrows genuine stack overflows (pinned bybun-inspect.test.ts).
|
Two facts from work on a related crash path: an The JSC's property walk ( One case still aborts on current main with this PR's change applied. It does not reproduce on this branch's older base. let deep = new Error("leaf");
for (let i = 0; i < 1000; i++) {
const next = new Error("level " + i);
next.inner = deep;
deep = next;
}
const top = new Error("top");
top.a = deep;
top.b = new Error("sibling");
try { console.error(top); } catch {}The depth guard in Branch Both cases need an assert build. A release build does not crash on them through |
|
A separate report reached me: const e = new Error("boom");
e.when = Object.assign(new Date(0), { toJSON() { throw new Error("toJSON threw"); } });
try { console.log([e]); } catch (err) { console.log("threw", err.message); }
console.log("survived");
One sibling is not covered by this PR. const e = new Error("boom");
e.entries = new Map([[Object.assign(new Date(0), { toJSON() { throw new Error("toJSON threw"); } }), {}]]);
console.log(e); // same assertion on a debug build, with or without this PRChange that fixes the sibling, and a test for all of the shapes above--- a/src/jsc/ConsoleObject.rs
+++ b/src/jsc/ConsoleObject.rs
@@ pub mod formatter { (MapIteratorCtx::for_each)
let Ok(key_tag) = Tag::get_advanced(key, global_object, opts) else {
return;
};
- let _ = this.formatter.format::<C>(
- key_tag,
- this.writer,
- key,
- this.formatter.global_this,
- );
+ if this
+ .formatter
+ .format::<C>(key_tag, this.writer, key, this.formatter.global_this)
+ .is_err()
+ {
+ return;
+ }
this.writer.write_all(b": ").expect("unreachable");The C++ iteration stops at the pending exception after the callback returns, so Test for test("an own property of an Error that throws while it is printed", async () => {
const calls = [
"console.log(e)",
"console.log([e])",
"console.log([1, e])",
"console.log([[e]])",
"console.log({ wrapped: [e] })",
'console.log([new Error("outer", { cause: e })])',
'console.log(new Map([["key", e]]).entries())',
"console.table(new Map([[e, 1]]))",
"Bun.inspect([e])",
"console.log(hostileMapKey)",
];
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const hostile = () => Object.assign(new Date(0), { toJSON() { throw new Error("toJSON threw"); } });
const e = new Error("boom");
e.when = hostile();
const hostileMapKey = new Error("boom");
hostileMapKey.entries = new Map([[hostile(), {}]]);
const attempt = (name, call) => {
try {
call();
console.error(name + ": returned");
} catch (err) {
console.error(name + ": threw " + err.message);
}
};
${calls.map(call => `attempt(${JSON.stringify(call)}, () => ${call});`).join("\n")}
`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe(calls.map(call => `${call}: threw toJSON threw\n`).join(""));
expect(exitCode).toBe(0);
});One interaction to know about. |
|
A scope note, from a measurement of the open error-printer PRs against four native stack overflows (table in #36602 (comment)).
|
|
Do not merge this revision yet. A check of deep error chains found a regression in this diff.
Repro: const { Worker } = require("node:worker_threads");
const w = new Worker(
`let e = new Error("leaf"); for (let i = 0; i < 500; i++) { const x = new Error("l" + i); x.cause = e; e = x; } throw e;`,
{ eval: true },
);
w.on("error", x => console.log(x === null ? "null" : x.message));A fix is in progress: the Worker must deliver the error of the user when only the rendering fails. |
60f95cd to
8d69b30
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the new VisitedRemove::new guard in print_error_instance_body for aliasing problems — it takes &raw mut formatter.map (no reference is created) and mirrors the existing guard in print_as, so it is not a new pattern — and the expect("unreachable") on get_or_put in visited_enter, which is the same call moved out of print_as and the old errors_to_append loop rather than a new panic path. The double render of a self-listing AggregateError is pre-existing (the member walk is untouched here).
Extended reasoning...
The change moves visited-set ownership for Error rendering from the console formatter into the native error printer in src/jsc/ConsoleObject.rs and src/jsc/VirtualMachine.rs, adds a raw-pointer RAII guard armed by a Cell, and adds subprocess tests; it touches no security-sensitive surface. Six confirmed findings are posted inline (exception-propagation change at the stack bound, output nits, a stale SAFETY comment, and a test-coverage gap), so a human look is already signalled; this note only records the memory-safety candidates that were examined and ruled out as pre-existing patterns.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/jsc/VirtualMachine.rs— Minor: an error whose cycle closes through a non-enumerablecause(thenew Error(msg, { cause })form) prints a bare[Circular]line after the stack trace instead ofcause: [Circular],under the key. The fallback at src/jsc/VirtualMachine.rs:7205-7208 pushescauseintoerrors_to_appendwithout thecircularcheck the enumerable loop applies at src/jsc/VirtualMachine.rs:7096. Fix: apply the same in-place[Circular]rendering to the non-enumerablecause(compare againsterror_instanceandvisited_contains) so both spellings ofcauseprint under the key, and add that shape to the test matrix. [also at: src/jsc/VirtualMachine.rs:7250 - nit: A cycle closed by a non-enumerablecause(thenew Error(msg, { cause })form) prints a bare[Circular]after the stack trace with nocause:key, unlike the enumerable case.]Why this was flagged
Input:
const e = new Error("x", { cause: undefined }); e.cause = e; throw e;(the constructor option makescausea non-enumerable own property, and assignment keeps that attribute), orconst a = new Error("a"); const b = new Error("b", { cause: a }); a.cause = b; throw a;. Any printer entry reaches print_error_instance_body. The property iterator at src/jsc/VirtualMachine.rs:7070 is created with DontEnumPropertiesMode::Exclude (src/jsc/bindings/JSPropertyIterator.cpp:87), so the loop with the newcircularcheck at src/jsc/VirtualMachine.rs:7096 never seescause. The fallback at src/jsc/VirtualMachine.rs:7203-7211 pushes the cycliccauseinto errors_to_append unconditionally. At src/jsc/VirtualMachine.rs:7239 the error is recorded, and the loop at 7244 writes "\n" then print_error_instance_js writes[Circular]at src/jsc/VirtualMachine.rs:6676 with no key and no trailing comma. The user seeserror: xfollowed by the frames and then a lone[Circular]line, while the enumerable spelling of the same cycle prints…Verification: nit — triggered whenever a cyclic error's cycle closes through a non-enumerable
cause(thenew Error(msg, { cause })form, which per spec InstallErrorCause createscausevia CreateNonEnumerableDataProperty, and a latere.cause = eassignment keeps the DontEnum attribute). Mechanism verified in /home/claude/bun/src/jsc/VirtualMachine.rs: the own-property iterator at 7070-7081 is built on… -
🟡
src/jsc/ConsoleObject.rs— Maintainers reading theVisitedRemoveDrop get a SAFETY comment that no longer describes its callers. src/jsc/ConsoleObject.rs:1562-1563 saysmap/armed"were taken viaaddr_of!on locals", but the newVisitedRemove::newcaller at src/jsc/VirtualMachine.rs:7060-7061 passes a field of a&mut Formatterparameter andCell::as_ptr(), and the# Safetyclause onnew(src/jsc/ConsoleObject.rs:1548) omits the no-live-borrow condition the Drop comment relies on. Fix: state the real contract once onVisitedRemove::new(pointers outlive the guard and no other borrow of the map is live at drop) and make the Drop comment reference it, so both theprint_asliteral site and the new site are covered.Why this was flagged
REVIEW.md and the root CLAUDE.md require SAFETY comments above
unsafeto be accurate. After this PR the Drop at src/jsc/ConsoleObject.rs:1562-1566 is reached from two constructors: the struct literal inprint_as(src/jsc/ConsoleObject.rs:3324,&raw mut self.map, a field of&mut self, not a local) and the newunsafe fn newcall at src/jsc/VirtualMachine.rs:7058-7064 (&raw mut formatter.mapandrecorded.as_ptr()from aCell<bool>). Neither matches "addr_of! on locals". The# Safetydoc onnew(src/jsc/ConsoleObject.rs:1548-1549) only requires the pointers to stay valid, while the Drop's second clause ("no other borrow is live at drop") is what actually makes the dereference sound, so a future caller can satisfy the documented contract and still violate the real one. The dismissal accepted that the invariant happens to hold today; the finding is that the documented contract is wrong for a newly added public unsafe API. Remedy: rewrite the# Safetyclause onnewto include both conditions and correct the Drop comment.Verification: nit. Triggering condition: any maintainer reading or auditing the
unsafeblock inVisitedRemove::dropafter this PR. The Drop SAFETY comment at /home/claude/bun/src/jsc/ConsoleObject.rs:1562-1563 is unchanged from base and still reads "map/armedwere taken viaaddr_of!on locals that outlive this guard; no other borrow is live at drop." After this PR the struct becomespub(crate)…
…oses the cycle
An Error that reaches itself through two of its own properties crashed the
process when it was printed:
const e = new Error('cyc'); e.cause = e; e.errors = [e]; throw e;
The set of values that are being rendered lived with two of the callers
that start the render of an error. The console formatter recorded an
error and removed it again before it called the printer. The loop that
prints the cause recorded the next error. The uncaught entries recorded
nothing.
Every render of an error passes print_error_instance_js. It now writes
[Circular] for an error that a caller further up is rendering. The body
records the error before the first render it nests, and
print_error_instance_js removes the record when the body returns. An
error that nests no render is never recorded. The console formatter no
longer records errors. It keeps its stack check for them.
An Error-valued own property that closes a cycle is printed in place,
under its key. That includes a cause that is not enumerable. A member of
an AggregateError that is being rendered prints [Circular] on its own
line. The walk over the members is not changed in any other way.
18a3f20 to
7bfdf30
Compare
ca0e47c to
f2c583d
Compare
|
Revision f2c583d answers the review of 2026-09-27. It replaces the revision that the hold notice of 2026-09-26 is about. Changed for the findings:
Not changed: an assigned The PR is a draft until the release numbers are in the description. |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, I also checked the record/release pairing of the new recorded flag: the JSValue::ZERO caller in print_externally_remapped_zig_exception can never set it (both property branches are skipped in that mode), print_error_instance_js removes the entry before result? so error exits are covered, and Tag::Error is only produced for JSType::ErrorInstance, so every value that now skips the prelude insert is caught by the visited_contains short-circuit — none of these leak a visited-map entry.
Extended reasoning...
The push since the last review moved the stack check ahead of the is_recorded_by_its_printer early return, added the trailing newline for [Circular] AggregateError members, and nested an object in the "printed twice" fixture, addressing the earlier inline nits. The remaining posted finding is a dead circular branch in the append loop; no security-sensitive surface is touched.
…ng rendered The loop over the own properties prints such an error in place, so it never reaches the queue.
Stacked on #44139.
Problem
const e = new Error('cyc'); e.cause = e; e.errors = [e]; throw e;prints about 2,100 lines, then SIGSEGV (rc 139).Formatter::print_error(src/jsc/ConsoleObject.rs) removed it before the call. The uncaught entries made none.Fix
print_error_instance_js(src/jsc/VirtualMachine.rs) prints[Circular]for an error that a caller is rendering. Every render passes it.cause: [Circular],.test/js/bun/util/inspect-error.test.js(15 new, 14 fail on main),test/js/node/worker_threads/worker_threads.test.ts(1 new).Background
[Circular].Downsides
throw: an error with an object property costs 1 more hash insert and lookup, an error with a cause 2 more lookups, a plain error none.console.logandBun.inspectcost less.throw(Guard the console and test-runner formatters against native stack overflow on deeply nested values #34884).Notes
Status. #44268 covers this PR and lists it under Fixes. This PR stays open as the small fallback until that one lands. I do not push to it any more.
All numbers: linux x64, main a4f1429 and this branch.
Pool takes / hash inserts / hash lookups per print (release build, gdb hit counts of
LocalKey::withfor the pool,HashMap<JSValue, ()>::get_or_put_slotand::get_index, difference of 80 and 40 prints):Bun.inspect(new Error("x")),console.logof itBun.inspectof an Error with primitive propertiesBun.inspectof an Error with an object propertyBun.inspectof an Error with a causeBun.inspectof an AggregateError of 2Bun.inspect({ a: 1, b: [1, { c: 2 }] })reportError(new Error("x")), also with primitive propertiesreportErrorof an Error with an object propertyreportErrorof an Error with a causereportErrorof an AggregateError of 2reportErrorandthrowuse the same printer entry.Syscalls (gdb
catch syscall, entries plus returns, release):throw new Error('x')477 -> 477. With an object property 477 -> 477. With a cause 477 -> 477. Fore.cause = e; e.errors = [e]510 -> 477 (write38 -> 4).Binary.
sizeof the release binary:.text80660492 -> 80659212 bytes (the same as #44139). Code of the printer functions (nm -S): 18958 -> 18744 bytes.Formatterhas no new field. Instructions were not measured:perfandvalgrindare not installed.Stack reserved per function (
sub ...,%rsp). Release:Formatter::print_error40 -> 32 B,print_error_instance_js5176 -> 5176 B,print_error_instance_body376 -> 376 B,agg_iter0 -> 24 B. Debug+ASAN:Formatter::print_error736 -> 448 B,print_error_instance_js3488 -> 3616 B,print_error_instance_body10816 -> 10752 B,agg_iter704 -> 800 B. One nested render through the cause loop, debug+ASAN: 14304 -> 14368 B. Through the formatter: 20576 -> 20352 B.Output. Debug+ASAN build, one process per output. 38 values through 10 entries (
console.log,console.error,console.log(v, v),Bun.inspectat depth 2, 0 and Infinity,util.inspect,throw,Promise.reject,reportError): 380 outputs.Bun.inspectat depth Infinity of an AggregateError that lists itself prints 1001 levels before the stack check stops it, main 1013.e.self = eatthrowself: [Circular],e.cause = e; e.errors = [e]atthrowa.x = [b]; b.y = batthrowa.cause = b; b.cause = aatconsole.lognew Error("b", { cause: a })witha.cause = b[Circular]cause: [Circular],e.cause = e; e.errors = [e]messageis the rendered textError: cycWhat the first revision of this PR had, and why it is gone. The first revision removed the un-record in
Formatter::print_errorand made that function returnErrwhen the printer left an exception pending. A check of deep chains found that anode:worker_threadsWorker then reportednullto its parent when the render of its uncaught error failed:on_unhandled_rejection(src/jsc/web_worker.rs) takes theJSC::Exceptioncell for the value onErr. #38560 fixes that reporter. TheErrchange is not part of this PR now. It can follow #38560 as its own change. The debug-build aborts that it fixed (comments of 2026-09-11 and 2026-09-13 on this PR) are not fixed by this revision.Also not fixed here. An AggregateError that lists itself still renders once per level, to the depth cap (#36602). An assigned
causeis still rendered twice at each nested level.Suites run (debug+ASAN):
inspect-error.test.js,inspect.test.js,bun-inspect.test.ts,reportError.test.ts,console-log.test.ts,circular-error-stack.test.ts,circular-error-stack-edge-cases.test.ts,test/js/bun/test/stack.test.ts.Self-review. 9 concerns raised, 9 addressed. Addressed by a smaller diff: the AggregateError walk keeps its shape (a self-listing AggregateError lost its header in a larger draft), no change to
web_worker.rs(#38560 owns it) or to the REPL, the groundwork is its own PR, fewer tests that spawn a process, the console formatter keeps its stack check for errors, release numbers are in this description.[human-review] gate passed · iteration 2 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 2
evidence per changed file