crash_handler: wait for a full non-blocking stderr instead of dropping the report

src/crash_handler/lib.rs

@@ -399,53 +399,61 @@ pub use bun_io::{FmtAdapter, Write};
 /// `bun_sys::stderr_writer()` (not yet exposed by T1).
 /// Only impls `bun_io::Write` — `write!` resolves to `bun_io::Write::write_fmt`
 /// (alloc-free stack `Bridge`, async-signal-safe).
+///
+/// Never returns `Err`: callers `abort()` on error, which would also skip the
+/// report upload and the re-raise of the original signal, so a closed stderr
+/// or a vanished reader silently drops the bytes. A full stderr is waited for.
+///
+/// Everything below is async-signal-safe: `bun_sys::write` is a bare
+/// `write(2)` (a bare kernel32 `WriteFile` on Windows, which matters there
+/// too: the CRT's `_write` takes a per-fd lock that the VEH handler would
+/// self-deadlock on when the faulting thread was inside CRT stdio),
+/// `bun_sys::posix::poll` is a bare `poll(2)`, and `bun_sys::Error` does not
+/// allocate.
 pub(crate) struct StderrWriter;
 pub(crate) fn stderr_writer() -> StderrWriter {
     StderrWriter
 }
 impl Write for StderrWriter {
-    fn write_all(&mut self, bytes: &[u8]) -> bun_io::Result<()> {
-        #[cfg(windows)]
-        {
-            // On Windows this is `GetStdHandle(STD_ERROR_HANDLE)` + kernel32
-            // `WriteFile`, NOT the CRT. Routing through MSVCRT `_write(2,…)`
-            // would (1) text-mode-translate `\n`→`\r\n` and (2) take the CRT
-            // per-fd lock, which can self-deadlock when the VEH crash handler
-            // fires on a thread that faulted *inside* CRT stdio. WriteFile is
-            // lock-free at the kernel32 layer.
-            // `WriteFile` is declared locally because `bun_windows_sys::
-            // kernel32` does not (yet) export it (cf. src/sys/lib.rs).
-            #[link(name = "kernel32")]
-            unsafe extern "system" {
-                fn WriteFile(
-                    hFile: bun_sys::windows::HANDLE,
-                    lpBuffer: *const u8,
-                    nNumberOfBytesToWrite: u32,
-                    lpNumberOfBytesWritten: *mut u32,
-                    lpOverlapped: *mut core::ffi::c_void,
-                ) -> i32;
-            }
-            let h = bun_sys::windows::kernel32::GetStdHandle(bun_sys::windows::STD_ERROR_HANDLE);
-            let mut written: u32 = 0;
-            // SAFETY: `h` is the cached stderr HANDLE (or INVALID_HANDLE_VALUE,
-            // in which case WriteFile fails harmlessly); `bytes` is valid for
-            // reads of `len`; `written` is a valid out-pointer; lpOverlapped
-            // is null for synchronous I/O.
-            unsafe {
-                WriteFile(
-                    h,
-                    bytes.as_ptr(),
-                    bytes.len() as u32,
-                    &mut written,
-                    core::ptr::null_mut(),
-                );
-            }
-        }
+    fn write_all(&mut self, mut bytes: &[u8]) -> bun_io::Result<()> {
         #[cfg(not(windows))]
-        {
-            // SAFETY: fd 2 is always open; libc::write is async-signal-safe.
-            unsafe {
-                libc::write(2, bytes.as_ptr().cast(), bytes.len() as _);
+        let stderr = bun_sys::Fd::stderr();
+        // Looked up live rather than through `Fd::stderr()`: the cache behind
+        // that is filled by `Output`'s stdio init, which runs after the crash
+        // handler is installed, and crashes in between still have to print.
+        #[cfg(windows)]
+        let Some(stderr) = bun_sys::windows::GetStdHandle(bun_sys::windows::STD_ERROR_HANDLE)
+            .map(bun_sys::Fd::from_system)
+        else {
+            return Ok(());
+        };
+        while !bytes.is_empty() {
+            match bun_sys::write(stderr, bytes) {
+                Ok(0) => break,
+                Ok(n) => bytes = &bytes[n..],
+                Err(err) => match err.get_errno() {
+                    // `bun_sys::write` retries EINTR itself on Linux, but not on
+                    // macOS (`write$NOCANCEL` is issued once).
+                    #[cfg(unix)]
+                    bun_sys::E::EINTR => {}
+                    // fd 2 is O_NONBLOCK once `process.stderr` has touched a
+                    // pipe (the flag is on the open file description, so it is
+                    // inherited too). A full pipe only means the reader is
+                    // behind: wait for it, as a blocking stderr would, instead
+                    // of dropping the rest of the report.
+                    #[cfg(unix)]
+                    bun_sys::E::EAGAIN => {
+                        let mut pfd = [bun_sys::posix::PollFd {
+                            fd: stderr.native(),
+                            events: bun_sys::posix::POLL_OUT,
+                            revents: 0,
+                        }];
+                        if bun_sys::posix::poll(&mut pfd, -1).is_err() {
+                            break;
+                        }
+                    }
+                    _ => break,
+                },
             }
         }
         Ok(())

test/cli/run/run-crash-handler.test.ts

@@ -1,7 +1,8 @@
 import { crash_handler } from "bun:internal-for-testing";
 import { describe, expect, test } from "bun:test";
 import { bunEnv, bunExe, isASAN, isDebug, isLinux, isPosix, isWindows, mergeWindowEnvs, tempDir } from "harness";
-import { rmSync } from "node:fs";
+import { mkfifo } from "mkfifo";
+import { closeSync, constants, openSync, rmSync, writeSync } from "node:fs";
 import path from "path";
 const { getMachOImageZeroOffset } = crash_handler;
 
@@ -129,6 +130,88 @@ describe.if(isPosix)("terminal signal reflects the crash cause", () => {
   });
 });
 
+// The report is written to fd 2 with many small raw write(2) calls. fd 2 is
+// often O_NONBLOCK: Bun puts the flag on a piped stderr as soon as
+// process.stderr is used, and since it lives on the open file description a
+// parent can hand down an fd that already has it. If the reader is behind at
+// that moment every write fails with EAGAIN, and the handler used to drop the
+// bytes and carry on, so the process died without printing anything. It has
+// to wait for the reader instead, like a blocking stderr would.
+describe.if(isPosix)("crash report reaches a full non-blocking stderr", () => {
+  const reportHeader = Buffer.alloc(60, "=").toString() + "\n";
+
+  test.concurrent.each([
+    ["panic", "invoked crashByPanic() handler", "SIGABRT"],
+    ["segfault", "Segmentation fault at address 0xDEADBEEF", "SIGSEGV"],
+  ] as const)("%s", async (approach, expectedReason, expectedSignal) => {
+    using dir = tempDir("crash-stderr-full", {});
+    // A fifo is a pipe whose read end this process holds without Bun.spawn
+    // draining it: the child's stderr has to stay full until the crash handler
+    // has tried to write to it.
+    const fifo = path.join(String(dir), "stderr");
+    mkfifo(fifo);
+    const readEnd = openSync(fifo, constants.O_RDONLY | constants.O_NONBLOCK);
+    let writeEnd: number | undefined = openSync(fifo, constants.O_WRONLY | constants.O_NONBLOCK);
+    try {
+      const fill = Buffer.alloc(64 * 1024, "f");
+      let filled = 0;
+      let fillStoppedBy: string | undefined;
+      while (fillStoppedBy === undefined) {
+        try {
+          filled += writeSync(writeEnd, fill);
+        } catch (e) {
+          fillStoppedBy = (e as NodeJS.ErrnoException).code;
+        }
+      }
+      expect({ fillStoppedBy, filled: filled > 0 }).toEqual({ fillStoppedBy: "EAGAIN", filled: true });
+
+      await using proc = Bun.spawn({
+        cmd: [
+          bunExe(),
+          "--debug-crash-handler-use-trace-string",
+          "-e",
+          // Both requires come first: loading bun:internal-for-testing takes
+          // over a second in debug builds, and the marker has to be as close
+          // as possible to the crash itself.
+          `const { crash_handler } = require("bun:internal-for-testing");
+           const { writeSync } = require("node:fs");
+           writeSync(1, "crashing\\n");
+           crash_handler.${approach}();`,
+        ],
+        env: noReportEnv,
+        stdio: ["ignore", "pipe", writeEnd],
+      });
+      // The child now holds the only write end, so EOF on the fifo means it died.
+      closeSync(writeEnd);
+      writeEnd = undefined;
+
+      const { value: marker } = await proc.stdout.getReader().read();
+      expect(new TextDecoder().decode(marker)).toBe("crashing\n");
+
+      // The child crashes right after the marker and has its whole report
+      // written (or, before the fix, dropped) within a millisecond or so.
+      // Nothing observable separates "still formatting" from "waiting in
+      // poll(2) for us", so give it a moment before draining the fifo: without
+      // the fix the child exits on its own, with it the race times out.
+      await Promise.race([proc.exited, Bun.sleep(1_000)]);
+
+      // Draining the fill unblocks the child; whatever follows it is the report.
+      const report = (await Bun.file(readEnd).text()).slice(filled);
+      await proc.exited;
+
+      expect(report).toStartWith(reportHeader);
+      expect(report).toContain(`panic(main thread): ${expectedReason}\n`);
+      expect(report).toContain("oh no: Bun has crashed. This indicates a bug in Bun, not your code.\n");
+      expect(report.trimEnd().split("\n").at(-1)).toMatch(/^ \/\d+\.\d+\.\d+\/\S+$/);
+      expect(report).toEndWith("\n\n");
+      expect(proc.signalCode).toBe(expectedSignal);
+    } finally {
+      if (writeEnd !== undefined) closeSync(writeEnd);
+      closeSync(readEnd);
+    }
+  });
+});
+
 // POSIX-only: Windows refuses to remove a directory that is any process's cwd.
 describe.if(isPosix)("cwd deleted before startup", () => {
   test.concurrent.each(["install", "test"])("bun %s prints the cwd-deleted hint", async cmd => {