require(esm): return the namespace when the module is evicted from require.cache during evaluation - #38072
require(esm): return the namespace when the module is evicted from require.cache during evaluation#38072robobun wants to merge 4 commits into
Conversation
… require.cache during evaluation require() of an ES module whose top level (or a dependency's) removes the module from require.cache while it is still evaluating threw "require() failed to evaluate module ... internal consistentency error". functionEsmLoadSync re-read the registry entry after loadModuleSync() returned to find the record, and overridableRequire / requireESMFromHijackedExtension then looked the namespace up again with $esmNamespaceForCjs instead of using what $requireESM returned. Both lookups fail once the entry is gone. Hold on to the record that was in the registry when the load started (fetchCommonJSModule parses the file into the registry before require() gets here) and use the namespace $requireESM returns. The eviction then only affects the next require(), which evaluates the file again, the same as it already behaved for a static import.
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
Comment |
|
Updated 3:15 PM PT - Aug 13th, 2026
✅ @robobun, your commit ecb26372b07de4792dd2e0520afde1c534273c48 passed in 🧪 To try this PR locally: bunx bun-pr 38072That installs a local version of the PR into your bun-38072 --bun |
|
Status: reproduced on the released binary and on main with a CommonJS file doing Fix is in this PR. The three new cases in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix is well-reasoned and the tests cover the variant matrix (self-evict, dependency clearing the whole cache with an export * resolved after eviction, and the require.extensions path). Because it changes how functionEsmLoadSync holds a module record across user-JS execution and removes the namespace === undefined fallthrough in overridableRequire, a human look at the JSC/module-loader interaction would still be worthwhile.
Checked: the raw AbstractModuleRecord* held across loadModuleSync() is stack-rooted (conservative scan); dynamicDowncast(nullptr) in the Pending arm is safe when no entry existed and the load's own entry was evicted; $requireESM throws on undefined so the removed fallthrough was only reachable in the bug case; the no-pre-existing-entry path (builtins/mocks) keeps its post-load registry read and behaves as before if that entry is also evicted.
Extended reasoning...
Overview
The PR fixes require(esm) throwing an internal-consistency error when the ES module (or a dependency) deletes its own require.cache entry during top-level evaluation. Three files: src/jsc/bindings/ZigGlobalObject.cpp (functionEsmLoadSync now captures the AbstractModuleRecord* from the registry before loadModuleSync() runs user JS, and reuses it afterward instead of re-reading the possibly-evicted registry), src/js/builtins/CommonJS.ts (overridableRequire and requireESMFromHijackedExtension now use the namespace $requireESM returns instead of a second $esmNamespaceForCjs lookup, and the dead namespace === undefined fallthrough is removed), and three new subprocess tests in test/js/bun/resolve/require.test.ts.
Security risks
None identified. This is module-loader control flow; no parsing of untrusted input, no auth/crypto, no new external inputs.
Level of scrutiny
High. functionEsmLoadSync and the CommonJS builtins are on every require(esm) path. The C++ side now holds a raw JSCell pointer across a call that runs arbitrary user JS — I believe this is safe (JSC's conservative stack scanner roots it, and REVIEW.md says never to blame the scanner), and the record is the same one loadModuleSync() evaluates since fetchCommonJSModule() already stored it in the entry. On the JS side, removing the fallthrough is correct because requireESM throws when exports === undefined, so control never returned to $evaluateCommonJSModule except in the bug scenario (yielding {}). But module loading interacts with SCC/TLA/cycle handling in subtle ways, so someone with JSC module-loader context should confirm the captured-record approach doesn't miss a case where loadModuleSync() replaces the record on an existing entry.
Other factors
The PR description is unusually thorough and enumerates the flow, why the held record is authoritative (namespace resolution walks each record's own loaded-modules map, not the registry), and what was left unchanged. Tests assert the eviction applies to the next require (evaluation counter increments, namespaces differ), assert stderr is empty and exit code last, use tempDir/bunRun per harness conventions, and path.join(dir, ...) on a DisposableString matches existing usage in test/js/bun/io/bun-write-leak.test.ts and others. I also checked that dynamicDowncast<CyclicModuleRecord>(record) with record == nullptr (the builtin/mock path when the freshly-created entry is itself evicted) returns null and falls through to the same error as before — no regression there. The "consistentency" typo fix is applied to both messages. Deferring only because this is core loader code, not because of any specific concern.
|
On the one open point (whether |
…d; share the namespace shaping Hold the ModuleRegistryEntry rather than its record: an entry that an import() graph is still fetching has no record when require() starts, and the record is created on that same cell during the load. Look the entry up in the (key, JavaScript) bucket that loadModuleSync() loads; registryEntry() also matches the JSON/HostDefined entry registered by an import with a type attribute, whose record this load does not evaluate. Fold the __esModule / "module.exports" shaping that overridableRequire and the Module._extensions path each had a copy of into one builtin, requireESMIntoModule, and drop the unreachable undefined check in requireESM.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the core require(esm) load path in functionEsmLoadSync (holding a raw ModuleRegistryEntry* across loadModuleSync(), which runs arbitrary user JS) and reshapes the builtin-JS side of the same flow, a maintainer familiar with the JSC module loader should confirm the record-immutability invariant and GC-safety reasoning.
What was reviewed:
- Traced that
functionEsmLoadSyncnow always returns a namespace or throws, so the removedundefinedfallthrough inoverridableRequire/requireESMwas dead outside the eviction case. - Checked the
registryEntry(any type, forentryExistedBefore) vsjavaScriptRegistryEntrysplit; the new attribute-typed test pins the right entry is used. - Verified the rename is complete across
JSCommonJSExtensions.cpp,ZigGlobalObject.{h,cpp},NodeModuleModule.cpp,BunBuiltinNames.h, and the lazy-getter registration.
Extended reasoning...
Overview
This PR fixes require() of an ES module that removes itself from require.cache while its top level is still running. It touches functionEsmLoadSync in ZigGlobalObject.cpp (holds the JavaScript-typed registry entry across loadModuleSync() instead of re-looking it up afterwards), consolidates the duplicated __esModule-shaping logic in CommonJS.ts into a single requireESMIntoModule builtin (called from both overridableRequire and the C++ Module._extensions loader), renames requireESMFromHijackedExtension accordingly across four files, adds a new private builtin name with a lazy getter, and adds five new subprocess tests in require.test.ts.
Security risks
None identified. This is module-loader control flow; no untrusted-input parsing, auth, or crypto.
Level of scrutiny
High. functionEsmLoadSync is on the hot path for every require() of an ES module, and ZigGlobalObject.cpp is one of the most critical files in the codebase. The correctness of the fix rests on a JSC-internal invariant (an entry's record is written only in fetchComplete and moduleLoadStep, never swapped once set) and on the conservative stack scanner keeping the held ModuleRegistryEntry*/AbstractModuleRecord* alive after the entry is unlinked from the registry mid-evaluation. The PR description argues both points carefully and the tests exercise the eviction, but a maintainer who knows the loader should confirm.
Other factors
The bug-hunt pass found nothing. The comment-cop bot's long-comment flags were addressed in follow-up commits and all threads are resolved. Tests use tempDir+bunRun per harness conventions, assert exact JSON output, cover both entry points (overridableRequire and the require.extensions wrapper), and include edge cases (dependency clears the whole cache with an export * resolved after eviction; entry still Fetching from a concurrent import(); attribute-typed sibling entry). The dedup of the two builtin-JS namespace-shaping blocks is a net simplification. No behavior change to the already-evaluated fast path or the TLA/Pending handling.
…#40123) ### Problem - `delete require.cache[path]` of an ES module that is still loading evicts its registry entry. The next `import()` of the path builds a second record for the same module while the loader's `[[LoadedModules]]` cache and pending microtasks hold the first one. Release: `Segmentation fault at address 0x10` in `JSModuleLoader::loadModule`. Debug: `ASSERT(loadedEntry->record() == loaded)` at `JSModuleLoader.cpp:630` (`import()`), `ASSERT(iter->value.m_module.get() == *resultRecord)` at `:955` (`require()`). When the records line up, the module evaluates twice. - Cause: `functionEsmRegistryDelete` (`src/jsc/bindings/ZigGlobalObject.cpp`) calls `removeEntry()` for every state. The `has` and `get` traps already answer "not in the cache" for a module that has not finished evaluating, so the delete acts on a key the cache says is absent. - Shape: a CommonJS dependency of an ES module runs the decache idiom (`delete require.cache[importer]; import(importer)`) while the importer is mid-load. ### Fix - `functionEsmRegistryDelete` evicts only entries whose load has settled: the record is `Evaluated` (with or without an error) or the entry caches a fetch, instantiation, or evaluation error. A loading module keeps its entry, so a later `import()` or `require()` joins the in-flight load. Node evaluates the module once in all of these cases. - Deleting an evaluated module still evicts it. The decache idiom and the leak tests are unchanged. - Verified: `test/cli/run/require-cache.test.ts`, 5 new tests. Four fail on the released binary (one segfault, three double evaluations). Also `test/js/node/module/`, `test/js/bun/test/mock/`, `test/js/bun/resolve/require*`, `test/js/bun/plugin/`, `test/cli/hot/hot.test.ts`. ### Background - The registry maps a module key to a `ModuleRegistryEntry`: the load promises, the status (`New`, `Fetching`, `Fetched`, or a failure), and once fetched the record. Bun exposes evaluated ES modules through `require.cache` so `delete` can evict them. `removeEntry()` is a Bun addition to JSC. - A module's CommonJS dependencies run while the module's graph is still being fetched (the CJS body runs when the synthetic module record is made), before the module itself evaluates. That is the window these repros hit. - `[[LoadedModules]]` is the per-realm and per-module cache from a specifier to the record it loaded. It is only valid while it agrees with the registry. <details><summary>Notes</summary> Direction: #39674 (WebKit bump for oven-sh/WebKit#472) fixes the same crash from the engine side and keeps the opposite semantics: deleting an in-flight module evicts it and the next `import()` loads the file again. Its `mock.module()` and `Bun.plugin` coverage is still needed with this change (those paths call `removeEntry()` directly and are not gated here). Two of its `require-cache.test.ts` tests assert the evict semantics (`loads: [1, 2]`, and a second `import()` that starts a replacement load while the first is held) and would need to change if this lands. #38072's tests assert that a module which deletes itself during its own evaluation re-evaluates on the second `require()`; with this change the second `require()` returns the same namespace. Repro (3 files, release 1.4.0): ```js // entry.mjs import("./a.mjs").then(ns => console.log("entry", ns.x)); // a.mjs import "./b.cjs"; export const x = 1; console.log("a evaluated"); // b.cjs delete require.cache[__dirname + "/a.mjs"]; // `in require.cache` is false here import("./a.mjs").then(ns => console.log("b", ns.x)); ``` Before: `a evaluated`, `entry 1`, then the segfault. `bun a.mjs`: `a evaluated` twice. With `require("./a.mjs")` in b.cjs: `a evaluated` twice (debug: the `:955` assert). Deleting from a CJS module `require()`d by the module's own top level (record `Evaluating`): `a evaluated` twice. After: one evaluation and both imports resolve to the same namespace in all four shapes. Node prints `a evaluated` once for each. Under the debug ASAN build the RSS leak tests in this file time out with and without this change (they did before it too). </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/run/require-cache.test.ts <!-- robobun:evidence:end -->
|
#40123 merged (bc4bea9). Measured on main at 1d487c4 (debug build) with this PR's four tests and no other change from this PR:
So the crash this PR was opened for is fixed on main. What is left here is the |
|
Follow-up to the comment above. #40123 does not make this PR unnecessary. The same TypeError still occurs on main through two other eviction paths. Measured on a debug build of main at a749e0a. Already on main
Still throws on main
// self-virtual.mjs
Bun.plugin({
name: "virtual",
setup(build) {
build.module(import.meta.path, () => ({ exports: { x: 2 }, loader: "object" }));
},
});
export const x = 1;
// entry.cjs
console.log(require("./self-virtual.mjs").x);
// root.mjs
import "./dep.cjs";
export const x = 1;
// dep.cjs
require("bun:test").mock.module(require.resolve("./root.mjs"), () => ({ x: 2 }));
// repro.test.cjs
const { test, expect } = require("bun:test");
test("require() of a module that a dependency mocks mid-load", () => {
expect(require("./root.mjs").x).toBe(1);
});
The fix here still appliesWith only the Suggested next stepKeep this PR open. Rebase it and reduce it to the |
This branch has not been deployed
2 participants
Problem
require()of an ES module that removes itself fromrequire.cachewhile its top level is still running (directly, or through a dependency that clears the cache) throws instead of returning the namespace:TypeError: require() failed to evaluate module "/abs/self-evict.mjs". This is an internal consistentency error.importandimport()of the same file work; only the require() path is affected. Reproduces on the released 1.4.x binaries and on main.functionEsmLoadSync(src/jsc/bindings/ZigGlobalObject.cpp, theregistryEntry(key)after theloadModuleSync()switch) re-reads the registry to find the record it just evaluated. The eviction removed the entry during evaluation, so it falls into the internal-error branch.overridableRequireandrequireESMFromHijackedExtension(src/js/builtins/CommonJS.ts) discard what$requireESMreturns and call$esmNamespaceForCjs(id)again. With the entry gone that isundefined, and require() would return the empty placeholdermodule.exports({}); that is what the tests get when only the C++ half is applied.jest.resetModules()called at the top level of a module that is itself being require()d hits the same path), to be handled separately from that PR.Fix
functionEsmLoadSync: read the record off the registry entry before callingloadModuleSync()and use that record afterwards (Pending/SCC check,evaluationError()check, namespace). The post-load registry lookup is kept only for the case where no entry existed before the call (builtin ES modules, module mocks), because there the load itself creates the entry.overridableRequire/requireESMFromHijackedExtension: use the namespace$requireESMreturns.$requireESMreturns a namespace or throws, so thenamespace !== undefinedfallthrough (which returned{}) is removed.functionEsmLoadSyncafterfetchCommonJSModule()parsed the file into the registry, andloadModuleSync()links and evaluates the record stored in that entry. An entry's record is written in only two places in JSC (ModuleRegistryEntry::fetchComplete, which runs once when the entry goes Fetching -> Fetched, andmoduleLoadStep'ssetRecord, which stores the value the same entry's module promise resolved with, i.e. that same record), so an entry that already has a record never switches to a different one, and the record read before the load is the instance whose top level ran. Its namespace does not depend on the registry either: export resolution, includingexport *, walks each record's own loaded-modules map.require.cachehas no ESM entries, so the delete is a no-op there).require.cacheproxy, which still answers from the registry via$esmNamespaceForCjs.bun bd test test/js/bun/resolve/require.test.ts: 3 new tests (module deletes itself; a dependency clears the whole cache while the root is mid-load, with anexport *resolved after the eviction; therequire.extensionswrapper path). All 3 fail on the released binary with the TypeError above, fail with{}exports when only the C++ change is applied, and fail with the TypeError when only the JS change is applied.test/js/bun/resolve(therequire-esm-*,esModule*files included),test/regression/issue/24387.test.ts,test/js/node/module/require-extensions.test.ts,node-module-module.test.js,test/js/bun/test/mock/mock-module*.test.ts,test/cli/run/require-cache.test.ts. The only failures were timeouts of the RSS leak loops and the 2000ximport()loops under the debug+ASAN build, on fixtures that never enter the require(esm) path, plus twoenableCompileCacheownership tests that also fail on the released binary when run as root.Background
JSModuleLoadermaps a resolved path to aModuleRegistryEntry, which holds the module record (the parsed module: its bindings, environment, and the records it imports).delete require.cache[key]for an ESM key calls$esmRegistryDelete, which removes the entry. The record itself stays alive while something references it, here the C++ local and then the namespace object handed back to the caller.overridableRequire(CommonJS.ts) calls the native$require;fetchCommonJSModuletranspiles the file, sees it is ESM, stores the source in the registry (which parses it into a record) and returns -1;overridableRequirethen calls$requireESM, which tries$esmNamespaceForCjs(registry lookup, undefined unless already evaluated) and otherwisefunctionEsmLoadSync, whoseloadModuleSync()(a Bun addition to JSC that drains the loader's promise reactions synchronously) links and evaluates the graph. The module's top level runs inside that call, which is when the eviction happens.getModuleNamespace(). Resolving its exports, includingexport *re-exports, uses the loaded-modules map each record filled in while its graph was loading, not the registry.requireESMFromHijackedExtension: the variant used when user code installs a function inrequire.extensionsthat calls the builtin loader (Module._extensions['.js']); it assigns the namespace tomodule.exportsitself, so it had the same second lookup.