-
Notifications
You must be signed in to change notification settings - Fork 4.9k
fix(child_process): kill() returns false once the child has exited #29002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 9 commits
b0f98ba
5a8ee9e
b95a678
1ff7556
8f7ed0b
9d7c4b0
7fb1931
a3e2690
6891684
2579c53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1484,15 +1484,12 @@ | |
|
|
||
| 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; | ||
|
Check failure on line 1492 in src/js/node/child_process.ts
|
||
|
claude[bot] marked this conversation as resolved.
Comment on lines
+1489
to
+1492
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The Extended reasoning...What the bug is and how it manifests The guard at if (delivered && signal !== 0) this.killed = true;This was added in commit 3a2f98d in response to inline review comment 3052094521, which asserted that "Node.js explicitly guards this with What Node.js actually does Verified directly against Node.js source on both ChildProcess.prototype.kill = function(sig) {
const signal = sig === 0 ? sig :
convertToValidSignal(sig === undefined ? 'SIGTERM' : sig);
if (this._handle) {
const err = this._handle.kill(signal);
if (err === 0) {
/* Success. */
this.killed = true; // <-- unconditional, no signal > 0 check
return true;
}
...There is no Why this is a regression introduced by this PR The pre-PR Bun code (visible in the diff's handle.kill(signal);
this.killed = true;
return true;That matched Node. This PR's added The test locks in the wrong behavior
expect(proc.kill(0)).toBe(true);
expect(proc.killed).toBe(false); // <-- would FAIL on real NodeRunning that second assertion against real Node.js fails, because Node sets Step-by-step proof
Impact Any code that probes a live child with How to fix Drop the if (delivered) this.killed = true;and either remove the assertion at |
||
| } catch (e) { | ||
| this.emit("error", e); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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"); | ||
| } | ||
|
robobun marked this conversation as resolved.
|
||
| }); | ||
|
|
||
| // 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; | ||
| } | ||
| }); | ||
|
robobun marked this conversation as resolved.
|
||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.