Stop the ordered property walk when the inspect callback throws

[robobun]

Repro

const custom = Symbol.for("nodejs.util.inspect.custom");
const obj = { a: { [custom]() { throw new Error("boom"); } }, b: 1 };

Bun.inspect(obj);                    // throws "boom"
Bun.inspect(obj, { sorted: true });  // returns "{\n  a: ,\n  b: 1,\n}"

// debug / ASAN build: aborts instead of returning
Bun.A0 = { [custom]() { throw new Error("boom"); } };
Bun.inspect(Bun, { sorted: true });

ASSERTION FAILED: Unexpected exception observed on thread ...
Error Exception: boom
!exception()
ExceptionScope.h(62) : void JSC::ExceptionScope::releaseAssertNoException()

The same walk is used for every object bun test formats for a snapshot, so on the unfixed build this records { "b": ... } as the snapshot of an object whose a threw while being formatted (and toMatchInlineSnapshot() writes that into the test file), while the same getter on the property that happens to sort last fails the matcher.

Cause

JSC__JSValue__forEachPropertyOrdered (src/jsc/bindings/bindings.cpp) called iter(...) for each key and just continued to the next one (there was a // TODO: properly propagate exception upwards above the call). With the callback's exception still pending, the loop called getPropertySlot for the next key. On the Bun object that key is a lazily initialized property backed by a native callback, whose exception check fires in debug/ASAN builds. In release builds the tryClearException() that guards the walk's own getter calls swallowed the callback's exception, so the result depended on the sort position of the throwing property: only an exception from the last key reached the caller.

Fix

RETURN_IF_EXCEPTION(scope, void()) after iter(...), which is what the unordered JSC__JSValue__forEachPropertyImpl has done since #15985. Propagating is the contract the rest of the code already assumes: the function is exported as check_slow, JSValue::for_each_property_ordered checks the scope after the call, and both callers (ConsoleObject print_object and pretty_format) propagate the error. The ordered variant was the one place that did not honor it. Nothing else needed to change on the Rust side: both property callbacks already return with the exception pending, exactly like the unordered path.

Tests

test/js/bun/util/inspect.test.js

• sorted: true throws and visits no further properties, same as the unsorted walk
• the throw inside a nested object also stops the enclosing object's walk (the exception crosses two levels of the function)
• Bun.inspect.table(..., { sorted: true }), the other caller of the ordered walk
• the Bun object case above, in a subprocess (aborts on an unfixed debug build, prints returned on an unfixed release build)

test/js/bun/test/snapshot-tests/bun-snapshots.test.ts

• a value two levels down whose $$typeof getter throws fails the snapshot matcher, and neither its siblings nor the outer object's remaining properties are formatted (this exercises the callback's format() error return as well as its Tag::get one); the inline snapshot argument is what the unfixed build used to record and match (a missing), so the test never writes anything on either build

All of these fail with USE_SYSTEM_BUN=1 and pass with bun bd test. Also ran bun-inspect-table.test.ts, console-table.test.ts and snapshot-tests/ against the debug build; the two failures there (snapshots/snapshot.test.ts "error snapshots" without a color TTY, and pretty-format-overflow.test.ts exiting 139 under ASAN) reproduce identically on a build without this change.

Related, not changed here

• When formatting the received value of a failed toEqual throws, DiffFormatter goes on to format the expected value with the exception still pending and hits the same kind of assertion in debug builds. That is reachable today whenever the throwing property is the last one and is the subject of Clear pending JSC exception in DiffFormatter catch blocks #28538 / test: clear pending JS exception when diff formatting throws #29784, so the new tests use snapshot matchers rather than toEqual.
• A formatting exception inside the snapshot matchers surfaces as Failed to pretty format value: (the original error is dropped). This change makes that message consistent across properties; bun:test: throw the formatter's own error from snapshot matchers instead of "Failed to pretty format value: " #37334 makes the matchers rethrow the original error instead. The two touch the same describe block in bun-snapshots.test.ts, and once bun:test: throw the formatter's own error from snapshot matchers instead of "Failed to pretty format value: " #37334 is in, the new test here should expect "boom" instead of the wrapper message (whichever lands second picks that up).

---

no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js

[robobun]

Status

Reproduced on main (827475e): Bun.inspect({ a: <inspect.custom that throws>, b: 1 }, { sorted: true }) returns "{\n a: ,\n b: 1,\n}" on a release build while the unsorted call throws; Bun.inspect(Bun, { sorted: true }) with such a property added aborts a debug build on releaseAssertNoException. Snapshot matchers recorded the object without the throwing property.

Fix: one RETURN_IF_EXCEPTION after the callback in JSC__JSValue__forEachPropertyOrdered, matching the unordered walk. Tests in inspect.test.js and bun-snapshots.test.ts fail on the unfixed build and pass with it.

[coderabbitai]

Walkthrough

The ordered property iterator now propagates callback exceptions. Snapshot formatting and Bun.inspect tests verify that traversal stops after getter or custom inspector errors.

Changes

Inspection exception propagation

Layer / File(s)	Summary
Iterator exception handling src/jsc/bindings/bindings.cpp	JSC__JSValue__forEachPropertyOrdered now checks and propagates exceptions raised by the property callback.
Inspection regression coverage test/js/bun/test/snapshot-tests/bun-snapshots.test.ts, test/js/bun/util/inspect.test.js	Tests verify that snapshot formatting and Bun.inspect stop traversal and propagate getter or custom inspector exceptions.

Possibly related PRs

• oven-sh/bun#37330: Both changes ensure pending property-enumeration exceptions stop traversal and propagate.
• oven-sh/bun#37309: Both changes update JSC__JSValue__forEachProperty* handling and add inspection regression tests.
• oven-sh/bun#37334: Both changes address exception propagation during snapshot formatting and test nested getter failures.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly states that ordered property iteration stops when the inspection callback throws.
Description check	✅ Passed	The description explains the cause, fix, affected behavior, and verification tests, although it uses different headings from the template.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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/snapshot-tests/bun-snapshots.test.ts`:
- Around line 83-92: Update the inline snapshot assertion around the throwing
formatter in the test to assert the propagated error message is exactly "boom",
rather than only checking that an error is thrown. Preserve the existing
snapshot setup and ensure the assertion verifies the getter exception itself is
retained.

🪄 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: 6eaf2cd2-2cd4-4cee-b208-237f4482d2ad

📥 Commits

Reviewing files that changed from the base of the PR and between 827475e and 10b7103.

📒 Files selected for processing (3)

• src/jsc/bindings/bindings.cpp
• test/js/bun/test/snapshot-tests/bun-snapshots.test.ts
• test/js/bun/util/inspect.test.js

[claude]

LGTM — one-line RETURN_IF_EXCEPTION after the iter(...) callback in forEachPropertyOrdered, mirroring what the unordered walk already does at bindings.cpp:5662.

What was reviewed:

• Confirmed the early return skipping properties.releaseData() is safe: the same function already returns early at line 5741 without it, and the unordered sibling does the same at lines 5583/5662 — PropertyNameArrayBuilder is RAII-managed.
• Verified both Rust callers (ConsoleObject.rs:5463, pretty_format.rs:2349) propagate the JsResult via ?, and the wrapper (JSValue.rs:2513) checks the scope after the call.
• Tests cover both callers, sorted vs unsorted parity, the subprocess abort repro, and the snapshot matcher path; the CodeRabbit feedback on .toThrow() was addressed in 7a3aae5.

Extended reasoning...

Overview

The PR adds a single RETURN_IF_EXCEPTION(scope, void()) after the iter(...) callback in JSC__JSValue__forEachPropertyOrdered (src/jsc/bindings/bindings.cpp:5804), replacing a // TODO: properly propagate exception upwards comment. Without it, an exception thrown by the per-property callback (e.g. a nested value's inspect.custom) was left pending while the loop continued to the next key: in release builds the following iteration's tryClearException() swallowed it; in debug/ASAN builds a lazily-initialized native property's exception check aborted. Two test files add coverage: inspect.test.js for Bun.inspect/Bun.inspect.table with {sorted: true} plus a subprocess repro against the Bun object, and bun-snapshots.test.ts for the snapshot matcher path.

Security risks

None. This is an exception-propagation fix in a formatting/inspect code path. No auth, crypto, or untrusted-size arithmetic is touched.

Level of scrutiny

Low-to-medium. The native change is one line and is byte-for-byte the same pattern as the unordered sibling JSC__JSValue__forEachPropertyImpl (lines 5548, 5583, 5662), which has done this since #15985. I checked the one non-obvious concern — whether the early return leaks by skipping properties.releaseData() — and confirmed it does not: the same function already returns early without it after getOwnPropertyNames throws (line 5741), and the unordered variant has three such early returns. PropertyNameArrayBuilder is stack-allocated with a RefPtr-backed destructor. I also verified the Rust side: for_each_property_ordered in JSValue.rs wraps the FFI call in a TopExceptionScope and returns JsResult, and both call sites (ConsoleObject.rs print_object and pretty_format.rs) propagate with ?.

Other factors

The tests are strong: they assert specific error messages ("boom" for Bun.inspect, "Failed to pretty format value" for the snapshot matcher), assert exact visited/reads arrays to prove the walk stopped, cover both sorted and unsorted for parity, cover Bun.inspect.table as the second caller, and run the debug-abort repro in a subprocess with pipes drained concurrently. The inline snapshot argument is deliberately the unfixed build's output so the test never writes anything on either build. The one CodeRabbit inline comment was addressed and resolved. The PR description explicitly scopes out two adjacent issues (DiffFormatter's pending-exception assert and the wrapper message dropping the original error) as separate work, which is the right call — they are pre-existing and not part of this bug class.

[claude]

LGTM — one-line RETURN_IF_EXCEPTION after the callback in JSC__JSValue__forEachPropertyOrdered, mirroring the unordered sibling exactly.

What was reviewed

• Confirmed the added check matches forEachPropertyImpl (bindings.cpp:5659-5662) byte-for-byte, including the comment; the function is already check_slow and for_each_property_ordered in JSValue.rs already checks the scope.
• Early return skips properties.releaseData() — same as the unordered path since #15985; PropertyNameArrayData is refcounted so the destructor releases it.
• Both Rust callers (ConsoleObject.rs:5459 via ?, pretty_format.rs:2337 via captured result) propagate the error.
• Tests cover both callers, nested walks, and the debug-build abort in a subprocess; the CodeRabbit thread on the snapshot assertion is resolved.

Extended reasoning...

Overview

The PR adds a single RETURN_IF_EXCEPTION(scope, void()) after the iter(...) callback in JSC__JSValue__forEachPropertyOrdered (bindings.cpp:5802-5804), replacing a TODO: properly propagate exception upwards comment. Two test files gain regression coverage: four cases in inspect.test.js for Bun.inspect/Bun.inspect.table with sorted: true, and one case in bun-snapshots.test.ts for the snapshot matcher path.

Security risks

None. This is exception-propagation plumbing in the console/inspect formatter — no auth, crypto, parsing of untrusted input, or resource limits involved.

Level of scrutiny

Low-to-medium. The native change is one line that copies the exact pattern (same macro, same comment text) already present at bindings.cpp:5659-5662 in the unordered forEachPropertyImpl. The function is already exported as [[ZIG_EXPORT(check_slow)]] and its Rust wrapper (JSValue.rs:2510-2516) already declares a scope and calls return_if_exception() after the FFI call — so the calling contract was already in place; only the C++ side wasn't honoring it. I verified both Rust callers (ConsoleObject.rs print_object via ?, pretty_format.rs via a captured result) propagate. The early return that skips properties.releaseData() mirrors the sibling exactly; PropertyNameArrayData is a RefPtr-held type, so scope exit releases it.

Other factors

• The PR description gives a full mechanism trace (why release builds swallowed the exception via the next iteration's tryClearException(), why debug builds abort on the lazy-property native callback's exception check) and confirms USE_SYSTEM_BUN=1 fails / bun bd passes for every new test.
• Tests are non-vacuous: the visited/reads arrays assert the walk actually stopped at the throwing property (not just that something threw), and the subprocess test asserts stdout === "caught: boom\n" so it fails on both the unfixed release output (returned) and an unfixed debug abort.
• The one CodeRabbit inline comment was addressed in 7a3aae5 (tightened bare toThrow() to toThrow("Failed to pretty format value")) and the thread is resolved.
• No prior claude[bot] reviews on this PR.

[robobun]

^{Updated 6:59 PM PT - Aug 10th, 2026}

✅ @robobun, your commit b060a5ca723160824a3236525fa84e7720be61d9 passed in Build #91819! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37331

That installs a local version of the PR into your bun-37331 executable, so you can run:

bun-37331 --bun

[claude]

LGTM — one-line RETURN_IF_EXCEPTION after the callback in JSC__JSValue__forEachPropertyOrdered, mirroring what the unordered walk already does.

What was reviewed

• Confirmed the sibling unordered path (forEachPropertyImpl, bindings.cpp:5662) has the identical early-return pattern, so skipping properties.releaseData() on the exception path is established (also matches the pre-existing early return at :5741 and WebCore's JSDOMConvertRecord.h).
• Both Rust callers (ConsoleObject.rs, pretty_format.rs) already propagate via ? on for_each_property_ordered's JsResult; the function is exported check_slow, so no Rust-side changes needed.
• Tests cover both callers (Bun.inspect and Bun.inspect.table), nested propagation across two walk levels, the debug-build abort case in a subprocess, and the snapshot matcher path — with visited/reads arrays proving the walk actually stops.
• CodeRabbit's toThrow() note was addressed with the correct wrapper message; thread resolved.

Extended reasoning...

Overview

The native change is a single line in src/jsc/bindings/bindings.cpp: adding RETURN_IF_EXCEPTION(scope, void()) after the iter(...) callback in JSC__JSValue__forEachPropertyOrdered, replacing a // TODO: properly propagate exception upwards comment. This makes the ordered property walk stop when the per-property callback throws, instead of continuing to the next property with a pending exception (which either got swallowed by the next iteration's tryClearException() in release builds, or tripped a debug-build releaseAssertNoException when the next property was a lazy native getter). Two test files gain regression coverage.

Security risks

None. This is exception-handling correctness in the object formatter/inspector; no auth, crypto, network, or filesystem surface.

Level of scrutiny

Low-to-medium. The native change is mechanical: it copies the exact pattern already present in the unordered sibling JSC__JSValue__forEachPropertyImpl (bindings.cpp:5662 and :5583), which was added in #15985 for the same reason. I checked that the early return skipping properties.releaseData() is not a leak: the same skip already occurs on the pre-existing early return at line 5741 (after getOwnPropertyNames fails), on both early returns in the unordered variant, and in WebCore's JSDOMConvertRecord.h — PropertyNameArrayBuilder is a stack RAII type. On the Rust side, the function is exported as check_slow and JSValue::for_each_property_ordered returns JsResult<()>; both callers (ConsoleObject::print_object at ConsoleObject.rs:5459 and pretty_format.rs:2337) already ?-propagate, so no changes were needed there — the PR description's claim checks out.

Other factors

The tests are unusually thorough for a one-line fix: they assert exact visitation order via a visited array (so the test can only pass if the walk actually stopped, not just if some error was thrown), cover both entry points that use the ordered walk (Bun.inspect({sorted:true}) and Bun.inspect.table), cover nested propagation across two walk levels, and run the debug-abort repro in a subprocess with concurrent pipe drains and a combined stdout/stderr/exitCode assertion. The snapshot test's inline snapshot argument is deliberately what the unfixed build recorded, so it never writes on either build. The one CodeRabbit review comment (bare toThrow()) was correctly addressed with toThrow("Failed to pretty format value") — the author's explanation that snapshot matchers wrap the error (unlike Bun.inspect, which the other tests cover with toThrow("boom")) is accurate and the thread is resolved. The PR description also flags the known interaction with #37334 for whichever lands second.

[claude]

LGTM — one-line RETURN_IF_EXCEPTION after the callback in JSC__JSValue__forEachPropertyOrdered, bringing it to parity with the unordered walk.

What was reviewed

• Confirmed the identical pattern already exists at bindings.cpp:5662 in forEachPropertyImpl; the early return before properties.releaseData() matches the pre-existing exit at :5741 and the unordered path (RefPtr-backed, destructor releases).
• Verified the Rust side already honors the check_slow contract: JSValue::for_each_property_ordered checks the scope, and both callers (ConsoleObject print_object, pretty_format) propagate via ?.
• Tests cover both callers (Bun.inspect/.table and snapshot matcher), nested propagation, and the debug-build assertion via subprocess; assertions on visited/reads prove the walk stops rather than merely throwing.

Extended reasoning...

Overview

The PR adds a single RETURN_IF_EXCEPTION(scope, void()) after the iter(...) callback in JSC__JSValue__forEachPropertyOrdered (src/jsc/bindings/bindings.cpp), replacing a // TODO: properly propagate exception upwards comment. Without it, an exception raised by the per-property callback (e.g. a nested value's nodejs.util.inspect.custom throwing) was swallowed by the next iteration's tryClearException() in release builds — producing a partial formatted string — and tripped releaseAssertNoException in debug/ASAN builds when the next property was a lazily-initialized native. The unordered JSC__JSValue__forEachPropertyImpl has had this exact check since #15985 (line 5662), so this brings the ordered variant to parity. Two test files add regression coverage.

Security risks

None. This is exception-propagation plumbing in the object formatter; no untrusted input parsing, auth, crypto, or resource handling is touched.

Level of scrutiny

Low-to-moderate. The native change is one line and mechanically mirrors the sibling function. I checked the one thing that could differ from a pure copy: the early return skips properties.releaseData(). That is not a leak — the same early return already exists in this function at line 5741 (after getOwnPropertyNames throws) and in the unordered variant at line 5662; PropertyNameArrayBuilder holds its data via RefPtr and releases in its destructor. On the Rust side, the function is exported check_slow, for_each_property_ordered wraps it in a TopExceptionScope and returns JsResult, and both callers (ConsoleObject.rs:5459, pretty_format.rs:2349) propagate with ?, so nothing else needed to change.

Other factors

Tests are thorough for a one-line fix: they exercise both Rust callers (Bun.inspect with sorted: true, Bun.inspect.table, and the snapshot matcher's pretty_format), assert the walk stops via a visited/reads array (not just that something threw), cover the nested case where the exception must cross two levels of the loop, and reproduce the debug-build abort in a subprocess. The CodeRabbit suggestion to assert "boom" in the snapshot test was correctly declined (the matcher wraps the error) and the assertion tightened to "Failed to pretty format value" instead; that thread is resolved. The PR description explicitly names the related-but-unchanged DiffFormatter issue and the interaction with #37334, so scope is clear.