napi: enforce the pending-exception gate in NAPI_PREAMBLE

[robobun]

What

With a napi exception pending (via napi_throw_error), Bun executed napi calls that Node.js refuses: napi_run_script ran the script, napi_object_freeze/napi_set_element mutated, napi_resolve_deferred resolved, and a further 10 entry points returned napi_ok. Node returns napi_pending_exception and performs zero side effects for every one of them.

The worst face: napi_object_freeze/napi_set_element returned napi_pending_exception like Node but only after performing the mutation, so a status-checking addon sees a matching code and a silently-diverged world.

Repro

napi_throw_error(env, "EGATE", "armed");          // exception now pending
napi_object_freeze(env, obj);                     // node: 10, no-op   bun: 10, but FROZEN
napi_set_element(env, arr, 7, five);              // node: 10, no-op   bun: 10, but arr[7]=5
napi_run_script(env, script, &r);                 // node: 10, not run bun: 0,  script RAN
napi_create_promise(env, &d, &p);                 // node: 10          bun: 0
napi_resolve_deferred(env, d, v);                 // node: 10          bun: 0, RESOLVED
// + instanceof, strict_equals, wrap, get_prototype, get_date_value,
//   get_array_length, create_date, create_dataview

Cause

napi_throw* stores the exception via env->scheduleException() into m_pendingException (a Strong<>) without throwing to the JSC VM. NAPI_PREAMBLE (napi.cpp:88) declared a throw scope and checked only the VM exception slot, so the stashed exception never tripped the gate. The tail NAPI_RETURN_SUCCESS_UNLESS_EXCEPTION did consult hasPendingException(), producing the "correct status, mutation already done" rows. Node's NAPI_PREAMBLE checks env->last_exception.IsEmpty() (the env slot) up front.

The Rust-implemented entry points (napi_get_prototype, napi_strict_equals, napi_create_promise, ...) had no preamble gate at all.

Fix

• NAPI_PREAMBLE now calls NAPI_RETURN_IF_EXCEPTION, which checks both the VM scope and env->hasPendingException(). Covers every C++ entry point Node.js gates with its own NAPI_PREAMBLE (object_freeze/seal, set/get/has/delete_element, property access, wrap/unwrap, get_date_value, coerce_to_*, define_class, new_instance, create_external, ...).
• napi_run_script, napi_instanceof, napi_create_dataview, napi_create_bigint_words, node_api_create_buffer_from_arraybuffer (which open their own throw scope via NAPI_PREAMBLE_NO_THROW_SCOPE) now check hasPendingException() at the top of that scope.
• New NAPI_PREAMBLE_NO_PENDING_CHECK for the entry points Node.js does not gate (CHECK_ENV/CHECK_ENV_NOT_IN_GC), so they keep succeeding with a pending exception: get_cb_info, get_global, get_new_target, create_reference, reference_ref/unref, get_reference_value, add_finalizer, node_api_post_finalizer, is_buffer, is_typedarray, is/detach_detached_arraybuffer, adjust_external_memory, node_api_symbol_for, node_api_get_module_file_name, get_value_bigint_*, get_value_external, get/set_instance_data, create_bigint_int64/uint64, create_symbol, add/remove_env_cleanup_hook, create_object, create_double, get_value_int32/uint32/int64/double/bool/string_*. Before this PR these happened to match Node only because the old preamble never saw the env slot.
• Removed the two per-site hasPendingException() workarounds in napi_create_external_buffer/arraybuffer that the preamble now covers.
• Rust side (napi_body.rs): added NapiEnv::has_pending_exception() (via new NapiEnv__hasPendingException FFI) and a preamble! macro, applied to every Rust entry point Node.js gates with NAPI_PREAMBLE: get_prototype, get_array_length, strict_equals, make_callback, create_promise, resolve_deferred, reject_deferred, create_date, create_buffer_copy.

Verification

test_pending_exception_gate in the napi addon arms napi_throw_error, then:

• calls 14 gated entry points and records every status;
• calls 9 non-gated entry points (get_global, create_reference, reference_unref, get_reference_value, create_bigint_int64, create_symbol, is_buffer, is_typedarray, get_instance_data);
• clears the exception and inspects side effects (Object.isFrozen, arr[7], a global the script would touch).

Run via checkSameOutput, so Bun's output is diffed byte-for-byte against Node.

Before / after

# node v26 == this PR               # bun 1.4.0 / main
napi_object_freeze: status=10       napi_object_freeze: status=10
napi_set_element: status=10         napi_set_element: status=10
napi_run_script: status=10          napi_run_script: status=10
napi_instanceof: status=10          napi_instanceof: status=0
napi_strict_equals: status=10       napi_strict_equals: status=0
napi_wrap: status=10                napi_wrap: status=0
napi_get_prototype: status=10       napi_get_prototype: status=0
napi_get_date_value: status=10      napi_get_date_value: status=0
napi_get_array_length: status=10    napi_get_array_length: status=0
napi_create_date: status=10         napi_create_date: status=0
napi_create_dataview: status=10     napi_create_dataview: status=0
napi_create_promise: status=10      napi_create_promise: status=0
napi_resolve_deferred: status=10    napi_resolve_deferred: status=0
napi_get_global: status=0           napi_get_global: status=0
napi_create_reference: status=0     napi_create_reference: status=0
napi_reference_unref: status=0      napi_reference_unref: status=0
...                                 ...
side_effect frozen=false            side_effect frozen=true
side_effect arr[7]=undefined        side_effect arr[7]=set
side_effect script_ran=false        side_effect script_ran=true

$ USE_SYSTEM_BUN=1 bun test test/napi/napi.test.ts -t 'pending-exception gate'
(fail) ... 13 lines differ

$ bun bd test test/napi/napi.test.ts -t 'pending-exception gate'
(pass) napi > pending-exception gate > refuses and performs no side effects while a napi exception is pending

The pre-existing test_deferred_exceptions and test_external_*_with_pending_exception tests continue to pass.

---

no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts

[robobun]

Reproduced: USE_SYSTEM_BUN=1 bun test test/napi/napi.test.ts -t 'pending-exception gate' fails (13-line diff vs Node: side effects + status=0), bun bd test passes.

PR: #34137

Review iterations: applied NAPI_PREAMBLE_NO_PENDING_CHECK to all C++ entry points Node.js implements with CHECK_ENV (31 total, audited against js_native_api_v8.cc/node_api.cc), fixed mid-body exception checks in the non-gated set, and extended the test to assert status=0 for 12 of them under a pending exception. Merged main (c1b4b26) and de-duplicated the hasPendingException helper that #34133 also added. The napi_throw_*-while-pending overwrite is covered by #32834.

CI: the new pending-exception gate test passes on every lane. Red lanes on build 72858 are unrelated and appear on multiple other branch builds:

• test-net-connect-memleak.js (also on builds 72796/72798/72799/72808)
• 30205.test.ts NapiFinalizerTask leak (also on builds 72798/72802/72805; this PR's diff vs main does not touch that code path)
• bake/deinitialization, watch-many-dirs, bun-install-proxy (all marked flaky by ci:errors)

Ready for review.

[robobun]

^{Updated 7:10 AM PT - Jul 14th, 2026}

❌ @robobun, your commit c1b4b26 has 1 failures in Build #72858 (All Failures):

• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64-baseline
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64

---

🧪   To try this PR locally:

bunx bun-pr 34137

That installs a local version of the PR into your bun-34137 executable, so you can run:

bun-34137 --bun

[coderabbitai]

Walkthrough

Changes

Bun’s N-API bindings now detect pending VM or environment exceptions, gate selected Rust entry points, adjust C++ exception checkpoints and preambles, and add regression tests covering returned statuses and suppressed side effects.

N-API pending exception behavior

Layer / File(s)	Summary
Exception detection and preamble contracts src/jsc/bindings/napi.cpp, src/runtime/napi/napi_body.rs	Adds environment and VM pending-exception detection, Rust wrappers, and gated versus non-gated preamble behavior.
C++ exception-safe API paths src/jsc/bindings/napi.cpp	Updates scoped exception handling, value accessors, object creation, script and BigInt operations, instanceof, and external-data adoption paths.
Rust N-API entry-point gating src/runtime/napi/napi_body.rs	Applies pending-exception preamble checks to prototype, array, equality, callback, promise, date, and buffer APIs.
Pending-exception regression coverage test/napi/napi-app/standalone_tests.cpp, test/napi/napi.test.ts	Adds standalone and harness tests verifying napi_pending_exception results and absence of observable side effects.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: enforcing the pending-exception gate in NAPI_PREAMBLE.
Description check	✅ Passed	The description covers the change and verification well, though it uses custom headings instead of the template's exact section names.

---

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

[coderabbitai]

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 `@src/jsc/bindings/napi.cpp`:
- Around line 2861-2863: Update the hasInstance flow near NAPI_CHECK_ARG to
store constructorObject->hasInstance(...)’s return value, check scope for
exceptions immediately afterward, and only then assign *result and return
napi_ok. Preserve the existing result mapping while ensuring thrown
Symbol.hasInstance exceptions are propagated instead of reporting success.

In `@src/runtime/napi/napi_body.rs`:
- Line 895: After the `getPrototypeOf` call in the surrounding N-API
implementation, immediately check `env.has_pending_exception()` before storing
or using the returned prototype and before returning `napi_ok`; propagate the
pending exception instead. Keep the existing `preamble!(env_)` handling
unchanged.

🪄 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: c57e5273-03f7-4272-a6e8-60b8afad59ac

📥 Commits

Reviewing files that changed from the base of the PR and between febdefa and 1d45ca1.

📒 Files selected for processing (4)

• src/jsc/bindings/napi.cpp
• src/runtime/napi/napi_body.rs
• test/napi/napi-app/standalone_tests.cpp
• test/napi/napi.test.ts

[claude]

I didn't find further issues in this pass, but this change flips the semantics of NAPI_PREAMBLE (affecting every C++ NAPI entry point) and re-classifies ~40 functions across C++ and Rust based on a manual audit against Node's js_native_api_v8.cc/node_api.cc — worth a human sign-off on the audit's completeness given it took three rounds to converge.

What was reviewed:

• Re-checked the df04551 additions (node_api_create_external_string_*, napi_add/remove_async_cleanup_hook, napi_get_value_bigint_uint64 mid-body swap) — all now match Node's CHECK_ENV behavior.
• Confirmed the removed per-site hasPendingException() guards in napi_create_external_buffer/arraybuffer are subsumed by the new NAPI_PREAMBLE gate (no ordering regression vs. data adoption).
• Verified 38 remaining NAPI_PREAMBLE(env) callers matches the author's stated audit count; spot-checked that the Rust preamble! macro correctly wraps get_env! and that NapiEnv__hasPendingException checks both env slot and VM scope.

Extended reasoning...

Overview

This PR changes NAPI_PREAMBLE in src/jsc/bindings/napi.cpp from checking only the JSC VM exception slot to also checking env->hasPendingException() (the slot populated by napi_throw*). To avoid over-gating, it introduces NAPI_PREAMBLE_NO_PENDING_CHECK and applies it to ~35 C++ entry points that Node.js implements with CHECK_ENV/CHECK_ENV_NOT_IN_GC rather than NAPI_PREAMBLE. It also adds a Rust-side preamble! macro (backed by a new NapiEnv__hasPendingException FFI export) applied to 9 Rust-implemented entry points, removes two now-redundant per-site guards, and reorders throw-scope declarations in 5 NAPI_PREAMBLE_NO_THROW_SCOPE functions to gate before doing work. A comprehensive checkSameOutput test diffs 26 entry points against Node under a pending exception.

Security risks

None identified. This is a Node.js compatibility fix in the NAPI layer; no auth, crypto, or untrusted-input parsing is touched. The change makes Bun more restrictive (refusing side-effecting calls while an exception is pending), which if anything reduces attack surface.

Level of scrutiny

High. The correctness of this PR hinges entirely on whether every one of ~75 NAPI entry points in napi.cpp and napi_body.rs was correctly classified as gating-vs-non-gating by cross-referencing Node.js source. My earlier review passes found the classification incomplete three separate times (27 functions in ddc4b2b, then 4 more + 1 mid-body check in df04551), each of which would have been a Node-compat regression. The author now states a systematic audit of all 38 remaining NAPI_PREAMBLE(env) callers was done, and this pass found nothing further — but the audit is against an external moving target (Node's js_native_api_v8.cc) that I cannot independently verify from this repo, and a single misclassification silently breaks addons in either direction (over-gating leaks refs/buffers; under-gating performs side effects Node refuses).

Other factors

• The test is strong: checkSameOutput byte-diffs against real Node for 14 gated + 12 non-gated functions plus 3 side-effect probes, so the covered set is locked. But ~35 modified entry points are not exercised by the test.
• Two CodeRabbit findings (missing tail exception checks after hasInstance / getPrototypeOf) were correctly deferred to in-flight PRs #34129/#34133 as pre-existing and out of scope.
• The napi_throw_*-while-pending overwrite (my earlier 🟡) was correctly deferred to #32834.
• New FFI surface (NapiEnv__hasPendingException) is minimal and read-only.

Given the breadth (every NAPI entry point's gating contract), the iterative fix history, and the reliance on a manual external-source audit, this should get a human look from someone who can independently spot-check the Node.js classification before merge.

[Jarred-Sumner]

Fix conflicts

[robobun]

Merged main (c1b4b26). Conflict resolution: took #34129's napi_instanceof (it uses NAPI_PREAMBLE, which this branch now gates on hasPendingException()), and de-duplicated the NapiEnv__hasPendingException FFI export / has_pending_exception method that #34133 and this branch both added independently. Dropped the per-macro hasPendingException() check in NAPI_CHECK_TO_OBJECT and the inline check in napi_get_prototype since NAPI_PREAMBLE/preamble! now cover them.

test_pending_exception_gate, test_deferred_exceptions, and the new napi_instanceof/CHECK_TO_OBJECT tests from #34129/#34133 all pass against Node via checkSameOutput.

[claude]

No new issues found. My earlier findings (over-gating of CHECK_ENV-only entry points, the four missed in the second pass, and the mid-body check in napi_get_value_bigint_uint64) are all addressed in df04551, and the c1b4b26 merge resolution checks out — every NAPI_CHECK_TO_OBJECT caller uses the gating NAPI_PREAMBLE, and has_pending_exception() has no other Rust callers affected by the widened FFI semantics. Given the scope (~40 entry points audited against Node's js_native_api_v8.cc/node_api.cc across two languages) this still warrants a human pass.

Checked this run: the merge's removal of the per-macro hasPendingException() guard from NAPI_CHECK_TO_OBJECT — all 14 call sites open with NAPI_PREAMBLE(env); the NapiEnv__hasPendingException dedup — only consumed by preamble!, so widening it to also consult the VM slot matches Node's NAPI_PREAMBLE and doesn't affect other callers.

Extended reasoning...

Overview

This PR changes the semantics of NAPI_PREAMBLE in src/jsc/bindings/napi.cpp to gate on env->hasPendingException() (the slot napi_throw* populates), adds a NAPI_PREAMBLE_NO_PENDING_CHECK variant applied to ~31 entry points that Node.js implements with CHECK_ENV/CHECK_ENV_NOT_IN_GC, adds explicit entry gates to 5 NAPI_PREAMBLE_NO_THROW_SCOPE users, and mirrors the gate on the Rust side via a new preamble! macro applied to 9 entry points in src/runtime/napi/napi_body.rs. It also removes two now-redundant per-site hasPendingException() workarounds and, after merging main, drops the redundant guard from NAPI_CHECK_TO_OBJECT and de-duplicates the NapiEnv__hasPendingException FFI export. Test coverage is a new test_pending_exception_gate addon function that exercises 14 gated + 12 non-gated entry points under a pending exception and asserts side-effect absence, all diffed byte-for-byte against Node via checkSameOutput.

Security risks

None. This tightens behavior to refuse operations while an exception is pending, matching Node.js. No auth, crypto, or untrusted-input parsing paths are involved.

Level of scrutiny

High. NAPI is the compatibility surface for the entire native-addon ecosystem, and the correctness of this change depends on a manual audit of every entry point against Node's js_native_api_v8.cc/node_api.cc to decide gating vs. non-gating. My earlier reviews found real regressions in that audit twice (over-gating ~27 functions, then 4 more), which the author fixed. A third audit miss would silently break addon error-handling paths, so a human should spot-check the final NAPI_PREAMBLE vs NAPI_PREAMBLE_NO_PENDING_CHECK partition against Node.

Other factors

• The PR went through three correction rounds; each found genuine issues, all now addressed and covered by the extended test.
• The c1b4b26 merge with main touched semantics (dropped a guard from NAPI_CHECK_TO_OBJECT, widened the FFI helper to also check the VM slot, took #34129's napi_instanceof). I verified all 14 NAPI_CHECK_TO_OBJECT callers use the gating preamble and the widened has_pending_exception() has no other Rust callers, so both changes are sound.
• Two related concerns (napi_throw_* overwrite, hasInstance/getPrototypeOf tail checks) are intentionally deferred to #32834/#34129/#34133 to avoid conflicts; that scoping is reasonable.
• Test coverage is strong: checkSameOutput locks both directions (status=10 for gated, status=0 for non-gated) plus side-effect assertions against Node.