Skip to content
15 changes: 15 additions & 0 deletions src/bundler/bundle_v2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5883,6 +5883,21 @@ pub mod bv2_impl {
bun_core::scoped_log!(Bundle, "failed with error: {}", err.name());
resolve_result.resolve_queue.clear();

// Retargeted to the browser by `ParseTask`: the failure is on the client graph.
if let Some(dev) = this.dev_server {
if result.use_directive == crate::UseDirective::Client
&& target == Target::Browser
&& this
.framework
.as_ref()
.and_then(|framework| framework.server_components.as_ref())
.is_some_and(|sc| sc.separate_ssr_graph)
{
dev.handle_client_component_boundary_failure(result.source.path.text)
.expect("oom");
}
}

// Preserve the parsed import_records on the graph so any plugin
// onResolve tasks already dispatched for *other* records in this
// same file can still dereference
Expand Down
1 change: 1 addition & 0 deletions src/bundler/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ bun_dispatch::link_interface! {
fn log_for_resolution_failures(abs_path: &[u8], graph: bake_types::Graph) -> *mut bun_ast::Log;
fn finalize_bundle(bv2: *mut bundle_v2::BundleV2<'_>, result: *mut bundle_v2::DevServerOutput<'_>) -> Result<(), crate::Error>;
fn handle_parse_task_failure(err: crate::Error, graph: bake_types::Graph, abs_path: &[u8], log: *const bun_ast::Log, bv2: *mut bundle_v2::BundleV2<'_>) -> Result<(), crate::Error>;
fn handle_client_component_boundary_failure(abs_path: &[u8]) -> Result<(), crate::Error>;
fn put_or_overwrite_asset(path: *const (), contents: &[u8], content_hash: u64) -> Result<(), crate::Error>;
fn track_resolution_failure(import_source: &[u8], specifier: &[u8], renderer: bake_types::Graph, loader: bun_ast::Loader) -> Result<(), crate::Error>;
fn is_file_cached(abs_path: &[u8], side: bake_types::Graph) -> Option<bake_types::CacheEntry>;
Expand Down
36 changes: 33 additions & 3 deletions src/runtime/bake/DevServer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2251,6 +2251,8 @@ fn check_route_failures(
resp: DevResponse,
) -> crate::Result<CheckResult> {
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();
Comment on lines +2254 to +2255

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

// 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);
Expand Down Expand Up @@ -4388,13 +4390,17 @@ pub(super) fn finalize_bundle(
dev.incremental_result.html_routes_soft_affected.clear();
ctx.gts.clear();

for index in &dev.incremental_result.client_components_affected {
// `trace_dependencies` appends to this list while it is being walked.
let mut i = 0;
while i < dev.incremental_result.client_components_affected.len() {
let index = dev.incremental_result.client_components_affected[i];
dev.server_graph.trace_dependencies(
*index,
index,
ctx.gts,
incremental_graph::TraceDependencyGoal::NoStop,
*index,
index,
)?;
i += 1;
}

for request in &dev.incremental_result.framework_routes_affected {
Expand Down Expand Up @@ -4984,6 +4990,30 @@ impl DevServer {
Ok(())
}

/// A "use client" file whose imports failed to resolve. The failure is on the client
/// node; this adds what `finalize_bundle` adds for a boundary that bundled, which is
/// what connects the file's server-side importers to that failure (and back).
Comment thread
robobun marked this conversation as resolved.
Outdated
pub(crate) fn handle_client_component_boundary_failure(
&mut self,
abs_path: &[u8],
) -> Result<(), AllocError> {
let _g = self.graph_safety_lock.guard();

let client_index = self.client_graph.insert_stale(abs_path, false)?;
self.client_graph.bundled_files.values_mut()[client_index.get() as usize].is_hmr_root =
true;

// `insert_stale` cannot set the bit for an index past the bitset's length.
let server_index = self.server_graph.insert_stale(abs_path, false)?;
self.server_graph.ensure_stale_bit_capacity(true)?;
self.server_graph
.stale_files
.set(server_index.get() as usize);
self.server_graph.bundled_files.values_mut()[server_index.get() as usize]
.is_client_component_boundary = true;
Ok(())
}

/// Return a log to write resolution failures into.
pub(crate) fn get_log_for_resolution_failures(
&mut self,
Expand Down
5 changes: 5 additions & 0 deletions src/runtime/bake/dev_server/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1171,6 +1171,11 @@ bun_bundler::link_impl_DevServerHandle! {
.handle_parse_task_failure(&err.into(), graph, abs_path, &*log, &mut *bv2)
.map_err(Into::into)
},
handle_client_component_boundary_failure(abs_path) => {
(*this)
.handle_client_component_boundary_failure(abs_path)
.map_err(Into::into)
},
put_or_overwrite_asset(path, contents, content_hash) => {
// `path` was erased from `&bun_resolver::fs::Path<'_>` at the
// `DevServerHandle::put_or_overwrite_asset_erased` call site. Re-wrap
Expand Down
184 changes: 184 additions & 0 deletions test/bake/dev/bundle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -865,3 +865,187 @@ devTest("barrel optimization: namespace re-export cycle through a star-exported
await c.expectMessage("result: object Y KEEP DEEP OTHER");
},
});

const separateSSRGraphFramework = {
...minimalFramework,
serverComponents: {
...minimalFramework.serverComponents!,
separateSSRGraph: true,
},
};

/** The response for a framework route whose bundle has errors. */
async function expectBuildFailed(response: Promise<Response>, error: string) {
const res = await response;
const html = await res.text();
expect(html.match(/<title>(.*)<\/title>/)?.[1]).toBe("Bun - Build Failed");
// The page embeds the serialized failures as base64; the messages are
// stored as plain text inside of it.
const encoded = html.match(/atob\("([^"]*)"\)/)?.[1];
expect(encoded).toBeString();
expect(atob(encoded!)).toContain(error);
expect(res.status).toBe(500);
}
Comment thread
robobun marked this conversation as resolved.

// A "use client" file with a separate SSR graph is bundled for the browser
// even when a server file imports it, so its errors are owned by the client
// graph. The route importing it still has to be associated with them: both
// right away and after unrelated hot updates, and regardless of whether the
// route is re-bundled on request (BUN_ASSUME_PERFECT_INCREMENTAL=0) or the
// graph is trusted (=1). Before the fix, the first hot update made the route
// run against a server module that was never emitted ("Failed to load bundled
// module 'components/Sibling.ts'").
for (const mode of ["0", "1"]) {
devTest(`route importing a failing "use client" file (BUN_ASSUME_PERFECT_INCREMENTAL=${mode})`, {
framework: separateSSRGraphFramework,
env: { BUN_ASSUME_PERFECT_INCREMENTAL: mode },
files: {
"routes/index.ts": `
import { good } from '../good';
import '../components/Sibling';
export default function (req, meta) {
return new Response('page: ' + good);
}
`,
"good.ts": `export const good = "v1";`,
"components/Sibling.ts": `
"use client";
import './sibling-missing';
export const sibling = 1;
`,
},
async test(dev) {
const error = `Could not resolve: "./sibling-missing"`;
await expectBuildFailed(dev.fetch("/"), error);
await expectBuildFailed(dev.fetch("/"), error);

await dev.write("good.ts", `export const good = "v2";`, { errors: null });
await expectBuildFailed(dev.fetch("/"), error);

// Creating the missing file re-bundles Sibling.ts through the
// directory watcher, this time as a working client component.
await dev.write("components/sibling-missing.ts", `export {};`, { errors: null });
await dev.fetch("/").equals("page: v2");

// The same thing for a component that has already been bundled once.
const otherError = `Could not resolve: "./other-missing"`;
await dev.write(
"components/Sibling.ts",
`
"use client";
import './other-missing';
export const sibling = 2;
`,
{ errors: null },
);
await expectBuildFailed(dev.fetch("/"), otherError);
await dev.write("good.ts", `export const good = "v3";`, { errors: null });
await expectBuildFailed(dev.fetch("/"), otherError);
await dev.write("components/other-missing.ts", `export {};`, { errors: null });
await dev.fetch("/").equals("page: v3");
},
});
}
// Here the failing file is only reachable through another client component.
// Both boundaries end up in `client_components_affected`, and tracing them in
// `finalize_bundle` appends to that list while it is being walked; walking it
// through a slice read the list's old buffer after it grew (fails under ASAN).
devTest('"use client" file that fails to bundle, imported from another "use client" file', {
framework: separateSSRGraphFramework,
files: {
"routes/index.ts": `
import '../components/Comp';
export default function (req, meta) {
return new Response('page');
}
`,
"components/Comp.ts": `
"use client";
import './Sibling';
export const comp = 1;
`,
"components/Sibling.ts": `
"use client";
import './sibling-missing';
export const sibling = 1;
`,
},
async test(dev) {
const error = `Could not resolve: "./sibling-missing"`;
await expectBuildFailed(dev.fetch("/"), error);
await dev.patch("routes/index.ts", { find: "'page'", replace: "'page2'", errors: null });
await expectBuildFailed(dev.fetch("/"), error);
await dev.write("components/sibling-missing.ts", `export {};`, { errors: null });
await dev.fetch("/").equals("page2");
},
});
// The failed file is re-bundled from the server side like any other client
// component. Dropping the directive while fixing it turns it into a plain
// server module, which deletes the client side of the boundary that never
// bundled. (Before the fix the file was re-bundled as a client module and the
// route could not load it.)
devTest('removing "use client" from a file that never bundled', {
framework: separateSSRGraphFramework,
files: {
"routes/index.ts": `
import { sibling } from '../components/Sibling';
export default function (req, meta) {
return new Response('page: ' + sibling);
}
`,
"components/Sibling.ts": `
"use client";
import './sibling-missing';
export const sibling = 1;
`,
},
async test(dev) {
await expectBuildFailed(dev.fetch("/"), `Could not resolve: "./sibling-missing"`);
await dev.write("components/Sibling.ts", `export const sibling = "server";`, { errors: null });
await dev.fetch("/").equals("page: server");
},
});
// `checkRouteFailures` collects the errors reachable from a route into the
// list that also holds the previous bundle's new errors. Without clearing it
// first, a route that was marked as possibly failing by an earlier bundle
// reports whatever the most recent bundle failed on, even when it does not
// import any of it. (With BUN_ASSUME_PERFECT_INCREMENTAL=0 the stale entries
// only cause a needless rebuild of the route.)
devTest("route marked by an earlier failure does not report another route's errors", {
framework: minimalFramework,
env: { BUN_ASSUME_PERFECT_INCREMENTAL: "1" },
files: {
"routes/a.ts": `
import { shared } from '../shared';
import { a } from '../a';
export default function (req, meta) {
return new Response('a: ' + shared + a);
}
`,
"routes/b.ts": `
import { shared } from '../shared';
export default function (req, meta) {
return new Response('b: ' + shared);
}
`,
"shared.ts": `export const shared = "s";`,
"a.ts": `export const a = "a";`,
},
async test(dev) {
await dev.fetch("/a").equals("a: sa");
await dev.fetch("/b").equals("b: s");

// Both routes import shared.ts, so both get marked as possibly failing.
await dev.write("shared.ts", `import './missing'; export const shared = "s";`, { errors: null });
await expectBuildFailed(dev.fetch("/a"), `shared.ts`);
await expectBuildFailed(dev.fetch("/b"), `shared.ts`);

// Fixing it does not revisit the routes; they stay marked until requested.
await dev.write("shared.ts", `export const shared = "s";`, { errors: null });
// Only /a imports a.ts.
await dev.write("a.ts", `import './missing'; export const a = "a";`, { errors: null });

await dev.fetch("/b").equals("b: s");
await expectBuildFailed(dev.fetch("/a"), `a.ts`);
},
});
Loading