fix(chunk-optimizer): follow entry facade edges in runtime placement cycle check#10101
Merged
Conversation
✅ Deploy Preview for rolldown-rs canceled.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a runtime-chunk placement regression where the static-cycle check missed an entry-facade back-edge created when an entry module is captured into another chunk (e.g. via codeSplitting.groups). By extending static reachability to follow that facade edge, try_merge_runtime_chunk can correctly reject unsafe merges that would reintroduce runtime-helper initialization cycles like __commonJSMin is not a function (#9993 / #9224).
Changes:
- Extend
chunk_reaches_via_static_importBFS to also traverse the entry-facade edge (entry chunk → chunk that physically contains the entry module), not just module import records. - Add an integration fixture + execution test for #9993 that asserts correct runtime helper placement / evaluation order.
- Add snapshot coverage (
artifacts.snap) pinning the expected emitted chunk layout.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rolldown/src/stages/generate_stage/chunk_optimizer.rs | Makes static reachability analysis more accurate by following entry-module facade edges during runtime-merge cycle checks. |
| crates/rolldown/tests/rolldown/issues/9993/_config.json | Adds a minimal multi-entry + code-splitting-group configuration that triggers the regression shape. |
| crates/rolldown/tests/rolldown/issues/9993/_test.mjs | Executes the built output to assert the regression is fixed (and documents pre-existing out-of-scope behavior). |
| crates/rolldown/tests/rolldown/issues/9993/artifacts.snap | Pins the expected post-fix chunk graph/output to prevent regressions without changing existing snapshots. |
| crates/rolldown/tests/rolldown/issues/9993/node3.cjs | Fixture: CJS entry-2 requiring an ESM module outside the group to reproduce the problematic edge shape. |
| crates/rolldown/tests/rolldown/issues/9993/node4.cjs | Fixture: CJS entry-1 used to validate multi-entry behavior and side-effect counts. |
| crates/rolldown/tests/rolldown/issues/9993/node5.js | Fixture: plain ESM module deliberately excluded from the chunk group to force the capture/back-edge scenario. |
Merging this PR will not alter performance
Comparing 
Footnotes
|
IWANABETHATGUY
approved these changes
Jul 4, 2026
Member
Author
Merge activity
|
graphite-app Bot
pushed a commit
that referenced
this pull request
Jul 4, 2026
…cycle check (#10101) ### Description This is also more like a workaround fix, I'm working on how to solve these issues in a row. It requires some structure changes, meanwhile let's fix it first since it's p1 --- Fixes #9993 (regression of #9224, introduced in 1.1.1 by #9419). When a `codeSplitting` group captures an **entry module** into another chunk, the entry chunk gets a render-time static import of that module's wrapper/namespace symbol from the capturing chunk (via `collect_depended_symbols`). `chunk_reaches_via_static_import` only walked module **import records**, so the runtime-merge decision in `try_merge_runtime_chunk` could not see that back-edge: - `runtime_target_would_create_static_cycle` reported "no cycle" for a target that does statically import a helper consumer back, and - `find_consumer_dominator` treated that entry chunk as a downstream sink. The runtime module was then merged into the entry chunk of `entry-2`, while the group chunk both called `__commonJSMin` at top level and was statically imported back by that entry chunk — so ESM evaluated the group chunk before the helper initialized: `TypeError: __commonJSMin is not a function`. This PR makes the reachability BFS also follow the entry-module facade edge (EntryPoint chunk → chunk physically containing its entry module). With it, the merge is rejected for such graphs and the runtime stays in a dependency-free leaf chunk — the same layout rolldown 1.0.3 emitted. A spurious edge (e.g. for an unwrapped entry with no re-exported symbols) only makes the merge decision more conservative; worst case is one extra `rolldown-runtime` chunk, never a broken output. No existing test snapshot changes. Verified against the reporter's standalone repro (https://github.com/thevuong/rolldown-9993-repro): `npm run repro` fails on 1.1.3 and current main, passes with this fix. ### Out of scope (pre-existing, not part of the 1.1.1 regression window) The fixture's chunk graph still contains a benign-looking static cycle (`v.js` ↔ `entry-2.js`) where `entry-2`'s facade calls the group chunk's top-level `require_node3` binding. Loading `entry-2.js` first is fine, but loading `entry-1.js` as the evaluation root still throws `require_node3 is not a function` — and rolldown **1.0.3 emits the same entry-1-first crash** for this input, so that cross-chunk evaluation-order defect predates this regression (it matches the reporter's follow-up observation that patching only the helper's emit form is insufficient in general). The new test documents this and pins only the regression.
graphite-app
Bot
force-pushed
the
fix/issue-9993
branch
from
July 4, 2026 05:20
4a407ae to
6209344
Compare
…cycle check (#10101) ### Description This is also more like a workaround fix, I'm working on how to solve these issues in a row. It requires some structure changes, meanwhile let's fix it first since it's p1 --- Fixes #9993 (regression of #9224, introduced in 1.1.1 by #9419). When a `codeSplitting` group captures an **entry module** into another chunk, the entry chunk gets a render-time static import of that module's wrapper/namespace symbol from the capturing chunk (via `collect_depended_symbols`). `chunk_reaches_via_static_import` only walked module **import records**, so the runtime-merge decision in `try_merge_runtime_chunk` could not see that back-edge: - `runtime_target_would_create_static_cycle` reported "no cycle" for a target that does statically import a helper consumer back, and - `find_consumer_dominator` treated that entry chunk as a downstream sink. The runtime module was then merged into the entry chunk of `entry-2`, while the group chunk both called `__commonJSMin` at top level and was statically imported back by that entry chunk — so ESM evaluated the group chunk before the helper initialized: `TypeError: __commonJSMin is not a function`. This PR makes the reachability BFS also follow the entry-module facade edge (EntryPoint chunk → chunk physically containing its entry module). With it, the merge is rejected for such graphs and the runtime stays in a dependency-free leaf chunk — the same layout rolldown 1.0.3 emitted. A spurious edge (e.g. for an unwrapped entry with no re-exported symbols) only makes the merge decision more conservative; worst case is one extra `rolldown-runtime` chunk, never a broken output. No existing test snapshot changes. Verified against the reporter's standalone repro (https://github.com/thevuong/rolldown-9993-repro): `npm run repro` fails on 1.1.3 and current main, passes with this fix. ### Out of scope (pre-existing, not part of the 1.1.1 regression window) The fixture's chunk graph still contains a benign-looking static cycle (`v.js` ↔ `entry-2.js`) where `entry-2`'s facade calls the group chunk's top-level `require_node3` binding. Loading `entry-2.js` first is fine, but loading `entry-1.js` as the evaluation root still throws `require_node3 is not a function` — and rolldown **1.0.3 emits the same entry-1-first crash** for this input, so that cross-chunk evaluation-order defect predates this regression (it matches the reporter's follow-up observation that patching only the helper's emit form is insufficient in general). The new test documents this and pins only the regression.
Merged
shulaoda
added a commit
that referenced
this pull request
Jul 8, 2026
## [1.1.5] - 2026-07-08 ### 🚀 Features - detect top-level import-binding reads as execution-order sensitive (#10180) by @hyf0 - sourcemap_filenames: add a sourcemapFileNames option (#9271) by @V1OL3TF0X - binding: record plugin hook result kind in tracing spans (#10154) by @IWANABETHATGUY - linking: skip side-effect-free modules in per-entry reachability (#10111) by @IWANABETHATGUY - improve error message for unresolved virtual imports (#10156) by @sapphi-red - add descriptive metadata to plugin API (#10106) by @sapphi-red - add `--configLoader=native` option (#10118) by @sapphi-red ### 🐛 Bug Fixes - improve invalid annotation warnings (#10185) by @hyf0 - keep deduplicated asset filenames stable once they can be observed (#10191) by @shulaoda - sourcemap_filenames: use public option name in pattern errors (#10188) by @IWANABETHATGUY - sourcemap_filenames: hash prepared sourcemap content (#10178) by @hyf0 - tree-shake unused circular declarators exported via export list (#10166) by @IWANABETHATGUY - dev: don't panic when an HMR rebuild hits an unresolved import (#10162) by @shulaoda - propagate errors from output.globals function (#9880) by @shulaoda - dev: revert cache mutations when a partial scan fails (#10110) by @shulaoda - dev: update importer relationships of cached modules in incremental build (#10107) by @shulaoda - hmr: fall back to full reload when a changed module is not registered as executed (#10132) by @shulaoda - chunk-optimizer: follow entry facade edges in runtime placement cycle check (#10101) by @hyf0 - dev: ignore watcher events after close (#10113) by @hyf0 - emit async wrapper for TLA modules under onDemandWrapping (#10086) by @IWANABETHATGUY - gate sideEffects:false modules' side effects on body demand (#10080) by @IWANABETHATGUY - rolldown_plugin_vite_resolve: return empty object for `browser: false` mapped modules (#10082) by @sapphi-red - reset the word-boundary state on newline in Hires::Boundary sourcemaps (#10025) by @shulaoda - trim an emptied chunk's outro/intro instead of skipping past it (#10029) by @shulaoda - test each edited chunk's own start against indent exclude ranges (#10026) by @shulaoda - preserve sourcemap mappings for indented lines when a CJS module shares the chunk (#10074) by @hyf0 ### 🚜 Refactor - separate tree-shaking side effects from execution order sensitivity (#10168) by @hyf0 - type construct_vite_preload_call to take an ObjectPattern (#10135) by @shulaoda - treeshake: single-source the own-export classification shared with the lazy-barrel loader (#10098) by @IWANABETHATGUY - dev: reuse Vite's bundledDev server (#10081) by @h-a-n-a - clippy: ban std HashMap/HashSet in favour of FxHashMap/FxHashSet (#10108) by @Boshen - treeshake: make body demand a second module bit instead of a stmt multimap (#10097) by @IWANABETHATGUY - seal used_symbol_refs by construction after its last writer (#10091) by @hyf0 - treeshake: replace inclusion mutual recursion with a worklist engine (#10096) by @IWANABETHATGUY - treeshake: split include_statements.rs into focused modules (#10095) by @IWANABETHATGUY - drop redundant is_user_defined filter on partitioned entries (#10050) by @shulaoda - project the retained export interface out of used_symbol_refs (#10089) by @hyf0 - track used external symbols separately from used_symbol_refs (#10088) by @hyf0 - make module namespace inclusion an explicit linking metadata field (#10087) by @hyf0 - rename statement evaluation metadata (#10078) by @hyf0 ### 📚 Documentation - virtual modules user-facing id convention (#10155) by @sapphi-red - cli: clarify disabling boolean/object flags like codeSplitting (#10153) by @IWANABETHATGUY - chore: remove Vite+ alpha banner (#10105) by @mdong1909 - write down the used_symbol_refs contract (#10090) by @hyf0 - dev/lazy: update design and implementation (#10079) by @h-a-n-a ### ⚡ Performance - ast_scanner: stop order-sensitivity checks once a module is flagged (#10190) by @IWANABETHATGUY - return impl ExactSizeIterator from slice-backed accessors (#10133) by @Boshen - binding: box dev and watcher napi futures (#10103) by @Boshen ### 🧪 Testing - move string_wizard replace unit tests to the JS magic-string suite (#10176) by @IWANABETHATGUY - dev: assert incremental scan state matches a fresh full build after each HMR step (#10115) by @shulaoda - dev: restore runtime assertions of delete_file_not_used_anymore (#10112) by @shulaoda - dev: fix flaky dev server tests in CI (#10152) by @h-a-n-a - add regression test for #10099 (lazyBarrel drops default-import binding but keeps its property reads) (#10109) by @IWANABETHATGUY ### ⚙️ Miscellaneous Tasks - deploy website to Void via GitHub OIDC (#10192) by @Boshen - deps: update oxc to 0.139.0 (#10161) by @shulaoda - deps: update test262 submodule for tests (#10160) by @rolldown-guard[bot] - rolldown_plugin_utils: remove dead asset-url and css scaffolding (#10131) by @shulaoda - deps: revert vite-plus to v0.2.1 (#10148) by @shulaoda - deps: update github actions (#10141) by @renovate[bot] - deps: update dependency rust to v1.96.1 (#10145) by @renovate[bot] - deps: update npm packages (#10142) by @renovate[bot] - deps: update rust crates (#10143) by @renovate[bot] - deps: update napi to v3.10.3 (#10121) by @renovate[bot] - rolldown_utils: remove unused time module (#10138) by @shulaoda - remove dead CopyModulePlugin::is_active method (#10129) by @shulaoda - remove dead LazyCompilationContext::is_lazy_module method (#10128) by @shulaoda - remove dead BuildDiagnostic::downcast_ref method (#10127) by @shulaoda - deps: update dependency vite-plus to v0.2.2 (#10084) by @renovate[bot] - deps: update rust crate oxc_sourcemap to v8.1.0 (#10122) by @renovate[bot] - deps: update crate-ci/typos action to v1.48.0 (#10124) by @renovate[bot] - enable more clippy restriction lints (#10114) by @Boshen - deps: update rust dependencies (#10100) by @Boshen - deps: update oxc resolver to v11.23.0 (#10083) by @renovate[bot] ###◀️ Revert - Revert "chore(deps): revert vite-plus to v0.2.1" (#10157) by @h-a-n-a - "fix(hmr): fall back to full reload when a changed module is not registered as executed (#10132)" (#10151) by @shulaoda ### ❤️ New Contributors * @V1OL3TF0X made their first contribution in [#9271](#9271) Co-authored-by: shulaoda <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is also more like a workaround fix, I'm working on how to solve these issues in a row. It requires some structure changes, meanwhile let's fix it first since it's p1
Fixes #9993 (regression of #9224, introduced in 1.1.1 by #9419).
When a
codeSplittinggroup captures an entry module into another chunk, the entry chunk gets a render-time static import of that module's wrapper/namespace symbol from the capturing chunk (viacollect_depended_symbols).chunk_reaches_via_static_importonly walked module import records, so the runtime-merge decision intry_merge_runtime_chunkcould not see that back-edge:runtime_target_would_create_static_cyclereported "no cycle" for a target that does statically import a helper consumer back, andfind_consumer_dominatortreated that entry chunk as a downstream sink.The runtime module was then merged into the entry chunk of
entry-2, while the group chunk both called__commonJSMinat top level and was statically imported back by that entry chunk — so ESM evaluated the group chunk before the helper initialized:TypeError: __commonJSMin is not a function.This PR makes the reachability BFS also follow the entry-module facade edge (EntryPoint chunk → chunk physically containing its entry module). With it, the merge is rejected for such graphs and the runtime stays in a dependency-free leaf chunk — the same layout rolldown 1.0.3 emitted. A spurious edge (e.g. for an unwrapped entry with no re-exported symbols) only makes the merge decision more conservative; worst case is one extra
rolldown-runtimechunk, never a broken output. No existing test snapshot changes.Verified against the reporter's standalone repro (https://github.com/thevuong/rolldown-9993-repro):
npm run reprofails on 1.1.3 and current main, passes with this fix.Out of scope (pre-existing, not part of the 1.1.1 regression window)
The fixture's chunk graph still contains a benign-looking static cycle (
v.js↔entry-2.js) whereentry-2's facade calls the group chunk's top-levelrequire_node3binding. Loadingentry-2.jsfirst is fine, but loadingentry-1.jsas the evaluation root still throwsrequire_node3 is not a function— and rolldown 1.0.3 emits the same entry-1-first crash for this input, so that cross-chunk evaluation-order defect predates this regression (it matches the reporter's follow-up observation that patching only the helper's emit form is insufficient in general). The new test documents this and pins only the regression.