napi: tolerate pending VM exception in napi_create_string_* (debug/asan abort) - #36091
Closed
robobun wants to merge 3 commits into
Closed
napi: tolerate pending VM exception in napi_create_string_* (debug/asan abort)#36091robobun wants to merge 3 commits into
robobun wants to merge 3 commits into
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
Collaborator
Author
|
Updated 5:37 AM PT - Jul 27th, 2026
❌ @robobun, your commit 61421a9 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36091That installs a local version of the PR into your bun-36091 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Collaborator
Author
|
Diff is green. The new CI failures on build 83299 are unrelated to this change:
Ready for review. |
dylan-conway
added a commit
that referenced
this pull request
Aug 14, 2026
### Problem - Regression from #37075: a `node:vm` `timeout` or `worker.terminate()` is lost when it lands while an addon is inside one of the ungated N-API functions. Release builds keep running the script forever (`vm.runInNewContext(...)` never returns, `await worker.terminate()` never resolves); debug builds abort on the next exception check with `ASSERTION FAILED: !!(scope).exception() == vm.traps().needHandling(JSC::VMTraps::NeedExceptionHandling)`, or in `napi_get_value_bigint_int64` with `ASSERTION FAILED: Unexpected exception observed` from `NAPI_RETURN_SUCCESS`. - Cause: #37075 rewrote `NAPI_PREAMBLE_NO_PENDING_CHECK` (`src/jsc/bindings/napi.cpp`) as an unconditional `JSC::SuspendExceptionScope`. That scope writes the `vm.exception` it saw on entry back when it exits. On entry nothing is pending; an exception check inside the body services the pending trap and raises the `TerminationException`; on exit the scope puts `null` back. The trap was consumed, so nothing raises it again. Of the 38 C++ users of the macro, the bodies with such a check are `napi_get_value_string_*`, `napi_get_value_bigint_int64/uint64`, `napi_create_bigint_int64/uint64` and `napi_create_symbol` with a description. - Two related gaps in the same set of functions, fixed along the way: before #37075 (and in the first version of this PR) the C++ macro checked for exceptions on entry, so a waiting termination made the ungated call itself fail with `napi_pending_exception`; and the ungated functions implemented in `src/runtime/napi/napi_body.rs` (`napi_create_array*`, `napi_create_string_*`, `napi_create_int32/uint32/int64`, `napi_get_undefined/null/boolean`, `napi_is_*`, `napi_get_*_info`, handle scopes) had no handling at all: `napi_create_array` delivered a termination itself and failed, and the ones using `JsResult` helpers failed (release) or asserted (debug) with any exception pending on the VM. Node's `CHECK_ENV`-only functions never fail for either reason, and node-addon-api aborts the process (`Error::ThrowAsJavaScriptException napi_throw`) when one of them does; a node-addon-api callback making only ungated calls hit that under a `vm` timeout on Bun and not on Node. ### Fix - `NapiUngatedScope` (napi.cpp) is what every ungated function now runs under: (a) it stashes the entry exception only when one is actually pending (`std::optional<JSC::SuspendExceptionScope>`), so a clean VM is left alone and whatever the body raises stays pending, and (b) it holds a `JSC::DeferTraps`, so no exception check inside the body (its own or in the JSC helpers it calls) services VM traps. A termination requested before or during the call stays a request and is delivered by the next check after the call returns (the caller's `RETURN_IF_EXCEPTION` in `NapiClass`, a JS loop check, or a gated N-API call). Nothing is lost, and the ungated calls never report a termination, as in Node. - `NAPI_PREAMBLE_NO_PENDING_CHECK` declares one; the 26 Rust functions construct the same C++ object in place through `NapiUngatedScope__construct/__destruct` from an `ungated!` macro that replaces `get_env!` (80 bytes of 8-aligned storage, checked by a `static_assert`; a guard destroys it on every return path). This also covers what #36091 and #36093 were addressing separately (`napi_create_string_*` and the `*_info` accessors with a VM exception pending). - Consequence of (b): no JS or addon code may run under the scope. The zero-length path of `node_api_create_external_string_{latin1,utf16}` ran the addon's finalizer there; the two functions now share one body (`createExternalString`) and run the finalizer after its scopes have closed. The other users only read, allocate, or register callbacks. - `napi_get_value_bigint_int64` checks for an exception after its conversion like the `uint64` variant already did. - Tests, in the `pending-exception gate` block of `test/napi/napi.test.ts` (addon side in `test/napi/napi-app/standalone_tests.cpp`, drivers in `module.js` and `ungated-calls-spin-worker.js`), all compared against Node's output; each round of ungated calls covers both the C++ and the Rust half: - a `node:vm` `timeout` on a script looping through the ungated functions: hangs without the fix (10/10 runs on a release build), asserts on a debug build - `worker.terminate()` of a worker doing the same: same failure modes - the ungated functions succeed with an exception pending on the VM and leave it pending - 200ms of ungated calls under a 20ms timeout with an exception pending: no call reports the timeout, the pending exception is still the original one afterwards, and the script still times out on return - Verified on a debug+ASAN build at 12d4d5c: the four tests pass, the rest of `test/napi/napi.test.ts` is unchanged, and the upstream `test_string`, `test_array`, `test_typedarray`, `test_dataview`, `test_handle_scope`, `test_promise`, `test_date`, `test_error`, `test_exception`, `test_number`, `test_conversions`, `2_function_arguments`, `test_buffer`, `test_worker_terminate` suites pass (a few GC-heavy files exceed the 5s harness timeout under a local debug build and pass when run directly; same without this change). A node-addon-api wrapped function looping under `vm` timeouts, which aborted on every run of 20 with the entry check, survives 60/60 as on Node. Supersedes #36091 and #36093 (per-function `SuspendExceptionScope` wrappers for `napi_create_string_*` and the `*_info` accessors); both are covered by `ungated!` here. ### Background - Ungated functions: Node implements pure value constructors/accessors (`napi_create_object`, `napi_get_cb_info`, `napi_get_value_*`, references, ...) with `CHECK_ENV` only, so an addon may call them while an exception is pending, and they never fail because execution is being terminated. node-addon-api relies on both. In Bun these are split between `napi.cpp` (`NAPI_PREAMBLE_NO_PENDING_CHECK`) and `napi_body.rs` (`ungated!`); `NAPI_PREAMBLE` / `preamble!` are the gated versions that refuse while an exception is pending. - VM traps: JSC delivers asynchronous requests (a watchdog timeout, which `node:vm`'s `timeout` uses, or a termination request, which `worker.terminate()` uses) by setting a trap bit. The next exception check that services traps (`RETURN_IF_EXCEPTION`, which most JSC entry points and Bun's `NAPI_RETURN_IF_VM_EXCEPTION` expand to) consumes the bit and throws the `TerminationException`, which is uncatchable from JS and is what unwinds the running script. Once thrown, it is the only record of the request. - `JSC::DeferTraps` makes trap servicing a no-op for its scope: checks inside it see no exception and the bits stay set, so the first check after the scope delivers the request. `JSC::SuspendExceptionScope` clears `vm.exception` on construction and writes the saved value back unconditionally on destruction. <details> <summary>Earlier shape of this PR (3b36256 to 63653e1)</summary> The first version kept the pre-#37075 entry check in the C++ macro (so a waiting termination was delivered by, and reported from, the ungated call) and wrapped the conditional suspend in a small scope object whose destructor re-raised a termination the body had delivered after putting the entry exception back. That fixed the hang but kept the Bun-only node-addon-api failure mode described under Problem, so it was replaced by the `DeferTraps` form (f599d79), which dbc32b6 then extended to the Rust-side functions. The Bun-only test that pinned the re-raise became the same-output "200ms of ungated calls under a 20ms timeout" test. </details> --------- Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
Member
|
Superseded by #38300 (merged as 54f0271): every ungated N-API function, including the Rust-side |
Collaborator
Author
|
Confirmed: on main at 54f0271 the Rust |
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
3 participants
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.
Repro
Under debug/asan builds this 3-call sequence aborts:
Second route needs no addon throw:
napi_create_bigint_wordspast the engine cap throws a RangeError into the VM and returns status 10, then anynapi_create_string_*hits the same abort.Cause
The Rust
napi_create_string_{utf8,latin1,utf16}bodies calledbun_core::String/BunString__*helpers through the generatedcrate::cppwrappers. Those wrappers open anExceptionValidationScopeand, when the C++ call returns a non-zeroJSValue, callassert_exception_presence_matches(false)which asserts no VM exception is pending. Whenvm.m_exceptionis already set (the scenario above), that assertion hitsTopExceptionScope__assertNoException->releaseAssertNoException-> abort. Release builds compile the validation scope out, so only asserts builds die.Node.js does not gate
napi_create_string_*behindNAPI_PREAMBLE; they returnnapi_okand leave the pending exception untouched.Fix
Add
napi_internal_create_string_{latin1,utf8,utf16}innapi.cppthat allocate the JS string under aJSC::SuspendExceptionScope, which stashesvm.m_exceptionfor the duration of the allocation and restores it on return. The Rust entry points call these directly instead of routing through theBunString__*validation-scope wrappers. The caller's exception is preserved and the string creators returnnapi_ok, matching Node.Test
test_create_string_with_vm_exceptionintest/napi/napi-app/standalone_tests.cppexercises both routes (napi_throw+napi_call_function, and napi_create_bigint_words over the cap) and all three encodings plus the non-ASCII UTF-8 branch, assertingstatus=0,pending_after_create=true, and that the originalError: E1survives. Output matches Node byte-for-byte viacheckSameOutput.Fail-before (src/ stashed,
bun bd test): aborts withreleaseAssertNoException, exit 134.After: all three creators return 0 on both routes, exception preserved.
no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts
Scope
The three string creators are the complete set of
get_env!entry points innapi_body.rswhose C++ callee returns a non-zeroJSValuewhile a VM exception is pending (the validation scope then asserts "no exception" and aborts).napi_create_array/napi_create_array_with_lengthalso route through a validation-scope wrapper, butconstructEmptyArrayhasRETURN_IF_EXCEPTION(scope, nullptr)before allocating, so they returnnapi_pending_exceptioninstead of aborting. That is a separate Node-compat status divergence (Node returnsnapi_ok), not the abort class, and is left for a follow-up.