Conversation
When a user rethrows an existing Error — e.g. from inside
`process.on('uncaughtException')` — JSC wraps the value in a fresh
`JSC::Exception` whose stack points at the `throw err` site rather
than the original throw site. In `fromErrorInstance` (and the
companion source-line collector) we were preferring that outer
wrapper stack, which hid the original stack stored on the Error
instance.
Swap the precedence so the Error's own `stackTrace()` wins. This
matches Node's behavior of using `err.stack` for the rethrown
Error and preserves context like the enclosing function name in
the reported trace.
Fixes #30504
|
Updated 12:41 PM PT - May 11th, 2026
❌ @robobun, your commit 5398dff has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30508That installs a local version of the PR into your bun-30508 --bun |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR prefers an ErrorInstance's own non-empty stackTrace() over a wrapper stack when building exception frames and source lines, preserving original throw sites when errors are rethrown; a test verifies uncaughtException rethrow preserves the original stack entry. ChangesError Stack Trace Preservation on Rethrow
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 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/node/process/process.test.js`:
- Around line 845-850: The test currently asserts exitCode before validating
stderr; change the assertion order so you first await and assert stderr contents
(checks using proc.stderr.text(), expecting "throwUncaughtError" and "Boom") and
only after those stderr expectations assert the exit code (exitCode === 7).
Update the block that assigns const [stderr, exitCode] = await
Promise.all([proc.stderr.text(), proc.exited]) so the
expect(stderr).toContain(...) checks come before expect(exitCode).toBe(7),
preserving the existing proc, stderr, and exitCode variables and behavior.
🪄 Autofix (Beta)
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: e645f49e-efce-4fd0-bb39-c2e62369250e
📒 Files selected for processing (2)
src/jsc/bindings/ZigException.cpptest/js/node/process/process.test.js
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| expect(exitCode).toBe(7); | ||
| // Stack must reference the original throw site inside throwUncaughtError, | ||
| // not only the `throw err` rethrow site inside the handler. | ||
| expect(stderr).toContain("throwUncaughtError"); | ||
| expect(stderr).toContain("Boom"); |
There was a problem hiding this comment.
🟡 nit: per CLAUDE.md ("Assert the exit code last"), move expect(exitCode).toBe(7) after the two expect(stderr).toContain(...) checks so a failure prints the actual stderr instead of just "Expected: 7". The neighboring tests have the same pre-existing ordering, but new code might as well follow the documented convention.
Extended reasoning...
What
The new test at test/js/node/process/process.test.js:846 asserts expect(exitCode).toBe(7) before the two stderr-content assertions:
const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);
expect(exitCode).toBe(7);
// Stack must reference the original throw site inside throwUncaughtError,
// not only the `throw err` rethrow site inside the handler.
expect(stderr).toContain("throwUncaughtError");
expect(stderr).toContain("Boom");The repo's root CLAUDE.md (Writing Tests, lines ~119/128) explicitly states:
When spawning processes, tests should expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0). This gives you a more useful error message on test failure.
Assert the exit code last.
Why it matters
If this test fails for any reason — say a future change makes the spawned process crash with a different exit code, or the rethrow path regresses in a way that also changes the exit status — the first failing assertion is what gets reported. With expect(exitCode).toBe(7) first, the failure message would be the relatively uninformative "Expected: 7, Received: " and the test would stop before printing what was actually written to stderr. Asserting the stderr content first means the diff shows the printed stack trace, which is exactly the thing this test exists to validate.
Step-by-step
- Suppose a regression causes the child to exit with code 1 and print a different stack to stderr.
Promise.allresolves withstderr = "<some error output>",exitCode = 1.- Line 846 runs first:
expect(1).toBe(7)→ fails with "Expected: 7, Received: 1". - The test stops; the
stderrcontent (the diagnostic you actually want) is never asserted or shown. - With the order swapped,
expect(stderr).toContain("throwUncaughtError")runs first and the failure message includes the full received stderr string, immediately showing what the stack actually looked like.
Why existing code doesn't prevent it
Nothing in the test file enforces assertion order. The immediately preceding tests ("aborts when the uncaughtException handler throws", lines ~808–823) use the same exit-code-first pattern, so this PR is following local style — but those are pre-existing and the documented project convention is unambiguous that exit code goes last.
Fix
Reorder the three assertions:
expect(stderr).toContain("throwUncaughtError");
expect(stderr).toContain("Boom");
expect(exitCode).toBe(7);This is purely a test-ergonomics nit; it does not affect correctness or coverage.
Error stack traces now come from the ErrorInstance's own stack (captured at `new Error()` construction) rather than the outer JSC::Exception wrapper (captured at the `throw` opcode). The reported column shifts from the end of the throw expression to the Error constructor call site.
…close test timeout)
|
Related: #36437 handles the same precedence swap plus the case where |
Adds the process-level cases from #30508 to process.test.js: a handler that rethrows, and one that reads err.stack before rethrowing, both must report the function that created the Error rather than the handler.
|
Consolidated into #36437, which prefers the Error's own stack the same way this PR does and additionally falls back to the |
1 participant
Fixes #30504.
Repro
Before
Bun points at the rethrow site (line 4) and drops every frame from the original throw.
After (and Node's behavior)
Cause
When the listener does
throw err, JSC wraps the pre-existing Error in afresh
JSC::Exceptionthat captures a new stack at the rethrow site.fromErrorInstance(src/jsc/bindings/ZigException.cpp) preferred thatwrapper stack over the Error's own
stackTrace(), so the originalframes captured at
new Error(...)construction were thrown away.Fix
Swap the precedence in
fromErrorInstanceand in the OnlySourceLinespath of
ZigException__collectSourceLinesso the Error's own stackwins. The wrapper stack is still used as a fallback when the Error has
none (rare — happens with synthetic rethrows of non-Error values, etc.).
Verification
test/js/node/process/process.test.js> "preserves theoriginal Error stack when the uncaughtException handler rethrows".
Fails on main (
Received: ... at <anonymous> (.../index.cjs:4:17)");passes on this branch.
process.test.js-t uncaughtException): 5/5 pass.test/regression/issue/08794.test.ts,circular-error-stack.test.ts,circular-error-stack-edge-cases.test.ts,23022-stack-trace-iterator.test.ts,fix-bindings-stack-trace.test.ts,prepare-stack-trace-crash.test.ts: all pass.test/js/bun/test/stack.test.ts,test/js/bun/sourcemap/internal-sourcemap.test.ts: all pass.test-events-uncaught-exception-stack.js,test-exception-handler.js,test-exception-handler2.js,test-emit-after-uncaught-exception.jsall exit 0.