Fix unchecked exception scopes when import("bun") materializes the namespace - #33150
Fix unchecked exception scopes when import("bun") materializes the namespace#33150robobun wants to merge 3 commits into
Conversation
|
Updated 10:04 PM PT - Jun 30th, 2026
❌ @robobun, your commit 2286220 has some failures in 🧪 To try this PR locally: bunx bun-pr 33150That installs a local version of the PR into your bun-33150 --bun |
WalkthroughThis PR changes BunObject export handling so lazy property getter exceptions are propagated during module export, removes bulk static reification from the native module wrapper, and adds a subprocess test for ChangesBunObject Export Exception Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/js/bun/util/BunObject.test.ts`:
- Around line 41-60: The current test in BunObject.test.ts relies on
BUN_JSC_validateExceptionChecks, which is not valid standalone coverage because
it can still pass with USE_SYSTEM_BUN=1. Replace this case with a user-visible
regression test around the import path exercised by the existing await
import('bun') scenario (or remove it and depend on ASAN/validator coverage), and
keep the assertions focused on observable behavior from the spawned Bun process
rather than the debug-only env var.
🪄 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: f63f4fc4-983e-4c2b-8a83-87ab9d8cfb44
📒 Files selected for processing (2)
src/jsc/bindings/BunObject.cpptest/js/bun/util/BunObject.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — applies the same already-vetted fix from generateNativeModule_NodeModule/NodeProcess to the bun module, with a regression test.
Extended reasoning...
Overview
This PR fixes an exception-check-validator assertion failure when import("bun") (with a runtime-computed specifier) materializes the namespace. It touches two files:
src/jsc/bindings/BunObject.cpp: drops the bulkreifyAllStaticProperties()call ingenerateNativeModule_BunObjectand changes the per-property export loop inexportBunObjectto propagate exceptions (RETURN_IF_EXCEPTION) instead of swallowing them intojsUndefined(). Also reordersexportNames.appendto after the exception check so the names/values arrays stay aligned on early return.test/js/bun/util/BunObject.test.ts: adds a child-process test that runs the computed import underBUN_JSC_validateExceptionChecks=1.
Security risks
None. This is purely about JSC exception-scope hygiene during native module generation. No auth, crypto, permissions, or untrusted input handling is involved.
Level of scrutiny
Low-to-moderate. While this is C++ JSC bindings code, the change is mechanical: it applies the exact same fix that was already made to generateNativeModule_NodeModule (NodeModuleModule.cpp:1157-1163) and NodeProcess (NodeProcessModule.h:16-24), both of which carry the same explanatory comment that this PR cross-references. The per-property get() loop already existed and already reified properties individually — the bulk reify was redundant work that also happened to be buggy.
Other factors
- The behavior change (propagate instead of swallow) matches
NodeProcessModule.h's loop, which usesRETURN_IF_EXCEPTION(scope, )after eachget(). Initializers are not expected to throw; if one ever does, rejecting the import is strictly better than silently exportingundefinedfor that name. - The pre-existing
await import('bun')test in the same file already asserts that every enumerableBunproperty is present and identical on the namespace, so any regression in the export list would be caught. - No outstanding reviewer comments; bug-hunting system found no issues.
…mespace generateNativeModule_BunObject bulk-reified every lazy property on the Bun object. JSObject::reifyAllStaticProperties runs the property callbacks back-to-back with no exception check between them, and the Bun.$ initializer enters JS, so the next initializer's scope construction aborts under the JSC exception check validator (the validateExceptionChecks option). On a real throw, the remaining initializers would all run with a pending exception. Drop the bulk reify like node:module and node:process already do: the per-export get() loop reifies one property at a time under a checked scope. Propagate an initializer exception out of that loop instead of turning it into an undefined export, so the import rejects with the real error.
b92326e to
31ed7dd
Compare
There was a problem hiding this comment.
LGTM — drops the bulk reify in favor of the per-property get() loop, matching the existing NodeModule/NodeProcess pattern.
Extended reasoning...
Overview
This PR touches two files: src/jsc/bindings/BunObject.cpp (the native ESM module generator for import("bun")) and test/js/bun/util/BunObject.test.ts. In generateNativeModule_BunObject, the bulk reifyAllStaticProperties() call is removed because exportBunObject's existing per-property get() loop already lazy-reifies each entry inside a properly checked scope. In exportBunObject, a throwing initializer now propagates (failing the import) instead of being swallowed into a silent undefined export. A spawned-child test exercises the path with BUN_JSC_validateExceptionChecks=1.
Security risks
None. This is internal JSC exception-scope hygiene with no auth, crypto, parsing, or untrusted-input surface. The only behavioral change is that an initializer throw (only possible under OOM/stack-overflow/termination) now rejects the import instead of producing a half-initialized namespace — strictly safer.
Level of scrutiny
Low-to-moderate. While this is native JSC bindings code, the change is mechanical and follows an established in-repo precedent: generateNativeModule_NodeModule (NodeModuleModule.cpp:1157) and node:process already dropped the bulk reify for exactly this reason, and the new comment cross-references that. The pre-existing await import('bun') test in the same file already asserts every enumerable Bun property is present and identical on the namespace, so dropping the bulk reify cannot silently change the export list without that test failing.
Other factors
The bug-hunting system found no issues. CodeRabbit's only concern (validity of a BUN_JSC_validateExceptionChecks-gated test) was withdrawn after the author explained the precedent in test/bundler/transpiler/macro-test.test.ts and that the ASAN CI lane runs with this validator enabled. The exportNames.append was correctly moved after the exception check so names/values stay aligned on early return. No CODEOWNERS cover this path.
The net twin of test-tls-connect-memleak.js (already quarantined for this matrix) asserts FinalizationRegistry delivery within a single gc() plus one setImmediate. On alpine x64 it flips whenever an unrelated change shifts the binary or heap layout; it failed both alpine lanes in builds 67305 and 67317 across every per-file retry while passing on darwin, glibc Linux, and Windows, and the code changed in this PR never runs in that test's process.
There was a problem hiding this comment.
No bugs found — the fix mirrors the existing generateNativeModule_NodeModule pattern and looks correct, but test/expectations.txt is CODEOWNER-gated and the C++ change alters error semantics on a core init path, so leaving this for a human sign-off.
Extended reasoning...
Overview
This PR touches three files:
src/jsc/bindings/BunObject.cpp: drops the bulkreifyAllStaticProperties()call ingenerateNativeModule_BunObjectand changesexportBunObject's per-property loop to propagate exceptions (RETURN_IF_EXCEPTION) instead of swallowing them viatryClearException()+jsUndefined().test/js/bun/util/BunObject.test.ts: adds a spawned-child regression test that runs a computedimport('bun')underBUN_JSC_validateExceptionChecks=1.test/expectations.txt: quarantinestest-net-connect-memleak.jsas FLAKY on linux-x64-musl, mirroring the existingtest-tls-connect-memleak.jsentry directly above it.
The C++ change is a direct port of the fix already applied to generateNativeModule_NodeModule (verified at src/jsc/modules/NodeModuleModule.cpp:1157), and the per-property get() loop already existed — only the bulk reify and the exception-swallowing wrapper were removed. The export list is unchanged (same enumerable static table entries plus default), and the existing await import('bun') test in the same file already asserts every enumerable Bun property round-trips identically.
Security risks
None identified. This is JSC exception-scope hygiene in module initialization; no auth, crypto, permissions, or user-input handling is involved.
Level of scrutiny
Moderate-to-high. While the diff is small and follows an established in-repo precedent, it sits on the import('bun') initialization path and changes error semantics: a throwing lazy initializer now fails the whole import instead of silently exporting undefined. The PR description argues (reasonably) that initializers are not expected to throw outside OOM/stack-overflow/termination, and that surfacing the real error is strictly better than a half-initialized namespace — but that's a behavior change a maintainer should ack.
Other factors
test/expectations.txtis owned by Jarred-Sumner per.github/CODEOWNERS, which by itself triggers a defer per my guidelines. The added entry is well-justified (twin of the already-quarantined tls test, same FinalizationRegistry-vs-setImmediate timing issue, same musl-x64 matrix, with build numbers cited) and looks routine.- The CodeRabbit concern about
BUN_JSC_validateExceptionCheckstest validity was raised, rebutted by the author with precedent (test/bundler/transpiler/macro-test.test.ts, ASAN CI lane runs the validator), and withdrawn — that thread is resolved. - No bugs were found by the bug-hunting system.
There was a problem hiding this comment.
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/expectations.txt`:
- Around line 29-33: The explanatory comment block for
test-net-connect-memleak.js is too long and must be shortened to 3 lines max.
Trim the existing multi-line note in the expectations entry so it keeps only the
essential context, or move the extra build/matrix details out of the comment;
update the block associated with the LINUX-X64-MUSL flaky test entry without
changing the actual expectation line.
🪄 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: 024af7be-f522-4e50-b6ba-ee57d1290a03
📒 Files selected for processing (1)
test/expectations.txt
There was a problem hiding this comment.
No bugs found. The reify-drop follows the established node:module/node:process pattern, but the switch from tryClearException()→jsUndefined() to RETURN_IF_EXCEPTION diverges from those siblings (both still swallow), and there's an unrelated musl test quarantine riding along — worth a maintainer's eyes.
Extended reasoning...
Overview
This PR touches three files: src/jsc/bindings/BunObject.cpp (drops bulk reifyAllStaticProperties() from generateNativeModule_BunObject and changes exportBunObject's per-export loop to propagate exceptions instead of swallowing them), test/js/bun/util/BunObject.test.ts (adds a spawned-child test under BUN_JSC_validateExceptionChecks=1), and test/expectations.txt (quarantines test-net-connect-memleak.js on LINUX-X64-MUSL).
Security risks
None identified. This is internal JSC exception-scope hygiene for the bun module namespace materialization path; no user-controlled input, auth, crypto, or network surface is touched.
Level of scrutiny
Medium-high. This is C++ JSC exception-scope handling on the core Bun object export path — exactly the class CLAUDE.md flags as most-blocked in review. The reify-drop itself is low-risk: it copies the exact pattern already applied in NodeModuleModule.cpp:1157 and NodeProcessModule.h:16, and the existing await import('bun') test in the same file verifies every enumerable Bun property still appears identically on the namespace.
Other factors
Two things nudge me toward deferral rather than approval:
-
Behavioral divergence from siblings: the new
RETURN_IF_EXCEPTION(scope, void())inexportBunObject's loop propagates initializer errors, whereas bothgenerateNativeModule_NodeModule(line ~1179) andNodeProcessModule.h(line ~39) still usetryClearException()+ fallback value. The PR description justifies propagation well, and it aligns with the "never swallow a failure" guideline — but a maintainer may want to decide whether to accept the divergence or align all three. -
Unrelated test quarantine: the
expectations.txtaddition fortest-net-connect-memleak.json musl is well-argued (twin of the already-quarantined tls variant, same FinalizationRegistry timing flake) and demonstrably unrelated to this fix, but quarantining a Node parallel test is a maintainer call.
The bug hunter found nothing, CodeRabbit's only concern was withdrawn after discussion, and the test coverage (both the new validator test and the pre-existing property-identity test) is solid.
|
CI status for the latest build (67425, commit 2286220):
I already used one retrigger, so I am not pushing more; that lane needs an infra fix or a manual retry. |
|
#37714 removes the bulk reification and the eager getter loop from |
… whole Bun object (#37714) Follow-up to #37525, which made the ES module view of the `src/js` builtins declare their accessors lazily. The `"bun"` module is not one of those: it is a native module (`generateNativeModule_BunObject` in `src/jsc/bindings/BunObject.cpp`), and its generator still built the record eagerly: - `object->reifyAllStaticProperties()` runs every `PropertyCallback` in `bunObjectTable` (about 60 of them: `$`, `SQL`/`sql`/`postgres`, `S3Client`/`s3`, `RedisClient`/`redis`, `Glob`, `TOML`, `WebView`, `secrets`, `stdin`/`stdout`/`stderr`, ...) and stores each result on the object, so that line alone constructs every lazy `Bun.*` property. - `exportBunObject` then called `get()` on each of them to fill the export values. Note that the most common forms never reach this code: the transpiler rewrites a literal `import { write } from "bun"`, `import * as b from "bun"`, `import("bun")` and `require("bun")` into reads of `globalThis.Bun` (`src/js_printer/lib.rs`, `ImportRecordTag::Bun`), which is why `import { write } from "bun"` only ever constructed `write`. The generator runs for `export * from "bun"` / `export { x } from "bun"` and for `import()` with a non-literal specifier, and every one of those constructed all 115 static properties. ## Repro ```js // reexport.mjs export * from "bun"; // entry.mjs import { describe } from "bun:jsc"; import { write } from "./reexport.mjs"; // describe() dumps the Structure, i.e. the static-table entries that have been constructed so far. console.log(describe(Bun).match(/\{[^}]*\}/)[0].split(",").length); ``` Before: 116 entries (`Symbol.toStringTag` plus all 115). After: 2 (`Symbol.toStringTag` and `write`). A non-literal `import(specifier)` of `"bun"` behaves the same way. A second consequence of the eager path: a property callback that throws fails the whole module. `Bun.redis` builds the default client from `REDIS_URL` and throws on an invalid URL, so with `REDIS_URL='http://[::1'` the entry above died at load with `TypeError: Invalid URL format` even though it only wanted `write`. (The `DECLARE_TOP_EXCEPTION_SCOPE` in `exportBunObject` that turned getter errors into `undefined` never got to run for these, because `reifyAllStaticProperties()` propagated first.) ## Fix `generateNativeModule_BunObject` now has the `LazySyntheticSourceGenerator` signature from #37525. It lists the object's own property names (`getOwnNonIndexPropertyNames` includes the static table entries that have not been reified, so dropping `reifyAllStaticProperties()` leaves the export list unchanged, `DontEnum` entries excluded as before), provides `default` = the Bun object, appends an empty `JSValue` for every other export, and returns the Bun object as the source. JSC's `materializeLazyExport` then reads `Bun[name]` the first time something binds to that export: when an importer links a named import of it (directly or through `export *` / `export { x as y } from`), or when it is read off a namespace object. Reading it reifies exactly that one property on the Bun object, the same thing a direct `Bun[name]` access does, so the binding is identical to the property (`main`, the one `CustomAccessor`, is read through its getter like before). Plumbing: the native module list in `NativeModuleList.h` gets a separate `BUN_FOREACH_LAZY_ESM_NATIVE_MODULE` group for generators with this signature (the codegen scanner numbers modules by their order in the file, and `bun` was already last, so the ids do not move); `_NativeModule.h` forward-declares that group with the new return type; `ModuleLoader.cpp` dispatches it through `SyntheticSourceProvider::createWithLazyExports`; and `generateNativeModule` in `InternalModuleRegistry.cpp` (the generated `createInternalModuleById` case, which only needs the default export) becomes a template so it accepts both signatures. ### Behaviour differences - When a `Bun.*` value is sampled: at first binding rather than at module load. The affected values are constructed on first read and then fixed, so the same object is handed out either way; for `main` (a live getter) a re-export now samples the value when it is first bound rather than when the module loads, which is the same behaviour #37525 gave the builtins' accessors. - A throwing callback now throws from whatever binds to that export (the import that links it, or the namespace read, including `Object.keys(ns)` / `console.log(ns)` since those read every export), and the other exports keep working. Previously it failed the module. Of the current callbacks, `Bun.redis` throws on an invalid `REDIS_URL` / `VALKEY_URL`, `Bun.embeddedFiles` can throw on OOM, and `Bun.$`, `Bun.sql`, `Bun.postgres`, `Bun.SQL` would throw if their builtin failed to load; `Bun.s3` reports its error as unhandled and yields `undefined` instead. The error surfacing on the `redis` binding is the same error a direct `Bun.redis` read produces, which the new test pins. - `mock.module("bun", ...)` on an already loaded record writes into the binding with `overrideExportValue`; `materializeLazyExport` is a no-op for a slot that has a value, so the mock wins and the real property is never constructed. - No bulk reification means nothing here runs callbacks back to back without exception checks, which is what #33150 was fixing in the eager path; this supersedes it. Checked with `BUN_JSC_validateExceptionChecks=1` on the re-export, the non-literal `import()`, the throwing `redis` case, and a full `Object.keys(ns)` materialization. ## Tests Added to `test/js/bun/resolve/builtin-esm-lazy-exports.test.ts` (the file from #37525), each in its own process. The readout is `bun:jsc`'s `describe(Bun)`, filtered to a sample of the lazy properties plus `write`; `Object.getOwnPropertyDescriptor` cannot be used because it reifies the property it is asked about. - `export *` / `export { x as y }` / `export { default as ... }` re-exports: linking `import { write }` constructs only `write`; `in` constructs nothing; reading the renamed export constructs `Glob`; reading through the star constructs `SQL`; each is identical to the `Bun.*` value and stable across reads; `default` is the Bun object. - Non-literal `import()`: nothing constructed on import; `Reflect.ownKeys` of the namespace equals `Object.keys(Bun)` plus `default` and constructs nothing; reading `TOML` constructs only `TOML`; every export is `===` the corresponding `Bun` property (which constructs everything). The specifier is imported from the helper module because a `const` holding it can get inlined into a literal `import("bun")` in some files. - `REDIS_URL` set to an invalid URL: the module still loads, `reexported.redis` throws the same error as `Bun.redis`, other exports work. - `mock.module("bun", () => ({ SQL: "mocked" }))` in a `bun test` child after the re-export was imported: the namespace sees the mock, `SQL` is never constructed, `Bun.SQL` is intact. All four fail on the merge base (everything shows up as constructed after import; the `REDIS_URL` case fails at load), and the eight existing cases in the file pass on both. Also run on this build: `test/js/bun/util/BunObject.test.ts` (which `console.log`s a non-literal `import()` namespace of `"bun"` and compares every property), `test/js/bun/resolve/`, `test/js/bun/test/mock/`, `test/js/node/stubs.test.js`, `node-module-module.test.js`, and `test-process-get-builtin.mjs`; all green apart from `load-same-js-file-a-lot.test.ts`, which times out identically on the unmodified build in this environment. ## Startup Wall time of `bun file.mjs`, min of 10 runs after a warm-up, debug (ASAN) builds of the merge base and of this branch on the same machine: | file | before | after | | --- | ---: | ---: | | empty module | 273 ms | 264 ms | | `import { write } from "bun"; write.length` | 277 ms | 251 ms | | same, but `write` comes from a module doing `export * from "bun"` | 555 ms | 279 ms | The literal form is rewritten by the transpiler and was already at the floor; the re-export form loses the roughly 280 ms (debug build) it spent constructing the object. Release numbers to follow in a comment.
|
Closing: superseded by #37714. #37714 (merged 2026-08-12) removed the Verified on current main (bdb7382): |
1 participant
Repro
The computed specifier matters: a literal
import("bun")is resolved by the transpiler and never reaches the native ESM module generator.Cause
generateNativeModule_BunObjectcalledreifyAllStaticProperties()on theBunobject before exporting it.JSObject::reifyAllStaticPropertiesruns every lazy property callback back-to-back with no exception check between them. Several of those callbacks open their own throw scopes, andconstructBunShell(Bun.$) calls into JS, so the exception check verifier aborts when the next callback opens its scope. On a real throw (stack overflow, termination) every remaining initializer would run with a pending exception.node:moduleandnode:processhad the same bug and were fixed by dropping the bulk reify (see the comment ingenerateNativeModule_NodeModule). Thebunmodule kept the old pattern.Fix
exportBunObject's per-exportget()already reifies one property at a time insideJSObject::get's own checked scope, which is the same path a normalBun.fooaccess takes. The export list is unchanged: same names (the enumerable static table entries plusdefault), same values.undefinedexport (tryClearExceptionthenjsUndefined). It now propagates the exception, so the import rejects with the real error instead of resolving to a half-initialized namespace. The initializers are not expected to throw; if one ever does, it is no longer silent.Verification
Added a test to
test/js/bun/util/BunObject.test.tsthat runs the computedimport("bun")in a child process withBUN_JSC_validateExceptionChecks=1.ASSERTION FAILED: exception check validation failed(exit 134) and the test failsawait import('bun')test that checks every enumerableBunproperty is present and identical on the namespaceThe validator only exists in assert-enabled builds, so on release builds the new test just checks that the import succeeds.
CI note: test-net-connect-memleak.js on alpine
Both alpine (linux-x64-musl) lanes failed
test/js/node/test/parallel/test-net-connect-memleak.json this branch (builds 67305 and 67317, every per-file retry) while main's alpine lanes pass it. The code changed here never runs in that test's process: on a binary without this fix, running that test withBUN_JSC_validateExceptionChecks=1exits 0, while a computedimport("bun")aborts, so the module generator is never reached from it. It is the net twin oftest-tls-connect-memleak.js, whichtest/expectations.txtalready quarantines onLINUX-X64-MUSLbecause the singlegc()plus onesetImmediateFinalizationRegistry assertion flips whenever an unrelated change shifts the binary or heap layout on musl x64. This PR adds the net variant to the same quarantine, next to its twin, with the same scope (musl x64 only).