Conversation
…ore ESM linking
When `mock.module()` is called at the top level of a test file alongside
static imports of the same module, ESM import hoisting causes the real
module to be linked before `mock.module()` executes. If the real module
doesn't export the expected names, JSC's linker throws a SyntaxError.
This fix detects top-level `mock.module("specifier", fn)` calls during
parsing and registers placeholder virtual modules with the expected
export names before the module graph is built. When `mock.module()`
later runs at evaluation time, it replaces the placeholder with the
real mock via `overrideExportValue`.
Closes #18358
Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Comment |
|
🔴 c58ed — 1 issue found
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Register placeholder virtual modules for mock.module() targets. | ||
| if (parse_result.ast.mock_module_specifiers.count() > 0) { | ||
| @import("./ModuleLoader.zig").registerPendingMockModules(vm, &parse_result, path); | ||
| } |
There was a problem hiding this comment.
🔴 Critical: Thread-safety bug: registerPendingMockModules is called from a worker thread (inside TranspilerJob.run()), but it creates JSC heap objects and mutates globalObject->onLoadPlugins which are not safe for concurrent access from non-JS threads. This call should be deferred to runFromJSThread() (which already runs on the main JS thread) by storing the mock specifier data on the TranspilerJob and processing it there.
Why this is a problem
Thread-Safety Violation in registerPendingMockModules on Worker Thread
This PR introduces a call to registerPendingMockModules at line 577-580 of RuntimeTranspilerStore.zig, inside the TranspilerJob.run() function. The problem is that run() executes on a worker thread dispatched via jsc.WorkPool (see schedule() at line 280-282 and runFromWorkerThread() at line 285-287), but registerPendingMockModules performs multiple JSC heap allocations and object mutations that are fundamentally unsafe to perform outside the main JavaScript thread.
Specifically, registerPendingMockModules in ModuleLoader.zig accesses jsc_vm.global and calls the C++ function Bun__registerPendingMockModule. This C++ function performs the following JSC heap operations: JSC::constructEmptyObject() (line 718), placeholderExports->putDirect() (line 722/727), JSModuleMock::createStructure() (line 732), JSModuleMock::create() (line 733), and globalObject->onLoadPlugins.addModuleMock() (line 738). Every one of these operations allocates on or mutates the JSC garbage-collected heap, which is not thread-safe for concurrent access from non-JS threads.
The same registerPendingMockModules call exists in ModuleLoader.zig at line 543, where it is safe because transpileSourceCode runs synchronously on the main JS thread. The RuntimeTranspilerStore is the async transpilation path, specifically designed to offload work to worker threads, and the newly added code does not account for this threading model.
Step-by-step proof of the bug
- A test file imports a module (e.g.,
import { foo } from './helper.ts') wherehelper.tscontainsimport { mock } from 'bun:test'; mock.module('./dep', ...)along with a staticimportof./dep. - The test runner loads the test file via the synchronous
ModuleLoader.transpileSourceCodepath (since it isis_main). - When the test file's import of
./helper.tsis resolved, and conditions are met for concurrent transpilation (concurrent_transpilerenabled,has_loadedis true, no plugin runner, etc.), aTranspilerJobis created andschedule()dispatches it to thejsc.WorkPoolthread pool. - A worker thread picks up the task and calls
runFromWorkerThread(), which callsrun(). - Inside
run(), the parser populatesparse_result.ast.mock_module_specifiersbecausehelper.tscontainsmock.module()calls andinject_jest_globalsis set. - At line 577-580,
registerPendingMockModulesis called on the worker thread. registerPendingMockModulescallsBun__registerPendingMockModule, which performs JSC heap allocations (constructEmptyObject,JSModuleMock::create) and mutates shared state (globalObject->onLoadPlugins.addModuleMock) without any lock or synchronization.- Concurrently, the main JS thread may be running GC, allocating objects, or accessing
onLoadPlugins—causing heap corruption, use-after-free, or crashes. - Only after
run()returns doesdispatchToMainThread()enqueue the result back to the main thread—but by then, the unsafe JSC operations have already been performed.
Suggested fix
The mock module registration should be deferred to the main JS thread. The TranspilerJob struct should store the mock specifier data (specifier strings and export names) extracted during parsing on the worker thread, and then runFromJSThread() (which already runs on the main JS thread after dispatchToMainThread()) should call registerPendingMockModules with the stored data. This follows the existing pattern where all JSC object creation is deferred to the main thread callback.
|
Closing this PR because it has been inactive for more than 90 days. |
1 participant
Summary
mock.module()failing when the mocked module is also statically imported in the same test filemock.module("specifier", fn)calls during parsing and pre-registers placeholder virtual modules with the expected export names before the ESM module graph is builtSyntaxError: Export named 'X' not foundwhen the real module doesn't have the expected exportsProblem
When a test file uses
mock.module()alongside a static import of the same module:ESM import hoisting causes the static import to be resolved and linked before
mock.module()executes. If the real module doesn't exportregisterApolloClient, JSC's linker throws aSyntaxError.Solution
During transpilation, the parser now detects top-level
mock.module("specifier", callback)calls and records the specifier strings. After transpilation but before the module graph is built, placeholder virtual modules are registered with the expected export names (derived from the file's static imports of the same specifier). When JSC's module loader encounters these specifiers, it finds the placeholder instead of the real module. Later, whenmock.module()actually executes, it replaces the placeholder with the real mock viaoverrideExportValue.Test plan
test/regression/issue/18358.test.tspasses with debug buildtest/js/bun/test/mock/mock-module.test.tstests all pass (no regressions)Closes #18358
🤖 Generated with Claude Code