Skip to content

bake: keep the client's stylesheet when a CSS root fails after parsing - #37886

Open
robobun wants to merge 1 commit into
mainfrom
farm/fb88958b/css-failed-root-keeps-styles
Open

bake: keep the client's stylesheet when a CSS root fails after parsing#37886
robobun wants to merge 1 commit into
mainfrom
farm/fb88958b/css-failed-root-keeps-styles

Conversation

@robobun

@robobun robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • Dev server, HTML route linking styles.css, a serve.static plugin whose onResolve returns undefined (or throws) for a url() in it. Hot-editing the sheet reports error: Could not resolve: "./missing.png" as it should, but the page also loses every rule of styles.css while the error is up. The same failure with no plugin, or a syntax error, keeps the old rules.
  • With no plugin, hot-editing a CSS root to @import "./not-css.js" reports Cannot import a ".jsx" file into a CSS file and applies the rejected stylesheet anyway.
  • Bundler side: the loop that drops failed CSS roots from the entry point set is followed by a step that adds back every entry point with a parsed stylesheet, so a root that failed after parsing still gets a chunk.
  • Dev server side: that chunk's asset is stored, then released when the failure is recorded; the hot update finds no asset for the hash and sends an empty stylesheet (the fallback from Robustness pass across install, css, ffi, crypto, spawn, shell, and node compat #36165), which wipes the page. In the @import case the rejected chunk is sent as is.

Fix

  • The bundler collects CSS entry points before the per-file loop, so a removal in the loop is final and a failed root produces no chunk. Only other difference: an entry point also imported on the server in the same bundle now keeps that flag.
  • The hot update skips CSS chunks whose asset no longer exists and patches the count in afterwards (covers the plugin-throws case, which still prints a chunk). The client keeps its current sheet, as it already does for JS files that fail after being received.
  • Why this is right: a failed rebuild has no new content, and the syntax error and builtin resolver paths already define the outcome as overlay plus the old rules until a rebuild succeeds. The plugin and @import paths now match them.
  • Verification: three new dev server tests (plugin falls through, plugin throws, root imports a non-CSS file) plus one tightened existing test, each checking the overlay text, that the old rules still apply, and that fixing the file restyles the client. All four fail on the release build; either source change alone leaves one failing.

Background

  • A CSS root is a stylesheet the dev server bundles as its own entry point into one CSS chunk. Its url() and @import references are resolved as part of that root's build, so a bad reference fails the whole root.
  • On each edit the dev server rebuilds the affected roots and sends connected clients a hot update listing CSS chunks by content hash. The chunk bytes live in an asset store keyed by that hash, so the payload writer looks each one up.
  • Recording a failure for a file puts it in the overlay and releases any asset registered for it. A syntax error leaves no parsed stylesheet; a resolution error routed through a plugin arrives after parsing, so the parsed sheet is still there when entry points are collected.
  • serve.static plugins are bunfig plugins loaded into the dev server's bundler. An onResolve that returns undefined falls through to the builtin resolver; one that throws fails the importing file with the thrown message.
Original description

Repro

Dev server, an HTML route linking styles.css, and a serve.static plugin whose onResolve matches missing.png and returns undefined (so resolution falls through to the builtin resolver and fails). With a client connected, change the stylesheet to

.a { background-image: url(./missing.png); }

The terminal and the overlay report error: Could not resolve: "./missing.png", as they do without the plugin, but the page also loses every rule of styles.css while the error is up. Without the plugin, and for a syntax error, the client keeps the rules it had (css.test.ts "css file with syntax error does not kill old styles"). The same happens when the plugin's onResolve throws, and a related form shows up without any plugin: hot-editing a CSS root to @import "./not-css.js" reports Cannot import a ".jsx" file into a CSS file and applies the rejected stylesheet anyway.

Cause

finish_from_bake_dev_server walks every file and removes CSS roots that failed from css_entry_points (a root whose resolution failure was reported asynchronously has had its parts cleared by run_resolver; a root with an invalid import is removed after scan_css_imports). It then adds every entry point that has a parsed stylesheet, which puts those roots straight back, so a failed root still gets a CSS chunk. The synchronous resolver path never hit this because it drops the stylesheet before it reaches the graph.

In finalize_bundle, pass 1 stores that chunk's asset and registers the file, prepare_and_log_resolution_failures then marks the file failed, which releases the asset again, and the hot update's CSS list looks the asset up by hash, finds nothing, and sends an empty stylesheet (the b"" fallback added in #36165 when this lookup was made hash based), which is what wipes the page. For the @import case the chunk's content is what gets applied, and registering the chunk also cleared the failure that had just been inserted for it.

Fix

  • bundle_v2.rs: seed css_entry_points from the entry points before the per-file loop, so the removals in that loop are final. Same comment and semantics as before; the only other difference is that an entry point imported on the server in the same bundle now keeps imported_on_server: true instead of having it reset by the late put.
  • DevServer.rs: the hot update payload skips CSS chunks whose asset no longer exists and patches the count in afterwards. A missing asset means the root failed after pass 1 stored its chunk (the plugin-throws case still prints a chunk since nothing clears its parts); there is nothing to send for it, and the client keeps its current sheet, which is what take_js_bundle_to_list already does for JS files that fail after being received.

This is the right behavior to have because a stylesheet that fails to rebuild has no new content for the client, and the dev server already defines the outcome for that situation on the syntax error and builtin resolver paths: overlay plus the previously applied rules, replaced when a rebuild succeeds. The plugin path only differed because the failed root still produced a chunk.

Related open PRs: #31945 (June) gates the same re-add on the file having parts. It was written for the debug crash that #31905 has since fixed at the source, and its gate would also cover the plugin fall-through case here, but not a root removed by scan_css_imports (parts intact) and not the payload side (plugin throws), which are the other two tests in this PR; moving the seeding above the loop makes every removal in the loop final, so this PR supersedes that diff. #37039 handles roots that fail to print and keeps failed roots in the route's CSS list; the asset check here also covers its print-failed chunks (their failure releases the asset in pass 1), so its css_chunk_printed filter becomes redundant once both land. #37844 edits the same loop in finish_from_bake_dev_server and is independent of this change.

Verification

Three new tests in test/bake/dev/css.test.ts (plugin falls through, plugin throws, root imports a non-CSS file), each asserting the overlay text, that the old rules are still applied, and that fixing the file restyles the connected client and serves 200 again. The existing "another root fails in the same rebuild" test now also asserts the failed root's rules on its client. All four fail on the release build (Selector '.a' was not found / Received: "#00f" / Selector '.first' was not found) and pass on the debug build. Each source change is needed: with only the bundle_v2.rs change, the plugin-throws test still fails; with only the DevServer.rs change, the non-CSS import test still fails. test/bake/dev css, html, plugins, bundle, hot and incremental-graph-edge-deletion suites pass on the debug build.

The new tests do not request the route while it is failed with a client connected: with BUN_ASSUME_PERFECT_INCREMENTAL=0 (the harness default) that rebuilds the route and ships the HTML module to the client, which is #31908 and unrelated to this change.

finish_from_bake_dev_server added every CSS entry point with a parsed
stylesheet after the loop that removes failed ones, so a root whose
resolution failure was reported asynchronously (through a plugin's
onResolve) or whose imports were rejected by scan_css_imports still
produced a CSS chunk. Seed the entry points before that loop instead.

In the hot update payload, skip CSS chunks whose asset no longer exists:
the root failed after its chunk was stored and insert_failure released
the asset. Sending an empty stylesheet for it wiped the styles the
client already had.
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 5 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6d5bf0bd-5819-4f24-afd2-2c49cc95a5e5

📥 Commits

Reviewing files that changed from the base of the PR and between 9a543cc and e1b47f6.

📒 Files selected for processing (3)
  • src/bundler/bundle_v2.rs
  • src/runtime/bake/DevServer.rs
  • test/bake/dev/css.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 3:05 PM PT - Aug 12th, 2026

@robobun, your commit e1b47f6 has 4 failures in Build #93387 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 37886

That installs a local version of the PR into your bun-37886 executable, so you can run:

bun-37886 --bun

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on the release build with the three scenarios in the PR body (plugin onResolve falls through, plugin onResolve throws, CSS root hot-edited to @import a .js file); the first two lose every rule of the stylesheet while the overlay is up, the third applies the rejected stylesheet. The new tests in test/bake/dev/css.test.ts fail on the release build and pass with this branch. Running a self-review next; will update here if anything changes.

@claude claude Bot left a comment

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.

I reviewed this PR and the automated bug hunt found no issues. The reordering in finish_from_bake_dev_server and the count-patching in the hot-update payload both look correct against the surrounding code, and the three new tests cover each failure mode. Because it touches ordering-sensitive dev-server/bundler state, carries a secondary semantic change to imported_on_server, and interacts with two in-flight PRs (#37039, #37844) editing the same paths, a human look would still be worthwhile.

What was reviewed:

  • bundle_v2.rs: confirmed the moved seed loop uses the same css_asts binding valid at the new location, and that the per-file loop's swap_remove/get_or_put sites now execute after seeding — removals are final.
  • DevServer.rs: confirmed the patched count uses to_le_bytes(), matching w_int!'s encoding, and the placeholder is 4 bytes at css_count_offset.
  • Checked that css_entry_points may already hold put_no_clobber entries from enqueue_entry_points_dev_server; the seed loop's .put() with imported_on_server: false is a no-op for those.
Extended reasoning...

Overview

The PR fixes a dev-server hot-reload bug where a CSS root that fails after parsing (async plugin onResolve failure, or @import of a non-CSS file) still produces a chunk, which then either wipes the client's stylesheet (asset released → b"" fallback) or applies the rejected content. Two source changes: in bundle_v2.rs, the entry-point seeding of css_entry_points moves from after the per-file loop to before it, so the loop's swap_remove calls stick; in DevServer.rs, the CSS section of the hot-update payload skips chunks whose asset is gone and patches the count in afterward. Three new tests plus one assertion added to an existing test.

Security risks

None. This is bundler/dev-server hot-reload logic; no auth, crypto, untrusted-input parsing, or filesystem-escape surface is touched.

Level of scrutiny

Medium-high. The dev server's incremental graph and asset registry are stateful and ordering-sensitive — the bug itself arose from a loop-ordering interaction. The bundle_v2.rs change carries a secondary semantic effect the author calls out: an entry point that is also imported server-side now keeps imported_on_server: true instead of having it reset by the late put(). That flag drives insert_css_file_on_server in DevServer.rs:4069, so this is a real behavior change (arguably a fix) that a maintainer should confirm is intended. The DevServer.rs change rewrites how the CSS-chunk count is encoded on the hot-update wire; I verified the placeholder write and later copy_from_slice(&css_count.to_le_bytes()) both match w_int!'s little-endian encoding and 4-byte width.

Other factors

  • The PR description explicitly notes interaction with two open PRs: #37039 (whose css_chunk_printed filter becomes redundant once both land) and #37844 (edits the same loop). A human should be aware of merge ordering.
  • Test coverage is strong: each of the three failure modes (plugin falls through, plugin throws, @import non-CSS) has its own test asserting the overlay error, that old rules persist, and that recovery restyles the client. The author states each source change is independently load-bearing (removing either breaks a test).
  • I confirmed enqueue_entry_points_dev_server may pre-seed css_entry_points via put_no_clobber with imported_on_server: false, so the moved seed loop's .put(... false) on those keys is a no-op — no regression there.

Given the subtlety of the state machine and the acknowledged secondary semantic change, deferring to a human.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

On the imported_on_server point, for whoever takes the human look: the flag only differs when a stylesheet is an explicit CSS entry point of a bundle (it or one of its @import children was edited) and a server-side file importing it is rebuilt in the same bundle. The per-file loop sets it to true for the server import, and the old late put reset it to false; now the true survives, which is the value every non-entry server-imported stylesheet already gets. true makes finalize_bundle call insert_css_file_on_server, the same refresh those stylesheets get on every rebuild, so the server importer's edge attaches through the cached index instead of depending on the server graph already having the node from an earlier bundle. No new code path is involved, so I did not add a test for it; the change is mentioned in the description so it is not mistaken for an accident.

No line comments to address. CI on e1b47f6 failed in the x64-asan and x64-musl build steps while downloading c-ares, mimalloc and WebKit (Failed to download after 5 attempts), which is unrelated to the diff; it will be re-run.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Don't emit a CSS chunk for files whose import resolution failed in the dev server #31945 - Fixes the same bug in the same "Find CSS entry points" block of finish_from_bake_dev_server: a CSS root that failed after parsing was re-added to start.css_entry_points and got a CSS chunk anyway (Don't emit a CSS chunk for files whose import resolution failed in the dev server #31945 gates the re-add on parts.len() != 0; this PR moves the block above the loop so the removal sticks).
  2. bake: report per-file print failures in the dev server instead of serving empty modules #37039 - Rewrites the same finalize_bundle CSS chunk send block for the same purpose — not emitting a CSS payload for a chunk with no real content so the client keeps its already-applied stylesheet (bake: report per-file print failures in the dev server instead of serving empty modules #37039 filters on print failures; this PR skips chunks whose asset was released).

🤖 Generated with Claude Code

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Checked both candidates.

#31945 (open since June, bundle_v2.rs only) adds parts.len() != 0 to the re-add loop this PR moves. It was written for the debug crash from #31903, whose cause #31905 later removed (the synchronous resolver path no longer parks the stylesheet), so the situation it describes no longer exists on main. Its gate would still cover one of the three cases here, the plugin fall-through, because run_resolver clears that root's parts. It does not cover a root removed after scan_css_imports (parts intact, so the gate re-adds it; the @import test here), and it has no counterpart to the payload change (the plugin-throws test here, where the chunk is printed and the failure is recorded afterwards). Moving the seeding above the loop makes every removal in that loop final, which is why this PR does that instead of adding the gate. I have added #31945 to the description; this PR supersedes its diff, so one of the two should be closed when this lands.

#37039 is about roots that fail to print and keeps failed roots in the route's CSS list. Its payload filter only skips print-failed chunks; a root whose failure is recorded after pass 1 still prints, so it still gets the empty stylesheet sent, and it does not change finish_from_bake_dev_server. The two compose (its print-failed chunks also have their asset released, so the check here covers them too), as already noted in the description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant