Clear pending JSC exception in DiffFormatter catch blocks#28538
Clear pending JSC exception in DiffFormatter catch blocks#28538
Conversation
|
Updated 5:05 PM PT - Mar 25th, 2026
❌ @Jarred-Sumner, your commit fa6b2da has some failures in 🧪 To try this PR locally: bunx bun-pr 28538That installs a local version of the PR into your bun-28538 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughHandle exceptions in diff formatting by clearing JS exceptions instead of swallowing them; add two regression tests ensuring overridden RegExp.toString (throwing or returning non-primitive) doesn't crash; modify Windows glob walker to apply NT filename pre-filter only for single-index active sets. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/test/diff_format.zig`:
- Around line 46-48: The boolean return value of
this.globalThis.clearExceptionExceptTermination() is currently ignored; change
both call sites to capture the result and return early (abort the current
formatting function) when it returns false so a pending termination exception
stops further buffer handling and formatting; specifically, after calling
this.globalThis.clearExceptionExceptTermination() at each location, if the
result is false then immediately return from the enclosing function (or
otherwise stop processing) before continuing to use the buffers.
In `@test/js/bun/test/expect-toEqual-regex-toString-crash.test.ts`:
- Around line 10-13: The child script swallows the matcher error but doesn't
assert it happened; update the test around the regex `re` and the
`Bun.jest().expect(re).toEqual({})` call to set a `threw` flag when the expected
throw occurs and, after the try/catch, call `process.exit(1)` if `threw` is
false so the child process fails when `toEqual` stops throwing; apply the same
pattern to the other script block referenced (lines 29-32) using the same
`threw` guard and `process.exit(1)` fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 088f27f0-6a51-4711-a7fc-fdae1710443c
📒 Files selected for processing (2)
src/bun.js/test/diff_format.zigtest/js/bun/test/expect-toEqual-regex-toString-crash.test.ts
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted crash fix with clear reasoning and regression coverage.
Extended reasoning...
Overview
This PR modifies src/bun.js/test/diff_format.zig (2 catch blocks) and adds a new test file reproducing two crash scenarios. The change is minimal: two catch {} blocks that previously swallowed Zig errors now also call clearExceptionExceptTermination() to discard the pending JSC exception before execution continues.
Security Risks
No security concerns. The change only affects error recovery behavior in test diff formatting, clearing a VM exception state that was already being partially handled. It does not affect auth, crypto, permissions, or any production-critical path.
Level of Scrutiny
Low. The fix is surgical — two identical one-liner additions inside existing catch blocks. The pattern of calling clearExceptionExceptTermination() after a JSC exception is standard. The description accurately identifies why the assertion failure occurred (exception pending when the next JSC operation ran), and the fix directly addresses that cause.
Other Factors
The bug report flags a nit: the two new tests spawn independent subprocesses but do not use test.concurrent. This is consistent with the project guideline in test/CLAUDE.md, but it is a style issue and does not affect correctness or block the fix. The regression tests themselves correctly verify the crash scenarios.
| test("expect.toEqual does not crash when regex has overridden toString", async () => { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| ` | ||
| const re = /abc/; | ||
| Object.defineProperty(re, "toString", { value: () => { throw new Error("nope"); } }); | ||
| try { Bun.jest().expect(re).toEqual({}); } catch {} | ||
| `, | ||
| ], | ||
| env: bunEnv, | ||
| }); | ||
|
|
||
| const exitCode = await proc.exited; | ||
|
|
||
| expect(exitCode).toBe(0); |
There was a problem hiding this comment.
🟡 Both tests spawn independent Bun subprocesses with no shared state but do not use test.concurrent, violating the project guideline in test/CLAUDE.md. Converting them to test.concurrent would allow them to run in parallel and reduce test suite runtime.
Extended reasoning...
Tests Should Use test.concurrent
The project has an explicit guideline in test/CLAUDE.md: "Prefer concurrent tests over sequential tests: When multiple tests in the same file spawn processes or write files, make them concurrent with test.concurrent or describe.concurrent unless it is very difficult to make them concurrent."
Both tests in expect-toEqual-regex-toString-crash.test.ts spawn independent Bun subprocess processes using Bun.spawn() with -e eval flags. Each test awaits its own proc.exited, checks the exit code, and has zero interaction with the other test — there is no shared state, no shared files, no shared resources of any kind.
The tests are trivially parallelizable. The fix is straightforward: change both test(...) calls to test.concurrent(...). This is a one-line change per test and would allow both subprocesses to be launched concurrently, reducing wall-clock time for this file.
Step-by-step proof of parallelizability:
- Test 1 calls
Bun.spawn(...)→ launches subprocess A → awaitsproc.exited→ checksexitCode === 0 - Test 2 calls
Bun.spawn(...)→ launches subprocess B → awaitsproc.exited→ checksexitCode === 0 - Subprocess A and subprocess B are entirely separate OS processes with no IPC, no shared files, and no shared memory.
- There is nothing in either test that could cause a race condition or interference if run concurrently.
Addressing the refutation: One verifier argued this is a style concern rather than a code bug and does not rise to blocking severity. This is correct — the tests are functionally correct as written. However, the project has an explicit documented convention for exactly this scenario, and the tests clearly fall under it. This is filed as nit severity: it does not affect correctness or block the fix from landing, but it should be addressed to stay consistent with project conventions.
robobun
force-pushed
the
farm/8e3ba937/fix-diff-format-exception
branch
from
0226146 to
29b8c83
Compare
robobun
force-pushed
the
farm/8e3ba937/fix-diff-format-exception
branch
from
29b8c83 to
c5a59d8
Compare
|
CI failures are all from |
There was a problem hiding this comment.
LGTM — the fix correctly handles both the regular-exception case (clear and continue) and the termination case (propagate error.WriteFailed), and the regression tests validate the fix.
Extended reasoning...
Overview
This PR modifies src/bun.js/test/diff_format.zig to replace empty catch {} blocks with proper JSC exception clearing, and adds a new regression test file for the crash scenario.
Security risks
None. The change only affects error-handling paths inside test diff formatting; there is no security-sensitive code involved.
Level of scrutiny
Low-to-medium. The fix is targeted and mechanical: two catch {} blocks that previously swallowed Zig errors without clearing the pending JSC exception are replaced with calls to clearExceptionExceptTermination(). When that returns true (regular exception cleared), execution continues normally. When it returns false (termination exception, not clearable), error.WriteFailed is returned, which propagates as a Zig error through the format writer so the caller short-circuits rather than attempting to throw a new JS exception on top of a live termination exception.
Other factors
The two prior concerns I raised have been addressed: (1) coderabbitai suggested checking the return value — the current diff uses return error.WriteFailed rather than the bare return; I flagged in my 🔴 comment; (2) the tests include the threw guard and process.exit(1) fallback. The remaining nits (missing stderr/stdout piping in spawned processes, and absence of test.concurrent) do not affect correctness and are minor enough not to block approval.
| test("expect.toEqual does not crash when regex has overridden toString", async () => { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| ` | ||
| const re = /abc/; | ||
| Object.defineProperty(re, "toString", { value: () => { throw new Error("nope"); } }); | ||
| let threw = false; | ||
| try { Bun.jest().expect(re).toEqual({}); } catch { threw = true; } | ||
| if (!threw) process.exit(1); | ||
| `, | ||
| ], | ||
| env: bunEnv, | ||
| }); | ||
|
|
||
| const exitCode = await proc.exited; | ||
|
|
||
| expect(exitCode).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🟡 The two spawned subprocesses omit stderr: "pipe" and stdout: "pipe", so any crash output or error messages are not captured. When a test fails, the only information available is the exit code, with no diagnostic context about what went wrong.
Extended reasoning...
Both Bun.spawn calls in expect-toEqual-regex-toString-crash.test.ts (lines 4-23 and 24-43) use only cmd and env options, omitting stderr: "pipe" and stdout: "pipe". The project documents an expected pattern in test/CLAUDE.md that requires piping both streams, collecting their output alongside proc.exited, and asserting expect(stderr).toBe("") before expect(exitCode).toBe(0).
The specific code path affected: when either test fails (non-zero exit code), the test runner reports expected 1 to be 0 with no additional context. Any panic message, stack trace, or assertion failure printed to stderr by the subprocess is silently discarded — it goes to the terminal output of the test runner process itself, not captured anywhere the test can inspect or report.
The existing test infrastructure does not provide any fallback capture mechanism. Without stderr: "pipe", there is no way to distinguish between "process crashed with SIGSEGV" and "process exited 1 for an unrelated reason." Similar crash regression tests in the same directory (e.g., pretty-format-overflow.test.ts) pipe both streams and check for specific crash markers like panic, integer overflow, and SIGTRAP.
The refutation noted that test/js/bun/test/expect-symbol-toPrimitive-crash.test.ts uses the identical pattern (no stderr/stdout piping). This is accurate — the pattern does exist in the codebase. However, the presence of an existing non-conforming test does not override the documented project convention. Both files should be updated to conform to test/CLAUDE.md; this PR is an opportunity to introduce the pattern correctly.
Impact is limited to developer experience: the tests correctly detect crashes via non-zero exit codes, so there is no false-pass risk. However, when a failure does occur (e.g., during a refactor or port to a new platform), engineers will see only expected 1 to be 0 with no crash context, making triage significantly harder. This matches the documented rationale for the project convention.
Step-by-step proof: (1) Subprocess panics or prints an assertion failure to stderr. (2) Because stderr is not piped, the output goes directly to the test runner's inherited stderr file descriptor. (3) The test awaits proc.exited and checks expect(exitCode).toBe(0). (4) The failure message is expected 1 to be 0 — no stderr content is available to the test, no diagnostic output is printed by the test framework. (5) A developer investigating the failure must re-run the subprocess manually with the -e flag to see what it printed.
robobun
force-pushed
the
farm/8e3ba937/fix-diff-format-exception
branch
from
c5a59d8 to
858c248
Compare
There was a problem hiding this comment.
LGTM — the termination case now correctly propagates error.WriteFailed instead of returning void, and the tests include the threw guard. Outstanding nits are minor and already documented in prior inline comments.
Extended reasoning...
Overview
This PR modifies two files: src/bun.js/test/diff_format.zig (two catch blocks in DiffFormatter.format) and a new regression test file. The change is narrowly scoped to exception-clearing logic during test diff formatting.
Security risks
None. This touches test infrastructure diff formatting, not auth, crypto, or permissions.
Level of scrutiny
Low-to-medium. The Zig change is small and targeted: two catch blocks that previously silently swallowed errors now call clearExceptionExceptTermination() and return error.WriteFailed if a termination exception is pending. Returning an error (rather than void) is the correct pattern; it propagates through the writer so the caller falls through its own error path rather than proceeding with a live termination exception on the VM and hitting assertNoException().
Other factors
The issues I raised in previous rounds have been addressed: the earlier revision used a bare return; (void) when termination was pending, which left the exception live; the current code returns error.WriteFailed instead. CodeRabbit's threw-flag and return-value suggestions are also incorporated. My two remaining nit-level comments (test.concurrent usage, missing stderr/stdout piping) are already on the PR and do not affect correctness.
robobun
force-pushed
the
farm/8e3ba937/fix-diff-format-exception
branch
from
858c248 to
c279dfc
Compare
There was a problem hiding this comment.
LGTM — the return error.WriteFailed on the termination path addresses the concern from my earlier inline comment; the fix correctly propagates a Zig error rather than silently returning void.
Extended reasoning...
Overview
Two files changed: src/bun.js/test/diff_format.zig (the core fix) and a new regression test file. The fix targets the two catch {} blocks in DiffFormatter.format that previously swallowed both the Zig error and the pending JSC exception, replacing them with clearExceptionExceptTermination() calls that properly clear regular JS exceptions (returning true, execution continues) or signal a pending termination exception by returning error.WriteFailed (returning false, propagates as Zig error).
Security Risks
No security-sensitive code touched. This is entirely within the test runner diff formatting path. No auth, crypto, or permissions code involved.
Level of Scrutiny
Low-to-medium. The change is small, targeted, and well-understood: two identical catch blocks get the same treatment. The logic is straightforward — clear the exception if it is clearable, otherwise propagate an error. The regression tests cover both crash scenarios (throwing toString and non-primitive-returning toString).
Other Factors
My inline comment from the 04:10 run raised the concern that return; (void) would silently succeed and allow throwPretty to proceed with a live termination exception. The current code uses return error.WriteFailed; which propagates as a Zig error through the std.Io.Writer.Error!void return type, causing the caller to see an error rather than success. This resolves that concern. The remaining open inline comments (test.concurrent, stderr piping) are style nits with no impact on correctness or crash prevention.
robobun
force-pushed
the
farm/8e3ba937/fix-diff-format-exception
branch
2 times, most recently
from
e1c8335 to
5efc066
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/test/expect-toEqual-regex-toString-crash.test.ts`:
- Line 4: Two independent Bun spawn tests (including the one named
"expect.toEqual does not crash when regex has overridden toString") should use
test.concurrent instead of test so they run in parallel; update both test
declarations (the test(...) calls for these two spawn tests) to
test.concurrent(...) preserving test names and bodies so each spawns
independently and asserts exit codes concurrently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2223f2e8-6960-4766-bb1a-5e82126669f7
📒 Files selected for processing (3)
src/bun.js/test/diff_format.zigsrc/glob/GlobWalker.zigtest/js/bun/test/expect-toEqual-regex-toString-crash.test.ts
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe } from "harness"; | ||
|
|
||
| test("expect.toEqual does not crash when regex has overridden toString", async () => { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether this file uses concurrent test declarations for both spawn tests.
rg -n '^\s*test(\.concurrent)?\(' test/js/bun/test/expect-toEqual-regex-toString-crash.test.tsRepository: oven-sh/bun
Length of output: 240
🏁 Script executed:
cat -n test/js/bun/test/expect-toEqual-regex-toString-crash.test.tsRepository: oven-sh/bun
Length of output: 1481
Use test.concurrent for these two independent spawn tests.
Both tests spawn independent Bun processes and check exit codes. Per repo test conventions, these should run concurrently.
♻️ Suggested change
-test("expect.toEqual does not crash when regex has overridden toString", async () => {
+test.concurrent("expect.toEqual does not crash when regex has overridden toString", async () => {
@@
-test("expect.toEqual does not crash when regex toString returns non-primitive", async () => {
+test.concurrent("expect.toEqual does not crash when regex toString returns non-primitive", async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/test/expect-toEqual-regex-toString-crash.test.ts` at line 4, Two
independent Bun spawn tests (including the one named "expect.toEqual does not
crash when regex has overridden toString") should use test.concurrent instead of
test so they run in parallel; update both test declarations (the test(...) calls
for these two spawn tests) to test.concurrent(...) preserving test names and
bodies so each spawns independently and asserts exit codes concurrently.
2e1a068 to
0f45bc5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/js/bun/test/expect-toEqual-regex-toString-crash.test.ts (2)
4-4: 🧹 Nitpick | 🔵 TrivialPrefer
test.concurrentfor these independent spawn tests.These two tests are independent and process-spawning; running them concurrently matches repo test preference.
♻️ Suggested change
-test("expect.toEqual does not crash when regex has overridden toString", async () => { +test.concurrent("expect.toEqual does not crash when regex has overridden toString", async () => { @@ -test("expect.toEqual does not crash when regex toString returns non-primitive", async () => { +test.concurrent("expect.toEqual does not crash when regex toString returns non-primitive", async () => {#!/bin/bash # Verify current test declaration mode in this file rg -n '^\s*test(\.concurrent)?\(' test/js/bun/test/expect-toEqual-regex-toString-crash.test.tsBased on learnings: "Prefer concurrent tests over sequential tests using
test.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files."Also applies to: 25-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/bun/test/expect-toEqual-regex-toString-crash.test.ts` at line 4, Change the two independent process-spawning tests from using test(...) to test.concurrent(...); specifically update the test declaration for the "expect.toEqual does not crash when regex has overridden toString" test (and the other similar test in the same file) so they run concurrently to match repo preference for spawn/file-writing tests.
10-15:⚠️ Potential issue | 🟠 MajorGuard against false passes when
toEqualunexpectedly stops throwing.Both child scripts only validate inside
catch. IfBun.jest().expect(re).toEqual({})no longer throws, the child exits 0 and the test passes incorrectly.♻️ Suggested fix
const re = /abc/; Object.defineProperty(re, "toString", { value: () => { throw new Error("nope"); } }); - try { Bun.jest().expect(re).toEqual({}); } catch (e) { - if (!e.message.includes("Expected: {}")) process.exit(1); - } + let threw = false; + try { Bun.jest().expect(re).toEqual({}); } catch (e) { + threw = true; + if (!(e instanceof Error) || !e.message.includes("Expected: {}")) process.exit(1); + } + if (!threw) process.exit(1); @@ const re = /abc/; Object.defineProperty(re, "toString", { value: () => Array }); - try { Bun.jest().expect(re).toEqual({}); } catch (e) { - if (!e.message.includes("Expected: {}")) process.exit(1); - } + let threw = false; + try { Bun.jest().expect(re).toEqual({}); } catch (e) { + threw = true; + if (!(e instanceof Error) || !e.message.includes("Expected: {}")) process.exit(1); + } + if (!threw) process.exit(1);Based on learnings: in Bun regression-style spawned-process tests, a
process.exit(1)fallback after expected-throw paths is an intentional safety net.Also applies to: 31-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/bun/test/expect-toEqual-regex-toString-crash.test.ts` around lines 10 - 15, The test currently only asserts inside the catch block so if Bun.jest().expect(re).toEqual({}) stops throwing the child will exit 0 and the test will false-pass; after the try { Bun.jest().expect(re).toEqual({}) } catch (e) { ... } block add a fallback process.exit(1) immediately following the try so the child fails when no exception is thrown (apply the same change to the other sibling block around lines referencing the same pattern, e.g., the second instance at 31-36).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/js/bun/test/expect-toEqual-regex-toString-crash.test.ts`:
- Line 4: Change the two independent process-spawning tests from using test(...)
to test.concurrent(...); specifically update the test declaration for the
"expect.toEqual does not crash when regex has overridden toString" test (and the
other similar test in the same file) so they run concurrently to match repo
preference for spawn/file-writing tests.
- Around line 10-15: The test currently only asserts inside the catch block so
if Bun.jest().expect(re).toEqual({}) stops throwing the child will exit 0 and
the test will false-pass; after the try { Bun.jest().expect(re).toEqual({}) }
catch (e) { ... } block add a fallback process.exit(1) immediately following the
try so the child fails when no exception is thrown (apply the same change to the
other sibling block around lines referencing the same pattern, e.g., the second
instance at 31-36).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6bdbe776-cb56-4f51-b7d5-e2a3e0017428
📒 Files selected for processing (3)
src/bun.js/test/diff_format.zigsrc/glob/GlobWalker.zigtest/js/bun/test/expect-toEqual-regex-toString-crash.test.ts
robobun
force-pushed
the
farm/8e3ba937/fix-diff-format-exception
branch
from
0f45bc5 to
d5678de
Compare
robobun
force-pushed
the
farm/8e3ba937/fix-diff-format-exception
branch
2 times, most recently
from
98be44a to
1e2eb6b
Compare
There was a problem hiding this comment.
LGTM — the termination path now returns error.WriteFailed (not void), the threw guard is in place, and no bugs were found.
Extended reasoning...
Overview
Two files changed: src/bun.js/test/diff_format.zig (the core fix) and a new regression test file. The fix replaces empty catch blocks in DiffFormatter.format with calls to clearExceptionExceptTermination(), returning error.WriteFailed when a termination exception is detected.
Security risks
None. This is entirely internal to the test-diff formatting path; it handles exception state cleanup in the JSC VM and has no external attack surface.
Level of scrutiny
Moderate. The change is small and self-contained, but it touches JSC exception-state management. My prior runs verified the call chain through throwPretty and confirmed that return error.WriteFailed is the correct response when the termination exception cannot be cleared — it propagates as a Zig error, causing the caller own exception handler to engage rather than hitting scope.assertNoException() with a live termination exception.
Other factors
The two issues raised in prior review rounds were both addressed in commit 29b8c83: (1) the termination branch now returns error.WriteFailed rather than silently returning void, and (2) the subprocess test scripts include a threw guard with process.exit(1) fallback. The CI failures noted in the timeline are a pre-existing compile error from an unrelated PR on main, not caused by this PR. Outstanding nits (test.concurrent, stderr piping) are style preferences and do not affect correctness.
When toEqual fails and formats a diff message, JestPrettyFormat may
call toZigString on a RegExp with an overridden toString that throws or
returns a non-primitive. The catch {} blocks in DiffFormatter.format
swallowed the Zig error but left the JSC exception pending on the VM,
causing an assertion failure when the code continued to create the
matcher error.
Return error.WriteFailed on termination exceptions so the caller
short-circuits instead of attempting to throw with a pending exception.
Also fix undeclared component_idx in GlobWalker.zig (pre-existing on main).
robobun
force-pushed
the
farm/8e3ba937/fix-diff-format-exception
branch
from
1e2eb6b to
37fa01f
Compare
There was a problem hiding this comment.
LGTM — the fix is correct and the previous major concerns have been addressed.
Extended reasoning...
Overview
This PR modifies two catch blocks in DiffFormatter.format (src/bun.js/test/diff_format.zig) to call clearExceptionExceptTermination() instead of silently swallowing exceptions, and adds two regression tests in test/js/bun/test/expect-toEqual-regex-toString-crash.test.ts.
Security Risks
No security implications. This is purely a VM exception state fix in the test framework diff formatter.
Level of Scrutiny
This is a focused bug fix in test infrastructure (not production code paths). The change is small — two catch blocks plus a new test file. The fix pattern (clearExceptionExceptTermination() returning an error on termination) matches how similar call sites are handled elsewhere in the codebase.
Other Factors
Earlier iterations of this PR had two correctness issues that I flagged: (1) returning void instead of error.WriteFailed when termination is pending, and (2) missing a threw guard in the subprocess test scripts. Both have been addressed in the current code. Two nit-level style issues remain open in my prior inline comments (test.concurrent and stderr: "pipe") but these do not affect correctness. The bug hunting system found no bugs, and the critical exception-handling path is now correct.
There was a problem hiding this comment.
LGTM — the fix correctly clears pending JSC exceptions in both catch blocks and returns error.WriteFailed on termination, and the regression tests include proper threw guards.
Extended reasoning...
Overview
This PR modifies src/bun.js/test/diff_format.zig to replace empty catch {} blocks in DiffFormatter.format with calls to clearExceptionExceptTermination(), propagating error.WriteFailed when a termination exception is pending. It also adds a new regression test file with two spawned-process tests covering the two crash scenarios.
Security risks
None. This is internal test infrastructure code that formats diff output when a test assertion fails. No user-facing APIs or security-sensitive code paths are involved.
Level of scrutiny
Low. The change is surgical and well-scoped: two catch blocks in a single formatting function, plus regression tests. The logic is straightforward — clear a JS exception and return an error if the VM is in a termination state so callers can short-circuit rather than proceeding to throw a new JS exception with a live termination exception still pending.
Other factors
All substantive issues raised in the review thread have been addressed: the return value of clearExceptionExceptTermination() is checked (returning error.WriteFailed rather than void), and the test scripts include threw guards with process.exit(1) fallbacks. The remaining open items (test.concurrent and stderr/stdout piping) are documented nits that do not affect correctness. CI failures noted in the thread are unrelated pre-existing compile errors from a different PR.
2 participants
When
toEqualfails and formats a diff message,JestPrettyFormatcallstoZigStringon values to stringify them for the diff output. For a RegExp with an overriddentoStringthat throws or returns a non-primitive (like a constructor), JSC sets a pending exception on the VM. Thecatch {}blocks inDiffFormatter.formatswallowed the Zig error but left the JSC exception pending, causing an assertion failure when the code proceeded to create the matcher error.Fix: call
clearExceptionExceptTermination()in the catch blocks so the pending JSC exception is properly cleared before continuing to format the matcher error message.Repro: