From 03d95a83327ece9d701ba5d3f3a6914e4c864dc1 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:29:11 +0000 Subject: [PATCH 1/4] error printer: collect build and resolve messages in the caller's ExceptionList The error printer copied a BuildMessage or ResolveMessage into the VM-wide log when the caller passed an ExceptionList. The development error page of a throwing handler rendered that whole log and then reset it. The page of a response body that failed after its status was sent did neither. The page showed no error, and the message stayed in the log for the next error page, for the print at exit, and for the `error` event of a Worker. ExceptionList is now a struct with the exceptions and the build messages of one report. The printer pushes into it and DevErrorPage renders it. A development error page no longer reads or resets the VM log, so it lists the errors of its own request only. The printer also reads the file of the top stack frame to show its source line. When the resolver had to read the directory of that file again, it logged what it found there into the VM log. It now logs into a scratch log for the length of that read. --- src/jsc/VirtualMachine.rs | 27 +- src/jsc/ZigException.rs | 4 +- src/jsc/lib.rs | 10 +- src/runtime/server/DevErrorPage.rs | 18 +- src/runtime/server/RequestContext.rs | 21 +- .../http/serve-stream-body-error-fixture.ts | 42 ++- .../bun/http/serve-stream-body-error.test.ts | 82 +++++ test/js/bun/http/serve.test.ts | 283 ++++++++++++++++++ 8 files changed, 436 insertions(+), 51 deletions(-) diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index 7c91b388148d..1c13735d14d4 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -58,7 +58,7 @@ pub use bun_core::STRING_ALLOCATION_LIMIT; pub(crate) type OnUnhandledRejection = fn(&mut VirtualMachine, &JSGlobalObject, JSValue); pub(crate) type MacroMap = bun_collections::ArrayHashMap; -pub type ExceptionList = Vec; +pub use crate::exception_list::ExceptionList; // ────────────────────────────────────────────────────────────────────────── // VirtualMachine struct (file-level @This()) @@ -6478,12 +6478,8 @@ impl VirtualMachine { let _ = writer.write_all(b"\n"); } self.had_errors = self.had_errors || build_error.msg.kind == bun_ast::Kind::Err; - if exception_list.is_some() { - // `log_mut()` is the centralized set-once `Option` - // accessor — single audited deref lives there. - if let Some(log) = self.log_mut() { - let _ = log.add_msg(build_error.msg.clone()); - } + if let Some(list) = exception_list { + list.build.push(build_error.msg.clone()); } bun_core::Output::flush(); return true; @@ -6497,12 +6493,8 @@ impl VirtualMachine { let _ = writer.write_all(b"\n"); } self.had_errors = self.had_errors || resolve_error.msg.kind == bun_ast::Kind::Err; - if exception_list.is_some() { - // `log_mut()` is the centralized set-once `Option` - // accessor — single audited deref lives there. - if let Some(log) = self.log_mut() { - let _ = log.add_msg(resolve_error.msg.clone()); - } + if let Some(list) = exception_list { + list.build.push(resolve_error.msg.clone()); } bun_core::Output::flush(); return true; @@ -6925,6 +6917,15 @@ impl VirtualMachine { break 'code bun_core::Utf8Bytes::Owned(text.unwrap_or_default()); } let mut log = bun_ast::Log::default(); + // What the resolver logs when the fetch reads the file's directory again is not part of this error. + let mut resolver_log = bun_ast::Log::default(); + // SAFETY: the resolver is a field of this VM, and the guard is dropped before `resolver_log`. + let _restore_resolver_log = unsafe { + bun_resolver::Resolver::scoped_log( + &raw mut self.transpiler.resolver, + NonNull::from(&mut resolver_log), + ) + }; let Ok(original_source) = bun_removable_features::call_removable_or!( RuntimeTranspiler, ReturnDefault, diff --git a/src/jsc/ZigException.rs b/src/jsc/ZigException.rs index fdbb3921b694..2b74a1c060b3 100644 --- a/src/jsc/ZigException.rs +++ b/src/jsc/ZigException.rs @@ -60,7 +60,7 @@ impl ZigException<'_> { pub(crate) fn add_to_error_list( &mut self, - error_list: &mut Vec, + error_list: &mut exception_list::ExceptionList, root_path: &[u8], origin: Option<&ZigURL>, ) { @@ -71,7 +71,7 @@ impl ZigException<'_> { return; } - error_list.push(exception_list::JsException { + error_list.exceptions.push(exception_list::JsException { name: Box::from(name.slice()), message: Box::from(message.slice()), runtime_type: self.runtime_type, diff --git a/src/jsc/lib.rs b/src/jsc/lib.rs index 01545c17228c..b4ce98f607a8 100644 --- a/src/jsc/lib.rs +++ b/src/jsc/lib.rs @@ -87,7 +87,7 @@ pub mod zig_stack_frame_code; pub mod zig_stack_frame_position; /// Owned snapshots of a [`ZigException`] (see `ZigException::add_to_error_list`), -/// collected into an [`ExceptionList`](virtual_machine::ExceptionList) so callers +/// collected into an [`ExceptionList`](exception_list::ExceptionList) so callers /// such as the `Bun.serve` development error page can report errors after the /// JSC exception itself is gone. pub mod exception_list { @@ -120,6 +120,14 @@ pub mod exception_list { pub code: JSErrorCode, pub stack: StackTrace, } + + /// What one error report collected for the caller that asked for it. + #[derive(Default)] + pub struct ExceptionList { + pub exceptions: Vec, + /// The message of each `BuildMessage` / `ResolveMessage` in the report. + pub build: Vec, + } } #[path = "array_buffer.rs"] pub mod array_buffer; diff --git a/src/runtime/server/DevErrorPage.rs b/src/runtime/server/DevErrorPage.rs index 1688c34e3597..7d79a61776b6 100644 --- a/src/runtime/server/DevErrorPage.rs +++ b/src/runtime/server/DevErrorPage.rs @@ -4,10 +4,10 @@ use std::io::Write as _; -use bun_ast::{Kind, Location, Log, Metadata, Msg}; +use bun_ast::{Kind, Location, Metadata, Msg}; use bun_core::fmt::{JSONFormatterUTF8Options, format_json_string_utf8, substitute_named}; use bun_core::strings; -use bun_jsc::exception_list::{JsException, StackTrace}; +use bun_jsc::exception_list::{ExceptionList, StackTrace}; const HTML_TEMPLATE: &[u8] = include_bytes!("dev-error-page.html"); @@ -15,9 +15,8 @@ pub(crate) struct DevErrorPage<'a> { /// One-line summary, e.g. `GET /foo failed`. pub message: &'a [u8], pub cwd: &'a [u8], - pub exceptions: &'a [JsException], - /// Build/resolve errors logged while handling the request. - pub log: Option<&'a Log>, + /// The thrown errors and the build/resolve errors of the failed request. + pub problems: &'a ExceptionList, } impl DevErrorPage<'_> { @@ -48,7 +47,7 @@ impl DevErrorPage<'_> { write_string(w, self.cwd); w.extend_from_slice(b",\"problems\":{\"exceptions\":["); - for (i, exception) in self.exceptions.iter().enumerate() { + for (i, exception) in self.problems.exceptions.iter().enumerate() { if i > 0 { w.push(b','); } @@ -69,12 +68,7 @@ impl DevErrorPage<'_> { w.push(b'}'); } - // `Log.errors` / `Log.warnings` don't count messages added via - // `add_msg`, which is how build/resolve errors reach this log. - let msgs: &[Msg] = match self.log { - Some(log) => &log.msgs, - None => &[], - }; + let msgs = &self.problems.build; let (mut errors, mut warnings) = (0u32, 0u32); for msg in msgs { errors += (msg.kind == Kind::Err) as u32; diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index d19fe4d6bcea..48bda6f43ee6 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -1200,12 +1200,7 @@ where } } - pub(crate) fn render_default_error( - &self, - log: &bun_ast::Log, - exceptions: &[jsc::exception_list::JsException], - message: &[u8], - ) { + pub(crate) fn render_default_error(&self, problems: &jsc::ExceptionList, message: &[u8]) { if !self.flags.has_written_status() { self.flags.set_has_written_status(true); if let Some(resp) = self.resp.get() { @@ -1224,8 +1219,7 @@ where let bb = DevErrorPage { message, cwd: bun_resolver::fs::FileSystem::get().top_level_dir(), - exceptions, - log: Some(log), + problems, } .render(); let try_end_ok = match self.resp.get() { @@ -3040,7 +3034,7 @@ where return false; } - let mut exception_list: jsc::ExceptionList = Vec::new(); + let mut exception_list = jsc::ExceptionList::default(); server .vm() .as_mut() @@ -3049,8 +3043,7 @@ where let bb = DevErrorPage { message: b"Stream error during server-side rendering", cwd: bun_resolver::fs::FileSystem::get().top_level_dir(), - exceptions: &exception_list, - log: None, + problems: &exception_list, } .render(); @@ -3677,21 +3670,19 @@ where // the single audited `&mut VirtualMachine` accessor. let vm = server.vm().as_mut(); if DEBUG_MODE { - let mut exception_list: jsc::ExceptionList = Vec::new(); + let mut exception_list = jsc::ExceptionList::default(); let prev_exception_list = vm.on_unhandled_rejection_exception_list; vm.on_unhandled_rejection_exception_list = Some(NonNull::from(&mut exception_list)); report_unhandled_error(vm, global_this, value); vm.on_unhandled_rejection_exception_list = prev_exception_list; - let log = vm.log_mut().unwrap(); bun_core::pretty_errorln!( "{:?} - {} failed", self.method, self.ensure_pathname() ); let msg = format!("{:?} - {} failed", self.method, self.ensure_pathname()); - self.render_default_error(log, &exception_list, msg.as_bytes()); - log.reset(); + self.render_default_error(&exception_list, msg.as_bytes()); return; } if status != 404 { diff --git a/test/js/bun/http/serve-stream-body-error-fixture.ts b/test/js/bun/http/serve-stream-body-error-fixture.ts index 95d39e1cfcd2..45bd728ab52a 100644 --- a/test/js/bun/http/serve-stream-body-error-fixture.ts +++ b/test/js/bun/http/serve-stream-body-error-fixture.ts @@ -49,6 +49,34 @@ let statusLineReceivedResolve: (() => void) | undefined; // the test observes any resulting rejection before declaring success. const cancelRan = Promise.withResolvers(); +// Failures that are a BuildMessage or a ResolveMessage. Neither is an Error +// with a stack: the dev error page lists them as build errors. +const throwBuildMessage = () => void new Bun.Transpiler().transformSync("export const oops = ;"); +const throwResolveMessage = () => void Bun.resolveSync("./does-not-exist.ts", import.meta.dir); +function throwBothMessages() { + const errors: unknown[] = []; + for (const fail of [throwBuildMessage, throwResolveMessage]) { + try { + fail(); + } catch (error) { + errors.push(error); + } + } + throw new AggregateError(errors, "two messages"); +} + +// Errors only after a chunk has already been flushed to the client. +function midStream(fail: () => void): ReadableStream { + return new ReadableStream({ + async pull(c) { + const reached = chunkReachedClient(); + c.enqueue("chunk-a"); + await reached; + fail(); + }, + }); +} + const sources: Record ReadableStream> = { // Already errored by the time Bun.serve starts rendering the body. "pull-throw": () => @@ -101,16 +129,13 @@ const sources: Record ReadableStream> = { c.error(new Error("boom")); }, }), - // Errors only after a chunk has already been flushed to the client. "mid-stream-reject": () => - new ReadableStream({ - async pull(c) { - const reached = chunkReachedClient(); - c.enqueue("chunk-a"); - await reached; - throw new Error("boom"); - }, + midStream(() => { + throw new Error("boom"); }), + "mid-stream-build-message": () => midStream(throwBuildMessage), + "mid-stream-resolve-message": () => midStream(throwResolveMessage), + "mid-stream-both-messages": () => midStream(throwBothMessages), // The client aborts the download mid-stream, which makes Bun cancel the body // stream; the source's cancel() then throws. That rejection belongs to a // promise Bun created internally and must not surface as unhandledRejection. @@ -236,6 +261,7 @@ const nativeBodies: Record Response | Promise> = { await Bun.sleep(0); throw new Error("boom"); }), + "rewriter-mid-stream-build-message": () => rewriterResponse(throwBuildMessage), // `return fetch(...)` whose upstream dies once its first chunk has made it // through to our client. "proxied-fetch-mid-stream": async () => { diff --git a/test/js/bun/http/serve-stream-body-error.test.ts b/test/js/bun/http/serve-stream-body-error.test.ts index 1fd9fbe9aaa8..bf6a2cd36675 100644 --- a/test/js/bun/http/serve-stream-body-error.test.ts +++ b/test/js/bun/http/serve-stream-body-error.test.ts @@ -162,6 +162,88 @@ test.concurrent.each([ expect(stderr).toContain("boom"); }); +// The same page for a body that fails with a build or resolve error. Such an +// error has no stack: the page lists it under `problems.build`, where a +// throwing handler's page lists it. The report used to leave this page empty +// and to leave the message in the VM-wide log instead, which printed it a +// second time when the process exited. +const buildMessage = { level: 1, text: "Unexpected ;", on: { build: true, resolve: "" } }; +const resolveMessage = { + level: 1, + text: expect.stringMatching(/^Cannot find module '\.\/does-not-exist\.ts'/), + on: { build: false, resolve: "./does-not-exist.ts" }, +}; +test.concurrent.each([ + ["mid-stream-build-message", "7\r\nchunk-a\r\n", [buildMessage]], + ["mid-stream-resolve-message", "7\r\nchunk-a\r\n", [resolveMessage]], + ["mid-stream-both-messages", "7\r\nchunk-a\r\n", [buildMessage, resolveMessage]], + ["rewriter-mid-stream-build-message", "e\r\nchunk-a\r\n", [buildMessage]], +])("%s under a dev server: the error page lists the message", async (variant, firstChunk, messages) => { + const { stdout, stderr, exitCode } = await runFixture(variant, "development", "dev-server"); + const { body, ...result } = JSON.parse(stdout); + expect({ result, exitCode }).toEqual({ + result: { + statusLine: "HTTP/1.1 200 OK", + cleanChunkedTerminator: true, + errorCb: 0, + unhandled: 0, + secondStatusLine: "HTTP/1.1 200 OK", + }, + exitCode: 0, + }); + expect(body).toStartWith(firstChunk); + const page = JSON.parse(/