Report exceptions thrown by DeferredWorkTimer tasks as uncaughtException#30846
Report exceptions thrown by DeferredWorkTimer tasks as uncaughtException#30846robobun wants to merge 4 commits into
Conversation
|
Updated 3:51 PM PT - May 22nd, 2026
✅ @robobun, your commit a1720ccbfc26eb2e8bc81d2f721fd1141de805ec passed in 🧪 To try this PR locally: bunx bun-pr 30846That installs a local version of the PR into your bun-30846 --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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR wraps deferred-work task execution in a top-level exception scope within the JSCTaskScheduler and reports uncaught exceptions at the event loop. Three integration tests verify that FinalizationRegistry cleanup callbacks which throw are properly surfaced via process uncaughtException across different contexts. ChangesException Handling for Task Scheduler
🚥 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/bun/util/finalization-registry-throw.test.ts`:
- Around line 48-70: The test can pass without exercising the throwing cleanup
path; change the FinalizationRegistry cleanup function to perform a detectable
side-effect (e.g., set globalThis.__finalizerRan = true or write a distinct
message via console.error) and after calling registry.register, Bun.gc(true),
and Bun.generateHeapSnapshot(), wait for the event loop tick
(setImmediate/Promise.resolve) and assert that globalThis.__finalizerRan is true
(or the stderr contains the message). Update references in this test around
FinalizationRegistry, registry.register, Bun.generateHeapSnapshot and the
GC/setImmediate sequence so the assertion verifies the cleanup callback actually
ran.
🪄 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: a18c25f0-33bb-4533-8080-400c1911e364
📒 Files selected for processing (2)
src/jsc/bindings/JSCTaskScheduler.cpptest/js/bun/util/finalization-registry-throw.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/js/bun/util/finalization-registry-throw.test.ts (1)
48-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert that the throwing finalizer path is actually exercised.
This can pass without proving the cleanup callback ran, so it may miss regressions in the intended heap-snapshot path.
Suggested tightening
- process.on("uncaughtException", () => {}); + let sawUncaught = false; + process.on("uncaughtException", () => { + sawUncaught = true; + }); const registry = new FinalizationRegistry(() => { ArrayBuffer(); }); @@ - Bun.gc(true); - Bun.generateHeapSnapshot(); - Bun.gc(true); - setImmediate(() => {}); + (async () => { + for (let i = 0; i < 20 && !sawUncaught; i++) { + Bun.gc(true); + Bun.generateHeapSnapshot(); + Bun.gc(true); + await new Promise(r => setImmediate(r)); + } + console.log(JSON.stringify({ sawUncaught })); + })(); @@ - expect(stdout).toBe(""); + expect(JSON.parse(stdout.trim())).toEqual({ sawUncaught: true });🤖 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/util/finalization-registry-throw.test.ts` around lines 48 - 70, The test currently swallows the finalizer-thrown exception with process.on("uncaughtException", () => {}), so it never proves the FinalizationRegistry finalizer ran; change the uncaughtException handler used with FinalizationRegistry to record/emit a visible marker (e.g., process.stdout.write or console.log a unique string like "FINALIZER_THROWN") when invoked, keep the registry callback that throws (FinalizationRegistry(() => { ArrayBuffer(); })), run the same sequence (register, nullify, Bun.gc, Bun.generateHeapSnapshot, Bun.gc) and then assert the marker appears in stdout/stderr (or assert a flag) so the test actually verifies the throwing finalizer path was exercised.
🤖 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.
Duplicate comments:
In `@test/js/bun/util/finalization-registry-throw.test.ts`:
- Around line 48-70: The test currently swallows the finalizer-thrown exception
with process.on("uncaughtException", () => {}), so it never proves the
FinalizationRegistry finalizer ran; change the uncaughtException handler used
with FinalizationRegistry to record/emit a visible marker (e.g.,
process.stdout.write or console.log a unique string like "FINALIZER_THROWN")
when invoked, keep the registry callback that throws (FinalizationRegistry(() =>
{ ArrayBuffer(); })), run the same sequence (register, nullify, Bun.gc,
Bun.generateHeapSnapshot, Bun.gc) and then assert the marker appears in
stdout/stderr (or assert a flag) so the test actually verifies the throwing
finalizer path was exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 856726fc-386b-468a-94ea-dfbdd43c6153
📒 Files selected for processing (11)
src/crash_handler/lib.rssrc/errno/lib.rssrc/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/util/finalization-registry-throw.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
robobun
force-pushed
the
farm/d84a530b/finalization-registry-throw-crash
branch
from
d432d9b to
853321d
Compare
There was a problem hiding this comment.
Thanks — both earlier concerns are addressed (direct Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(defaultGlobalObject(...)) call covers node:vm, and the heap-snapshot test now positively asserts the caught error). Deferring to a maintainer mainly so this can be reconciled with the two duplicate PRs (#30835 / #30844) flagged above.
Extended reasoning...
Overview
The substantive change is ~7 lines in src/jsc/bindings/JSCTaskScheduler.cpp: runPendingWork now wraps the deferred-work task in a TopExceptionScope, clears any non-termination exception, and reports it via Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(defaultGlobalObject(realm), …) so FinalizationRegistry cleanup throws reach process.on('uncaughtException'). A new test file covers the GC, generateHeapSnapshot, and node:vm trigger paths. bake-codegen.ts gets a one-line JSON.stringify around the OVERLAY_CSS define. The remaining ~9 Rust files are pure rustfmt/autofix reflow of #[cfg(...)] / cfg_attr attributes with no semantic change.
Security risks
None identified. No new inputs are parsed, no auth/crypto/permission surfaces touched; the change only routes an already-thrown JS exception to the existing uncaught-exception reporter.
Level of scrutiny
Moderate. The diff is small and mirrors JSC's own DeferredWorkTimer::doWork(), but it sits on the core JSC↔Bun event-loop boundary and changes user-observable behavior (exceptions from FinalizationRegistry / WASM-streaming callbacks now fire uncaughtException). That plus the bot-flagged duplicate PRs (#30835 / #30844) implementing the same fix means a maintainer should pick the canonical one rather than me auto-approving.
Other factors
Both inline comments I left on the previous revision have been addressed: the node:vm method-table gap is closed by calling the Zig implementation directly on defaultGlobalObject(...) (commit 853321d, with a dedicated test), and the heap-snapshot test now registers 1000 objects, loops, and asserts the captured message contains ArrayBuffer instead of only checking for empty stderr. The Buildkite failures listed by robobun are pre-existing lint/build warnings on main unrelated to this diff. No new issues found in this pass.
|
CI status: the only failing test is Duplicates: #30835 and #30844 implement the same core fix; this one additionally covers the |
When a FinalizationRegistry cleanup callback (or any other
DeferredWorkTimer task) throws, the exception was left pending on the
VM. The Rust caller's validation scope would then trip
releaseAssertNoException() in debug/ASAN builds, and in release the
exception would leak into the next JS execution.
Mirror JSC's DeferredWorkTimer::doWork(): catch the exception after
running the task, clear it (unless it's a termination exception), and
route it through reportUncaughtExceptionAtEventLoop so it reaches
process.on('uncaughtException') like Node.js.
robobun
force-pushed
the
farm/d84a530b/finalization-registry-throw-crash
branch
from
9772358 to
9f3472d
Compare
TicketData::target() has ASSERT(isTargetObject()) whose definition lives in the inlines header; without it the release-asan link fails with an undefined symbol.
…ghtException Going through the method table resolved to JSC's no-op for NodeVMGlobalObject. Use defaultGlobalObject() to resolve vm contexts to the owning Bun global and call the Zig reporter directly. Strengthen the heapsnapshot test to assert the throwing finalizer actually ran, and add a node:vm context test.
robobun
force-pushed
the
farm/d84a530b/finalization-registry-throw-crash
branch
from
9f3472d to
a1720cc
Compare
1 participant
What does this PR do?
Fixes a debug/ASAN assertion failure (
releaseAssertNoException) when aFinalizationRegistrycleanup callback throws.Fuzzilli fingerprint:
568b6f604bcb11d9Root cause
Bun routes JSC's
DeferredWorkTimertasks throughBun__runDeferredWork→runPendingWork, which invokedjob->task(ticket)with no exception handling.JSFinalizationRegistry::runFinalizationCleanupreturns with the exception still pending when a user callback throws, so the validation scope inJSCDeferredWorkTask::runtrippedreleaseAssertNoException()in debug/ASAN builds. In release builds the pending exception was only caught accidentally by the post-taskdrainMicrotasks()call, which aborts the nextTick drain for that tick, and fornode:vm-created registries it was silently dropped.Fix
Mirror JSC's own
DeferredWorkTimer::doWork(): wrap the task in aTopExceptionScope, clear any non-termination exception after it runs, and report it viaZig::GlobalObject::reportUncaughtExceptionAtEventLoop(defaultGlobalObject(realm), …)so it reachesprocess.on('uncaughtException')— matching Node.js — including when the registry lives in anode:vmcontext. Termination exceptions are left pending for the caller.Repro
How did you verify your code works?
test/js/bun/util/finalization-registry-throw.test.tscovering the plain-GC,generateHeapSnapshot, andnode:vmcontext trigger paths; all assert the error reachesuncaughtException.caught: null) on a pre-fix binary and pass with the fix.Rebase notes
Rebased onto current main; dropped the earlier autofix.ci formatting commit (main now carries equivalent formatting) and the temporary
bake-codegen.tsquoting workaround (no longer needed now that the define auto-quote recovery from #30679 is on main).