Don't emit a CSS chunk for files whose import resolution failed in the dev server - #31945
Don't emit a CSS chunk for files whose import resolution failed in the dev server#31945Jarred-Sumner wants to merge 2 commits into
Conversation
When a CSS file parses successfully but one of its url()/@import records fails to resolve, run_resolution_for_parse_task converts the result to an error and parks the parsed stylesheet on the graph's css row so teardown can drop it. The comment there assumes the linker never runs after a resolution error, but the dev server intentionally proceeds with failed files. finish_from_bake_dev_server's CSS entry-point recovery loop keyed only on css.is_some(), so the failed file — already removed from css_entry_points by the empty-parts filter — was re-added and became a CSS chunk in a bundle whose css_file_count is 0. Debug builds died on the !chunk.content.is_css() assertion in generate_chunks_in_parallel (killing the dev server on any CSS edit with an unresolvable url), and release builds emitted a bogus empty CSS chunk for the failed file. Gate the recovery loop on the file having parts, the same invariant the loop above uses to filter failed files; successfully parsed CSS always has a lazy-export part. Covered by the existing "syntax error crash" and "css import before create project relative" tests in test/bake/dev/css.test.ts, which previously panicked in debug builds (13 pass / 0 fail after this change, 11/2 before).
|
Updated 4:58 AM PT - Jun 12th, 2026
❌ @Jarred-Sumner, your commit 2fd8e0d has 5 failures in
🧪 To try this PR locally: bunx bun-pr 31945That installs a local version of the PR into your bun-31945 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthroughfinish_from_bake_dev_server now only adds CSS entry-point roots when the CSS AST for a file exists and the parsed parts list for that entry point is non-empty, preventing empty/failed-resolution CSS rows from being treated as valid chunks. ChangesDevServer CSS entry-point validation
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal fix that reuses the same empty-parts invariant already applied a few lines above.
Extended reasoning...
Overview
This PR adds a single guard condition to the CSS entry-point recovery loop in finish_from_bake_dev_server (src/bundler/bundle_v2.rs). Previously the loop re-added any entry point whose css row was Some, but run_resolution_for_parse_task parks the parsed stylesheet on that row even when import resolution fails. The fix additionally requires parts[entry_point].len() != 0, which is the exact invariant the surrounding code (lines ~5150-5154 and ~5219-5225) already uses to distinguish failed files from successfully parsed ones. The else-branch of the preceding loop already removes such files from css_entry_points; this change stops the recovery loop from putting them right back.
Security risks
None. This is dev-server-only bundler bookkeeping that strictly narrows which files become CSS chunks. No user input parsing, auth, crypto, or filesystem boundary is touched.
Level of scrutiny
Low. The diff is +12/-1 (mostly comment), confined to one function on the dev-server path, and mirrors an existing, well-commented invariant in the same block. The PR description gives a precise root-cause analysis matching the code, and the failure mode (debug debug_assert!(!chunk.content.is_css()) panic / bogus empty chunk in release) is consistent with what the surrounding code does when css_file_count is 0.
Other factors
The bug-hunting system found no issues, CodeRabbit had no actionable comments, no CODEOWNER covers this path, and the fix is verified by existing tests in test/bake/dev/css.test.ts that were failing on debug builds and now pass. The added check can only reduce the set of CSS entry points (never add), so the worst-case regression would be a missing CSS chunk for a file that somehow has CSS but no parts — but the in-code comment at ~5153 explicitly states actual empty files always carry a lazy-export part.
| // 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 | ||
| { |
There was a problem hiding this comment.
🔴 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
Somecss row:run_resolution_for_parse_taskparks 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_placeand says: "It must not be parked on the graph row either... the graph row for this file staysNone."
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
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.- On HEAD,
run_resolution_for_parse_taskhits the resolution-failure path → L5752 takesresult.ast.cssand drops it → the graph'scss[source_index]staysNone. - Back in
finish_from_bake_dev_serverat L5260:css[ep].is_some()isfalsefor that file, so the entry is skipped beforeparts[ep].len() != 0is ever evaluated. - 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() != 0guard as belt-and-suspenders, but rewrite the comment to stop claimingrun_resolution_for_parse_taskparks 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 aSomecss row with no parts.
| if css[entry_point.get() as usize].is_some() | ||
| && parts[entry_point.get() as usize].len() != 0 | ||
| { |
There was a problem hiding this comment.
🟣 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.
2 participants
What does this PR do?
Fixes a dev-server crash (debug builds) and bogus output (release builds) when a CSS file's
url()/@importfails to resolve.Root cause: when a CSS file parses successfully but a record fails to resolve,
run_resolution_for_parse_taskconverts the result to an error and parks the parsed stylesheet on the graph's css row so teardown can drop it. The comment there assumes the linker never runs after a resolution error — but the dev server intentionally proceeds with failed files.finish_from_bake_dev_server's CSS entry-point recovery loop keyed only oncss.is_some(), so the failed file (already removed fromcss_entry_pointsby the empty-parts filter a few lines up) was re-added and became a CSS chunk in a bundle whosecss_file_countis 0. Debug builds then died ondebug_assert!(!chunk.content.is_css())ingenerate_chunks_in_parallel— killing the dev server on any CSS edit with an unresolvable url, with no recovery — and release builds emitted an empty CSS chunk for the failed file.The fix gates the recovery loop on the file having parts (the same invariant the loop above uses to filter failed files; successfully parsed CSS always carries a lazy-export part).
How did you verify your code works?
The existing
test/bake/dev/css.test.tstests "syntax error crash" and "css import before create project relative" reproduce this deterministically on debug builds — they were failing on main (11 pass / 2 fail, each eating a ~100s timeout after the DevServer died) and pass after this change (13 pass / 0 fail). Release builds never executed the assertion, which is why CI stayed green.Also verified the full lifecycle standalone with a debug build: serve a page with a stylesheet (200) → edit the CSS to an unresolvable
url((500, server stays alive — previously panicked here) → fix the CSS (200 again).