bundler: fix dev server crash when a CSS rebuild fails import resolution - #31905
Conversation
|
Need an answer fast? Review this PR in Change Stack to ask focused questions about the PR or a changed range. WalkthroughWhen CSS import resolution fails during parsing, the bundler now drops the parsed stylesheet instead of storing it in the graph, preventing failed files from being treated as valid CSS modules. A new HMR test verifies that a CSS URL resolution error produces a 500 and that fixing the CSS restores a 200 response. ChangesCSS Resolution Error Handling
Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
…king it on the graph When a CSS file parses successfully but one of its import records fails to resolve, the parse result is converted to an error and the parsed stylesheet was moved onto the graph row so teardown could free it. The dev server proceeds with failed files and treats a populated css slot as a successfully parsed CSS file, so the failed file was re-added as a CSS entry point in finish_from_bake_dev_server. That produced a CSS chunk while graph.css_file_count stayed 0, tripping debug_assert!(!chunk.content.is_css()) in generate_chunks_in_parallel and crashing the dev server on hot reload. Drop the stylesheet at the failure site instead. The graph row stays None for failed files, matching the reference behavior, and the leak fix that introduced the parking is preserved. Fixes #31903
Connect a client so the errors option is actually validated against the overlay, and update the stale comment above the import_records preservation to scope the no-linker claim to non-dev builds. Recovery is asserted via fetch without a connected client: the recovery patch ships the HTML route as a JS module without the route-reload flag, which trips a client-runtime debug assert. Tracked in #31908.
50fd91a to
c3f60a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bundler/bundle_v2.rs`:
- Around line 5737-5749: The code currently only drops the parsed stylesheet in
the resolve_import_records error path (the block using result.ast.css.take() and
unsafe drop_in_place) but not when async plugin onResolve failures occur,
leaving items_css[source_index].is_some() set and allowing
finish_from_bake_dev_server() to re-add a failed CSS entry point; extract the
stylesheet invalidation/drop logic into a shared helper (e.g.,
invalidate_failed_css or clear_parsed_stylesheet) and call it from both the
resolve_import_records error branch and the plugin onResolve error branch so
that whenever a CSS resolution fails (either via last_error or onResolve), the
AST/css arena is removed and items_css[source_index] is cleared consistently to
prevent re-adding failed CSS chunks (update calls in resolve_import_records(),
the onResolve error handling site, and ensure finish_from_bake_dev_server() will
no longer see a populated css slot).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 310e14e6-d86a-442a-9bee-a3888b60310f
📒 Files selected for processing (2)
src/bundler/bundle_v2.rstest/bake/dev/css.test.ts
|
CI status for the merging maintainer: the remaining red lanes on build 60948 are not related to this diff.
Locally on the debug/ASAN build: all 14 tests in test/bake/dev/css.test.ts pass, including the two that crash on main and the new regression test, plus the html/bundle/hot/plugins bake suites. |
Fixes #31903
Repro
Debug builds panic during a dev server hot reload when a CSS file parses but fails import resolution (for example an unterminated
url(that tokenizes to a URL, or aurl(./missing.png)pointing at a file that does not exist):Both of these existing tests crash on main with
DevServer crashed while waiting for hot reload:Cause
graph.css_file_countis only incremented when a parse task completes successfully with a CSS AST. When resolution fails after a successful CSS parse,run_resolution_for_parse_taskconverts the result to an error, and (since the leak fixes in #30875) moved the parsed stylesheet onto the graph row so teardown could free it. That comment assumed the linker never runs after such an error, but the dev server intentionally proceeds with failed files, andfinish_from_bake_dev_servertreats a populatedcssslot as "successfully parsed CSS" when discovering CSS entry points. The failed file was therefore re-added as a CSS entry point and got a CSS chunk whilecss_file_countstayed 0, tripping thedebug_assert!(!chunk.content.is_css())ingenerate_chunks_in_parallel. In release builds the assert compiles out and the CSS dedup/prepare pass is silently skipped for that chunk. The Zig implementation never stored the CSS AST on this path, so the dev server's invariant held there.The parked AST also diverged from the reference in
find_imported_files_in_css_order(a failed file's stale rules could be included in another chunk's import order) andscan_css_imports.Fix
Drop the stylesheet at the failure site in
run_resolution_for_parse_taskinstead of parking it on the graph row. The graph row staysNonefor failed files, restoring the invariant every dev server consumer of thecsscolumn relies on, and the stylesheet's non-arena allocations are still freed (samedrop_in_placethe teardown pass uses).Verification
test/bake/dev/css.test.ts"css url resolve error on hot reload is recoverable" fails on the unfixed build withDevServer crashed while waiting for hot reloadand passes with the fix: a connected client asserts the exact error overlay text and the route returns 500, then recovery back to a 200 is checked after the error is fixed. Exercising the recovery with the client still connected hits a separate, pre-existing HMR patch bug (the HTML route module is shipped without the route-reload flag), tracked in Dev server ships HTML route module as a JS HMR module when a failed CSS root recovers #31908.test/bake/dev/css.test.ts(14 tests, including the two previously-crashing ones),test/bake/dev/html.test.ts,test/bake/dev/bundle.test.ts,test/bake/dev/hot.test.ts, andtest/bake/dev/plugins.test.tspass on the debug/ASAN build.Bun.buildwith a CSS entry whoseurl()fails to resolve) still reportsCould not resolveand is clean under ASAN.