Skip to content

Don't emit a CSS chunk for files whose import resolution failed in the dev server #31945

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Jarred-Sumner wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Don't emit a CSS chunk for files whose import resolution failed in the dev server #31945

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
953e052
bundler: don't emit a CSS chunk for files whose import resolution failed
Jarred-Sumner Jun 6, 2026
2fd8e0d
Merge branch 'main' into claude/dev-server-failed-css-chunk
Jarred-Sumner Jun 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/bundler/bundle_v2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5247,9 +5247,19 @@
// Find CSS entry points. Originally, this was computed up front, but
// failed files do not remember their loader, and plugins can
// asynchronously decide a file is CSS.
//
// A file whose import-record resolution failed also has a `Some`
// css row: `run_resolution_for_parse_task` parks the parsed
// stylesheet there so teardown can drop it. Such files have no
// parts (the invariant the loop above filters by), and must not
// become CSS chunks — the loop above already removed them from
// `css_entry_points`.
let css = asts.items_css();
let parts = asts.items_parts();
for entry_point in &self.graph.entry_points {
if css[entry_point.get() as usize].is_some() {
if css[entry_point.get() as usize].is_some()
&& parts[entry_point.get() as usize].len() != 0
{

Check failure on line 5262 in src/bundler/bundle_v2.rs

View check run for this annotation

Claude / Claude Code Review

Comment contradicts merged #31905; guard may now be redundant 

This comment is now stale: PR #31905 (90589f93, merged into this branch via 2fd8e0db) changed `run_resolution_for_parse_task` to *drop* the parsed stylesheet rather than park it on the graph row — see L5744-5756, whose comment explicitly says "It must not be parked on the graph row either... the graph row for this file stays `None`". After that merge, `css[ep]` is `None` for the resolution-failed case, so the existing `.is_some()` check already excludes it and the new `parts.len() != 0` guard is

Check notice on line 5262 in src/bundler/bundle_v2.rs

View check run for this annotation

Claude / Claude Code Review

scan_css_imports error path still re-added by recovery loop 

Pre-existing, but adjacent to this fix: there's a second removal path the recovery loop still undoes. When `scan_css_imports` returns `Errors` (L5169 — e.g. a CSS entry point `@import`s a non-CSS-loader file), the file is `swap_remove`d from `css_entry_points` at L5185-5187 with the comment "do not treat it as a valid CSS chunk", but its parts are *not* cleared and its css row is still `Some`. If that file is in `graph.entry_points`, this loop sees `css.is_some() && parts.len() != 0` and `put()`
Comment on lines +5251 to +5262

@claude claude Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 This comment is now stale: PR #31905 (90589f9, merged into this branch via 2fd8e0d) changed run_resolution_for_parse_task to drop the parsed stylesheet rather than park it on the graph row — see L5744-5756, whose comment explicitly says "It must not be parked on the graph row either... the graph row for this file stays None". After that merge, css[ep] is None for the resolution-failed case, so the existing .is_some() check already excludes it and the new parts.len() != 0 guard is dead for its documented purpose. Either drop this change as superseded by #31905, or keep the guard as defense-in-depth but rewrite the comment so it stops describing behavior that no longer exists.

Extended reasoning...

What the bug is

This PR and PR #31905 both fixed the same dev-server crash (a CSS file whose url()/@import resolution fails was being emitted as a CSS chunk), but they fixed it in different places. #31905 landed on main first and was then merged into this branch via 2fd8e0d. The merge produced no textual conflict, so both fixes now coexist — but the new comment at L5251-5256 describes the pre-#31905 behavior of run_resolution_for_parse_task, which #31905 explicitly removed.

The contradiction

  • L5251-5256 (this PR) says: "A file whose import-record resolution failed also has a Some css row: run_resolution_for_parse_task parks the parsed stylesheet there so teardown can drop it."
  • L5744-5756 (bundler: fix dev server crash when a CSS rebuild fails import resolution #31905, same file) does result.ast.css.take() + drop_in_place and says: "It must not be parked on the graph row either... the graph row for this file stays None."

These two comments, ~500 lines apart in the same file, describe mutually exclusive behavior of the same function. The L5251 comment is the one that's wrong on current HEAD.

Step-by-step proof

  1. git merge-base --is-ancestor 90589f93 HEAD → yes; git merge-base --is-ancestor 90589f93 953e052 → no. So bundler: fix dev server crash when a CSS rebuild fails import resolution #31905 was not in this PR's base when the comment was written (it was accurate then), but is in this branch now after the merge.
  2. On HEAD, run_resolution_for_parse_task hits the resolution-failure path → L5752 takes result.ast.css and drops it → the graph's css[source_index] stays None.
  3. Back in finish_from_bake_dev_server at L5260: css[ep].is_some() is false for that file, so the entry is skipped before parts[ep].len() != 0 is ever evaluated.
  4. Therefore the new guard never fires for the case its comment describes; the comment documents a code path that bundler: fix dev server crash when a CSS rebuild fails import resolution #31905 deleted.

Why existing code doesn't prevent it

This is a semantic merge conflict — git saw no overlapping hunks, so the merge at 2fd8e0d applied cleanly. Nothing in the build or tests flags two contradictory comments, and the redundant guard is behaviorally harmless, so CI stays green.

Impact

No runtime bug — the behavior is correct either way (#31905 already prevents the crash). The problems are:

  • A factually wrong comment that directly contradicts another comment in the same file, which will mislead anyone reading either one.
  • The entire code change in this PR may now be redundant. If it's kept as defense-in-depth that's fine, but it should be a deliberate decision with an accurate justification, not an artifact of a stale merge.

How to fix

Pick one:

  • Drop this PR's hunk as superseded by bundler: fix dev server crash when a CSS rebuild fails import resolution #31905 (same crash, fixed closer to the source).
  • Keep the parts.len() != 0 guard as belt-and-suspenders, but rewrite the comment to stop claiming run_resolution_for_parse_task parks the stylesheet — e.g. note that it's a defensive check mirroring the empty-parts filter in the loop above, in case any other path leaves a Some css row with no parts.

Comment on lines +5260 to +5262

@claude claude Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing, but adjacent to this fix: there's a second removal path the recovery loop still undoes. When scan_css_imports returns Errors (L5169 — e.g. a CSS entry point @imports a non-CSS-loader file), the file is swap_removed from css_entry_points at L5185-5187 with the comment "do not treat it as a valid CSS chunk", but its parts are not cleared and its css row is still Some. If that file is in graph.entry_points, this loop sees css.is_some() && parts.len() != 0 and put()s it right back. This doesn't trip the debug_assert crash (the file parsed, so css_file_count > 0) and existed before this PR — mentioning only because you're already in this loop with the intent of not re-adding failed CSS entry points.

Extended reasoning...

What the gap is. The recovery loop at L5259-5269 re-inserts into css_entry_points any graph.entry_points file that has css.is_some() and (with this PR) parts.len() != 0. The PR's new parts.len() != 0 gate correctly screens out files removed via the empty-parts branch at L5241-5243 (the resolution-failure case). But there is a second removal path in the loop above: when scan_css_imports returns Errors at L5169, the file is removed at L5185-5187 via swap_remove with the explicit comment "Since there is an error, do not treat it as a valid CSS chunk". That removal is not protected by the new gate, because the file took the part_list.len() != 0 branch (L5151) to reach scan_css_imports in the first place, and handle_parse_task_failure does not clear parts or the css slot — it only records the failure into the dev server's incremental graph.

Concrete walkthrough. Take a CSS file a.css that is itself a dev-server entry point (enqueue_entry_item at L2680-2682 pushes all entry items, CSS included, to self.graph.entry_points; L3080-3086 also seeds it into css_entry_points). It contains @import './b.ts';. Parsing succeeds, so a.css has a non-empty part list (the lazy-export part) and css[a] is Some. In the main loop: part_list.len() != 0 → true, maybe_css.is_some() → true, it's pushed to css_total_files (L5156), then scan_css_imports sees the .ts import has a non-CSS loader and returns Errors. handle_parse_task_failure is called and L5185 swap_removes a from css_entry_points. Then the recovery loop runs: a is in graph.entry_points, css[a].is_some() is true, parts[a].len() != 0 is true → put() re-inserts it, defeating L5185.

Why the new gate doesn't catch it. The PR's gate keys on the same invariant the loop above uses to enter the failure branch — non-empty parts. A file that fails via scan_css_imports necessarily had non-empty parts (it's inside the if part_list.len() != 0 block), so parts.len() != 0 is always true for it.

Impact. Milder than the bug this PR fixes: because the file parsed successfully, css_file_count was incremented (L6957) and the file is in css_total_files (pushed at L5156 before the error check), so generate_chunks_in_parallel takes the css_file_count > 0 branch and the debug_assert!(!chunk.content.is_css()) does not fire. The result is just that a chunk is generated for a file the dev server has already marked failed via handle_parse_task_failure — the user still sees the InvalidCssImport error, but L5185's stated intent ("do not treat it as a valid CSS chunk") is a no-op for entry-point CSS.

Pre-existing. Before this PR the loop checked only css.is_some(), so it re-added the file then too. The PR's change is monotonically narrower and does not make this worse. Flagging only because the PR's added comment scopes the loop to "must not become CSS chunks — the loop above already removed them", and L5185 is one such removal the loop still undoes.

Possible fix (if you want to cover it here). Either also clear parts[index] / set css[index] = None alongside the swap_remove at L5185, or have the recovery loop check css_entry_points membership / a separate failed-set rather than re-deriving from css.is_some() && parts.len() != 0.

start.css_entry_points.put(
Index::init(entry_point.get()),
CssEntryPointMeta {
Expand Down
Loading