process.dlopen: check for exception after writing replaced exports

[robobun]

Repro

With BUN_JSC_validateExceptionChecks=1 on an assert-enabled build, requiring any N-API addon whose napi_register_module_v1 returns a value other than the original exports object aborts at module load:

ERROR: Unchecked JS exception:
    This scope can throw a JS exception: putInlineSlow @ vendor/WebKit/Source/JavaScriptCore/runtime/JSObject.cpp:836
        (ExceptionScope::m_recursionDepth was 9)
    But the exception was unchecked as of this scope: Process_functionDlopen @ ../../src/jsc/bindings/BunProcess.cpp:397
        (ExceptionScope::m_recursionDepth was 8)
ASSERTION FAILED: exception check validation failed

This is the shape of every "module exports a single function/class" addon (Node's own js-native-api/4_object_factory and 5_function_factory conformance tests included), and is why those two entries are in test/no-validate-exceptions.txt. #32911 fixes the N-API entry-point side of the validator failures and explicitly leaves this one, which is in the process.dlopen wrapper itself, for a separate change.

Cause

When napi_register_module_v1 returns a replacement exports value, Process_functionDlopen writes it back to module.exports via JSObject::put. put can run a user-defined setter and opens its own ThrowScope inside putInlineSlow; the function then returned without observing that write's exception state, so the outer scope's destructor tripped verifyExceptionCheckNeedIsSatisfied. The parallel call site in Napi::executePendingNapiModule (the self-registration path) already has the matching check.

Fix

Add the missing RETURN_IF_EXCEPTION(scope, {}) after the put, matching executePendingNapiModule. If the setter throws, the exception now propagates to the require() call instead of being left pending past an unchecked scope; on release builds (where the verifier is compiled out) the observable behaviour is unchanged for non-throwing setters.

Verification

test/napi/dlopen-replace-exports-exception-check.test.ts compiles a minimal addon with the system C compiler against the in-repo N-API headers (no node-gyp) that exports napi_register_module_v1 directly (no static-constructor NAPI_MODULE registration, so the dlsym path in Process_functionDlopen is taken) and returns a new function as its exports. It is require()d in a child process under BUN_JSC_validateExceptionChecks=1.

git stash push -- src/ && bun bd test test/napi/dlopen-replace-exports-exception-check.test.ts
  # exit 134, SIGABRT with the putInlineSlow / Process_functionDlopen report above
git stash pop && bun bd test test/napi/dlopen-replace-exports-exception-check.test.ts
  # 1 pass, stdout 'loaded 42'

On release builds the validator is compiled out, so the env var is a no-op and the test still passes.

[robobun]

Reproduced on a debug+ASAN build with BUN_JSC_validateExceptionChecks=1 using the addon in test/napi/dlopen-replace-exports-exception-check.test.ts (aborts with SIGABRT on main, exit 134; passes with this change).

CI on 5f4c7c5: the diff is green. The only red is test/js/node/test/parallel/test-net-connect-memleak.js on the two alpine x64 lanes, which is a pre-existing failure on main (unrelated to this change and being handled separately). The remaining handful are flakes that passed on retry. Ready for review.

[robobun]

^{Updated 7:29 PM PT - Jul 19th, 2026}

❌ @robobun, your commit 5f4c7c5 has 1 failures in Build #75917 (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 34746

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

bun-34746 --bun

[coderabbitai]

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/napi/dlopen-replace-exports-exception-check.test.ts`:
- Around line 11-18: Shorten the comment above the N-API initialization test to
no more than three lines, retaining only the non-obvious requirement that a
differing Init return value causes module.exports to be written and requires an
exception check, along with the relevant reason for directly exporting
napi_register_module_v1.

🪄 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: b157ea4c-85ab-4cc6-951b-47e3c79e159c

📥 Commits

Reviewing files that changed from the base of the PR and between 99fc2f8 and 428b0c7.

📒 Files selected for processing (2)

• src/jsc/bindings/BunProcess.cpp
• test/napi/dlopen-replace-exports-exception-check.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 14 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: aba7cbbe-c96e-4b99-ad90-9cdf892c4ea5

📥 Commits

Reviewing files that changed from the base of the PR and between 0a17ce6 and 5f4c7c5.

📒 Files selected for processing (2)

• src/jsc/bindings/BunProcess.cpp
• test/napi/dlopen-replace-exports-exception-check.test.ts

---

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

[claude]

LGTM — one-line RETURN_IF_EXCEPTION after JSObject::put, matching the parallel site in Napi::executePendingNapiModule.

What was reviewed

• Confirmed the sibling write at napi.cpp:788 already has the identical check, so this brings the dlsym path to parity.
• Test compiles a minimal C addon against the in-repo headers (src/runtime/napi/ exists), skips when no cc/Windows, drains pipes concurrently, and asserts on a combined {stdout, stderr, exitCode} object.
• Ruled out: stale test/no-validate-exceptions.txt entries — those cover the N-API entry-point side handled separately (#32911), not this wrapper.

Extended reasoning...

Overview

Single-line change to src/jsc/bindings/BunProcess.cpp: adds RETURN_IF_EXCEPTION(scope, {}) immediately after strongModule->put(...) in Process_functionDlopen. When an N-API addon's napi_register_module_v1 returns a value different from the original exports, dlopen writes it back to module.exports via JSObject::put, which opens its own ThrowScope in putInlineSlow. Without the check, the outer scope's destructor trips verifyExceptionCheckNeedIsSatisfied under BUN_JSC_validateExceptionChecks=1. The parallel self-registration path in napi.cpp:786-790 already has exactly this check, so the change is bringing the two call sites to parity.

A new test (test/napi/dlopen-replace-exports-exception-check.test.ts) compiles a minimal C addon with the system cc against the in-repo N-API headers, exports napi_register_module_v1 directly (forcing the dlsym path rather than static-ctor self-registration), returns a fresh function as exports, and requires it in a subprocess with the exception validator enabled.

Security risks

None. The added line only observes and propagates an existing exception; no new inputs are parsed, no allocations, no privilege changes. The test compiles a local C file into a shared object in a temp dir and loads it in a child process — standard for the test/napi/ suite.

Level of scrutiny

Low. This is the canonical "exception check after every call that can enter JS" fix the review guide calls the most-blocked category. The change is one line, the mechanism is well-explained and independently verifiable against the sibling site, and the PR description shows a stash-on/stash-off repro proving the test fails for the right reason on main. On release builds (validator compiled out) behaviour is unchanged for non-throwing setters; if a user setter does throw, the exception now propagates to require() instead of being left pending — strictly more correct.

Other factors

• The test follows harness conventions: tempDir, bunEnv/bunExe, skipIf(isWindows || !cc), concurrent pipe drain via Promise.all, combined-object assertion so stderr appears in the failure diff without being asserted exactly (per the "never assert stderr is exactly empty" rule).
• CodeRabbit's only inline comment (trim the explanatory comment to ≤3 lines) is marked resolved and the current diff reflects the trimmed 3-line version.
• Verifier agents examined whether test/no-validate-exceptions.txt suppression entries should be removed in this PR and concluded no — those entries are tied to the N-API entry-point work in #32911, which the PR description explicitly scopes out.
• No prior claude[bot] reviews on this PR; no outstanding human reviewer comments.