diff --git a/docs/runtime/child-process.mdx b/docs/runtime/child-process.mdx index 4e82dae2f440..aff5312ae78f 100644 --- a/docs/runtime/child-process.mdx +++ b/docs/runtime/child-process.mdx @@ -559,7 +559,7 @@ interface Subprocess extends AsyncDisposable { readonly signalCode: NodeJS.Signals | null; readonly killed: boolean; - kill(exitCode?: number | NodeJS.Signals): void; + kill(signal?: number | NodeJS.Signals): boolean; ref(): void; unref(): void; diff --git a/packages/bun-types/bun.d.ts b/packages/bun-types/bun.d.ts index 37d9bc45fcf7..11c2d3536cee 100644 --- a/packages/bun-types/bun.d.ts +++ b/packages/bun-types/bun.d.ts @@ -7249,10 +7249,13 @@ declare module "bun" { readonly killed: boolean; /** - * Kill the process - * @param exitCode The exitCode to send to the process + * Send a signal to the process, or probe for its existence with `0` + * (POSIX-style existence check that does not terminate the child). + * @param signal The signal to send to the process. Defaults to `"SIGTERM"`. + * @returns `true` if the signal was sent (or, for `signal === 0`, the + * process is still alive); `false` if the process has already exited. */ - kill(exitCode?: number | NodeJS.Signals): void; + kill(signal?: number | NodeJS.Signals): boolean; /** * This method will tell Bun to wait for this process to exit after you already diff --git a/src/js/node/child_process.ts b/src/js/node/child_process.ts index 4f6e056aadee..4cbfb36b45dd 100644 --- a/src/js/node/child_process.ts +++ b/src/js/node/child_process.ts @@ -1484,15 +1484,12 @@ class ChildProcess extends EventEmitter { const handle = this.#handle; if (handle) { - if (handle.killed) { - this.killed = true; - return true; - } - try { - handle.kill(signal); - this.killed = true; - return true; + // Don't gate on handle.killed: Bun flips it on any exit, signalled or not. + const delivered = handle.kill(signal); + // kill(0) is a POSIX existence probe, not a kill — don't mark killed. + if (delivered && signal !== 0) this.killed = true; + return delivered; } catch (e) { this.emit("error", e); } diff --git a/src/jsc/ProcessAutoKiller.rs b/src/jsc/ProcessAutoKiller.rs index 91308dacb455..ccb377b4a1ab 100644 --- a/src/jsc/ProcessAutoKiller.rs +++ b/src/jsc/ProcessAutoKiller.rs @@ -40,7 +40,12 @@ impl ProcessAutoKiller { 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; + // Count only processes where the signal was actually + // delivered — a `.detached` poller or OS-reported ESRCH + // returns `Ok(false)` from `kill` and should not count. + if let Ok(true) = p.kill(SignalCode::DEFAULT.0) { + count += 1; + } } } // SAFETY: key live until this releases the ref taken on insert. diff --git a/src/jsc/ProcessAutoKiller.zig b/src/jsc/ProcessAutoKiller.zig index d687231b575c..e15ae38f8ca5 100644 --- a/src/jsc/ProcessAutoKiller.zig +++ b/src/jsc/ProcessAutoKiller.zig @@ -31,7 +31,7 @@ fn killProcesses(this: *ProcessAutoKiller) u32 { defer process.key.deref(); if (!process.key.hasExited()) { log("process.kill {d}", .{process.key.pid}); - count += @as(u32, @intFromBool(process.key.kill(@intFromEnum(bun.SignalCode.default)) == .result)); + count += @intFromBool(process.key.kill(@intFromEnum(bun.SignalCode.default)).isTrue()); } } return count; diff --git a/src/runtime/api/bun/process.zig b/src/runtime/api/bun/process.zig index 13eef7d36594..13d80aa58d38 100644 --- a/src/runtime/api/bun/process.zig +++ b/src/runtime/api/bun/process.zig @@ -573,7 +573,18 @@ pub const Process = struct { bun.destroy(this); } - pub fn kill(this: *Process, signal: u8) Maybe(void) { + /// Sends `signal` to the process. + /// + /// Returns: + /// - `.result = true` if the signal was delivered. + /// - `.result = false` if the child could not be reached — either the + /// poller is already `.detached` (we observed the exit on our side) + /// or the OS reported `ESRCH`. Callers that just want best-effort + /// termination can ignore this, but anything JS-visible (e.g. + /// `subprocess.kill()`) needs to propagate it so Node's + /// `ChildProcess.kill()` can return `false`. + /// - `.err` for any other error. + pub fn kill(this: *Process, signal: u8) Maybe(bool) { if (comptime Environment.isPosix) { switch (this.poller) { .waiter_thread, .fd => { @@ -584,9 +595,17 @@ pub const Process = struct { // if the process was already killed don't throw if (errno_ != .SRCH) return .{ .err = bun.sys.Error.fromCode(errno_, .kill) }; + + return .{ .result = false }; } + + return .{ .result = true }; }, - else => {}, + // `.detached` means we never armed the poller or we already + // called `detach()` from `onExit`. Either way there is no + // live child to signal — report "not delivered" rather than + // claiming success we didn't attempt. + .detached => return .{ .result = false }, } } else if (comptime Environment.isWindows) { switch (this.poller) { @@ -596,19 +615,17 @@ pub const Process = struct { if (err.errno != @intFromEnum(bun.sys.E.SRCH)) { return .{ .err = err }; } + + return .{ .result = false }; } - return .{ - .result = {}, - }; + return .{ .result = true }; }, - else => {}, + .detached => return .{ .result = false }, } } - return .{ - .result = {}, - }; + return .{ .result = false }; } }; diff --git a/src/runtime/api/bun/subprocess.rs b/src/runtime/api/bun/subprocess.rs index 1469676e56f7..e4d83e9587a5 100644 --- a/src/runtime/api/bun/subprocess.rs +++ b/src/runtime/api/bun/subprocess.rs @@ -651,7 +651,10 @@ impl Subprocess<'_> { this.stderr.with_mut(|s| s.unref()); match this.try_kill(this.kill_signal) { - bun_sys::Result::Ok(()) => {} + // Delivered or not doesn't matter for asyncDispose — the caller + // just wants the child dead; falling through to `getExited` + // below handles both the "already gone" and "just killed" cases. + bun_sys::Result::Ok(_) => {} bun_sys::Result::Err(err) => { // Signal 9 should always be fine, but just in case that somehow fails. return Err(global.throw_value(err.to_js(global))); @@ -717,23 +720,27 @@ impl Subprocess<'_> { } match this.try_kill(sig) { - bun_sys::Result::Ok(()) => {} + // `true` when the signal was sent, `false` when the process had + // already exited (nothing to signal). Node's `ChildProcess.kill()` + // needs to see `false` to return `false` to user code. + bun_sys::Result::Ok(delivered) => Ok(JSValue::js_boolean(delivered)), bun_sys::Result::Err(err) => { // EINVAL or ENOSYS means the signal is not supported in the current platform (most likely unsupported on windows) - return Err(global_this.throw_value(err.to_js(global_this))); + Err(global_this.throw_value(err.to_js(global_this))) } } - - Ok(JSValue::UNDEFINED) } pub fn has_killed(&self) -> bool { self.process().has_killed() } - pub fn try_kill(&self, sig: SignalCode) -> bun_sys::Result<()> { + /// Returns `Ok(true)` if `sig` was delivered to the child, or + /// `Ok(false)` if the child had already exited (either because we + /// already saw the exit on our side or the OS reported `ESRCH`). + pub fn try_kill(&self, sig: SignalCode) -> bun_sys::Result { if self.has_exited() { - return bun_sys::Result::Ok(()); + return bun_sys::Result::Ok(false); } self.process_mut().kill(sig.0) } diff --git a/src/runtime/api/bun/subprocess.zig b/src/runtime/api/bun/subprocess.zig index 4170f72ce32c..6634aaf698df 100644 --- a/src/runtime/api/bun/subprocess.zig +++ b/src/runtime/api/bun/subprocess.zig @@ -395,23 +395,27 @@ pub fn kill( if (globalThis.hasException()) return .zero; switch (this.tryKill(sig)) { - .result => {}, + // `true` when the signal was sent, `false` when the process had + // already exited (nothing to signal). Node's `ChildProcess.kill()` + // needs to see `false` to return `false` to user code. + .result => |delivered| return JSValue.jsBoolean(delivered), .err => |err| { // EINVAL or ENOSYS means the signal is not supported in the current platform (most likely unsupported on windows) return globalThis.throwValue(try err.toJS(globalThis)); }, } - - return .js_undefined; } pub fn hasKilled(this: *const Subprocess) bool { return this.process.hasKilled(); } -pub fn tryKill(this: *Subprocess, sig: SignalCode) bun.sys.Maybe(void) { +/// Returns `.result = true` if `sig` was delivered to the child, or +/// `.result = false` if the child had already exited (either because we +/// already saw the exit on our side or the OS reported `ESRCH`). +pub fn tryKill(this: *Subprocess, sig: SignalCode) bun.sys.Maybe(bool) { if (this.hasExited()) { - return .success; + return .{ .result = false }; } return this.process.kill(@intFromEnum(sig)); } diff --git a/src/runtime/shell/subproc.rs b/src/runtime/shell/subproc.rs index d76adc2f3c62..d418df277876 100644 --- a/src/runtime/shell/subproc.rs +++ b/src/runtime/shell/subproc.rs @@ -403,7 +403,11 @@ impl ShellSubprocess { return Ok(()); } - self.proc().kill(u8::try_from(sig).expect("int cast")) + // Shell subprocess kill is best-effort — drop the "delivered?" bool + // from process.kill's Maybe, keep errors. + self.proc() + .kill(u8::try_from(sig).expect("int cast")) + .map(|_delivered| ()) } // fn has_called_getter(self: &Subprocess, comptime getter: @Type(.enum_literal)) -> bool { diff --git a/src/runtime/shell/subproc.zig b/src/runtime/shell/subproc.zig index 1f8db8ad3964..a448bfd608e8 100644 --- a/src/runtime/shell/subproc.zig +++ b/src/runtime/shell/subproc.zig @@ -498,7 +498,9 @@ pub const ShellSubprocess = struct { return .success; } - return this.process.kill(@intCast(sig)); + // Shell kill is best-effort — drop the "delivered?" bool, keep errors. + if (this.process.kill(@intCast(sig)).asErr()) |err| return .{ .err = err }; + return .success; } // fn hasCalledGetter(this: *Subprocess, comptime getter: @Type(.enum_literal)) bool { diff --git a/src/spawn/process.rs b/src/spawn/process.rs index 4f19ce213aa8..4ea61e294da1 100644 --- a/src/spawn/process.rs +++ b/src/spawn/process.rs @@ -654,7 +654,14 @@ impl Process { self.exit_handler = ProcessExitHandler::default(); } - pub fn kill(&mut self, signal: u8) -> Maybe<()> { + /// Sends `signal` to the process. + /// + /// Returns `Ok(true)` if the signal was delivered, `Ok(false)` if the + /// child could not be reached — either the poller is detached (we + /// observed the exit on our side) or the OS reported `ESRCH`. Anything + /// JS-visible (e.g. `subprocess.kill()`) needs to propagate this so + /// Node's `ChildProcess.kill()` can return `false`. + pub fn kill(&mut self, signal: u8) -> Maybe { #[cfg(unix)] { // Spec process.zig:550 — `.waiter_thread, .fd => kill(); else => {}`. @@ -683,9 +690,12 @@ impl Process { if errno_ != bun_sys::E::ESRCH { return Err(bun_sys::Error::from_code(errno_, bun_sys::Tag::kill)); } + return Ok(false); } + return Ok(true); } - _ => {} + // Detached: no live child to signal. + _ => return Ok(false), } } #[cfg(windows)] @@ -700,14 +710,16 @@ impl Process { if err.errno != bun_sys::E::ESRCH as u16 { return Err(err); } + return Ok(false); } - return Ok(()); + return Ok(true); } - _ => {} + _ => return Ok(false), } } - Ok(()) + #[cfg(not(any(unix, windows)))] + Ok(false) } } diff --git a/test/integration/bun-types/fixture/spawn.ts b/test/integration/bun-types/fixture/spawn.ts index 2036413158a0..595e477b0396 100644 --- a/test/integration/bun-types/fixture/spawn.ts +++ b/test/integration/bun-types/fixture/spawn.ts @@ -119,10 +119,10 @@ function depromise(_promise: Promise): T { proc.killed; // boolean — was the process killed? proc.exitCode; // null | number proc.signalCode; // null | "SIGABRT" | "SIGALRM" | ... - proc.kill(); + tsd.expectType(proc.kill()).is(); proc.killed; // true - proc.kill(); // specify an exit code + tsd.expectType(proc.kill(9)).is(); // specify a signal proc.unref(); } diff --git a/test/regression/issue/29001.test.ts b/test/regression/issue/29001.test.ts new file mode 100644 index 000000000000..7b9564580ba2 --- /dev/null +++ b/test/regression/issue/29001.test.ts @@ -0,0 +1,73 @@ +import { spawn as bunSpawn } from "bun"; +import { describe, expect, test } from "bun:test"; +import { bunEnv, bunExe, isPosix } from "harness"; +import { spawn } from "node:child_process"; +import { once } from "node:events"; + +// https://github.com/oven-sh/bun/issues/29001 — kill() must return false +// once the child has exited, matching Node. +describe.concurrent("issue #29001 — kill() reports failure after exit", () => { + test.if(isPosix)("node:child_process ChildProcess.kill() returns false after exit", async () => { + const proc = spawn(bunExe(), ["-e", "process.exit(0)"], { + env: bunEnv, + stdio: "ignore", + }); + + // Assert the clean exit first so a fixture crash surfaces clearly. + const [code, signal] = await once(proc, "close"); + expect(code).toBe(0); + expect(signal).toBe(null); + + expect(proc.kill("SIGTERM")).toBe(false); + expect(proc.kill("SIGQUIT")).toBe(false); + expect(proc.kill(0)).toBe(false); + + // Child exited on its own — proc.killed only flips on our signal. + expect(proc.killed).toBe(false); + }); + + test.if(isPosix)("node:child_process ChildProcess.kill() returns true while alive", async () => { + const proc = spawn("cat", [], { stdio: ["pipe", "ignore", "ignore"] }); + + try { + // Signal 0 is an existence probe — succeeds but must not mark killed. + expect(proc.kill(0)).toBe(true); + expect(proc.killed).toBe(false); + } finally { + proc.kill("SIGKILL"); + await once(proc, "close"); + } + }); + + // These Bun.spawn tests cover the cross-platform hasExited() fast path + // in Subprocess.tryKill (short-circuits before Process.kill), so they + // don't exercise the OS-level ESRCH branch. + test("Bun.spawn subprocess.kill() returns false after exit", async () => { + await using proc = bunSpawn({ + cmd: [bunExe(), "-e", "process.exit(0)"], + env: bunEnv, + stdio: ["ignore", "ignore", "ignore"], + }); + + const exitCode = await proc.exited; + expect(exitCode).toBe(0); + expect(proc.signalCode).toBe(null); + + expect(proc.kill("SIGTERM")).toBe(false); + expect(proc.kill(0)).toBe(false); + }); + + test.if(isPosix)("Bun.spawn subprocess.kill() returns true while alive", async () => { + await using proc = bunSpawn({ + cmd: ["cat"], + stdio: ["pipe", "ignore", "ignore"], + }); + + try { + expect(proc.kill(0)).toBe(true); + } finally { + proc.kill("SIGKILL"); + await proc.exited; + } + }); +});