Skip to content

bake: keep a route failed when the "use client" file it imports fails to bundle - #37850

Open
robobun wants to merge 7 commits into
mainfrom
farm/662773cb/bake-failed-use-client-boundary
Open

bake: keep a route failed when the "use client" file it imports fails to bundle#37850
robobun wants to merge 7 commits into
mainfrom
farm/662773cb/bake-failed-use-client-boundary

Conversation

@robobun

@robobun robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • Dev server with serverComponents.separateSSRGraph = true, a route imports a "use client" file whose own import is missing. The first requests get the "Bun - Build Failed" page; after any hot update in the route's graph the route runs instead, the server throws error: Failed to load bundled module 'components/Sibling.ts'. This is not a dynamic import, and therefore is a bug in Bun's bundler. and the user gets a generic 500 with the real error gone. Same with BUN_ASSUME_PERFECT_INCREMENTAL=0 and =1.
  • Cause: with a separate SSR graph such a file is bundled for the browser, so its failure lives on the client graph and the server graph has no node for it at all. Nothing connects the route to the failure. It looked right at first only because the route check reads a list still holding the previous bundle's failures; the hot update reset that list.
  • The same stale list makes a route marked by an earlier failure report the latest bundle's errors even when it imports none of them (with =0 it only causes a needless rebuild).
  • Also on main: two nested working "use client" files can panic on the first request with Server Incremental Graph is missing component for "", depending on parse order. A copy of a boundary arriving from the SSR graph was taken as the file dropping its directive, so its client side was deleted while the same bundle was adding it.

Fix

  • A "use client" file that fails to resolve (separate SSR graph only) now gets the graph shape a bundled boundary has: a stale server node flagged as a boundary, and a client node flagged as an HMR root. Property to check: every trace treats a failed boundary exactly like a bundled one, and the failure itself still lives on the client node, so fixing the file clears it through the normal path. For a boundary that bundled before this is a no-op.
  • The route check clears the per-bundle failure list before tracing, so a route reports only failures reachable from its own imports.
  • A copy of a file coming from the SSR graph no longer counts as it dropping "use client"; only its server-graph parse decides that. The walk over affected client components goes by index because the trace appends to the list being walked (ASAN reports the old buffer as a use-after-free; bake: fix dev server crashes in SCB dependency tracing and route syntax error reporting #31697 carries the same change).
  • Verification: six new dev-server tests. Each fails on main as described in the original except the nested failing file case, which passes on main and only guards the list walk under ASAN; all pass on a debug/ASAN build. Left out, tracked separately: a server file that once failed is rebuilt into the SSR graph from then on, and a deleted failed client node keeps its failure entry.

Background

  • Bake's dev server keeps an incremental graph per side (server, client): a node per file, an edge per import, and each bundling failure attached to the node it happened on. A route gets the Build Failed page only if a failure is reachable by walking its imports; a failure with no path from the route is invisible to it.
  • A "use client" boundary is a file with a node in both graphs: the server node is flagged as a boundary and the client node as an HMR root, and those two flags are what let a trace cross between the graphs.
  • separateSSRGraph is a framework option that adds an SSR graph beside the server one. With it on, a "use client" file is bundled for the browser even when server code imports it, and SSR-side code imports it as a plain module, not as a boundary.
  • BUN_ASSUME_PERFECT_INCREMENTAL=0 re-bundles a route on each request; =1 trusts the graph. The main test runs in both modes.
  • Each bundle collects the failures it adds into a list that is reset when the next bundle starts; the route check reused that list as its scratch space.
Original description

Repro

Dev server with a framework that sets serverComponents.separateSSRGraph = true:

// routes/index.ts
import { good } from '../good';
import '../components/Sibling';
export default () => new Response('page: ' + good);

// components/Sibling.ts
"use client";
import './sibling-missing';   // does not exist

The first requests to / correctly get the "Bun - Build Failed" page. After a hot update of good.ts (any file in the route's graph), the next request runs the route instead, the server runtime throws

error: Failed to load bundled module 'components/Sibling.ts'. This is not a dynamic import, and therefore is a bug in Bun's bundler.
GET - / failed

and the user gets a generic 500 with the real bundling error gone. Same with BUN_ASSUME_PERFECT_INCREMENTAL=0 and =1. A "use client" component that had bundled once before it started failing was not affected, only one that never bundled. The same missing association also breaks the other things that happen to such a file: removing the directive while fixing it re-bundled it as a client module (route still unloadable), and deleting it did not re-bundle the route.

Cause

With a separate SSR graph, ParseTask switches a "use client" file to the browser target even when a server file imported it, so its resolution failure is recorded on a client graph node (get_log_for_resolution_failures). The boundary bookkeeping that creates the server side of a boundary (reference proxy, server_component_boundaries) only runs for files that resolve, so the server graph had no node for Sibling.ts at all. process_chunk_dependencies for the route therefore attached no edge, index_failures could not reach the route from the failure, and check_route_failures (which traces the route's imports) found nothing.

It still looked right at first because check_route_failures collects into incremental_result.failures_added, and that list still held the previous bundle's failures (it is only reset when the next bundle starts). The hot update reset it, the trace found nothing, and the route was loaded. The same stale list also makes a route that was marked by an earlier bundle report the most recent bundle's errors even if it does not import any of them (with =0 it only triggers a needless rebuild).

Fix

  • run_resolution_for_parse_task reports a "use client" file that failed to resolve to the dev server (new handle_client_component_boundary_failure callback, only under separateSSRGraph, where the retargeting happens). The dev server gives it the shape a bundled boundary has: a stale server node flagged as a client component boundary, so server importers attach their edges to it and import traces continue into the client graph, plus the client node marked as an HMR root so dependency traces cross back to the server side. The failure itself stays owned by the client node, so fixing the file removes it through the existing receive_chunk path, and the file is re-bundled from the server side exactly like an existing boundary (invalidate skips HMR roots on the client). For a boundary that had bundled before, all of this is already true and the call is a no-op.
  • check_route_failures clears failures_added before tracing.
  • finalize_bundle walks client_components_affected by index. trace_dependencies appends every boundary it visits to that list while the loop iterates it; with the failing file reachable through another boundary the list now grows past its capacity during the loop, which ASAN reports as a use-after-free of the old buffer (the new test hit it). bake: fix dev server crashes in SCB dependency tracing and route syntax error reporting #31697 contains the same change as part of a larger crash fix; it is needed here for this PR's own test.
  • receive_chunk (server side) no longer treats a chunk coming from the SSR graph as the file having dropped its directive. A "use client" file imported by an SSR graph file is bundled there as a plain SSR module; only its RSC graph parse says whether it is still a boundary. Before, when such a chunk arrived in the same bundle that registered the boundary, the client side of the boundary was deleted while the bundle was still adding it, and processing the client chunk then panicked with Server Incremental Graph is missing component for "". This is reachable on main with two nested working "use client" files (depends on parse order; it reproduces reliably on the released build here), and deterministically in the delete/re-create flow of a failed boundary once the first fix associates the route with it.

Why this is the right place: the route has to learn about the failure through the graph, since that is what every later decision (index_failures, check_route_failures, rebuilds, HMR notifications) is derived from. Attributing the failure to the server graph instead would leave client-side importers of the same file without an edge; creating both halves mirrors what a successful bundle produces and keeps a single failure entry per file.

Found but left out (tracked separately): a server file that had a resolution failure is marked is_ssr by prepare_and_log_resolution_failures and is rebuilt into the SSR graph from then on (this is what bundles the route into the SSR graph in the delete/re-create test); and a failed client node removed by disconnect_and_delete_file keeps its entry in bundling_failures.

Verification

New tests in test/bake/dev/bundle.test.ts; each fails on main as described, except the one marked, and all pass with this branch on a debug/ASAN build:

  • route importing a failing "use client" file, with BUN_ASSUME_PERFECT_INCREMENTAL=0 and =1: Build Failed before and after an unrelated hot update, recovers when the missing file is created, and the same once the component has bundled once (main: generic 500 after the hot update)
  • failing "use client" file that is only reachable through another "use client" file (passes on main; guards the list walk under ASAN, which the first fix makes reachable)
  • removing "use client" from a file that never bundled (main: generic 500)
  • deleting and re-creating a "use client" file that never bundled (main: generic 500 after the delete; with only the first fix: the panic above)
  • working "use client" file imported from another "use client" file (main: the panic above on the first request)
  • a route marked by an earlier failure does not report another route's error with =1 (main: Build Failed page with the other route's error)

test/bake/dev/bundle.test.ts (28 tests), esm, hot, css, plugins, incremental-graph-edge-deletion and test/bake/framework-router.test.ts pass on the debug build.

With a separate SSR graph, a "use client" file imported by a server file
is parsed for the browser, so a resolution failure in it is owned by the
client graph. The bundler only registers the boundary (and so the server
side node) for files that bundled, which left the route with no edge to
the failure: it was only served as "Build Failed" while the previous
bundle's failures_added list happened to still hold the error, and ran
the route against a never-emitted module after the next hot update.

Have the bundler report such a file to the dev server, which gives it
the same two-sided shape as a bundled boundary: a stale server node
flagged as a boundary for server importers to attach to, and the client
node marked as an HMR root.

check_route_failures now clears failures_added before tracing, so a
route marked by an earlier bundle does not report (or rebuild because
of) errors in files it does not import.

finalize_bundle walks client_components_affected by index, since
trace_dependencies appends to that list while it is being walked; with
two boundaries involved this reallocated the list under the iterator.
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The dev server now receives failed "use client" boundary resolutions, updates SSR and client graph state, isolates route failure tracing, and preserves failures across incremental builds. Regression tests cover repeated failures, recovery, directive changes, nested boundaries, and route isolation.

Changes

Client boundary failure handling

Layer / File(s) Summary
Boundary failure dispatch
src/bundler/bundle_v2.rs, src/bundler/lib.rs, src/runtime/bake/dev_server/mod.rs
Browser resolution reports failed "use client" files through the new DevServerHandle dispatch method and runtime vtable handler.
Graph failure state and route tracing
src/runtime/bake/DevServer.rs, src/runtime/bake/dev_server/incremental_graph.rs
Failed paths are registered in both graphs. The client node becomes an HMR root, while the server node remains stale and becomes a client-component boundary. Route tracing clears route-local entries and supports appended affected boundaries. SSR updates no longer remove client-graph boundaries.
Failure persistence and recovery coverage
test/bake/dev/bundle.test.ts
Tests cover separate SSR graphs, repeated failures, incremental modes, dependency recovery, directive removal, nested client boundaries, deletion and recreation, and route-specific errors.

Suggested reviewers: jarred-sumner, dylan-conway

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the primary fix for route failures caused by unresolved "use client" imports.
Description check ✅ Passed The description explains the problem, fix, scope, and verification in detail, although it uses equivalent headings instead of the template headings.

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

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced and fixed, waiting on CI.

Reproduced with the new tests in test/bake/dev/bundle.test.ts against the released build (USE_SYSTEM_BUN=1): the "route importing a failing "use client" file" tests, "removing "use client" from a file that never bundled", "deleting and re-creating ..." and "route marked by an earlier failure ..." fail there with the generic 500 (Failed to load bundled module 'components/Sibling.ts') or the wrong route's error, and "working "use client" file imported from another "use client" file" crashes the released build with Server Incremental Graph is missing component for "". All of them pass with this branch on a debug/ASAN build. The "fails to bundle, imported from another "use client" file" test passes on main and covers the client_components_affected walk under ASAN.

Review feedback so far has been addressed and the two adjacent issues found on the way are tracked separately (see the PR description).

@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 bug-hunting pass found no issues. Because it changes incremental-graph invariants for server-component boundaries (new stale server node + is_client_component_boundary/is_hmr_root flags that later traces and receive_chunk depend on), a human look is still worthwhile.

What was reviewed:

  • failures_added.clear() in check_route_failures — entries are Cloned SerializedFailures owned by bundling_failures, so dropping them is safe; the existing scope-exit clear() doesn't cover the first request after a bundle.
  • Index-based walk of client_components_affected — confirmed trace_dependencies pushes to that vec through owner() raw-pointer paths, so the previous iterator could dangle on realloc.
  • handle_client_component_boundary_failure — called from the same bundle-thread context as log_for_resolution_failures/handle_parse_task_failure; insert_stale + explicit ensure_stale_bit_capacity matches how insert_stale_extra skips setting the bit when the index is past stale_files.bit_length.
Extended reasoning...

Overview

The PR fixes a bake dev-server bug where a route importing a "use client" file that fails to resolve (under separateSSRGraph) loses its failure association after the first hot update, resulting in a generic 500 instead of the Build Failed page. Five files touched: bundle_v2.rs calls a new handle_client_component_boundary_failure dispatch method when a client-directive file fails resolution; lib.rs and dev_server/mod.rs wire the interface; DevServer.rs implements the method (creates the server-side boundary node so importers can attach edges), clears failures_added before check_route_failures traces, and switches the client_components_affected walk in finalize_bundle from an iterator to index-based (trace_dependencies appends to the same vec via raw-pointer sibling access, so the old iterator could read a freed buffer after realloc). Four new dev-server tests exercise both BUN_ASSUME_PERFECT_INCREMENTAL modes, a nested-boundary case, and the stale-failures_added cross-route case.

Security risks

None. This is dev-server-only bookkeeping for error propagation and HMR; no auth, no untrusted-input parsing beyond what the existing bundler already does.

Level of scrutiny

Medium-high. The individual edits are small and each is well-argued in the PR body, but they touch the incremental-graph invariants that invalidate, receive_chunk, trace_dependencies, and trace_imports all depend on. handle_client_component_boundary_failure synthesizes a server node with is_client_component_boundary = true and a client node with is_hmr_root = true for a file that never bundled — the PR body explains why this mirrors what a successful boundary produces and why later receive_chunk transitions handle it, and the tests cover recovery, but someone familiar with the bake graph should confirm there's no edge-deletion or disconnect_and_delete_file path that assumes a boundary node always has bundled content.

Other factors

  • No prior human reviews on the PR; only a rate-limited CodeRabbit placeholder.
  • The PR description is unusually thorough — it names the exact mechanism for each of the three changes and cross-references #31697 for the iterator-invalidation piece.
  • Tests are placed in the existing test/bake/dev/bundle.test.ts, follow harness conventions (devTest, errors: null), and assert specific error messages via the serialized-failure payload rather than generic status checks.
  • No outstanding reviewer comments to address.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:06 AM PT - Aug 12th, 2026

@robobun, your commit e4a61a9 is building: #93490

Comment thread src/bundler/bundle_v2.rs Outdated
Comment thread src/runtime/bake/DevServer.rs Outdated
Comment thread src/runtime/bake/DevServer.rs Outdated
Comment thread src/runtime/bake/DevServer.rs Outdated
Comment thread src/runtime/bake/DevServer.rs Outdated
Comment thread src/runtime/bake/DevServer.rs Outdated
Comment thread src/bundler/bundle_v2.rs Outdated
Comment thread src/runtime/bake/DevServer.rs Outdated

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🔇 Additional comments (10)
src/bundler/bundle_v2.rs (2)

5886-5892: Comment length was already flagged here.

The comment-cop bot already reported the paragraph-long justification comment on this range. Keep only the durable invariant, or restructure the code so the explanation is not needed.


5893-5905: 🗄️ Data Integrity & Integration

⚠️ Unverified finding
Sandbox verification was unavailable.

Verify the failure-report gate matches the boundary condition.

The gate reports the failure only when result.use_directive == crate::UseDirective::Client and target == Target::Browser. In handle_client_component_boundary_failure the server node is created with insert_stale(abs_path, false), so the SSR variant is not covered. Confirm that no Target::Bake_Server_Components_SSR parse of the same "use client" file reaches this error path with separate_ssr_graph = true, and that the key passed here (result.source.path.text) matches the key used by finalize_bundle for the same file (path.key_for_incremental_graph()).

src/runtime/bake/DevServer.rs (5)

2254-2257: Comment length was already flagged here; the clear itself is correct.

The comment-cop bot already reported the paragraph-long justification comment on this range. The failures_added.clear() before the trace correctly scopes the collected failures to this route.


4395-4397: Comment length was already flagged here.

The comment-cop bot already reported the paragraph-long justification comment on this range.


4997-5006: Comment length was already flagged in this function.

The comment-cop bot already reported the paragraph-long doc comment and the inline justification comments in handle_client_component_boundary_failure.

Also applies to: 5013-5015, 5020-5022


4398-4408: LGTM!


5016-5029: 🩺 Stability & Availability

⚠️ Unverified finding
Sandbox verification was unavailable.

Verify the client node also becomes stale and that both nodes stay paired.

The server node gets an explicit stale bit through ensure_stale_bit_capacity(true) and stale_files.set(...). The client node only gets is_hmr_root = true; the code relies on the later failure insertion to mark it stale. If a rebuild path clears the client failure without re-running this function, the client node stays non-stale while is_hmr_root is set, and IncrementalGraph::trace_dependencies on the client side then calls server_graph.get_file_index(key) and panics when the server node is gone. Confirm that insert_failure for the client graph always sets the stale bit for this path, and that deletion of one side removes the flag on the other.

src/bundler/lib.rs (1)

328-328: LGTM!

src/runtime/bake/dev_server/mod.rs (1)

1174-1178: LGTM!

test/bake/dev/bundle.test.ts (1)

869-875: LGTM!

Also applies to: 897-947, 952-980, 986-1006, 1013-1050

🤖 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 `@test/bake/dev/bundle.test.ts`:
- Around line 878-887: Update expectBuildFailed to store the atob markup match,
assert that it exists before accessing its capture group, and include the
response HTML in the assertion failure message. Preserve the existing
serialized-failure decoding and error-content assertions after the match is
validated.
🪄 Autofix

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: 8b3c8582-b5c8-4063-83fc-5c0bfe9692c3

📥 Commits

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

📒 Files selected for processing (5)
  • src/bundler/bundle_v2.rs
  • src/bundler/lib.rs
  • src/runtime/bake/DevServer.rs
  • src/runtime/bake/dev_server/mod.rs
  • test/bake/dev/bundle.test.ts

Comment thread test/bake/dev/bundle.test.ts
Comment thread src/runtime/bake/DevServer.rs Outdated
@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

On the question of code paths that might assume the synthesized boundary node has bundled content: I went through the consumers of the two flags.

  • disconnect_and_delete_file (reached when the directive is removed and the file bundles as a plain server module) only disconnects edges, removes the directory-watch deps that borrow the client key, and tombstones the key; it never touches content. The failed node has no import edges (failed files are not in the chunk, so process_chunk_dependencies never ran for it), and its deps were registered against the client key by track_resolution_failure, so they are removed there. This is now covered by the removing "use client" from a file that never bundled test, which also fails on main (the file was re-bundled as a client module and the route could not load it).
  • on_file_deleted on the server node (file deleted while failing) disconnects edges and re-enqueues the importers, no content involved.
  • invalidate takes the same path as an existing boundary: the server node is is_rsc, so it is re-bundled as a server entry point, and the client node is skipped because it is an HMR root.
  • trace_imports / trace_dependencies look the other side up by key; both nodes exist from this point on, and the only thing that deletes the client node also clears the server flag.
  • generate_client_bundle is only reached for routes that are Loaded, and every route that reaches this node through its imports also reaches its failure. An existing boundary that starts failing already produces the same content-less client node today.
  • receive_chunk on the server node later either keeps the flag (proxy arrives, client_components_added) or clears it and deletes the client node; the server runtime's componentManifestDelete tolerates an entry it never registered.

One related pre-existing issue is intentionally left out of this PR: when disconnect_and_delete_file removes a client node that is failed, its entry stays in bundling_failures (also true for boundaries that bundled before they started failing). That is being tracked separately.

@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 didn't find any bugs. Because it changes incremental-graph state transitions in the bake DevServer (synthesizing boundary nodes on the failure path, plus two adjacent fixes), a human familiar with this subsystem should also take a look.

What was reviewed:

  • handle_client_component_boundary_failure against insert_stale's found-existing paths — the client-side content free is either on an Unknown/empty node or on one invalidate already staled, so the "already-bundled boundary" case stays a no-op as claimed.
  • The client_components_affected walk — trace_dependencies does append to the same list mid-walk; the index loop is correct.
  • failures_added.clear() in check_route_failures — the only writer between bundles is the trace itself, so clearing there is right.
  • The bundler-side condition (UseDirective::Client + Target::Browser + separate_ssr_graph) matches the retargeting the description names.
Extended reasoning...

Overview

Three related fixes in the bake DevServer, plus dispatch plumbing and tests:

  • src/bundler/bundle_v2.rs / lib.rs / dev_server/mod.rs: new handle_client_component_boundary_failure slot on DevServerHandle, invoked from run_resolution_for_parse_task's error branch when a "use client" file was retargeted to the browser under separateSSRGraph.
  • src/runtime/bake/DevServer.rs: the new method inserts stale nodes into both graphs and sets is_hmr_root (client) / is_client_component_boundary + stale bit (server); check_route_failures clears incremental_result.failures_added before tracing; finalize_bundle iterates client_components_affected by index because trace_dependencies appends to it.
  • test/bake/dev/bundle.test.ts: five new dev-server tests covering both BUN_ASSUME_PERFECT_INCREMENTAL modes, nested boundaries (the ASAN repro), directive removal, and cross-route failure isolation.

Security risks

None. Dev-server-only bundler bookkeeping; no auth, crypto, untrusted-input parsing, or network surface.

Level of scrutiny

Medium-high. Each individual edit is small, but correctness rests on incremental-graph invariants that are not local to the diff — the author's own timeline comment enumerating every consumer of is_hmr_root / is_client_component_boundary / disconnect_and_delete_file shows how much context is needed to be confident the synthesized node behaves like a real bundled boundary on every later path (invalidate, receive_chunk, generate_client_bundle, deletion). That analysis reads correctly to me, but this is exactly the class of change where a maintainer who owns bake's graph model should confirm nothing was missed.

Other factors

  • Tests are well-constructed: verified to fail on the released build (USE_SYSTEM_BUN=1), cover both env modes and the recovery path, and include an ASAN guard for the list-growth-during-iteration case. The expectBuildFailed helper asserts the actual serialized error payload rather than just the status.
  • All bot feedback (comment-cop on comment length, CodeRabbit on the helper's null-match) has been addressed and the threads are resolved.
  • The index-loop change overlaps #31697, which the description acknowledges.
  • No prior claude[bot] review on this PR.

…ports it

A "use client" file imported from an SSR graph file is bundled there as a
plain SSR module. receive_chunk treated any non-boundary chunk for the
file's server node as the file having dropped its directive and deleted
the client side of the boundary, which in a bundle that also registers
the boundary leaves a tombstoned client node behind and panics with
"Server Incremental Graph is missing component" while processing the
client chunk. Only the RSC graph's parse of the file decides that.
Comment thread src/runtime/bake/dev_server/incremental_graph.rs Outdated

@coderabbitai coderabbitai 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.

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/runtime/bake/DevServer.rs`:
- Around line 2254-2255: Update the route logic around the graph-state
allocation and failures_added so stale failure state is cleared and any required
deferred cleanup is registered before the allocation that may return early.
Ensure every allocation-error path completes cleanup, preventing later routes
from observing failures from a previous route.
🪄 Autofix

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: a40a4109-5433-446b-b2a5-fb3c1b3d7a19

📥 Commits

Reviewing files that changed from the base of the PR and between 7854006 and e4a61a9.

📒 Files selected for processing (4)
  • src/bundler/bundle_v2.rs
  • src/runtime/bake/DevServer.rs
  • src/runtime/bake/dev_server/incremental_graph.rs
  • test/bake/dev/bundle.test.ts

Comment on lines +2254 to +2255
// Still holds the last bundle's failures, which this route may not import.
dev.incremental_result.failures_added.clear();

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.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Clear failure state before graph-state allocation.

Line 2253 can return before Line 2255 clears stale entries or registers deferred cleanup. A later route can then report failures from another route after this allocation error.

Proposed fix
-    let mut gts = dev.init_graph_trace_state(0)?;
     // Still holds the last bundle's failures, which this route may not import.
     dev.incremental_result.failures_added.clear();
     // Note: erase to a raw pointer so the deferred cleanup only fires on
     // scope exit when no other borrow of `dev` is live.
     let dev_ptr = std::ptr::from_mut::<DevServer>(dev);
     scopeguard::defer! {
         // SAFETY: see Note above.
         unsafe { (*dev_ptr).incremental_result.failures_added.clear() }
     };
+    let mut gts = dev.init_graph_trace_state(0)?;

As per coding guidelines: “Every error, abort, and timeout path must complete the operation.”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Still holds the last bundle's failures, which this route may not import.
dev.incremental_result.failures_added.clear();
// Still holds the last bundle's failures, which this route may not import.
dev.incremental_result.failures_added.clear();
// Note: erase to a raw pointer so the deferred cleanup only fires on
// scope exit when no other borrow of `dev` is live.
let dev_ptr = std::ptr::from_mut::<DevServer>(dev);
scopeguard::defer! {
// SAFETY: see Note above.
unsafe { (*dev_ptr).incremental_result.failures_added.clear() }
};
let mut gts = dev.init_graph_trace_state(0)?;
🤖 Prompt for 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.

In `@src/runtime/bake/DevServer.rs` around lines 2254 - 2255, Update the route
logic around the graph-state allocation and failures_added so stale failure
state is cleared and any required deferred cleanup is registered before the
allocation that may return early. Ensure every allocation-error path completes
cleanup, preventing later routes from observing failures from a previous route.

Source: Coding guidelines

@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 didn't find any bugs. Because it changes incremental-graph state-machine logic across the server/SSR/client graphs (synthesizing boundary nodes, changing when receive_chunk demotes a boundary, iterating a list that grows mid-walk), a look from someone familiar with bake's graph invariants would still be worthwhile.

On the open CodeRabbit thread at check_route_failures: init_graph_trace_state(0)? only early-returns on OOM allocating the trace bitsets, and the pre-existing scopeguard::defer! was already registered after that same ? — so the ordering matches pre-PR behavior and is not a regression introduced here.

What was reviewed

  • handle_client_component_boundary_failure: checked that insert_stale on both sides preserves an existing node's failed/is_hmr_root flags, and that setting the server stale bit after ensure_stale_bit_capacity is what makes the node re-bundle from the server side.
  • The !is_ssr_graph guard in receive_chunk: confirmed the RSC-graph parse of the same file is what carries scb, so an SSR-graph chunk with scb=false is not evidence the directive was dropped.
  • The index-based walk of client_components_affected: trace_dependencies pushes to this list via dev_incremental_result(), so the for-over-slice iterator held a pointer into the old buffer across a possible grow.
Extended reasoning...

Overview

The PR fixes a bake dev-server bug where a route importing a "use client" file that fails to resolve (under separateSSRGraph: true) loses its failure association after the first hot update, causing the route to run against a never-emitted server module and return a generic 500. Four coupled changes: (1) bundle_v2.rs calls a new handle_client_component_boundary_failure dev-server hook when a retargeted "use client" file fails to resolve; (2) DevServer.rs implements that hook by inserting stale nodes on both graphs with the flags a bundled boundary would have; (3) check_route_failures clears incremental_result.failures_added before tracing so a route doesn't inherit the previous bundle's failures; (4) finalize_bundle walks client_components_affected by index because trace_dependencies appends to it mid-iteration; (5) receive_chunk on the server side no longer treats an SSR-graph chunk as the file dropping its "use client" directive. Six new dev-server tests cover each path plus a pre-existing panic in nested working boundaries.

Security risks

None. This is dev-server-only incremental-graph bookkeeping; no untrusted input parsing, no auth/crypto, no user-facing API surface change.

Level of scrutiny

High. The bake incremental graph maintains parallel server/client state with cross-graph lookups (owner() sibling projections), and the correctness of synthesizing a boundary node depends on every consumer of is_client_component_boundary / is_hmr_root / the stale bit tolerating a node that never had bundled content. The author enumerated those consumers in a PR comment (disconnect_and_delete_file, on_file_deleted, invalidate, trace_imports/trace_dependencies, generate_client_bundle, receive_chunk), which is exactly the analysis a reviewer needs — but it's the kind of invariant enumeration a bake maintainer should confirm rather than an automated pass. The !is_ssr_graph guard is a one-token change with non-local consequences (it changes when the client side of a boundary is deleted).

Other factors

  • The bug hunter found nothing; I independently checked the flag-preservation semantics of insert_stale_extra on both sides and the growth-during-iteration path in finalize_bundle.
  • The one unresolved CodeRabbit comment is about statement ordering around an OOM-only ? in check_route_failures; the pre-existing scopeguard::defer! was already registered after that same ?, so the new .clear() placement matches existing behavior and is not a regression.
  • Test coverage is thorough (both BUN_ASSUME_PERFECT_INCREMENTAL modes, recovery, directive removal, delete/re-create, nested boundaries, cross-route failure isolation), and the description states each fails on main as described.
  • CI build #93490 was still in progress at the time of the last timeline update.
  • Two adjacent pre-existing issues were found and explicitly deferred (noted in the PR description), which is appropriate scoping.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

While checking that #37862 composes with this branch I ran the new tests at e4a61a9 on a linux x64 debug (ASAN) build. working "use client" file imported from another "use client" file crashes the dev server on the hot update, reproducibly, on this branch by itself (without #37862 applied as well):

==28181==ERROR: AddressSanitizer: SEGV on unknown address 0x6c6de1fc0060 (... T12)
==28181==The signal is caused by a READ memory access.
error: DevServer crashed while waiting for hot reload

The harness stops the process before the stack is printed; the thread id suggests a bundler thread. The other seven "use client" tests in the file pass here, and the SSR receive guard does fix the first-load crash for that import shape (the initial request succeeds, it is the update that crashes).

For what it is worth, with #37862 cherry-picked on top, "removing "use client" from a file that never bundled" style demotions of a failing file publish { removed: [owner], added: [] } and later Build Failed pages no longer list the file, so the two changes compose.

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