napi: don't deliver terminations inside ungated functions

[robobun]

Problem

• Regression from Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #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: Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #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 Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #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 napi: tolerate pending VM exception in napi_create_string_* (debug/asan abort) #36091 and napi: tolerate pending VM exception in ArrayBuffer/typed-array info accessors (debug/asan abort) #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.

Earlier shape of this PR (3b36256 to 63653e1)

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.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 4 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 770cc2f1-9f51-47ef-bdeb-6fd1668d3040

📥 Commits

Reviewing files that changed from the base of the PR and between 347d291 and 12d4d5c.

📒 Files selected for processing (6)

• src/jsc/bindings/napi.cpp
• src/runtime/napi/napi_body.rs
• test/napi/napi-app/module.js
• test/napi/napi-app/standalone_tests.cpp
• test/napi/napi-app/ungated-calls-spin-worker.js
• test/napi/napi.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: approved by @dylan-conway at 12d4d5c. The fix is the NapiUngatedScope form (conditional stash + DeferTraps), used by the C++ macro since f599d79 and by the 26 Rust-side ungated functions since dbc32b6. Ready to merge once CI for 12d4d5c is in.

Reproduced on a release build that includes #37075 (1.4.0-canary da3851e) with a raw addon calling napi_get_value_bigint_*, napi_get_value_string_utf8, napi_create_symbol and napi_create_bigint_* in a loop: vm.runInNewContext(..., { timeout }) never returned in 10/10 runs and await worker.terminate() never resolved in 3/3, while an addon going through NAPI_PREAMBLE functions completed every time. On a debug build the same fixtures abort with ASSERTION FAILED: !!(scope).exception() == vm.traps().needHandling(JSC::VMTraps::NeedExceptionHandling).

Verified at 12d4d5c on a debug+ASAN build: the four pending-exception gate tests pass with the fixtures that now cover both halves, the rest of test/napi/napi.test.ts is unchanged, 14 upstream node-api suites covering the Rust-side functions pass (GC-heavy files that exceed the local 5s harness limit pass when run directly, as without the branch), 30 raw vm terminations and the worker driver are delivered, and a node-addon-api wrapped function looping under vm timeouts survives 60/60.

[robobun]

^{Updated 6:11 PM PT - Aug 13th, 2026}

@dylan-conway, your commit 5b0b5dc is building: #95257

[robobun]

Ran the branch as of 04a818e (5b0b5dc is a comment change) on a debug+ASAN build:

• test/napi/napi.test.ts: the four pending-exception gate tests pass; the rest of the file is unchanged. Two tests hit the 5s timeout in the full run (bigint conversion ... returns the right error code, runs a finalizer that another finalizer registered during env cleanup); both take 3.7 to 5s under a local debug build regardless of this branch (three sequential spawns, or a worker start) and pass in isolation, 3/3 each.
• Raw addon loops: 50 vm terminations and the worker.terminate() driver, all delivered.
• A node-addon-api wrapped function (napitests' napi_get_typeof) looping under 20 vm timeouts survives 3/3 runs, as on Node. With the entry-check form of the preamble it aborted on every run with Error::ThrowAsJavaScriptException napi_throw, so the DeferTraps form also closes that gap.

PR description updated to describe the DeferTraps form; the earlier shape is kept in a details block at the bottom.

[robobun]

Build 95257 (5b0b5dc) is down to its last few jobs with nothing red. The only test/napi entries are retries that passed: napi_wrap > has the right lifetime (the gcUntil weak-reference check, Windows 2019) and upstream test_fatal_exception in the parallel batch; neither touches the new tests or the preamble. Nothing further pending from my side.

[claude]

I reviewed this revision and found no bugs (the earlier CRLF issue is fixed). Because the change reworks NAPI_PREAMBLE_NO_PENDING_CHECK — used by 38 call sites — around JSC's DeferTraps/SuspendExceptionScope interaction with TerminationException, and makes "no JS/addon code under this macro" a load-bearing invariant for all of them, a human look would still be worthwhile.

What was reviewed:

• Macro destruction order: throw scope → DeferTraps → optional SuspendExceptionScope; a stashed entry exception is restored last, and trap bits stay set for the caller's next check.
• createExternalString refactor: template deduction (std::char_traits<char>::length ≡ strlen), disposeNow only set after env/result are validated, finalizer runs after the preamble scopes close.
• napi_get_value_bigint_int64 now matches the uint64 variant's post-conversion check.
• Tests: all four compare against Node via checkSameOutput, printf-driven output split on /\r?\n/, worker test has a 30 s timeout for debug builds.

Extended reasoning...

Overview

The PR fixes a regression from #37075 where a node:vm timeout or worker.terminate() request that lands while an addon is inside an ungated N-API function is silently lost (release builds hang forever; debug builds assert). The fix rewrites NAPI_PREAMBLE_NO_PENDING_CHECK in src/jsc/bindings/napi.cpp to (a) only construct SuspendExceptionScope when an exception is actually pending, so its unconditional restore-on-destruct doesn't null out a termination the body raised, and (b) hold JSC::DeferTraps for the body so no exception check inside it services a VM trap. Because DeferTraps means addon/JS code must not run under the macro, the two node_api_create_external_string_* functions (whose zero-length path ran the addon's finalizer under the preamble) are refactored into a shared createExternalString<ExternalChar, Char> template that signals disposeNow back to the wrapper, which runs the finalizer after the preamble scopes have closed. napi_get_value_bigint_int64 gains the same post-conversion NAPI_RETURN_IF_VM_EXCEPTION the uint64 variant already had. Four new tests in test/napi/napi.test.ts (drivers in module.js, ungated-calls-spin-worker.js, addon side in standalone_tests.cpp) exercise vm-timeout, worker-terminate, engine-exception-pending, and timeout-during-ungated-calls, all compared against Node's output.

Security risks

None identified. This is exception/termination-delivery plumbing; no untrusted input parsing, no auth/crypto/permissions surface. The createExternalString template preserves the existing length <= INT_MAX and null-pointer checks unchanged.

Level of scrutiny

High. NAPI_PREAMBLE_NO_PENDING_CHECK is expanded at 38 sites, and the new form makes "no JS or addon code may run under it" load-bearing for every one of them (a violation would run user code with VM traps deferred). The PR audited and fixed the two known violators (external-string finalizers), but confirming no other user of the macro can reach addon code — and that the DeferTraps / conditional-SuspendExceptionScope / throw-scope destruction ordering is sound across all JSC exception-check configurations — is exactly the kind of JSC-internals judgment a maintainer should sign off on. This is C++ in src/jsc/bindings/ interacting with JSC's termination machinery, which REVIEW.md flags as the most-blocked category.

Other factors

• My earlier inline comment (Windows CRLF in the printf-driven test) was addressed in 256b0fe; both printf tests now split on /\r?\n/.
• The comment-cop feedback was addressed across several commits (04a818e, 5b0b5dc); the author explicitly justified keeping the four-line macro comment as documenting invariants the code cannot show.
• The PR description is unusually thorough, the fix was verified on debug+ASAN, and the tests are compared against Node, so the risk of a silent behavior regression is low. The remaining question is whether the DeferTraps approach is the right long-term shape for these 38 call sites, which is a design call for a human.

[robobun]

On the "no JS or addon code under the macro" invariant, for whoever merges: I went through the remaining users of NAPI_PREAMBLE_NO_PENDING_CHECK with that question.

• Synchronous addon code: the zero-length node_api_create_external_string_* path was the only one, and f599d79 moves it out. napi_reference_ref/unref only touch the strong handle (NapiRef::clear() is what runs the finalizer, and only GC/teardown call it); napi_set_instance_data drops the previous NapiFinalizer without calling it (as Node does); the cleanup-hook functions only add to or remove from the lists; napi_add_finalizer and napi_create_reference register, they do not run anything.
• JS: the value getters operate on values they have already type-checked (isString(), isHeapBigInt()), so toString() / toBigInt64() cannot reach valueOf/toString of an object; everything else is allocation or plain reads.
• Residual: an allocation in one of these bodies can trigger a GC, and a NAPI_VERSION_EXPERIMENTAL module's finalizers run synchronously from GC (non-experimental ones are queued to the event loop). Those run with traps deferred until the ungated call returns, but they run in GC context where no script can run anyway, so a termination is at most delayed by the rest of that call. That is the same window the Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075 form had.

[robobun]

Went over dbc32b6 / 12d4d5c and ran it on a debug+ASAN build:

• NapiUngatedScope: members are declared suspended then deferTraps, so the traps flag is restored before the exception is put back, the same order the macro had; SuspendExceptionScope only touches the NeedExceptionHandling bit, which DeferTraps does not affect. The object is 72 bytes in both build types (no verification-only members in either part), so the 80-byte static_assert holds for debug and release; the TopExceptionScope, whose size does differ, stays in the C++ macro. On the Rust side the guard is bound to a named _napi_ungated_scope, so it lives to the end of the function and drops on every early return, and the storage outlives it.
• The 26 Rust functions only read, allocate, or open/close handle scopes; none reaches JS or addon code.
• Full test/napi/napi.test.ts: the gate tests pass with the extended rounds, nothing else changed. 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: pass (four GC-heavy files hit the local 5s harness limit under debug and pass when run directly, as they do without this branch). Raw vm/worker loops and the node-addon-api probe: all delivered, 60/60 survived.
• The engine-exception fixture now also exercises what napi: tolerate pending VM exception in napi_create_string_* (debug/asan abort) #36091 and napi: tolerate pending VM exception in ArrayBuffer/typed-array info accessors (debug/asan abort) #36093 were about (napi_create_string_utf8 and friends with a VM exception pending), so those can probably be closed once this lands; noted in the description.