bun test: only kill the timed-out test's own subprocesses on timeout

src/jsc/ProcessAutoKiller.rs

@@ -8,10 +8,13 @@ bun_core::declare_scope!(AutoKiller, hidden);
 #[derive(Default)]
 pub struct ProcessAutoKiller {
     /// Keys are intrusively-refcounted `*Process` (ref()'d on insert, deref()'d
-    /// on remove/drop). Stored as raw ptr for identity-hash semantics.
-    pub(crate) processes: ArrayHashMap<*mut Process, ()>,
+    /// on remove/drop). Stored as raw ptr for identity-hash semantics. Values
+    /// are the [`Self::scope`] each process was spawned in.
+    pub(crate) processes: ArrayHashMap<*mut Process, u32>,
     pub enabled: bool,
     pub(crate) ever_enabled: bool,
+    /// The test runner begins a scope per test, so a timeout only kills what that test spawned.
+    scope: u32,
 }
 
 impl ProcessAutoKiller {
@@ -24,34 +27,57 @@ impl ProcessAutoKiller {
         self.enabled = false;
     }
 
+    pub fn begin_scope(&mut self) {
+        self.scope = self.scope.wrapping_add(1);
+    }
+
     pub fn kill(&mut self) -> Result {
-        Result {
-            processes: self.kill_processes(),
+        let mut count: u32 = 0;
+        while let Some(entry) = self.processes.pop() {
+            count += Self::kill_and_release(entry.key);
         }
+        Result { processes: count }
     }
 
-    fn kill_processes(&mut self) -> u32 {
+    /// Kills the current scope's processes. Earlier scopes stay tracked so that
+    /// [`Self::kill`] still covers them.
+    pub fn kill_scope(&mut self) -> Result {
         let mut count: u32 = 0;
-        while let Some(entry) = self.processes.pop() {
-            {
-                // SAFETY: every key in `processes` was ref()'d on insert and is
-                // live until the matching deref() below; popped entry is
-                // exclusively owned for this scope so `&mut Process` is unaliased.
-                let p: &mut Process = unsafe { &mut *entry.key };
-                if !p.has_exited() {
-                    bun_core::scoped_log!(AutoKiller, "process.kill {}", p.pid);
-                    count += p.kill(SignalCode::DEFAULT.0).is_ok() as u32;
-                }
+        let mut index = self.processes.len();
+        while index > 0 {
+            index -= 1;
+            if self.processes.values()[index] != self.scope {
+                continue;
             }
-            // SAFETY: key live until this releases the ref taken on insert.
-            unsafe { Process::deref(entry.key) };
+            // Walking backwards, so the entry swapped into `index` was already visited.
+            let (process, _) = self.processes.swap_remove_at(index);
+            count += Self::kill_and_release(process);
         }
-        count
+        Result { processes: count }
+    }
+
+    /// `process` must already be removed from `processes`; this releases its ref.
+    fn kill_and_release(process: *mut Process) -> u32 {
+        let killed = {
+            // SAFETY: every key in `processes` was ref()'d on insert and is
+            // live until the matching deref() below; the entry was removed
+            // from the map by the caller, so `&mut Process` is unaliased.
+            let p: &mut Process = unsafe { &mut *process };
+            if p.has_exited() {
+                false
+            } else {
+                bun_core::scoped_log!(AutoKiller, "process.kill {}", p.pid);
+                p.kill(SignalCode::DEFAULT.0).is_ok()
+            }
+        };
+        // SAFETY: key live until this releases the ref taken on insert.
+        unsafe { Process::deref(process) };
+        killed as u32
     }
 
     pub fn clear(&mut self) {
         for process in self.processes.keys() {
-            // SAFETY: see kill_processes — key is live until deref().
+            // SAFETY: see kill_and_release — key is live until deref().
             unsafe { Process::deref(*process) };
         }
 
@@ -69,7 +95,7 @@ impl ProcessAutoKiller {
         if self.enabled {
             // Alloc failure means we never took
             // a ref, so just bail. `put` here is fallible only on OOM.
-            if self.processes.put(process.as_ptr(), ()).is_err() {
+            if self.processes.put(process.as_ptr(), self.scope).is_err() {
                 return;
             }
             // SAFETY: caller passes a live Process; we take a ref to extend its
@@ -99,7 +125,7 @@ pub struct Result {
 impl Drop for ProcessAutoKiller {
     fn drop(&mut self) {
         for process in self.processes.keys() {
-            // SAFETY: see kill_processes — key is live until deref().
+            // SAFETY: see kill_and_release — key is live until deref().
             unsafe { Process::deref(*process) };
         }
         // `self.processes` storage freed by its own Drop.

src/runtime/test_runner/Execution.rs

@@ -299,7 +299,7 @@ impl Execution {
         let _g = group_begin!();
 
         // if the concurrent group has one sequence and the sequence has an active entry that has timed out,
-        //   kill any dangling processes
+        //   kill the dangling processes it spawned
         // when using test.concurrent(), we can't do this because it could kill multiple tests at once.
         if let Some(current_group) = self.active_group() {
             // reshaped for borrowck — capture range, drop &mut group, re-borrow sequences
@@ -311,9 +311,12 @@ impl Execution {
                     // SAFETY: arena-owned entry, alive for lifetime of BunTest
                     let entry = unsafe { entry.as_ref() };
                     let now = Timespec::now_force_real_time();
-                    if entry.timespec.order(&now) == core::cmp::Ordering::Less {
+                    // EPOCH: this entry has no timeout; a timer left armed by an earlier entry fired.
+                    if !entry.timespec.eql(&Timespec::EPOCH)
+                        && entry.timespec.order(&now) == core::cmp::Ordering::Less
+                    {
                         // SAFETY: bun_vm() returns the live per-thread VM.
-                        let kill_count = global_this.bun_vm().as_mut().auto_killer.kill();
+                        let kill_count = global_this.bun_vm().as_mut().auto_killer.kill_scope();
                         if kill_count.processes > 0 {
                             bun_core::pretty_errorln!(
                                 "<d>killed {} dangling process{}<r>",
@@ -565,7 +568,9 @@ impl Execution {
 
     fn on_group_started(global_this: &JSGlobalObject) {
         // SAFETY: bun_vm() returns the live per-thread VM.
-        global_this.bun_vm().as_mut().auto_killer.enable();
+        let auto_killer = &mut global_this.bun_vm().as_mut().auto_killer;
+        auto_killer.begin_scope();
+        auto_killer.enable();
     }
 
     fn on_group_completed(global_this: &JSGlobalObject) {

test/cli/test/test-timeout-behavior.test.ts

@@ -1,5 +1,5 @@
 import { expect, test } from "bun:test";
-import { bunEnv, bunExe, isFlaky, isLinux } from "harness";
+import { bunEnv, bunExe, isFlaky, isLinux, tempDir } from "harness";
 import path from "path";
 
 if (isFlaky && isLinux) {
@@ -33,3 +33,120 @@ if (isFlaky && isLinux) {
     expect(combined).toContain("(pass) slow test after test timeout");
   });
 }
+
+// Shared by the inline test files below. An echo child stays alive until it is
+// killed, and `echo()` only gets its message back while the child is alive: a
+// killed child never runs again, so its stdout just reports EOF.
+const echoChildHelpers = /* ts */ `
+  function spawnEcho() {
+    return Bun.spawn({
+      cmd: [process.execPath, "-e", "process.stdin.pipe(process.stdout)"],
+      stdin: "pipe",
+      stdout: "pipe",
+      stderr: "ignore",
+    });
+  }
+
+  async function echo(child: ReturnType<typeof spawnEcho>, message: string) {
+    child.stdin.write(message);
+    await child.stdin.flush();
+    const reader = child.stdout.getReader();
+    let received = "";
+    while (received.length < message.length) {
+      const { value, done } = await reader.read();
+      if (done) break;
+      received += Buffer.from(value).toString();
+    }
+    reader.releaseLock();
+    return received;
+  }
+`;
+
+async function runTestFile(source: string, args: string[] = []) {
+  using dir = tempDir("test-timeout-kill", { "kill.test.ts": echoChildHelpers + source });
+  await using proc = Bun.spawn({
+    cmd: [bunExe(), "test", ...args, "./kill.test.ts"],
+    cwd: String(dir),
+    env: bunEnv,
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+  const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+  return { combined: stdout + stderr, exitCode };
+}
+
+test.concurrent.each([[[]], [["--isolate"]]])(
+  "a test timeout only kills the processes spawned by that test (args: %p)",
+  async args => {
+    const { combined, exitCode } = await runTestFile(
+      /* ts */ `
+        import { afterAll, beforeAll, expect, test } from "bun:test";
+
+        const children: ReturnType<typeof spawnEcho>[] = [];
+        afterAll(() => children.forEach(child => child.kill()));
+
+        beforeAll(() => {
+          children.push(spawnEcho());
+        });
+
+        test("spawns a child that outlives the test", () => {
+          children.push(spawnEcho());
+        });
+
+        test("times out", async () => {
+          children.push(spawnEcho());
+          await new Promise(() => {});
+        }, 100);
+
+        test("children spawned by beforeAll and by an earlier test are still running", async () => {
+          const [fromBeforeAll, fromEarlierTest] = children;
+          expect(await Promise.all([echo(fromBeforeAll, "beforeAll"), echo(fromEarlierTest, "earlier test")])).toEqual([
+            "beforeAll",
+            "earlier test",
+          ]);
+        });
+      `,
+      args,
+    );
+
+    // Only the child of the test that timed out is killed.
+    expect(combined).toContain("killed 1 dangling process");
+    expect(combined).not.toContain("dangling processes");
+    expect(combined).toContain("(pass) spawns a child that outlives the test");
+    expect(combined).toContain("(fail) times out");
+    expect(combined).toContain("(pass) children spawned by beforeAll and by an earlier test are still running");
+    expect(combined).toContain(" 2 pass\n");
+    expect(combined).toContain(" 1 fail\n");
+    expect(exitCode).toBe(1);
+  },
+);
+
+test.concurrent("a test without a timeout keeps its processes when an earlier test's timer fires", async () => {
+  const { combined, exitCode } = await runTestFile(/* ts */ `
+    import { expect, test } from "bun:test";
+
+    // Arms a 50ms timer for this test. The runner never disarms it, so it fires
+    // while the next test is running.
+    test("finishes immediately", () => {}, 50);
+
+    test("has no timeout and spawns a child", async () => {
+      const child = spawnEcho();
+      try {
+        // Nothing in this file can observe the stale timer firing, so wait on a
+        // timer with a later deadline instead: the runner's timer and this one
+        // are in the same heap and fire in deadline order, so the 50ms timer
+        // (armed before this test started) has fired once this resolves.
+        await Bun.sleep(100);
+        expect(await echo(child, "still here")).toBe("still here");
+      } finally {
+        child.kill();
+      }
+    }, 0);
+  `);
+
+  expect(combined).not.toContain("dangling process");
+  expect(combined).toContain("(pass) finishes immediately");
+  expect(combined).toContain("(pass) has no timeout and spawns a child");
+  expect(combined).toContain(" 2 pass\n");
+  expect(exitCode).toBe(0);
+});