Repository navigation
Conversation
…ookup
The original of a mocked builtin is the builtin loaded once more, under the
key `<builtin>?actual`. Two places turned that key back into the builtin:
runVirtualModule() and fetchBuiltinModuleWithoutResolution(). Both did so only
while a table of mocks existed.
Three failures came from that:
- A mock of a loaded builtin calls its factory before the mock is registered.
With no table yet, or after Bun.plugin.clearAll(), importOriginal() rejected
with `ENOENT reading "node:fs"`.
- Outside `bun test` the builtin lookup runs before runVirtualModule(), so the
key was never a builtin there.
- runVirtualModule() rewrote the caller's key. require("bun:sqlite") in the
factory of its own mock then found that mock and threw `Circular import`.
The two call sites of Bun__fetchBuiltinModule now share one helper. When the
key misses, the helper checks it for the query and asks again for the builtin.
It is not gated on `bun test` or on a registered mock. The only mock state it
reads is whether a module is registered under that exact key. The rewrite in
runVirtualModule() and the gated arm in fetchBuiltinModuleWithoutResolution()
are removed.
builtinModuleOfOriginalKey() takes the builtin's name from
withoutOriginalModuleQuery(), so it also accepts the `&actual` that a copy of
the original carries.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit details
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. |
|
Status How this was reproduced, on a debug build of main (2596ac5): // fs.test.ts
import { expect, mock, test } from "bun:test";
import * as fs from "node:fs";
const patched = mock.module("node:fs", async importOriginal => ({ ...(await importOriginal()), readFileSync: () => "mocked" }));
test("importOriginal of a loaded builtin", async () => {
await patched;
expect(fs.readFileSync("x")).toBe("mocked");
});
PR: #48305 CI on d72c5ea (build 139138, at 06:10 UTC on 2026-11-09)
No change to the diff is pending. The PR is ready for a maintainer to review. |
|
Updated 2:29 AM PT - Nov 9th, 2026
✅ @robobun, your commit d72c5ea61d008d47f9e42391cd2a30984fd979b9 passed in 🧪 To try this PR locally: bunx bun-pr 48305That installs a local version of the PR into your bun-48305 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the lifetime of the original BunString that specifier/builtinKey is rebound to in both callers of fetchBuiltinModuleOrItsOriginal — it is a function-scope local whose EncodedSlice points at the static builtinModuleKeys literal, so the ZeroCopy moduleKey view in fetchESMSourceCode does not dangle; and endsWithOriginalModuleQuery branches on is8Bit() before reading bytes. The new tests were not run as part of this review.
Extended reasoning...
The change rewires the two Bun__fetchBuiltinModule call sites (CJS require and ESM fetch) through a helper that decodes a <builtin>?actual key on a miss, removes the specifier rewrite in runVirtualModule, and broadens builtinModuleOfOriginalKey to strip any &actual/&spy tail; it touches no security-sensitive surface. Three confirmed findings are posted inline (coverage naming under an onLoad plugin, a registered virtual module under the literal ?actual key now failing on the CJS path, and spy/patch mocks under a ?actual key skipping their exports snapshot), so a human look is already signalled; this note only records the pointer-lifetime and encoding checks that were examined and ruled out.
…erves runVirtualModule() again names what an onLoad plugin answers for `<builtin>?actual` by the builtin, as it did while a mock was registered. It now does so for the first mock of a run too, so the name no longer depends on the table of mocks. Under `bun test` a key that something is registered under is decoded where only the builtins are asked: require() of such a `node:` name gives the builtin, as before. Outside `bun test` the registered module keeps its answer. esmExportsBeforePatches() asks isBuiltinModuleKey() about the mock's own specifier. It no longer builds the key of the original to decode it again.
There was a problem hiding this comment.
I reviewed the updated PR (through d72c5ea) and found no bugs; the follow-up commit addresses the three inline comments from my earlier review. Because this changes the gating of the builtin lookup in the module loader and the rename step in runVirtualModule, a human look is still worthwhile.
What was reviewed:
- Lifetimes:
originalin both call sites holds a view of a static builtin-key literal, andspecifier/builtinKeyonly point at it inside the calling function;createCommonJSModuleand the registry still use the unrewritten key. - The rename in the
fallbacklambda runs afteronLoadPlugins.runwith no check macro inside the lambda, andbuiltinOfOriginalModuleKeyenters no JS;endsWithOriginalModuleQueryreadsspan8()of the WTFStringImpl only, so the "makes no String" claim holds. - A
&spy-suffixed original of a builtin is still not decoded at the builtin lookup, as on the base branch (pre-existing, not introduced here). - Not run: the new fixtures in mock-module.test.ts and BUN_JSC_validateExceptionChecks=1.
Extended reasoning...
The change touches src/jsc/bindings/ModuleLoader.cpp and src/jsc/bindings/BunPlugin.cpp, moving the "?actual" to builtin translation into a helper used by both the CommonJS and ESM builtin lookups without the bun-test/registered-mock gate, and moving the specifier rename in runVirtualModule into the onLoad fallback; it adds about 360 lines of subprocess fixtures. It touches no auth, crypto, or injection surface. The second commit was pushed after my earlier review and resolves each of the three inline comments (plugin-served originals keep the builtin's name, esmExportsBeforePatches uses isBuiltinModuleKey, and the virtual-module gate is skipped under bun test). No CODEOWNERS entry covers the changed files and the hunt ran dry, but the module loader is core runtime with a new check on every non-builtin load and subtle test-vs-run gating, so a maintainer should still look rather than relying on automated approval.
|
The review above lists two items as not run. Both were run locally on d72c5ea, with debug builds (ASAN):
|
Problem
importOriginal()in the factory of a mocked builtin rejects withENOENT reading "node:fs". It needs a loaded builtin and no registered mock. Example:mock.module("node:fs", factory)below the import.node:fs?actual.runVirtualModule(src/jsc/bindings/BunPlugin.cpp:5280) andfetchBuiltinModuleWithoutResolution(src/jsc/bindings/ModuleLoader.cpp:656) decode it only whilevirtualModulesexists. The first registered mock allocates that map.mockModule()calls the factory of a loaded module before it registers the mock.Fix
fetchBuiltinModuleOrItsOriginalwraps both call sites ofBun__fetchBuiltinModule. When the key misses, it checks for the query and asks for the builtin. It has no gate, soBun.plugin.clearAll()andbun runwork.runVirtualModulerenames the key only for anonLoadanswer. Its rewrite before the lookup maderequire("bun:sqlite")in its own factory throwCircular import.builtinModuleOfOriginalKey()useswithoutOriginalModuleQuery(), sonode:os?actual&actualdecodes.test/js/bun/test/mock/mock-module.test.ts: 8 new tests fail on main. Alsoplugins.test.ts. Self-reviewed: 11 concerns raised, 9 addressed, 2 in part (Notes).Background
importOriginalreturns the real module behind a mock (bun test: Vitest compatibility #44833, unreleased). For a builtin, Bun loads it again under the key<builtin>?actual.runVirtualModule: it leavesbun runand CommonJS failing.Downsides
isBunTest || hasVirtualModules()avoids 12 of them and keepsbun runfailing.bun run, anonLoadplugin in thenodenamespace is not asked for that original.Notes
Origin. No GitHub issue. #48218 lists this failure under "Not changed here". #44833 is in no release, so this should merge before the first release that has it.
Main and this PR, debug builds of main 2596ac5 and of this branch:
mock.module("node:fs", async importOriginal => ...)below the import, one fileENOENT reading "node:fs"vi.mockand a--preloadthat imports the builtin, also with--isolateand--isolate=modulesvi.mockin a second test file, after one that imported the builtinvi.doMockin a test. A factory that callsrequire("./fake.cjs")first. A factory that does not awaitimportOriginal()oswith no prefix,bun:sqlite,node:buffer,node:processimportOriginal()afterBun.plugin.clearAll(): in a pending factory, or kept and called laterASSERTION FAILED: m_status == Status::Fetchingnode:os?actual&actual)require("bun:sqlite")in the factory of its own mock, which the first import callsCircular import: "bun:sqlite" is imported by a module that its own mock factory loads while it runsbun run:importOriginal(), andvi.importActual()of a mocked loaded builtinbun run:require("node:os")in a pending registered factory (works in 1.4.3)Failed to fetch builtin modulenode:fsin the working directoryThese pass on main too: a run where another mock is registered first, and a factory that spreads
vi.importActual(),jest.requireActual(),require()orimport().The three causes and their hunks.
bun test: before the first mock is registered, or afterBun.plugin.clearAll(). Fixed by the decode on a miss of the builtin lookup.bun run: the builtin lookup runs beforerunVirtualModule, so the key was never a builtin. Fixed by the same hunk.require()of a builtin whose id has nonode:prefix, in a factory that the first import calls.runVirtualModulerewrote the caller's key tobun:sqlite, and the mock lookup infetchBuiltinModuleWithoutResolutionthen found the running mock. Fixed inrunVirtualModule: it renames the key only after anonLoadplugin answers, which is after the mock lookup.Why the decode reads no state of running factories. lldb at
importOriginalModule: when the factory callsrequire("./fake.cjs")beforeimportOriginal(),runningModuleMocks.size()is 0 andvirtualModulesis null.keyOfImportWhileModuleMocksRun()detaches a running mock that is not registered yet. A table made before the factory runs does not coverBun.plugin.clearAll().Measurements. Release builds (LTO) of main 2596ac5 and of this branch.
fetchBuiltinModuleWithoutResolutionforrequire()of a file: 603 on main, 615 here.runVirtualModulewith no plugin and no mock: 232 on main, 234 here. The ES module site runs the same 12 instructions at the lookup (llvm-objdump). 0 calls, 0lockinstructions, no allocation. A key that ends inlruns 5 more.call Bun__fetchBuiltinModule,test,je). The helper is inlined. The decode is one function out of line.require("node:os"), user-mode instructions per call from aperf_event_opencounter, median of 5 runs.bun run: 2,679 on main, 2,679 here. In abun testfile after onemock.module(): 3,364 on main, 3,251 here. Main decoded the key at every call there.import(), 2,000require()), whole process, median of 7 runs: 553.50 M instructions on main, 553.10 M here. Runs of one binary differ by 0.7 %. The counts above predict a difference of 56,000.Bun__fetchBuiltinModule, lldb breakpoint counts on debug builds: 1,002 on both for 1,000 loads with no mock. 4 on main and 5 here for oneimportOriginal()of a builtin.size: text 137,931,057 to 137,932,081. Data and bss are equal. By symbol, fromnm -S:fetchESMSourceCode<true>+429,fetchESMSourceCode<false>+350,fetchBuiltinModuleWithoutResolution+45,runVirtualModulewithOnLoad::run+225,esmExportsBeforePatches-169, and the newbuiltinOfOriginalModuleKeyis 330.extern "C"symbols +0.perf,valgrind,straceandbloatyare not in the build container.test/bundler/compile-aot-size.test.tsskips its 42 tests on a local build.Behaviour outside the symptom.
bun run: the builtin lookup runs before plugins. AnonLoadplugin in thenodeorbunnamespace is not asked for the original of a builtin. It is not asked for the builtin either. The new test pinsasked: [].<builtin>?actualkey that reaches the loader with no mock now loads the builtin. A replacedModule._resolveFilenameor anonResolveplugin can return one. The resolver still rejects such a specifier in a script:No such built-in module: node:fs?actual.build.module()or a mock registered under exactly such a key. Outsidebun testit keeps its answer: the new test usesnode:zlib?actual, and fails without that check. Underbun test,import()asks it first andrequire()of such anode:name gives the builtin, both as on main. A new test pins both.bun testand anonLoadplugin that serves a builtin: the plugin is asked first, is shown the same path, and what it returns for the original has the builtin's name in stack frames and coverage rows. On main that name held only once a mock was registered. In the first mock of a run the frame wasnode:os?actual:1:93. It isnode:os:1:93in both cases now, and a new test pins both.Not changed here. All four behave the same on main and with this PR.
vi.importActual()andjest.requireActual()in the factory of a loaded module return the loaded module, because the mock is not registered yet. A factory that calls through it,platform: () => "wrapped:" + actual.platform(), throwsRangeError: Maximum call stack size exceeded. Related: mock.module: captured module namespace is mutated in-place, breaking partial-stub delegation pattern #30242. With this PR the same factory works withimportOriginal.build.module()module:ENOENT reading "virt:thing". Bun.plugin: load the original of a mocked module under the path onLoad was shown #48218 lists it too.Bun.plugin.clearAll()orvi.doUnmock():ResolveMessage: Cannot find packagewith a NUL character as the name.<builtin>?actual&spyis not decoded at the loader. Only a builtin with no CommonJS form gets that key.vi.doMock("bun:wrap", { spy: true })givesENOENT reading "bun:wrap".Self-review. 11 concerns. Addressed: a test that reaches
node:os?actual&actual, one strip function for the query, tests that call what the factory was given after the mock applies, an assertion that could not fail, tests fornode:buffer,node:processandrequire("bun:sqlite")underbun run, thebun runplugin order named and tested, the three causes named, a one-line comment in place of four, the counts and the gate question, derived constants. In part:vi.importActual()andjest.requireActual()in the factory, which never reach the changed code. Notest.todois added for the call-through case: it is the behaviour of mock.module: captured module namespace is mutated in-place, breaking partial-stub delegation pattern #30242.BunPlugin.h:281is not edited. It is still a true example, and an edit there conflicts with the hunk that Bun.plugin: load the original of a mocked module under the path onLoad was shown #48218 adds below it.Review of fee07ab. The review left three optional findings. All three are addressed in d72c5ea:
runVirtualModulerenames the key again where a plugin answered.require()of a key that something is registered under threwFailed to fetch builtin moduleunderbun test. The check for a registered key now applies outsidebun testonly, whererunVirtualModuleis asked after the builtins.esmExportsBeforePatches()built the key of the original and decoded it again. It asksisBuiltinModuleKey()now, which gives the answers of main for every specifier.Open PRs near this code. #48178 and #47819 edit the
findModuleMockline infetchBuiltinModuleWithoutResolution. #48218 editswithoutOriginalModuleQuery().git merge-treeof d72c5ea with each of the three heads is clean.Suites run with the debug build.
mock-module.test.ts(315 pass, 1 todo),plugins.test.ts(270),mock-hoist.test.ts(134),vitest-flavor.test.ts(191),mock-fn.test.js(848),builtin-esm-lazy-exports.test.ts(25),import-attributes.test.ts(58),transpiler-cache.test.ts(34),isolation.test.ts(151 pass, and 1 test that times out at 5 s on main too),mock-module-non-string.test.ts,mock-module-resolve-log.test.ts,6874,6879,missing-module.test.js,disallow-code-generation-from-strings.test.ts, regression tests11664,12548,ENG-24434. The new tests also pass withBUN_JSC_validateExceptionChecks=1.test/aot/mock-actual-builtin.test.tsfails insetters.test.mjson both builds.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/test/mock/mock-module.test.ts