fix: prevent chunk optimizer from creating import cycles#9228
Conversation
How to use the Graphite Merge QueueAdd the label graphite: merge-when-ready to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
25c7103 to
0ffffa8
Compare
ffb93b6 to
025e266
Compare
✅ Deploy Preview for rolldown-rs canceled.
|
b786017 to
1085e72
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
## [1.0.0-rc.18] - 2026-04-29 ### 💥 BREAKING CHANGES - optimization: default unspecified inlineConst.mode to smart (#9248) by @IWANABETHATGUY ### 🐛 Bug Fixes - rolldown_plugin_vite_import_glob: return error instead of panicking when virtual module uses a relative glob (#9241) by @shulaoda - binding: treat empty inlineConst object as omitted (#9247) by @IWANABETHATGUY - rolldown: keep enum declaration for optional-chain access (#9229) by @Dunqing - link_stage: restore inline let-else in exports-kind filter (#9237) by @IWANABETHATGUY - dev/lazy: avoid module reinitialization in lazy compilation patches (#9179) by @h-a-n-a - dev: visit identifier references for runtime rewrites in HMR finalizer (#9191) by @h-a-n-a - chunk-optimizer: pick dominator for runtime placement to avoid cycles (#9164) by @IWANABETHATGUY - make `this.emitFile` chunk path synchronous to avoid deadlock (#9031) by @lazarv - use sentinel id for `browser: false` ignored modules (#9192) by @shulaoda - prevent chunk optimizer from creating import cycles (#9228) by @IWANABETHATGUY ### 🚜 Refactor - replace tokio::sync::Mutex with std::sync::Mutex for non-IO data (#9176) by @shulaoda - rolldown_plugin_vite_import_glob: do not rewrite import path for absolute base (#9195) by @shulaoda - runtime_helper: wrap DependedRuntimeHelperMap in a struct (#9215) by @IWANABETHATGUY - drop redundant clear() in determine_safely_merge_cjs_ns (#9206) by @IWANABETHATGUY - clean up generate_lazy_export (#9208) by @IWANABETHATGUY - bitset: return bool from set_bit to fuse guard-and-set (#9207) by @IWANABETHATGUY - link_stage: simplify exports-kind filter and clarify safety comments (#9205) by @IWANABETHATGUY ### 📚 Documentation - determine_module_exports_kind (#9252) by @IWANABETHATGUY - fix dead link to esbuild ESM/CJS interop tests (#9230) by @Copilot - remove CSS bundling references (#9234) by @shulaoda - correct IncrementalFullBuild row in BundleMode table (#9214) by @IWANABETHATGUY - design: add bundler data lifecycle design doc (#9212) by @hyf0 - remove minifier alpha status notices (#9202) by @sapphi-red ### ⚙️ Miscellaneous Tasks - upgrade oxc to 0.128.0 (#9260) by @shulaoda - deps: bump rolldown-ariadne to 0.6.0 (#9254) by @IWANABETHATGUY - deps: update github actions (#9259) by @renovate[bot] - deps: update github actions (#9258) by @renovate[bot] - remove renovate overrides (#9257) by @Boshen - use ubuntu-latest for security workflow (#9256) by @Boshen - notify Discord around release publish (#9251) by @Boshen - add release environment to npm publish workflow (#9250) by @Boshen - justfile: drop the `--` separator before forwarded args in `vp run` (#9246) by @shulaoda - deps: update test262 submodule for tests (#9243) by @sapphi-red - add more tracing instrumentations (#9220) by @sapphi-red - rolldown_plugin_vite_import_glob: remove outdated sourcemap doc comment (#9213) by @shulaoda - update security workflow (#9201) by @Boshen ### ❤️ New Contributors * @lazarv made their first contribution in [#9031](#9031) Co-authored-by: shulaoda <165626830+shulaoda@users.noreply.github.com>

Summary
Fixes a regression introduced by #9049: when the chunk optimizer merges a shared module into an entry chunk, it can produce a static import cycle between that entry chunk and a manual-split chunk that also depends on the shared module.
Problem
would_create_circular_dependencydecides whether mergingsource(the temp chunk holding shared modules) intotarget(an existing entry chunk) is safe. The merge is unsafe iff, post-merge,targetwould reach itself in the dependency graph.#9049 split the algorithm into two cases based on whether
sourcehas dependencies, in order to reduce false positives that were blocking legitimate merges (the PR reported ~1,046 blocked merges and +12.5% bundle size on a 2,200-entry application). However, thesource has depsbranch only does a forward BFS fromsource.depsand drops the post-merge alias step that the original algorithm used to model new back-edges introduced by the merge:Without that step, the BFS cannot see cycles where the closing edge is one of the back-edges created by retargeting
source's importers ontotarget.Concrete repro shape (issue #9225)
main.js(entry) statically importsapi.jsandenv.js, plus dynamic importslazy.js,env-user.js,dep-user.js.api.jsis captured by a manual-split group{ name: "api", test: "api\\.js" }.api.jsstatically importsenv.js;env-user.jsstatically importsenv.js.env.jsis a shared module not captured by any group (includeDependenciesRecursively: false).The optimizer selects
mainas the static dominator forenv.js's consumers and mergesenv.jsinto it. After the merge:api.jsandenv-user.jsrewrite their./env.jsimports to./main.js.main.jsstill statically imports./api.js(pre-existing edge).main.js ⇄ api.jsin the output.In the
source has depsbranch, the BFS starts fromsource.deps = { dep.js's chunk }.dep.jshas no further deps and does not reachmain, so the BFS returnsfalseand the merge proceeds. The dropped alias step would have queuedtargetwhen the BFS visitedapi's chunk (whosedependenciescontainsource), correctly detecting the cycle.Fix
Revert
would_create_circular_dependencyto the pre-#9049 unified algorithm:source.deps ∪ target.deps.c, whensource ∈ c.dependencies, also enqueuetarget(modeling the post-merge edgec → target).targetis dequeued, the merge would create a cycle.This is sound: it catches both the forward path (
source.deps → target) and the back-edge path (target → … → c → source-becomes-target).Why reverting #9049 is safe: the
issues/9049fixture is already covered by #9085The existing
crates/rolldown/tests/rolldown/issues/9049/fixture continues to pass after this revert, because #9049'swould_create_circular_dependencychange was never load-bearing for that fixture. Bisecting between rc.15 and rc.16 (rc.15 produces 4 chunks, rc.16 produces 3) pinpoints the actual fix as #9085 —fix: relax overly conservative side-effect leak check in chunk optimizer(commit5ff1a7265). #9085 narrows the side-effect leak guard intry_insert_into_existing_chunk: the merge is now blocked only when a different user-defined entry can also reach the dynamic entries depending on the common chunk, instead of unconditionally blocking on any such dependency. That relaxation is what allowsservices/svc{0,1}.jsto merge into theroute0/route1chunks for this fixture.The fixture's name (
issues/9049) is therefore historically misleading: it tests behavior fixed by #9085, not #9049. The false-positive regressions #9049 was addressing in real-world bundles remain a concern but are not exercised by any in-tree fixture; a follow-up can re-tighten the check (e.g. by tracking dependent aliases more precisely) without losing the regression coverage this PR adds.Regression test
crates/rolldown/tests/rolldown/issues/9225/reproduces the cycle that #9049 was masking._config.jsonsets upmanualCodeSplitting.groups = [{ name: "api", test: "api\\.js" }]withincludeDependenciesRecursively: false._test.mjswalksdist/*.js, parses the static-import edges, and asserts the graph has no cycle. It additionally executes the bundled entry, checksglobalThis.__rolldown_issue_7449_value === 300000, and verifies that the side-effect moduledep.jsruns exactly once even when the dynamic chunks (which all transitively depend ondep.js) load.Without this fix,
_test.mjsfails on the cycle assertion before any runtime check runs.Testing
cargo test -p rolldown --test integration— 1692 passed, 0 failed, 70 ignored.just test-node(Rolldown vitesttest:main+test:watcher+ Rollup compatibility suite) — 722 + 31 + 916 passed, 0 failed.issues/9225fixture passes both the cycle assertion and the runtime checks.Closes #9225.
`