fix(napi): make napi_create_error succeed when pending exception exists#29785
Open
HaleTom wants to merge 15 commits into
Open
fix(napi): make napi_create_error succeed when pending exception exists#29785HaleTom wants to merge 15 commits into
HaleTom wants to merge 15 commits into
Conversation
When a JSC VM-level exception is pending, napi_create_error (and napi_create_type_error, napi_create_range_error, node_api_create_syntax_error) incorrectly returned napi_pending_exception, causing NAPI packages to call napi_fatal_error and crash Bun. Remove DECLARE_THROW_SCOPE and all RETURN_IF_EXCEPTION checks from createErrorWithNapiValues, matching Node.js behavior where these functions only create error values without checking VM exception state. Closes oven-sh#22259
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR aligns Bun’s N-API error-construction APIs with Node.js by ensuring napi_create_error / napi_create_type_error / napi_create_range_error / node_api_create_syntax_error succeed even when a JSC VM-level exception is already pending, and updates tests to cover that specific scenario (issue #22259).
Changes:
- Remove VM-exception checks inside
createErrorWithNapiValuesso error objects can be created even whenvm.m_exceptionis already set. - Add a new standalone regression test (
test_issue_22259) that creates a VM-level exception vianapi_run_script("throw ...")and then calls the four error-creation APIs. - Wire the new regression test into the TypeScript N-API test runner.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/bun.js/bindings/napi.cpp |
Removes ThrowScope / RETURN_IF_EXCEPTION guards from error-value creation to match Node’s behavior under pending VM exceptions. |
test/napi/napi-app/standalone_tests.cpp |
Adds a standalone C++ regression test for issue #22259 and registers it. |
test/napi/napi.test.ts |
Adds the new standalone regression test to the existing “exception pending” test case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
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:
Walkthrough
ChangesN-API error creation + regression test
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/napi/napi.test.ts`:
- Around line 561-564: The test calls
checkSameOutput("test_deferred_exceptions", []) and
checkSameOutput("test_issue_22259", []) but only awaits the first call, so the
second may run after the test completes; update the test in napi.test.ts to
await both calls (e.g., await checkSameOutput("test_deferred_exceptions", []);
await checkSameOutput("test_issue_22259", []);) ensuring both promises are
awaited in the it block so the regression check for "test_issue_22259" cannot
finish after the test ends.
🪄 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: 41006fb2-d45d-4f4c-9475-b9af77827f9e
📒 Files selected for processing (3)
src/bun.js/bindings/napi.cpptest/napi/napi-app/standalone_tests.cpptest/napi/napi.test.ts
- Reorder test: create error_msg/error_code BEFORE napi_run_script so test doesn't depend on napi_create_string_utf8 behavior with pending VM exception - Return ok(env) instead of nullptr in test_issue_22259 - await both checkSameOutput calls in napi.test.ts - Reword napi.cpp comment to avoid hard 'won't throw' guarantee
Author
|
@coderabbitai help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
Author
|
@coderabbitai Pls review issues raised in your last review, and all changes since then. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
[full_review] 🧠 Learnings used |
Author
|
@coderabbitai there was no output in your previous message other than |
Contributor
✅ Actions performedFull review triggered. |
- Narrow OOM comment in createErrorWithNapiValues: getString() on validated string values does not allocate, so it cannot throw - Expand test_issue_22259 comment to document that RETURN_IF_EXCEPTION checks vm.m_exception (via vm.hasExceptionsAfterHandlingTraps()), not m_pendingException, and that napi_run_script sets vm.m_exception via JSC::evaluate() before scheduleException() sets m_pendingException
HaleTom
force-pushed
the
claude/napi-create-error-pending-exception
branch
from
95a6afc to
3914905
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/napi/napi-app/standalone_tests.cpp`:
- Around line 2161-2207: After running throw_script and before clearing the
exception, explicitly verify the original VM exception is still pending and is
the original "vm-level" error: call napi_is_exception_pending(env, &is_pending)
and assert is_pending is true; then call napi_get_and_clear_last_exception(env,
&pending_exception) and validate pending_exception matches the original VM error
by either using napi_strict_equals(env, pending_exception, <the original
exception value if available, e.g. throw_result>, &equals) and asserting equals
is true, or by retrieving the message property (napi_get_named_property +
napi_get_value_string_utf8) and asserting it equals "vm-level". This ensures
none of the napi_create_*error calls cleared or replaced the pending VM
exception.
🪄 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: a405ffae-c08f-492d-943c-0615186a5b6a
📒 Files selected for processing (2)
src/bun.js/bindings/napi.cpptest/napi/napi-app/standalone_tests.cpp
…ding Add a loop test that calls napi_create_error 5 times with a VM-level exception still pending, validating each call succeeds and produces a valid object. Destroy each loop-created value to avoid resource leaks. This addresses the adversarial review finding that the test was happy-path-only.
The comment now describes behavior (does not check VM exception state) rather than making a hard guarantee about not throwing.
…22259
Add explicit checks that:
1. napi_is_exception_pending() returns true after napi_run_script("throw...")
2. The cleared exception has message "vm-level" (the original VM error)
This addresses the unresolved CodeRabbit review comment at:
oven-sh#29785 (comment)
…ending_exception redeclaration The loop test was calling napi_destroy which doesn't exist in N-API. Also fix redeclaration of pending_exception variable that was already declared earlier in the function. Found during build verification.
…ment - Add napi_is_exception_pending() check to verify VM exception is pending before calling napi_create_* functions (CodeRabbit review comment) - Remove loop test and excessive validation that may cause test timeout - Keep test focused on core behavior: verify pending, then call napi_create_* The simplified test addresses the review comment while avoiding timeout issues that were likely caused by excessive validation while VM exception is pending.
… of napi_run_script napi_run_script crashes in Bun with downcast<JSC::Symbol> assertion when executing a throwing script. Replace with napi_call_function + a throwing callback which sets vm.m_exception via JSC::call and works in both Bun and Node.js. Also removes the duplicate function body from the previous commit, and deletes the weak JS regression test that did not exercise napi_create_error.
Author
|
Retrieving PR review comment #4366034663 |
…status in test_issue_22259
Author
Production-ready bar for this PR
Findings1. Correctness & functional completeness
2. Architecture & boundary integrity
3. Code clarity, clean code & maintainability
4. Comments & code documentation[NON-BLOCKING] Regression-test comments contradict the actual test and implementation
5. Tests & validation[NON-BLOCKING] Native validation verified by direct NAPI addon execution
[NON-BLOCKING] The test prints an unexpected
|
Author
Work done after the REVIEW file was generated1. Fixed stale comments in the regression test
2. Made the napi_call_function status check fail the testThe test logged an unexpected 3. Validation evidenceRan the regression test directly against the debug build that includes the C++ changes. All 3 expected output lines produced: The full EROFS root causePaths Workaround for future test runs: |
…issue_22259 test - Remove unused msg variable in thrower_cb (Copilot review feedback) - Validate pending_exception's type and message content after clearing
…apiValues Address Copilot review feedback: save the pre-existing VM exception state on entry so that only NEW exceptions from our own operations (getString / createErrorWithCode) are surfaced. Pre-existing exceptions continue to be silently ignored, matching Node.js behavior. This fixes oven-sh#22259 while being more defensive than the original approach.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/jsc/bindings/napi.cpp:1
- This removes the throw/exception scope entirely, which means any new exception thrown during
getString(...)orcreateErrorWithCode(...)(e.g., allocation/engine exceptions) can be left pending while the function still returnsnapi_ok. The previous implementation handled exceptions (but incorrectly failed when an exception was already pending). Consider restoring a scope and only treating exceptions as errors if they are newly introduced during this helper (e.g., snapshot the current exception at entry and only fail if the exception pointer changes). This preserves the Node-like behavior for pre-existing exceptions while still reporting real failures from inside this function.
#include "BunProcess.h"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/jsc/bindings/napi.cpp:1
- The function no longer checks for new exceptions that may occur during
getString(...)/createErrorWithCode(...). While it’s correct to avoid failing due to an already-pending VM exception (the regression), this change also risks returningnapi_okeven if error creation itself triggered an exception (e.g., allocation failure causingvm.m_exceptionto be set, orcreateErrorWithCodeproducing an empty value). Recommendation: keep a scope and only translate exceptions that are raised by this function (e.g., snapshot whether an exception was present on entry, then if an exception is present on exit and wasn’t on entry, returnnapi_pending_exception/ set last error accordingly).
#include "BunProcess.h"
Comment on lines
+1082
to
+1099
| // napi_create_error-like functions should succeed even when a VM-level | ||
| // exception is already pending (matching Node.js behavior). We remember | ||
| // whether an exception was already present on entry so we only report | ||
| // NEW exceptions that may be raised by our own operations below. | ||
| bool hadPreexistingException = vm.hasExceptionsAfterHandlingTraps(); | ||
|
|
||
| auto wtf_code = js_code.isEmpty() ? WTF::String() : js_code.getString(globalObject); | ||
| RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_pending_exception)); | ||
| auto wtf_message = js_message.getString(globalObject); | ||
| RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_pending_exception)); | ||
|
|
||
| *result = toNapi( | ||
| createErrorWithCode(vm, globalObject, wtf_code, wtf_message, type), | ||
| globalObject); | ||
| RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_pending_exception)); | ||
|
|
||
| // Surface new exceptions originating from getString() / createErrorWithCode(), | ||
| // but ignore any exception that was already pending before we entered. | ||
| if (!hadPreexistingException && vm.hasExceptionsAfterHandlingTraps()) { | ||
| return napi_set_last_error(env, napi_pending_exception); | ||
| } |
Author
There was a problem hiding this comment.
Good point about the masking risk. The current approach is a conscious trade-off matching Node.js behavior: "don't fail and don't disturb the existing pending exception."
The operations in question (getString() on already-validated strings, createErrorWithCode() with validated inputs) are unlikely to throw genuinely new exceptions in practice. Snapshotting/restoring the actual exception value adds significant complexity (we'd need to capture the JSC exception object, run in a scope, then conditionally restore) without clear benefit for the real-world use case.
That said, the concern is valid in theory. If a future change makes getString() or createErrorWithCode() more exception-prone, this guard should be revisited. For now, the pragmatic approach is: if there was a pre-existing exception, we trust that our own operations won't introduce new ones, and we return napi_ok to match Node's behavior.
Author
There was a problem hiding this comment.
Updated the approach: instead of snapshotting and restoring the pre-existing exception (which would require JSC APIs not safely available here — vm.setException doesn't exist, JSC::throwException needs a ThrowScope), we snapshot the pre-existing exception without clearing it, then only report napi_pending_exception if there was no pre-existing exception and a new one appeared.
Key insight: getString() (on already-validated isString() inputs) and createErrorWithCode() do not consult vm.m_exception — they run correctly regardless of whether a pre-existing exception is pending. So we don't need to clear/restore, we just need to detect genuinely new exceptions.
Behavior matrix:
- No pre-existing exception, ops succeed →
napi_ok✓ - No pre-existing exception, ops throw →
napi_pending_exception✓ - Pre-existing exception, ops succeed →
napi_ok, pre-existing exception preserved ✓ - Pre-existing exception, ops throw (OOM, near-impossible on validated strings) →
napi_ok, pre-existing exception replaced (acceptable — OOM would crash anyway)
HaleTom
added a commit
to HaleTom/bun
that referenced
this pull request
May 23, 2026
Remove napi-related artifacts from this branch that came from other PRs (oven-sh#29785, oven-sh#30047, oven-sh#30216). The napi files now match main exactly, making this PR clean and self-contained with only the nested-resolutions feature. Restored files: - src/jsc/bindings/napi.cpp - test/napi/napi-app/standalone_tests.cpp - test/napi/napi.test.ts - test/napi/napi-app/binding.gyp Removed files (not on main): - test/napi/napi-app/isolate_finalizer_addon.c - test/napi/napi-app/reentrant_register_addon.cpp
…andlingTraps() in createErrorWithNapiValues Replace the hadPreexistingException boolean guard with a direct JSC::Exception* pointer snapshot. vm.exception() is a simple field read; vm.hasExceptionsAfterHandlingTraps() fires handleTraps() which can invoke watchdog, GC, and other pending trap callbacks. Our operations (getString on validated strings, createErrorWithCode) do not consult vm.m_exception and run correctly in its presence, so no clearing or restoring is needed. We only detect genuinely new exceptions that appeared when there was none before.
HaleTom
added a commit
to HaleTom/bun
that referenced
this pull request
May 23, 2026
Remove napi changes from PR oven-sh#29785 (napi_create_error pending exception) that were accidentally included in this branch. All napi files now match upstream/main exactly. Files restored: - src/jsc/bindings/napi.cpp - test/napi/napi-app/standalone_tests.cpp - test/napi/napi-app/binding.gyp - test/napi/napi-app/isolate_finalizer_addon.c (restored from deletion) - test/napi/napi-app/reentrant_register_addon.cpp (restored from deletion) - test/napi/napi.test.ts
Comment on lines
+1082
to
+1098
| // napi_create_error and friends must succeed even when a VM-level exception | ||
| // is already pending (matching Node.js behavior). Our operations (getString | ||
| // on already-validated strings, createErrorWithCode) do not consult | ||
| // vm.m_exception and run correctly in its presence. We only detect a NEW | ||
| // exception that appeared when there was none before. | ||
| JSC::Exception* preExistingException = vm.exception(); | ||
|
|
||
| auto wtf_code = js_code.isEmpty() ? WTF::String() : js_code.getString(globalObject); | ||
| RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_pending_exception)); | ||
| auto wtf_message = js_message.getString(globalObject); | ||
| RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_pending_exception)); | ||
|
|
||
| *result = toNapi( | ||
| createErrorWithCode(vm, globalObject, wtf_code, wtf_message, type), | ||
| globalObject); | ||
| RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_pending_exception)); | ||
|
|
||
| if (!preExistingException && vm.exception()) { | ||
| return napi_set_last_error(env, napi_pending_exception); | ||
| } |
Comment on lines
+2406
to
+2407
| napi_value call_result; | ||
| status = napi_call_function(env, global, throw_fn, 0, nullptr, &call_result); |
Comment on lines
+2451
to
+2468
| puts("napi_create_error functions succeeded with VM-level exception pending"); | ||
|
|
||
| // Loop test: call napi_create_error multiple times with VM exception still pending. | ||
| for (int i = 0; i < 5; i++) { | ||
| napi_value loop_error_val; | ||
| status = napi_create_error(env, error_code, error_msg, &loop_error_val); | ||
| if (status != napi_ok) { | ||
| printf("napi_create_error loop iteration %d failed: %d\n", i, status); | ||
| return nullptr; | ||
| } | ||
| napi_valuetype loop_type; | ||
| status = napi_typeof(env, loop_error_val, &loop_type); | ||
| if (status != napi_ok || loop_type != napi_object) { | ||
| printf("napi_create_error loop iteration %d: unexpected type %d (status %d)\n", i, loop_type, status); | ||
| return nullptr; | ||
| } | ||
| } | ||
| puts("napi_create_error loop test passed (5 iterations with VM exception pending)"); |
robobun
added a commit
that referenced
this pull request
Jun 2, 2026
Tree-sitter inside node:worker_threads deterministically panicked at worker exit once ~15 finalizers had run: panic: NAPI FATAL ERROR: Error::New napi_create_error at NapiEnv::cleanup -> NapiRef::callFinalizer -> NapiFinalizer::call Two things were broken. First, NapiEnv::cleanup ran each finalizer without clearing any pending exception after it returned, so a throw from one finalizer leaked into the next. Node.js doesn't do that -- finalizers run with no JS frame above them to catch, so propagating the exception is pointless and only confuses later finalizers. Second, createErrorWithNapiValues refused to produce an Error value while a VM-level exception was pending (DECLARE_THROW_SCOPE + RETURN_IF_EXCEPTION at the top). Node.js's napi_create_error has no such check -- it is a pure value-producing function. Because napi_is_exception_pending deliberately skips the VM-scope check during cleanup for safety, node-addon-api's Error::New(env) saw 'no pending exception', fell through to napi_create_error, got back napi_pending_exception, and tripped NAPI_FATAL_IF_FAILED -> napi_fatal_error. Fix both layers: clear env/VM exceptions after each finalizer in NapiEnv::cleanup, and drop the pending-exception check from createErrorWithNapiValues. Matches Node.js on both counts. Regression test: a standalone addon wraps two JS objects; the first finalizer calls a throwing JS function via napi_call_function (leaving the JSC VM exception pending without clearing it), the second calls napi_create_error and prints the returned status. Before the fix the status is 10 (napi_pending_exception) and the debug ASAN build aborts on the leaked exception first; after the fix the status is 0 (napi_ok) and the process exits cleanly. Closes #30286. Related: #22259, #28248, #29785.
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
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.
Closes #22259
Summary
When a JSC VM-level exception is pending,
napi_create_error(andnapi_create_type_error,napi_create_range_error,node_api_create_syntax_error) incorrectly returnnapi_pending_exception, causing NAPI packages like@napi-rs/canvasto callnapi_fatal_errorand crash Bun.This matches Node.js behavior, where these functions only create error values without checking VM exception state.
What changed
In
createErrorWithNapiValues(src/bun.js/bindings/napi.cpp):DECLARE_THROW_SCOPE(vm)+ all 3RETURN_IF_EXCEPTIONchecksgetString()won't throw new exceptionsRoot cause
The
RETURN_IF_EXCEPTIONmacro checksvm.m_exception(JSC VM-level exception state). When a JS callback throws during native code execution, a VM-level exception is set, and thennapi_create_errorwould fail withnapi_pending_exception. NAPI packages that follow the common pattern of callingnapi_fatal_erroron unexpected status would then crash Bun entirely.Why the previous PR (#28248) was insufficient
PR #28248 had the correct C++ fix but its regression test was ineffective: it used
do_throw()which only sets a NAPI env-level pending exception (m_pendingException), not a JSC VM-level exception (vm.m_exception). SinceRETURN_IF_EXCEPTIONchecks VM-level state, the test would pass with both old and new code—it was not an actual regression test.This PR fixes the regression test by using
napi_call_functionwith a C++ callback that callsnapi_throw_error. This setsm_pendingException, thennapi_call_function's internalthrowPendingException()propagates it tovm.m_exception, creating a genuine VM-level exception before callingnapi_create_error.napi_run_scriptwas considered but crashes in Bun with adowncast<JSC::Symbol>assertion—a separate pre-existing bug.How to verify
Regression test in
test/napi/napi-app/standalone_tests.cpp:napi_call_function+ a throwing C++ callbacknapi_create_error,napi_create_type_error,napi_create_range_error,node_api_create_syntax_error)napi_objectOriginal reproduction from Unclear napi panic can occur when no space is available #22259: Running
@napi-rs/canvasbenchmarks no longer crashes withnapi_fatal_error.Related issues
panic: Error::New napi_get_last_error_info(node-llama-cpp, likely same root cause)@napi-rs/canvas#18718 — Crash on@napi-rs/canvasbenchmarking (mentioned in Unclear napi panic can occur when no space is available #22259)napi_fatal_errorin stack (likely same root cause)Test plan
napi_object