Fix require() of ESM with diamond through barrel deadlocking (#30493)#30497
Fix require() of ESM with diamond through barrel deadlocking (#30493)#30497robobun wants to merge 1 commit into
Conversation
Adds regression test for #30493. The fix is in oven-sh/WebKit#223: add the 'modulePromise already settled' bail to moduleRegistryModuleSettled so the queued fetch-chain reaction doesn't call fetchComplete a second time after hostLoadImportedModule's inline sync path already handled the entry. Repro: require('./app.js') where app.js directly imports shared.js AND reaches it through a.js/b.js -> barrel.js -> shared.js, with shared.js importing a synthetic builtin (e.g. 'import path from "path"'). On Linux debug the assertion in ModuleRegistryEntry::fetchComplete trips; on Darwin the process deadlocks waiting on a promise chain whose microtask already ran.
WalkthroughThis PR adds a regression test for Bun's module loader to ensure CommonJS ChangesModule Loader Regression Test
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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/regression/issue/30493.test.ts`:
- Line 37: The assertion hardcodes "/" as the path separator which fails on
Windows; update the test that calls
expect(JSON.parse(stdout.trim())).toEqual(...) to use a platform-aware separator
(e.g., import isWindows from the test harness or use path.sep) and build the
expected object conditionally (use "\\" when isWindows is true, otherwise "/")
so the expected values for fields "a" and "shared" use the correct separator
across platforms.
🪄 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: d79c5f2a-208e-4b4d-99d0-4475c05705b8
📒 Files selected for processing (1)
test/regression/issue/30493.test.ts
|
|
||
| expect(stderr).toBe(""); | ||
| // shared: path.sep on darwin/linux is "/", on windows "\\". | ||
| expect(JSON.parse(stdout.trim())).toEqual({ a: "/", b: "barrel", shared: "/" }); |
There was a problem hiding this comment.
Assertion will fail on Windows due to hardcoded path separator.
The comment on line 36 correctly notes that path.sep is "/" on Unix and "\\" on Windows, but the assertion hardcodes "/" for both a and shared fields. This test will fail on Windows CI.
🔧 Proposed fix for cross-platform compatibility
Import isWindows from harness and use a platform-conditional expectation:
import { expect, test } from "bun:test";
-import { bunEnv, bunExe, tempDir } from "harness";
+import { bunEnv, bunExe, isWindows, tempDir } from "harness";Then update the assertion:
+ const sep = isWindows ? "\\\\" : "/";
expect(stderr).toBe("");
// shared: path.sep on darwin/linux is "/", on windows "\\".
- expect(JSON.parse(stdout.trim())).toEqual({ a: "/", b: "barrel", shared: "/" });
+ expect(JSON.parse(stdout.trim())).toEqual({ a: sep, b: "barrel", shared: sep });
expect(exitCode).toBe(0);🤖 Prompt for 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.
In `@test/regression/issue/30493.test.ts` at line 37, The assertion hardcodes "/"
as the path separator which fails on Windows; update the test that calls
expect(JSON.parse(stdout.trim())).toEqual(...) to use a platform-aware separator
(e.g., import isWindows from the test harness or use path.sep) and build the
expected object conditionally (use "\\" when isWindows is true, otherwise "/")
so the expected values for fields "a" and "shared" use the correct separator
across platforms.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Superseded by #30283 — same bug, same fix. That PR uses a runtime-probe skip so the regression test stays CI-green until |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // shared: path.sep on darwin/linux is "/", on windows "\\". | ||
| expect(JSON.parse(stdout.trim())).toEqual({ a: "/", b: "barrel", shared: "/" }); |
There was a problem hiding this comment.
🔴 This assertion will fail on Windows CI: shared.js exports path.sep, which is "\\" on Windows, so the actual output will be { a: "\\", b: "barrel", shared: "\\" } rather than { a: "/", ... }. The comment on line 36 even notes this — import path from node:path and assert against path.sep (or branch on isWindows from harness).
Extended reasoning...
What the bug is
The test fixture's shared.js does import path from 'path'; export const SHARED = path.sep;. a.js returns SHARED from a(), and app.js exports { a: a(), b: b(), shared: SHARED }. On POSIX platforms path.sep is "/", but on Windows it is "\\" (a single backslash). The assertion on line 37 hard-codes { a: "/", b: "barrel", shared: "/" }, so on Windows the toEqual will fail even though the module loader behaved correctly.
The comment on line 36 explicitly acknowledges this — “on windows "\\"” — but the assertion immediately below it doesn't account for it.
Code path
entry.jsdoesrequire('./app.js')and printsJSON.stringify(mod.default).app.jsevaluatesa()→ returnsSHARED→path.sep.- On Windows,
node:pathresolves to the win32 implementation wheresep === '\\'. JSON.stringifyproduces{"a":"\\\\","b":"barrel","shared":"\\\\"}on stdout.JSON.parse(stdout.trim())yields{ a: '\\', b: 'barrel', shared: '\\' }.expect(...).toEqual({ a: '/', b: 'barrel', shared: '/' })fails:'\\'≠'/'.
Why nothing prevents it
There is no test.skipIf(isWindows) and no platform branching in the assertion. Bun's regression suite runs on Windows CI (test/harness.ts:24 exports isWindows = process.platform === "win32", and many sibling regression tests like 26298.test.ts / 25628.test.ts use it), so this file will execute there.
Impact
The new regression test will be red on Windows CI, blocking the merge (or, if landed, breaking Windows CI for everyone) for a reason unrelated to the actual deadlock fix being tested.
Fix
Import path (or isWindows) in the test and assert the platform-correct separator:
import path from "node:path";
// ...
expect(JSON.parse(stdout.trim())).toEqual({ a: path.sep, b: "barrel", shared: path.sep });Alternatively, since the specific value of SHARED is irrelevant to the deadlock repro (it just needs shared.js to import a synthetic builtin), change shared.js to export a platform-invariant constant like path.delimiter-agnostic literal, e.g. export const SHARED = path.posix.sep; and keep the assertion as-is.
Step-by-step proof
- Given: Windows runner,
process.platform === 'win32'. import path from 'path'inshared.jsloads the win32 path module →path.sep === '\\'.SHARED = '\\';a()returns'\\';app.jsdefault export ={ a: '\\', b: 'barrel', shared: '\\' }.entry.jsprints{"a":"\\\\","b":"barrel","shared":"\\\\"}.- Test parses it back to
{ a: '\\', b: 'barrel', shared: '\\' }. toEqual({ a: '/', b: 'barrel', shared: '/' })→ fails on keysaandshared.- Test reports failure; Windows CI goes red.
1 participant
Fixes #30493. Requires oven-sh/WebKit#223 to land first.
Repro
require('./app.js')where:app.jsimportsa.js,b.js, andshared.jsdirectlya.jsandb.jsboth import frombarrel.jsbarrel.jsre-exports fromshared.jsshared.jsimports a synthetic builtin likenode:pathOn Linux debug:
ASSERT(m_status == Status::Fetching)atvendor/WebKit/Source/JavaScriptCore/runtime/ModuleRegistryEntry.cpp:254. On Darwin: process hangs. Regressed by the module-loader rewrite in #29393.Root cause
During the synchronous drain that services
require(esm),hostLoadImportedModulehas aBUN_JSC_ADDITIONSinline fast path (JSModuleLoader.cpp:712) that handles a dep whose fetchPromise is already fulfilled but whose modulePromise is still pending: it callsmakeModule, thenentry->fetchComplete(record), thenmodulePromise->fulfillPromise(record).Meanwhile, the async pipeline had queued a
ModuleRegistryFetchSettledmicrotask for the same entry. That handler has a correct bail-out (modulePromise->status() != Pending \u2192 return) \u2014 but it checks this after callingmakeModuleand attachingModuleRegistryModuleSettledto the newmakeModulePromise. Wait, even better: actually the fetch-settled handler does bail early. The leak is that a separate, earlier invocation ofModuleRegistryFetchSettled(queued before the inline path ran) already created its ownmakeModulePromiseand attachedModuleRegistryModuleSettledto it. That queuedmoduleRegistryModuleSettledhas no bail-out of its own, so it runs and callsentry->fetchComplete(otherRecord)a second time \u2014 the ASSERT trips; in release it silently overwrites the entry's record with a duplicate parse that nothing else references.The specific trigger for
node:pathis that it's aSyntheticSourceProvider:makeModuleruns the generator (InternalModuleRegistry::requireId) synchronously, which executes JS and gives the inline re-entry path plenty of opportunity to hit.Fix
The WebKit-side fix (oven-sh/WebKit#223) mirrors the
moduleRegistryFetchSettledguard intomoduleRegistryModuleSettled: skip whenmodulePromise->status() != Pending. The inline path has already produced a valid record and resolved the promise; the queued copy is redundant.This Bun PR adds the regression test. It needs to land together with the WebKit bump (will follow once #223 merges + preview/release tarballs are built).
Test
test/regression/issue/30493.test.ts\u2014 spawnsbun run entry.json the diamond+synthetic-builtin setup and asserts the expected JSON output. Verified locally with adebug-localBun built against the WebKit PR branch:Fail-before verified by reverting the WebKit change only (no Bun source changes needed to exercise the bug):
Existing ESM-require tests (
test/js/bun/resolve/require-esm-*.test.ts,test/js/node/module/require-extensions.test.ts) all still pass.