Don't change ESM evaluation order when the Node compile cache is enabled - #37179
Don't change ESM evaluation order when the Node compile cache is enabled#37179robobun wants to merge 4 commits into
Conversation
Enabling NODE_COMPILE_CACHE (or module.enableCompileCache()) forced every import through the synchronous transpile path so the cache's fetch hook would see each module. That path classifies a .cjs file with no CommonJS features as CommonJS from its extension alone, and CommonJS modules imported from ESM evaluate eagerly during graph loading, so enabling the cache flipped ESM evaluation order: a featureless .cjs statically imported after an .mjs sibling ran before it. The concurrent path classifies such a file as ESM and evaluates it in graph order. Add the compile cache fetch and parse-failure hooks to RuntimeTranspilerStore so concurrently transpiled modules read and populate the cache too, and drop the force-synchronous gate. The cache no longer changes which pipeline a module goes through, so evaluation order is identical with and without it, cold and warm.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 4 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 (2)
WalkthroughThe transpiler now records Node compile-cache bytecode and parse failures for JavaScript-like modules. Module classification uses extensions and package metadata. Concurrent transpilation remains enabled when the cache is active. Tests cover cache paths and failed ChangesNode compile-cache integration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I checked: (1) node_compile_cache::fetch/note_parse_failure are safe to call from the transpiler worker thread — both take STATE.lock() and the returned blob pointer is process-lifetime (entry map / RETIRED_BLOBS); (2) the bytecode_cache ownership on the concurrent path is the same borrowed-blob pattern the sync path already uses at jsc_hooks.rs:3239-3250, so no new lifetime hazard is introduced; (3) the is_cjs argument to fetch on both new call sites (AST-derived on the print path, cache-entry-derived on the transpiler-cache-hit path) matches the sync path's derivation, so cache keys agree across paths.
Extended reasoning...
The one flagged issue is a debug-log-only parity nit on the parse-failure branch. I traced the three areas most likely to hide a real bug in this change — worker-thread safety of the compile-cache mutex, bytecode blob lifetime when handed to ResolvedSource from off-thread, and is_cjs keying consistency between sync and concurrent fetch calls — and each mirrors existing sync-path behavior. Recording these so a follow-up pass doesn't re-derive them.
The concurrent path's module_type comes from package.json alone, so a parse-failed .mjs inside a "type": "commonjs" package was recorded (and logged under NODE_DEBUG_NATIVE=COMPILE_CACHE) as CommonJS while the synchronous path and Node record it as ESM. Sniff the extension first, consulting package.json only for .js/.ts, matching transpile_file.
|
Addressed the review in ad30d37: parse-failure compile cache entries on the concurrent path now key off the file extension first (package.json "type" only applies to .js/.ts), matching the synchronous path and Node. Added a test that pins the NODE_DEBUG_NATIVE=COMPILE_CACHE output for a parse-failed .mjs inside a "type": "commonjs" package. All compile-cache tests and the module suites pass locally. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/node/module/node-module-module.test.js`:
- Around line 137-144: Update the cache activation cases in the test around run
and enableCompileCache to explicitly set NODE_COMPILE_CACHE to undefined for the
no-cache and API invocations, preventing inherited environment state. Extend the
cache assertions to verify populated entries under both cc and cc-api, so each
activation path is independently observable.
- Around line 125-136: Update the local async run helper to accept the expected
stdout and assert it before checking the child process exit code; keep the
exitCode assertion last, and adjust its callers to use this assertion flow
instead of comparing stdout afterward.
🪄 Autofix
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: 4bc523e4-e322-4ca4-9579-784ba44261fd
📒 Files selected for processing (3)
src/jsc/RuntimeTranspilerStore.rssrc/runtime/jsc_hooks.rstest/js/node/module/node-module-module.test.js
💤 Files with no reviewable changes (1)
- src/runtime/jsc_hooks.rs
Move the is-enabled/is-file/JS-like guards into node_compile_cache (fetch_for_transpiled_module, note_parse_failure_for_module) so the five call sites across the synchronous and concurrent transpile paths share one definition, and extract the extension sniff into ModuleType::from_extension, used by both transpile_file and the concurrent path's parse-failure hook. Test feedback: spawn cache runs with NODE_COMPILE_CACHE cleared from the inherited env, assert stdout before exit codes, and verify the API enable path populated its own cache directory.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/jsc/RuntimeTranspilerStore.rs:900-910— The extension→module_type sniff at lines 905-909 duplicates the logic fromtranspile_file(jsc_hooks.rs:4347-4374), and the 4-line comment above it exists to explain the mirror — which is what tripped comment-cop and runs against REVIEW.md's "one implementation, in the right place". The non-duplicating fix the earlier review pointed at is to pass the already-computed extension-sniffedmodule_typethroughtranspiler_store.transpile(...)(which already threadspackage_json), or extract a shared helper — then both the duplicatedmatchand the justifying comment go away.Extended reasoning...
What the bug is
Commit ad30d37 addressed the earlier review comment (parse-failure entries were keyed by package.json type instead of extension) by re-implementing the extension sniff inline at RuntimeTranspilerStore.rs:905-909:
let is_cjs = match path.name().ext { b".cjs" | b".cts" => true, b".mjs" | b".mts" => false, b".js" | b".ts" => !matches!(module_type, ModuleType::Esm), _ => true, };
This is a compressed copy of the block at jsc_hooks.rs:4347-4374 (
.cjs/.cts→Cjs,.mjs/.mts→Esm,.js/.ts→package.json type, else→Unknown). The 4-line comment at 901-904 ("module_typehere comes from package.json alone; mirror the synchronous path's extension sniff (transpile_file) so both paths record the same type…") exists solely to explain why the duplication is there.Why this is flagged
Two repository rules apply directly:
- REVIEW.md, Architecture & layering: "One implementation, in the right place. Never copy a helper or constant table between modules — share or derive it."
- CLAUDE.md #13: "If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code."
The repo's automated
comment-copbot has already flagged this line (and three others in the same file) with exactly that message, and those inline comments are unresolved on the PR.Why existing code doesn't prevent it
The earlier inline review that ad30d37 responded to explicitly named the non-duplicating fix: "the extension-sniffed
module_typeis already computed intranspile_filebefore the concurrent dispatch and could be passed through." The response fixed the behavior but not by the suggested route — it re-derived the value locally instead of threading the already-computed one.Step-by-step: how to fix without duplicating
transpile_file(jsc_hooks.rs:4347) computesmodule_type: ModuleTypefrom the extension + package.json.- ~70 lines later at jsc_hooks.rs:4420, it calls
(*jsc_vm).transpiler_store.transpile(jsc_vm, global_ref, specifier, &lr.path, referrer, concurrent_loader, lr.package_json). This call already threadslr.package_jsonthrough so the job can setresolved_source.tag. - Add
module_typeas one more parameter toRuntimeTranspilerStore::transpile(), store it onTranspilerJob(a plainModuleTypeenum,Copy), and use it directly inrun()— replacing both thethis_tag-derived reconstruction and the new extensionmatch. - Delete the 4-line justification comment;
is_cjsbecomes!matches!(self.module_type, ModuleType::Esm)exactly as the sync path'snote_compile_cache_parse_failurecomputes it, from the same source value.
Alternatively, extract jsc_hooks.rs:4347-4374 into a shared
fn sniff_module_type(ext: &[u8], pkg: Option<&PackageJSON>) -> ModuleTypeand call it from both sites.Impact
Behavior is currently equivalent between the two copies, so this is maintainability only, not a correctness bug. The risk is drift: if a new extension is added to one copy (e.g.
.cjsx) and not the other, the sync and concurrent paths diverge again — which is precisely the failure mode this PR just fixed. Nit-level; not blocking merge.
|
That finding is against the intermediate state in ad30d37; f46945b already made the suggested change. The extension sniff now lives in one place, ModuleType::from_extension (src/options_types/bundle_enums.rs), used by both transpile_file's module_type computation and the concurrent path's parse-failure hook, and the inline match plus its justification comment are gone. The compile cache guards were consolidated the same way (fetch_for_transpiled_module, note_parse_failure_for_module in NodeCompileCache.rs), so both transpile paths share one definition. |
There was a problem hiding this comment.
I reviewed this PR (including the follow-up commits addressing the earlier module-type classification note and the comment-cop/CodeRabbit feedback) and didn't find any bugs. Because it changes which thread the compile-cache hooks run on and removes the guard that kept compile-cache loads on the synchronous transpile path, a human look would still be worthwhile.
Checked that fetch/note_parse_failure are mutex-guarded and blob pointers are process-lifetime (RETIRED_BLOBS), so calling them from worker threads is sound.
Verified ModuleType::from_extension is behavior-identical to the inlined sniff it replaces in transpile_file (including the .jsx/.tsx → Unknown fallthrough).
Confirmed the unconditional bytecode_cache writes on the sync path are equivalent to the prior if let Some form ((null, 0) is the disabled default).
Extended reasoning...
Overview
The PR fixes an ESM evaluation-order regression when NODE_COMPILE_CACHE is enabled: previously enabling the cache forced every import through the synchronous transpile path (which classifies .cjs by extension → eager CJS evaluation), diverging from the concurrent path's package.json-only classification. The fix wires the compile-cache fetch/parse-failure hooks into RuntimeTranspilerStore::run() so the concurrent path can participate, then removes the !node_compile_cache::is_enabled() guard from transpile_file's concurrent-dispatch condition. Along the way it consolidates the three duplicated is_enabled() && path.is_file() && loader.is_java_script_like() guards into two shared helpers in NodeCompileCache.rs and extracts the extension-sniff into ModuleType::from_extension.
Security risks
None identified. The compile cache reads/writes files under a user-controlled directory and the changed code paths don't touch validation of that directory or the on-disk format; they only change which thread invokes the existing hooks.
Level of scrutiny
High. transpile_file and TranspilerJob::run() are the module-loading hot path, and the change alters thread affinity: node_compile_cache::fetch (which does file I/O and mmap under STATE.lock()) now runs on transpiler worker threads instead of only the JS thread. The mutex makes this correct, and returned blob pointers are process-lifetime (displaced blobs go to RETIRED_BLOBS, never freed), so there's no UAF across the thread hop into ResolvedSource. But this is exactly the kind of change where a maintainer should confirm the intended concurrency model.
Other factors
- My earlier inline finding (parse-failure
is_cjsderived from package.json instead of extension on the concurrent path) was fixed in ad30d37 and the fix was then deduplicated intoModuleType::from_extensionin f46945b, with a regression test asserting theNODE_DEBUG_NATIVE=COMPILE_CACHEoutput. - The comment-cop and CodeRabbit threads are all resolved; the test now asserts stdout before exit code, unsets
NODE_COMPILE_CACHEfor the no-cache/API cases, and checks bothccandcc-apiwere populated. - The PR description notes the added tests were not run locally ("Platform-specific test(s) that do not run on this machine") and defers to CI — CI results should be checked before merge.
- Worker threads now serialize on
STATE.lock()during cache-file I/O; that's a throughput consideration (not correctness) worth a maintainer glance.
2 participants
Enabling the Node compile cache changed ESM evaluation order: a CommonJS module statically imported after an ESM sibling evaluated before it.
The API enable path (
require("node:module").enableCompileCache(dir)in a preload) flipped it the same way;NODE_DISABLE_COMPILE_CACHE=1restored the order.Cause. With the cache enabled,
transpile_fileskipped the concurrent transpiler for every import so the cache's fetch hook (which only existed on the synchronous path) would see each module. The two paths classify this file differently:module_typefrom the extension, so a.cjsfile with no CommonJS features is classified CommonJS. CommonJS imported from ESM evaluates eagerly while the module graph is still loading (makeModuleruns the synthetic source generator, which callsevaluateCommonJSModuleOnce), so its side effects run before any ESM sibling evaluates.RuntimeTranspilerStore) derivesmodule_typefrom package.json alone, classifies the featureless.cjsas ESM, and it evaluates in graph order.So turning the cache on silently moved every import onto the path with the other classification. The same flip was already reachable without the cache via anything else that forces the synchronous path (for example
BUN_FEATURE_FLAG_DISABLE_ASYNC_TRANSPILER=1).Fix. Make the cache work on the concurrent path instead of forcing everything off it:
RuntimeTranspilerStorenow calls the compile cache fetch hook after printing (and on the runtime-transpiler-cache hit path, skipping UTF-16 output like the synchronous path does), attaching validated bytecode to theResolvedSource, and mirrors the parse-failure bookkeeping.!node_compile_cache::is_enabled()condition on the concurrent-transpiler dispatch is removed.The cache no longer changes which pipeline a module goes through, so evaluation order is identical with and without it. The hook is thread-safe (the cache state is behind a mutex; blobs live for the process).
Verification. The repro prints
esm,cjsin all variants (no cache, cold, warm,NODE_DISABLE_COMPILE_CACHE, API enable via preload), andNODE_DEBUG_NATIVE=COMPILE_CACHEconfirms warm runs accept the cached bytecode for modules transpiled on the concurrent path, including real CommonJS deps. All 14 portedtest-compile-cache-*.jsNode tests pass, as do the--watchpersist test and the module loading suites. New test covers the order in all variants and asserts the cache directory was actually populated so the cached runs prove something.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/node/module/node-module-module.test.js