jsc: report exceptions thrown by deferred work tasks#30849
Closed
robobun wants to merge 2 commits into
Closed
Conversation
Bun overrides JSC's DeferredWorkTimer hooks and runs scheduled tasks (FinalizationRegistry cleanup, Atomics.waitAsync, etc.) via runPendingWork() in JSCTaskScheduler.cpp. Unlike JSC's stock DeferredWorkTimer::doWork(), this did not catch exceptions thrown by the task, so a throwing FinalizationRegistry cleanup callback left a pending exception on the VM which tripped releaseAssertNoException() in the event loop's validation scope. Match JSC's behavior: wrap the task in a top exception scope, clear any non-termination exception and route it through reportUncaughtExceptionAtEventLoop so it reaches process 'uncaughtException' handlers instead of aborting.
Contributor
WalkthroughThis PR adds exception scope handling for deferred JSC work, introduces a FinalizationRegistry exception test, refactors embedded file resolution, and reformats platform-specific conditional compilation attributes for consistency across multiple files. ChangesException handling and file resolution improvements
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Collaborator
Author
|
Updated 10:18 AM PT - May 15th, 2026
❌ @autofix-ci[bot], your commit 8815ee1 has 16 failures in
🧪 To try this PR locally: bunx bun-pr 30849That installs a local version of the PR into your bun-30849 --bun |
Contributor
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/js/bun/jsc/finalization-registry-throw.test.ts`:
- Around line 38-46: Move the exit code assertion to the end of each subprocess
test so failures show process output first: in the block where you await
Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) and assign
stdout, stderr, exitCode, delay the line expect(exitCode).toBe(0) until after
all other expectations (e.g., after the stderr assertions and the stdout "CAUGHT
from cleanup callback" check); apply the same change to the second subprocess
test around the other Promise.all usage (the block referenced by stdout, stderr,
exitCode later in the file).
- Around line 39-40: Remove the brittle negative stderr checks by deleting the
two expect calls expect(stderr).not.toContain("ASSERTION FAILED") and
expect(stderr).not.toContain("releaseAssertNoException") (and the duplicate pair
referenced at 72-73) from finalization-registry-throw.test.ts; instead assert
the concrete expected behavior for the test (e.g., existing exit/signal
expectations or expected stdout content) using the existing test harness
assertions so the test verifies positive outcomes rather than the absence of
panic strings.
🪄 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: c3edaf5b-2ab7-4cfd-af56-e7200523dad0
📒 Files selected for processing (12)
src/crash_handler/lib.rssrc/errno/lib.rssrc/jsc/bindings/JSCTaskScheduler.cppsrc/perf/tracy.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/run_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/jsc_hooks.rssrc/runtime/webview/ChromeProcess.rssrc/spawn/process.rssrc/spawn_sys/spawn_process.rstest/js/bun/jsc/finalization-registry-throw.test.ts
Comment on lines
+38
to
+46
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).not.toContain("ASSERTION FAILED"); | ||
| expect(stderr).not.toContain("releaseAssertNoException"); | ||
| expect(exitCode).toBe(0); | ||
| // GC timing is non-deterministic; if the callback ran it must have been | ||
| // routed through uncaughtException rather than crashing the process. | ||
| if (!stdout.includes("SKIPPED")) { | ||
| expect(stdout).toContain("CAUGHT from cleanup callback"); | ||
| } |
Contributor
There was a problem hiding this comment.
Assert exitCode last in both subprocess tests.
expect(exitCode) is currently not the last assertion in either test, which weakens failure diagnostics for spawned-process failures.
As per coding guidelines: "Assert the exit code last in tests - this gives a more useful error message on test failure."
Also applies to: 71-77
🤖 Prompt for 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.
In `@test/js/bun/jsc/finalization-registry-throw.test.ts` around lines 38 - 46,
Move the exit code assertion to the end of each subprocess test so failures show
process output first: in the block where you await
Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) and assign
stdout, stderr, exitCode, delay the line expect(exitCode).toBe(0) until after
all other expectations (e.g., after the stderr assertions and the stdout "CAUGHT
from cleanup callback" check); apply the same change to the second subprocess
test around the other Promise.all usage (the block referenced by stdout, stderr,
exitCode later in the file).
Comment on lines
+39
to
+40
| expect(stderr).not.toContain("ASSERTION FAILED"); | ||
| expect(stderr).not.toContain("releaseAssertNoException"); |
Contributor
There was a problem hiding this comment.
Remove negative checks for assertion/panic-style stderr strings.
These output-string negation checks are disallowed and brittle here; rely on behavior/assertions that must hold (signal/exit/expected stdout path) instead.
As per coding guidelines: "Never write tests that check for no 'panic' or 'uncaught exception' or similar in output - these will never fail in CI."
Also applies to: 72-73
🤖 Prompt for 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.
In `@test/js/bun/jsc/finalization-registry-throw.test.ts` around lines 39 - 40,
Remove the brittle negative stderr checks by deleting the two expect calls
expect(stderr).not.toContain("ASSERTION FAILED") and
expect(stderr).not.toContain("releaseAssertNoException") (and the duplicate pair
referenced at 72-73) from finalization-registry-throw.test.ts; instead assert
the concrete expected behavior for the test (e.g., existing exit/signal
expectations or expected stdout content) using the existing test harness
assertions so the test verifies positive outcomes rather than the absence of
panic strings.
Collaborator
Author
|
Duplicate of #30835 (same fix, submitted earlier, CI passing). |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Comment on lines
+39
to
+40
| expect(stderr).not.toContain("ASSERTION FAILED"); | ||
| expect(stderr).not.toContain("releaseAssertNoException"); |
Contributor
There was a problem hiding this comment.
🟡 nit: Per root CLAUDE.md ("Writing Tests"), tests should never assert that stderr does not contain "panic"/"uncaught exception"/assertion strings — these checks never fail in CI. The expect(stderr).not.toContain("ASSERTION FAILED") / "releaseAssertNoException" lines here and at lines 72-73 can be dropped; expect(exitCode).toBe(0) and expect(proc.signalCode).toBeNull() already catch the regression (the release assert aborts via SIGABRT).
Extended reasoning...
What
Root CLAUDE.md, Writing Tests section, states:
NEVER write tests that check for no "panic" or "uncaught exception" or similar in the test output. These tests will never fail in CI.
This PR adds four such assertions in test/js/bun/jsc/finalization-registry-throw.test.ts:
- Lines 39-40:
expect(stderr).not.toContain("ASSERTION FAILED")/expect(stderr).not.toContain("releaseAssertNoException") - Lines 72-73: same pair in the second test
Grep across test/ shows no other file uses not.toContain("ASSERTION FAILED"), so this introduces a pattern the repo has explicitly avoided.
Why these assertions don't add signal
- CI builds don't print this string.
ASSERTION FAILED: … releaseAssertNoExceptionis the debug-build assertion banner. In CI's release builds,releaseAssertNoExceptioncallsRELEASE_ASSERT→WTFCrash, which aborts without writing that exact text. So in the environment that matters, the negative substring match is vacuously true even when the bug regresses. - They pass when the callback never fires. GC timing is non-deterministic (the test itself acknowledges this with the
SKIPPEDbranch). If the cleanup callback never runs, stderr is empty andnot.toContain(...)passes trivially — it's not actually exercising the fix.
Step-by-step on a regression
Suppose this PR's C++ change is reverted and the test runs in CI:
- Subprocess registers the FinalizationRegistry, GC fires the cleanup callback, callback throws.
releaseAssertNoExceptiontriggersRELEASE_ASSERT→ process receives SIGABRT.proc.exitedresolves withexitCode === null,proc.signalCode === "SIGABRT".- Test 1:
expect(exitCode).toBe(0)fails → regression caught. ✅ - Test 2:
expect(proc.signalCode).toBeNull()fails → regression caught. ✅ - Meanwhile, stderr in a release build does not contain the literal string
"ASSERTION FAILED", so lines 39-40 / 72-73 still pass. They contributed nothing.
The exit-code/signal assertions are doing all the work; the stderr checks are dead weight that violate the documented convention.
Fix
Delete lines 39-40 and 72-73. The tests remain correct — expect(exitCode).toBe(0) (test 1) and expect(proc.signalCode).toBeNull() (test 2) are sufficient and are the assertions that actually fail on regression.
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
1 participant
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.
Fuzzilli fingerprint:
ef17fd336d106f22What
Bun overrides JSC's
DeferredWorkTimerhooks and runs scheduled tasks (FinalizationRegistry cleanup,Atomics.waitAsync, etc.) viarunPendingWork()inJSCTaskScheduler.cpp. Unlike JSC's stockDeferredWorkTimer::doWork(), this did not catch exceptions thrown by the task, so a throwingFinalizationRegistrycleanup callback left a pending exception on the VM which trippedreleaseAssertNoException()in the event loop's validation scope:Repro
Fix
Match JSC's
DeferredWorkTimer::doWork(): wrap the task in a top exception scope, clear any non-termination exception and route it throughreportUncaughtExceptionAtEventLoopso it reachesprocess.on('uncaughtException')handlers (matching Node.js) instead of aborting.