Skip to content

fix: prevent chunk optimizer from creating import cycles#9228

Merged
IWANABETHATGUY merged 3 commits intomainfrom
04-25-fix_prevent_chunk_optimizer_from_creating_import_cycles
Apr 25, 2026
Merged

fix: prevent chunk optimizer from creating import cycles#9228
IWANABETHATGUY merged 3 commits intomainfrom
04-25-fix_prevent_chunk_optimizer_from_creating_import_cycles

Conversation

@IWANABETHATGUY
Copy link
Copy Markdown
Member

@IWANABETHATGUY IWANABETHATGUY commented Apr 25, 2026

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_dependency decides whether merging source (the temp chunk holding shared modules) into target (an existing entry chunk) is safe. The merge is unsafe iff, post-merge, target would reach itself in the dependency graph.

#9049 split the algorithm into two cases based on whether source has 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, the source has deps branch only does a forward BFS from source.deps and drops the post-merge alias step that the original algorithm used to model new back-edges introduced by the merge:

// pre-#9049 step (removed in the source-has-deps branch)
if self.chunks[chunk_idx].dependencies.contains(&source_chunk_idx) {
  queue.push_back(target_chunk_idx);
}

Without that step, the BFS cannot see cycles where the closing edge is one of the back-edges created by retargeting source's importers onto target.

Concrete repro shape (issue #9225)

  • main.js (entry) statically imports api.js and env.js, plus dynamic imports lazy.js, env-user.js, dep-user.js.
  • api.js is captured by a manual-split group { name: "api", test: "api\\.js" }.
  • api.js statically imports env.js; env-user.js statically imports env.js.
  • env.js is a shared module not captured by any group (includeDependenciesRecursively: false).

The optimizer selects main as the static dominator for env.js's consumers and merges env.js into it. After the merge:

  • api.js and env-user.js rewrite their ./env.js imports to ./main.js.
  • main.js still statically imports ./api.js (pre-existing edge).
  • → static cycle main.js ⇄ api.js in the output.

In the source has deps branch, the BFS starts from source.deps = { dep.js's chunk }. dep.js has no further deps and does not reach main, so the BFS returns false and the merge proceeds. The dropped alias step would have queued target when the BFS visited api's chunk (whose dependencies contain source), correctly detecting the cycle.

Fix

Revert would_create_circular_dependency to the pre-#9049 unified algorithm:

  • BFS from source.deps ∪ target.deps.
  • For each visited chunk c, when source ∈ c.dependencies, also enqueue target (modeling the post-merge edge c → target).
  • If target is 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/9049 fixture is already covered by #9085

The existing crates/rolldown/tests/rolldown/issues/9049/ fixture continues to pass after this revert, because #9049's would_create_circular_dependency change 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 #9085fix: relax overly conservative side-effect leak check in chunk optimizer (commit 5ff1a7265). #9085 narrows the side-effect leak guard in try_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 allows services/svc{0,1}.js to merge into the route0/route1 chunks 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.json sets up manualCodeSplitting.groups = [{ name: "api", test: "api\\.js" }] with includeDependenciesRecursively: false.
  • _test.mjs walks dist/*.js, parses the static-import edges, and asserts the graph has no cycle. It additionally executes the bundled entry, checks globalThis.__rolldown_issue_7449_value === 300000, and verifies that the side-effect module dep.js runs exactly once even when the dynamic chunks (which all transitively depend on dep.js) load.

Without this fix, _test.mjs fails on the cycle assertion before any runtime check runs.

Testing

  • cargo test -p rolldown --test integration1692 passed, 0 failed, 70 ignored.
  • just test-node (Rolldown vitest test:main + test:watcher + Rollup compatibility suite) — 722 + 31 + 916 passed, 0 failed.
  • New issues/9225 fixture passes both the cycle assertion and the runtime checks.

Closes #9225.
`

Copy link
Copy Markdown
Member Author

IWANABETHATGUY commented Apr 25, 2026


How to use the Graphite Merge Queue

Add 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.

@IWANABETHATGUY IWANABETHATGUY changed the base branch from fix-9057-dominator-runtime-placement to graphite-base/9228 April 25, 2026 03:55
@IWANABETHATGUY IWANABETHATGUY force-pushed the 04-25-fix_prevent_chunk_optimizer_from_creating_import_cycles branch from ffb93b6 to 025e266 Compare April 25, 2026 03:55
@IWANABETHATGUY IWANABETHATGUY changed the base branch from graphite-base/9228 to main April 25, 2026 03:55
@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 25, 2026

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 52edb03
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/69ec4f331120520008fd85e9

@IWANABETHATGUY IWANABETHATGUY force-pushed the 04-25-fix_prevent_chunk_optimizer_from_creating_import_cycles branch from b786017 to 1085e72 Compare April 25, 2026 05:19
@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review April 25, 2026 06:41
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 25, 2026

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing 04-25-fix_prevent_chunk_optimizer_from_creating_import_cycles (52edb03) with main (0ffffa8)

Open in CodSpeed

Footnotes

  1. 10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@IWANABETHATGUY IWANABETHATGUY merged commit 31d0403 into main Apr 25, 2026
35 checks passed
@IWANABETHATGUY IWANABETHATGUY deleted the 04-25-fix_prevent_chunk_optimizer_from_creating_import_cycles branch April 25, 2026 11:58
This was referenced Apr 29, 2026
shulaoda added a commit that referenced this pull request Apr 29, 2026
## [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>
@rolldown-guard rolldown-guard Bot mentioned this pull request Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Circular chunk dependency causes TDZ error with @noble/curves + @noble/hashes (viem/wagmi)

2 participants