bun:ffi: don't re-throw JSC's TerminationException from a JSCallback - #32792
Conversation
FFI_Callback_* called the JS function through the NakedPtr profiledCall overload, then cleared and re-threw the returned exception. For the TerminationException raised by worker.terminate() that re-throw happens after the outermost VMEntryScope has already retired the termination request, which violates VM::setException's precondition and re-enters JS on the terminated worker VM: ASSERTION FAILED: !isTerminationException(exception) || hasTerminationRequest() JavaScriptCore/runtime/VM.cpp(1072) : void JSC::VM::setException(Exception *) Leave the exception pending on the VM instead, like any other host function, and skip callback tasks posted to a global whose script execution has already stopped.
|
Warning Review limit reached
More reviews will be available in 2 minutes and 22 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughJS FFI callback invocation now routes through a shared helper, and threadsafe callbacks skip JS re-entry when the target global is no longer running. The test suite adds coverage for exception propagation through ChangesFFI callback invocation and shutdown behavior
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:34 PM PT - Jun 26th, 2026
✅ @robobun, your commit ed2c62e50fee65d64ca1c57a988487db74ba3a4d passed in 🧪 To try this PR locally: bunx bun-pr 32792That installs a local version of the PR into your bun-32792 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Looked at both before adding This PR only changes what happens when the callback's
If either still reproduces with #30165 applied, that would be a distinct third bug in this path. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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/ffi/ffi.test.js`:
- Around line 721-724: Replace the time-based polling in the ffi test with an
Atomics-based signal: in the worker path that stores to the shared flag, call
Atomics.notify after writing the value, and in the parent path wait on the same
shared location instead of looping with Bun.sleep. Update the wait logic around
Atomics.load/flag in the ffi test so it blocks on the observable condition and
resumes when the worker notifies, using the existing worker callback/frame
synchronization points.
🪄 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: 49fb5c1c-e010-4d62-90e7-f286d53161af
📒 Files selected for processing (2)
src/jsc/bindings/JSFFIFunction.cpptest/js/bun/ffi/ffi.test.js
- skip both new tests on Windows ARM64, where TinyCC (and with it JSCallback/CFunction) is compiled out and the constructors throw - run the throwing-callback test in a spawned script: bun test's exit path does not finalize the CFunction's native handle, which the ASan lane's leak checker reports against the test process - enqueue a single callback task: a worker terminated with a task still queued re-buffers it in EventLoop::deinit, and that buffer is unreachable to LSan once the worker arena is freed
|
Two pre-existing issues surfaced while getting the new tests green on the x64-asan lane (it runs with
A plain
|
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/ffi/ffi.test.js`:
- Around line 712-715: The subprocess regression in the ffi test is too loose
because it allows any stderr output while still expecting exitCode 0. Update the
combined-object assertion in the ffi.test.js case around the subprocess check to
pin stderr to an empty string, matching the bunEnv subprocess test pattern and
keeping the strongest invariant.
🪄 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: e9f52c4d-1564-4458-8bc9-7b4341729c11
📒 Files selected for processing (1)
test/js/bun/ffi/ffi.test.js
Zig::GlobalObject::scriptExecutionStatus always reports Running from C++: its extern, Bun__VM__scriptExecutionStatus, resolves to the phase_c_exports stub, so the branch could never fire. Entry traps already turn a post-termination JS entry into the TerminationException this change handles.
…s inside it JSCallback.close() drops the Function synchronously, and Function::drop calls TCC::State::destroy, which unmaps the page holding the compiled trampoline. When close() is called from inside the callback itself, the native caller (e.g. qsort) is still on the stack: the in-flight invocation returns into the unmapped page and the caller keeps jumping to the freed function pointer for the rest of the native call. There is no safe point anywhere on that native stack. Even after the last FFI_Callback_call_* frame returns, the trampoline still has to run its own return-value conversion and ret out of the page that was just unmapped. Fix: - FFICallbackFunctionWrapper counts how many FFI_Callback_call_* frames are on the JS thread's stack (FFICallbackCallScope, an RAII guard in each of the nine non-threadsafe entry points). The threadsafe entry point runs on a foreign thread and only posts a task, so it is not touched. - close() from inside the callback no longer drops the Function. It arms a pending-close slot on the wrapper. Closing with no call in flight (the normal case) still drops immediately, so a tight create/close loop does not accumulate TinyCC states. - The outermost FFICallbackCallScope destructor (call depth reaching 0) is the only place that enqueues the deferred drop as an event-loop task. No JS can execute between that point and the event loop draining its queue, so nothing can free the trampoline while a frame still has to return into it. Enqueuing from close() itself would leave a hole: a full EventLoop::tick() triggered from inside the callback (bun:jsc's drainMicrotasks, bun:test's resolves/rejects via wait_for_promise) would run the deferred drop early. - The enqueued task routes back through the same check instead of dropping unconditionally. A nested tick from a later native invocation of the same callback can still run it while a trampoline frame is live, and it then re-arms the wrapper so the new outermost scope reschedules it. A task that re-enqueues itself would hang instead: tick() drains the queue until it is empty. Deferring the drop also keeps the JSC::Strong on the JS function alive for the remaining invocations the native caller makes before it returns, so a callback that closes itself keeps working until the caller is done. Rebased on top of the invokeFFICallback() refactor from #32792: the per-arity entry points now delegate to a shared helper, so the RAII guard moves to the top of each (now one-line) entry-point body.
… call invokeFFICallback (added on main in #32792) declares its own ThrowScope, so the lambda's scope that guards decodeThreadsafeCallbackArgument must be released before that call or JSC's exception-scope validator reports the simulated throw as unchecked.
… call invokeFFICallback (added on main in #32792) declares its own ThrowScope, so the lambda's scope that guards decodeThreadsafeCallbackArgument must be released before that call or JSC's exception-scope validator reports the simulated throw as unchecked.
2 participants
What
Calling
worker.terminate()while the worker thread is inside abun:ffiJSCallbackmakes the FFI callback glue re-throw JSC's own TerminationException. Assertion-enabled builds abort on the worker thread:Reproduces 5/5 with a debug build (no native library needed,
CFunctionovercb.ptris enough):The racier sibling of the same bug shows up as
ASSERTION FAILED: !exception()inExceptionScope.h(61)when the next queued callback task entersInterpreter::executeCallImplwith the re-thrown exception still pending.Cause
Every
FFI_Callback_*entry point insrc/jsc/bindings/JSFFIFunction.cppcalled the JS function through theNakedPtr<Exception>overload ofprofiledCall(which clears the VM's exception) and then unconditionally re-threw it withscope.throwException().A threadsafe
JSCallbackruns from a posted event-loop task, so itsprofiledCallis the outermost VM entry. Whenworker.terminate()lands inside that callback, JSC throws the TerminationException, and the outermostVMEntryScopeteardown retires the termination request (VM::clearHasTerminationRequest). Re-installing the engine's own TerminationException after that violatesVM::setException's precondition, and in a no-assert build it leaves a half-terminated worker VM with a stale pending TerminationException that the next JS entry trips over.Fix
The clear-and-rethrow round trip was an inlined, incorrect re-implementation of "leave the exception pending". Do that directly: all ten
FFI_Callback_*entry points now share oneinvokeFFICallback()helper that uses the plainprofiledCalloverload andRETURN_IF_EXCEPTION, leaving whatever JSC threw (user exception or TerminationException) pending on the VM like any other host function.Verification
Two tests in
test/js/bun/ffi/ffi.test.js:JSCallback tolerates worker.terminate() arriving inside the callback: spawns the repro above as a child. Unfixedbun-debugaborts with the assertion (SIGABRT); fixed passes 10/10.JSCallback exceptions propagate out of the native call: pins the normal-exception contract (a throwing callback still raises at the FFI call site), identical before and after.The assertion only exists in
ASSERT_ENABLEDJSC builds, so the release binary passes the first test before and after; debug/ASAN is where it distinguishes. Both tests are skipped on Windows ARM64, where TinyCC (and with itJSCallback/CFunction) is compiled out. The full existingJSCallback/CFunction/threadsafe-callback matrix inffi.test.jspasses with the change.Related: #32780 and #32778 fix independent
JSCallbackbugs in the same functions (re-entering JS while a user exception is pending; trampoline freed under a native caller). This one is about the TerminationException specifically and composes with both, but whichever lands later needs a small rebase.