From 03badf1c5d815b95bcfb0ba1e8b1758fb9de180a Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 2 Oct 2026 05:16:34 +0000 Subject: [PATCH] bun test --parallel: fail the run when a worker is killed or exits non-zero outside a test file reap_worker read a worker's exit status only while a file was in flight or before the worker was ready. A worker that was between files, or shutting down after its last one, was reaped without a look, so the run exited 0. The coordinator now prints one error line for such a worker, counts it as an error in the summary and exits 1. It does not abort the run. A status that the wait could not report (ECHILD) is not counted. A crash during worker startup is counted the same way, so its banner can no longer end in exit 0 when no file is queued or in flight. --- docs/test/parallel.mdx | 4 +- docs/test/runtime-behavior.mdx | 2 + src/runtime/cli/test/parallel/Coordinator.rs | 72 ++++++- src/runtime/cli/test/parallel/Worker.rs | 2 +- .../cli/test/parallel-startup-failure.test.ts | 176 +++++++++++++++++- 5 files changed, 252 insertions(+), 4 deletions(-) diff --git a/docs/test/parallel.mdx b/docs/test/parallel.mdx index ee82ae508ad0..c9cab5d04125 100644 --- a/docs/test/parallel.mdx +++ b/docs/test/parallel.mdx @@ -59,7 +59,9 @@ Flags that affect how tests execute (`--timeout`, `--preload`, `--define`, `--co The coordinator merges coverage, JUnit XML and snapshot writes, so `--parallel --coverage --reporter=junit --reporter-outfile=junit.xml` produces one report. -If a worker crashes (a test calls `process.exit`), the coordinator reports the file that worker was running as failed, and a replacement worker picks up the remaining files. A crash from a fatal signal aborts the whole run, so later passing files can't mask it. With a single effective worker (`--parallel=1`, or a suite with one test file), `bun test` runs the files in the main process, so `process.exit` ends the run with that exit code. +If a worker crashes (a test calls `process.exit`), the coordinator reports the file that worker was running as failed, and a replacement worker picks up the remaining files. A crash from a fatal signal while a file is running, or while a worker starts, aborts the whole run, so later passing files can't mask it. With a single effective worker (`--parallel=1`, or a suite with one test file), `bun test` runs the files in the main process, so `process.exit` ends the run with that exit code. + +A worker can also be killed or exit with a non-zero code outside a test file: between two files, or while it shuts down after its last one. The coordinator then prints an error line, counts one error in the summary and exits with code `1`. It does not abort the run, so the other workers finish their files. Coverage, snapshot writes and the failure recap from that worker can be incomplete. ### When `--parallel` helps, and when it doesn't diff --git a/docs/test/runtime-behavior.mdx b/docs/test/runtime-behavior.mdx index 83eefb7d44b2..bce6a40a35f7 100644 --- a/docs/test/runtime-behavior.mdx +++ b/docs/test/runtime-behavior.mdx @@ -89,6 +89,8 @@ test("test without timeout", async () => { `bun test` tracks unhandled promise rejections and errors that occur between tests. If any occur, `bun test` exits with a non-zero code even when no test failed. In both examples below the error happens while the file is being loaded, so the file's tests are not run at all. +With [`--parallel`](/test/parallel), the error count in the summary also includes each worker process that was killed or exited with a non-zero code outside a test file. + This helps catch errors in asynchronous code that might otherwise go unnoticed: ```ts title="test.ts" icon="/icons/typescript.svg" diff --git a/src/runtime/cli/test/parallel/Coordinator.rs b/src/runtime/cli/test/parallel/Coordinator.rs index 25e1a381ad91..5ea633faab71 100644 --- a/src/runtime/cli/test/parallel/Coordinator.rs +++ b/src/runtime/cli/test/parallel/Coordinator.rs @@ -651,6 +651,13 @@ impl<'a> Coordinator<'a> { if is_panic_status(status) { self.abort_on_worker_startup_panic(status); } + } else { + // Sent `.ready` and has no file in flight: between files, or + // shutting down after its last one. + let corrupt_frame = w.ipc.corrupt_frame.get(); + if corrupt_frame || is_abnormal_exit(status) { + self.account_exit_outside_file(worker_idx, corrupt_frame, status); + } } // SAFETY: fresh derivation — `abort_on_worker_panic` above retags the slots. @@ -739,6 +746,51 @@ impl<'a> Coordinator<'a> { self.files_done += 1; } + /// Fail the run for a worker exit that no file takes the blame for. The + /// summary counts it as an error and the run exits 1. + fn record_worker_failure(&mut self) { + self.reporter.jest.unhandled_errors_between_tests += 1; + } + + /// A worker with no file in flight was killed, exited non-zero, or + /// corrupted its IPC stream. Its test results are in, but what it sends + /// while it shuts down (coverage, the failure recap) and its last snapshot + /// writes can be missing. A crash signal does not abort the run here: + /// after a worker's last file nothing is queued, so an abort would only + /// kill the siblings that are still finishing theirs. + #[cold] + fn account_exit_outside_file( + &mut self, + worker_idx: u32, + corrupt_frame: bool, + status: &SpawnStatus, + ) { + let panicked = is_panic_status(status); + // Terminated by `terminate_workers_after_panic`; that panic is already reported. + if self.stop_reason == Some(StopReason::WorkerPanicked) && !panicked { + return; + } + self.break_dots(); + self.end_group(); + if corrupt_frame && !panicked { + // The kill was ours (`Worker::on_channel_done`), or was skipped + // because the worker had already exited: name the cause. + bun_core::pretty_error!( + "error: test worker {} killed outside a test file (corrupt IPC frame, something wrote to fd 3)\n", + worker_idx + 1, + ); + } else { + let mut buf = [0u8; 32]; + bun_core::pretty_error!( + "error: test worker {} exited outside a test file ({})\n", + worker_idx + 1, + bstr::BStr::new(describe_status(&mut buf, status)), + ); + } + Output::flush(); + self.record_worker_failure(); + } + fn mark_crashed(&mut self, file_idx: u32, elapsed_ms: i64) { self.crashed_files.push(file_idx); if let Some(file) = self.test_records.get_mut(file_idx as usize) { @@ -789,7 +841,8 @@ impl<'a> Coordinator<'a> { } /// `abort_on_worker_panic` for the pre-`.ready` case: no file was - /// dispatched yet, so there is none to name. + /// dispatched yet, so there is none to name. The crash is recorded by + /// itself: the sweep fails nothing when no file is queued or in flight. fn abort_on_worker_startup_panic(&mut self, status: &SpawnStatus) { self.break_dots(); let mut buf = [0u8; 32]; @@ -801,6 +854,7 @@ impl<'a> Coordinator<'a> { bstr::BStr::new(describe_status(&mut buf, status)), ); Output::flush(); + self.record_worker_failure(); self.terminate_workers_after_panic(b"aborted: worker panicked during startup"); } @@ -965,6 +1019,22 @@ fn is_panic_status(status: &SpawnStatus) -> bool { false } +/// The OS reported that the worker was killed or exited non-zero. `Err` is +/// not an abnormal exit but an unknown one: when SIGCHLD is inherited as +/// SIG_IGN the kernel reaps every child and each wait fails with ECHILD, so +/// counting it would fail every run there. +fn is_abnormal_exit(status: &SpawnStatus) -> bool { + match status { + SpawnStatus::Signaled(_) => true, + // `code` is the low byte of the exit code; 0x100 is not a clean exit. + #[cfg(windows)] + SpawnStatus::Exited(e) => e.raw != 0, + #[cfg(not(windows))] + SpawnStatus::Exited(e) => e.code != 0, + SpawnStatus::Err(_) | SpawnStatus::Running => false, + } +} + /// Fatal NTSTATUS exit codes — the Windows mirror of the signal list /// above. An allowlist, not `>= 0xC0000000`, because high exit codes are /// not all faults: 0xC000013A is Ctrl+C (the SIGINT analog), and foreign diff --git a/src/runtime/cli/test/parallel/Worker.rs b/src/runtime/cli/test/parallel/Worker.rs index fd4dd124303a..b12089779af1 100644 --- a/src/runtime/cli/test/parallel/Worker.rs +++ b/src/runtime/cli/test/parallel/Worker.rs @@ -70,7 +70,7 @@ pub(crate) struct Worker { pub(crate) reap_pending: bool, /// Set when this process sends `.ready`; reset before each respawn. /// `inflight == None` with `!reached_ready` at reap distinguishes a - /// startup failure from a clean post-shutdown exit. + /// startup failure from an exit between files or after shutdown. pub(crate) reached_ready: bool, /// Consecutive (re)spawns of this slot that exited before `.ready`; /// reset on `.ready`. Bounds the respawn loop in `reap_worker`. diff --git a/test/cli/test/parallel-startup-failure.test.ts b/test/cli/test/parallel-startup-failure.test.ts index 8d3ae7d7ba17..b3cf66cc14a1 100644 --- a/test/cli/test/parallel-startup-failure.test.ts +++ b/test/cli/test/parallel-startup-failure.test.ts @@ -3,9 +3,14 @@ // spawns a coordinator + multiple workers), and file-level pass/fail is // what the surrounding tooling checks. This test must be evaluated in // isolation from those unrelated timing-sensitive cases. +// +// It covers the worker exits that no test file takes the blame for: before +// the worker is ready, and between files or after its last one. import { expect, test } from "bun:test"; -import { bunEnv, bunExe, isASAN, isDebug, tempDir } from "harness"; +import { bunEnv, bunExe, isASAN, isDebug, isLinux, isWindows, tempDir } from "harness"; +import { mkfifo } from "mkfifo"; +import path from "path"; // The BUN_TEST_WORKER_EXIT_BEFORE_READY hook is compiled only into // debug/ASAN builds so a stray env var can't disable --parallel in release. @@ -87,7 +92,176 @@ test.skipIf(!hasHook)( expect(stderr).toContain("a.test.js"); expect(stderr).toContain("b.test.js"); expect(stderr).not.toContain("retrying"); + // The crash is counted by itself, so the run also fails when the sweep + // finds no queued file to fail. One error for each worker that crashed. + expect(stderr).toMatch(/^ [12] errors?$/m); expect(exitCode).not.toBe(0); }, 60_000, ); + +// A worker that ends with no file in flight (between files, or while it shuts +// down after its last one) has already reported its tests. How it ended still +// counts: a healthy worker exits 0, and one that was killed or exited non-zero +// can take its coverage, its snapshot writes and the failure recap with it. No +// file takes the blame, so the run fails. +// +// `--no-isolate` keeps a node:test file's process.on("exit") listeners until +// the worker exits, so `body` runs after the worker's last file was reported. +async function runWithExitListener(body: string, { imports = "", launcher = [] as string[] } = {}) { + using dir = tempDir("parallel-exit-outside-file", { + "a.test.js": `import { test } from "node:test"; ${imports} + process.on("exit", () => { ${body} }); + test("a", () => {});`, + "b.test.js": `import { test } from "bun:test"; test("b", () => {});`, + }); + await using proc = Bun.spawn({ + cmd: [...launcher, bunExe(), "test", "--parallel=2", "--no-isolate"], + env: { ...bunEnv, BUN_TEST_PARALLEL_SCALE_MS: "0", BUN_CRASH_REPORT_URL: "", BUN_ENABLE_CRASH_REPORTING: "0" }, + cwd: String(dir), + stderr: "pipe", + stdout: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toContain("PARALLEL"); + return { stderr, exitCode }; +} +// Each case runs a coordinator and two workers. +const workerExitTimeout = isASAN || isDebug ? 60_000 : 20_000; + +test.concurrent.each(["process.exit(7);", "process.exitCode = 7;"])( + "--parallel: a worker that exits non-zero after its last file fails the run (%s)", + async body => { + const { stderr, exitCode } = await runWithExitListener(body); + expect(stderr).toContain("error: test worker 1 exited outside a test file (exit code 7)\n"); + expect(stderr).toContain("\n 2 pass\n 0 fail\n 1 error\n"); + expect(exitCode).toBe(1); + }, + workerExitTimeout, +); + +// A crash signal is reported the same way and does not abort the run: once a +// worker is past its last file nothing is queued, so an abort would only take +// down the sibling that still runs b.test.js. +test.concurrent.skipIf(isWindows).each(["SIGKILL", "SIGTERM", "SIGABRT"])( + "--parallel: a worker killed by %s after its last file fails the run without aborting it", + async signal => { + const { stderr, exitCode } = await runWithExitListener(`process.kill(process.pid, "${signal}");`, { + // SIGABRT dumps core, and CI flags every new core file. + launcher: ["/bin/sh", "-c", `ulimit -c 0 && exec "$@"`, "--"], + }); + expect(stderr).toContain(`error: test worker 1 exited outside a test file (${signal})\n`); + expect(stderr).not.toContain("Aborting"); + expect(stderr).toContain("\n 2 pass\n 0 fail\n 1 error\n"); + expect(exitCode).toBe(1); + }, + workerExitTimeout, +); + +test.concurrent( + "--parallel --no-isolate: an exit listener that leaves the exit code alone keeps the run green", + async () => { + const { stderr, exitCode } = await runWithExitListener(""); + expect(stderr).not.toContain("worker"); + expect(stderr).toContain("\n 2 pass\n 0 fail\n"); + expect(exitCode).toBe(0); + }, + workerExitTimeout, +); + +// fd 3 is the IPC channel. The coordinator kills a worker whose stream stops +// decoding, and it names that cause whether the kill landed or the worker had +// already exited on its own. +test.concurrent( + "--parallel: garbage on fd 3 after a worker's last file fails the run and names the cause", + async () => { + const { stderr, exitCode } = await runWithExitListener(`writeSync(3, Buffer.alloc(32, 0xff));`, { + imports: `import { writeSync } from "node:fs";`, + }); + expect(stderr).toContain( + "error: test worker 1 killed outside a test file (corrupt IPC frame, something wrote to fd 3)\n", + ); + expect(stderr).not.toContain("worker crashed"); + expect(stderr).toContain("\n 2 pass\n 0 fail\n 1 error\n"); + expect(exitCode).toBe(1); + }, + workerExitTimeout, +); + +// The worker writes a.test.js's snapshot file when it shuts down, before it +// sends its coverage. The file is a FIFO here: the write blocks once the pipe +// is full, and the first bytes carry the worker's pid. A kill at that point is +// what an OOM kill during the coverage report looks like to the coordinator: +// every test passed, and the worker's coverage never arrives. With it go the +// table and the coverageThreshold check, so the exit status is the only sign. +test.concurrent.skipIf(isWindows)( + "--parallel --coverage: a worker killed while it shuts down fails the run", + async () => { + using dir = tempDir("parallel-killed-in-shutdown", { + "bunfig.toml": `[test]\ncoverageThreshold = 0.9\ncoverageSkipTestFiles = true\n`, + "lib.js": `export function used() { return 1; }\nexport function unused() { return 2; }\n`, + "a.test.js": `import { test, expect } from "bun:test"; import { used } from "./lib.js"; + test("a", () => { + expect(used()).toBe(1); + expect("pid=" + process.pid).toMatchSnapshot(); + expect(Buffer.alloc(1024 * 1024, "x").toString()).toMatchSnapshot(); + });`, + "b.test.js": `import { test } from "bun:test"; test("b", () => {});`, + "__snapshots__": {}, + }); + const fifo = path.join(String(dir), "__snapshots__", "a.test.js.snap"); + mkfifo(fifo); + await using reader = Bun.spawn({ + cmd: ["head", "-c", "512", fifo], + stdin: "ignore", + stdout: "pipe", + stderr: "ignore", + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", "--parallel=2", "--coverage"], + // CI=false: a worker only writes a new snapshot outside CI. + env: { ...bunEnv, BUN_TEST_PARALLEL_SCALE_MS: "0", CI: "false" }, + cwd: String(dir), + stderr: "pipe", + stdout: "pipe", + }); + const pid = Number((await reader.stdout.text()).match(/pid=(\d+)/)?.[1]); + expect(pid).toBeGreaterThan(0); + process.kill(pid, "SIGKILL"); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toContain("PARALLEL"); + expect(stderr).toContain("error: test worker 1 exited outside a test file (SIGKILL)\n"); + expect(stderr).toContain("\n 2 pass\n 0 fail\n 1 error\n"); + expect(exitCode).toBe(1); + }, + workerExitTimeout, +); + +// When bun inherits SIG_IGN for SIGCHLD the kernel reaps the workers itself +// and every wait fails with ECHILD. An exit status that is unknown is not a +// failure, or every run would be red there. bash passes the ignored signal on +// to the program it execs; dash does not. +test.concurrent.skipIf(!isLinux || Bun.which("bash") == null)( + "--parallel: a healthy run stays green when the worker exit statuses are unknown (SIGCHLD ignored)", + async () => { + using dir = makeDir("parallel-sigchld-ignored"); + await using proc = Bun.spawn({ + cmd: ["bash", "-c", `trap "" CHLD; exec "$0" "$@"`, bunExe(), "test", "--parallel=2"], + env: { + ...bunEnv, + BUN_TEST_PARALLEL_SCALE_MS: "0", + // LeakSanitizer cannot run while SIGCHLD is ignored: its own waitpid() fails. + ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=0"].filter(Boolean).join(":"), + }, + cwd: String(dir), + stderr: "pipe", + stdout: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toContain("PARALLEL"); + expect(stderr).not.toContain("outside a test file"); + expect(stderr).toContain("\n 2 pass\n 0 fail\n"); + expect(exitCode).toBe(0); + }, + workerExitTimeout, +);