fix(child_process): kill() returns false once the child has exited

[robobun]

What

Before: ChildProcess.kill() always returned true, even after awaiting the child's close event. Node returns false in that case so callers can distinguish "signal delivered" from "process was already gone".

import { spawn } from 'node:child_process';
const proc = spawn('echo', ['hi'], { stdio: 'inherit' });
await new Promise(r => proc.on('close', r));
console.log(proc.kill('SIGQUIT')); // Node: false, Bun (old): true

Why

Two bugs cooperated to hide the ESRCH signal:

1. src/bun.js/api/bun/subprocess.zig tryKill returned .success whenever hasExited() was true, and the kill JS binding threw the result away and returned undefined.
2. src/js/node/child_process.ts ChildProcess.prototype.kill then short-circuited on handle.killed (which Bun's Subprocess flips to true for any exited process, even one that exited on its own) and returned true from the happy path.

How

• Process.kill now returns Maybe(bool) — .result = true if the signal was delivered, .result = false if kill(2) / uv_process_kill reported ESRCH.
• Subprocess.tryKill propagates that and also reports false when the Subprocess already observed the exit on its side.
• The Subprocess.kill JS binding returns a boolean (the public Bun.Subprocess#kill() API signature is updated accordingly).
• ChildProcess.prototype.kill returns that boolean and only sets this.killed = true when the signal was actually delivered — matching Node, which documents childprocess.killed as "set to true after subprocess.kill() is used to successfully send a signal."
• Bun's internal shell subprocess keeps its existing "best-effort success" contract; only the JS-visible path changed.

Tests

test/regression/issue/29001.test.ts covers both node:child_process and Bun.spawn:

• kill() returns false for SIGTERM / SIGQUIT / signal 0 after close.
• kill(0) returns true while the child is alive.

Fail-before / pass-after confirmed with USE_SYSTEM_BUN=1. Official Node parallel tests test-child-process-kill.js and test-child-process-destroy.js still pass, as do test/js/bun/spawn/spawn-kill-signal.test.ts.

Fixes #29001

[robobun]

^{Updated 8:39 AM PT - May 17th, 2026}

✅ Your commit 2579c53 has passed in Build #55511! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 29002

That installs a local version of the PR into your bun-29002 executable, so you can run:

bun-29002 --bun

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Process kill APIs now return a boolean indicating whether the signal was delivered (true) or the target process was not reachable/already exited (false). Types, core APIs, call sites, Node shim, and tests were updated to propagate and assert this boolean semantics.

Changes

Cohort / File(s)	Summary
Types packages/bun-types/bun.d.ts	Changed Subprocess.kill(...) signature from returning void to returning boolean; updated JSDoc to document boolean semantics (true = delivered, false = not reachable).
Core Process APIs src/bun.js/api/bun/process.zig, src/bun.js/api/bun/subprocess.zig	Process.kill and Subprocess.tryKill changed from Maybe(void) → Maybe(bool); platform branches map "no such process" to { .result = false } and successful delivery to { .result = true }; Subprocess.kill now returns a JS boolean on success.
Call Sites / Aggregation src/bun.js/ProcessAutoKiller.zig, src/shell/subproc.zig	Callers updated to handle Maybe(bool) via switch: increment/count only when result is true; .err and false outcomes are no longer treated as delivered.
Node shim src/js/node/child_process.ts	ChildProcess.kill(sig?) now uses delivered = handle.kill(signal), sets this.killed only when delivered && signal !== 0, and returns delivered (false when process already exited).
Tests test/regression/issue/29001.test.ts	Added tests verifying kill() returns false for already-exited processes and true for live processes across node:child_process and bunSpawn (immediate-exit and long-running cat scenarios).

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: kill() now returns false when the child process has exited, matching Node.js behavior.
Description check	✅ Passed	The description follows the template with 'What' and 'Why' sections, clearly explaining the bug, root causes, and solution. Additional 'How' and 'Tests' sections provide implementation details and test coverage.
Linked Issues check	✅ Passed	The PR fully addresses issue #29001 by propagating ESRCH through native and JS layers so kill() returns accurate false/true boolean values matching Node.js semantics.
Out of Scope Changes check	✅ Passed	All code changes are scoped to kill() semantics and error propagation. Changes span Process, Subprocess, ChildProcess layers and include targeted regression tests, all aligned with the fix objective.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/bun.js/api/bun/process.zig (1)

529-565: ⚠️ Potential issue | 🟠 Major

Don't report success from the detached fallback.

Line 564 now returns .result = true even when no OS kill was attempted because this.poller was .detached. initPosix() creates a live Process in that state before watch() runs, so a pre-watch kill() can still no-op but claim delivery.

Suggested fix

 pub fn kill(this: *Process, signal: u8) Maybe(bool) {
     if (comptime Environment.isPosix) {
-        switch (this.poller) {
-            .waiter_thread, .fd => {
-                const err = std.c.kill(this.pid, signal);
-                if (err != 0) {
-                    const errno_ = bun.sys.getErrno(err);
-
-                    // if the process was already killed don't throw
-                    if (errno_ != .SRCH)
-                        return .{ .err = bun.sys.Error.fromCode(errno_, .kill) };
-
-                    return .{ .result = false };
-                }
-            },
-            else => {},
+        const rc = std.c.kill(this.pid, signal);
+        if (rc != 0) {
+            const errno_ = bun.sys.getErrno(rc);
+            if (errno_ != .SRCH)
+                return .{ .err = bun.sys.Error.fromCode(errno_, .kill) };
+            return .{ .result = false };
         }
+        return .{ .result = true };
     } else if (comptime Environment.isWindows) {
         switch (this.poller) {
             .uv => |*handle| {
                 if (handle.kill(signal).toError(.kill)) |err| {
                     if (err.errno != `@intFromEnum`(bun.sys.E.SRCH)) {
                         return .{ .err = err };
                     }
                     return .{ .result = false };
                 }

                 return .{ .result = true };
             },
-            else => {},
+            else => return .{ .result = false },
         }
     }

-    return .{ .result = true };
+    return .{ .result = false };
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/bun.js/api/bun/process.zig` around lines 529 - 565, The kill() function
currently returns .result = true at the end even when this.poller is .detached
and no OS kill was attempted; update Process.kill to not claim success from that
detached fallback by handling the .detached poller case (or adding an explicit
default in the switch over this.poller) and returning .{ .result = false } when
the process is detached and no kill was performed; ensure only code paths that
actually call std.c.kill or handle.kill and succeed return .{ .result = true }
and remove the unconditional true return at the end so .detached no longer
reports success.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/regression/issue/29001.test.ts`:
- Around line 21-28: Update the post-exit assertions to verify the process
exited successfully by capturing the arguments from the once(proc, "close") wait
and asserting the expected exit values instead of only checking proc.kill()
return values; specifically, change the wait call around once(proc, "close") to
destructure the close event result (e.g., code and signal from the "close" event
for proc) and assert code === 0 and signal === null (or the expected success
values) before asserting that proc.kill(...) returns false; apply the same
pattern for the second occurrence around lines 51-54 so both post-exit blocks
validate the actual exit code/signal as well as the kill() false checks.
- Around line 24-28: Add an assertion that verifies the ChildProcess.killed
state after the process has exited: after the existing
expect(proc.kill(...)).toBe(false) checks, assert that proc.killed is true
(i.e., expect(proc.killed).toBe(true)) to cover the changed public behavior of
ChildProcess.killed for the already-exited path and prevent regressions; locate
the check near the existing kill calls on the proc variable in the test.

---

Outside diff comments:
In `@src/bun.js/api/bun/process.zig`:
- Around line 529-565: The kill() function currently returns .result = true at
the end even when this.poller is .detached and no OS kill was attempted; update
Process.kill to not claim success from that detached fallback by handling the
.detached poller case (or adding an explicit default in the switch over
this.poller) and returning .{ .result = false } when the process is detached and
no kill was performed; ensure only code paths that actually call std.c.kill or
handle.kill and succeed return .{ .result = true } and remove the unconditional
true return at the end so .detached no longer reports success.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 80e00ae0-3881-4935-ba72-273bc1b02ffd

📥 Commits

Reviewing files that changed from the base of the PR and between 1afabdd and e166588.

📒 Files selected for processing (7)

• packages/bun-types/bun.d.ts
• src/bun.js/ProcessAutoKiller.zig
• src/bun.js/api/bun/process.zig
• src/bun.js/api/bun/subprocess.zig
• src/js/node/child_process.ts
• src/shell/subproc.zig
• test/regression/issue/29001.test.ts

[coderabbitai]

✅ No merge conflicts detected when merging into main.

Your branch is good to go!

[coderabbitai]

✅ No merge conflicts detected when merging into main.

Your branch is good to go!

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

packages/bun-types/bun.d.ts (1)

7121-7131: ⚠️ Potential issue | 🟡 Minor

Clarify the public kill()/killed contract around signal === 0.

The new JSDoc still overstates the behavior for kill(0): no signal is delivered there, it only probes process existence. That also makes the adjacent killed doc ("Has the process exited?") actively misleading after this PR, because killed now tracks a successful non-probe signal send, not exit state.

📝 Suggested doc fix

-    /**
-     * Has the process exited?
-     */
+    /**
+     * Whether a non-zero signal was successfully sent to the process.
+     *
+     * This does not indicate that the process has exited, and `kill(0)` does
+     * not set this to `true`.
+     */
     readonly killed: boolean;

     /**
-     * Kill the process by sending it a signal.
+     * Send a signal to the process, or probe for its existence with `0`.
      * `@param` signal The signal to send to the process. Defaults to `"SIGTERM"`.
-     * `@returns` `true` if the signal was delivered, `false` if the process
-     * had already exited (the OS reported `ESRCH`).
+     * `@returns` `true` if the signal was sent, or if `signal` is `0` and the
+     * process still exists; `false` if the process had already exited (the OS
+     * reported `ESRCH`).
      */
     kill(signal?: number | NodeJS.Signals): boolean;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/bun-types/bun.d.ts` around lines 7121 - 7131, The JSDoc for the
Process API misstates behavior for kill(0) and makes killed ambiguous: update
the documentation for kill(signal?: number | NodeJS.Signals) to state that
passing signal === 0 performs a non-delivery liveness probe (no signal is sent)
and that the return value is true when the probe or signal was successfully
delivered/observed and false if the process does not exist (ESRCH); also update
the JSDoc for the readonly killed boolean to clarify it does NOT simply mean
"Has the process exited?" but rather reflects whether a non-probe signal was
successfully sent/delivered (or whether kill() returned true for non-zero
signals), so callers understand the distinction between probing with kill(0) and
sending actual signals.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@packages/bun-types/bun.d.ts`:
- Around line 7121-7131: The JSDoc for the Process API misstates behavior for
kill(0) and makes killed ambiguous: update the documentation for kill(signal?:
number | NodeJS.Signals) to state that passing signal === 0 performs a
non-delivery liveness probe (no signal is sent) and that the return value is
true when the probe or signal was successfully delivered/observed and false if
the process does not exist (ESRCH); also update the JSDoc for the readonly
killed boolean to clarify it does NOT simply mean "Has the process exited?" but
rather reflects whether a non-probe signal was successfully sent/delivered (or
whether kill() returned true for non-zero signals), so callers understand the
distinction between probing with kill(0) and sending actual signals.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: dd8e195b-1607-4e7c-9f71-9ef15d5bf48a

📥 Commits

Reviewing files that changed from the base of the PR and between e166588 and 3a2f98d.

📒 Files selected for processing (4)

• packages/bun-types/bun.d.ts
• src/bun.js/api/bun/process.zig
• src/js/node/child_process.ts
• test/regression/issue/29001.test.ts

[claude]

LGTM — all previously-raised bugs (detached poller, exitCode→signal rename, kill(0)/killed guard, Windows test coverage) have been addressed.

Extended reasoning...

Overview

This PR fixes ChildProcess.kill() (and Bun.spawn subprocess.kill()) to return false once the child has already exited, matching Node.js semantics. It touches 7 files: the Zig process/subprocess layers, the JS child_process layer, the shell subprocess, ProcessAutoKiller, type definitions, and a regression test.

Previous bugs — all resolved

Four bugs I raised in earlier reviews were all fixed in commit 3a2f98d:

1. Detached poller (🔴): process.zig now returns .{ .result = false } for .detached on both POSIX and Windows instead of falling through to a spurious true.
2. Misleading parameter name (🟡): bun.d.ts parameter renamed from exitCode to signal with updated JSDoc.
3. kill(0) / this.killed (🔴): child_process.ts now guards with if (delivered && signal \!== 0) before setting this.killed = true, matching Node's signal > 0 guard.
4. Windows test coverage gap (🟡): The Bun.spawn 'returns false after exit' test now runs on all platforms (no test.if(isPosix) guard).

Security risks

No security-sensitive paths. The change affects signal delivery semantics and return values only.

Level of scrutiny

Medium — touches low-level process kill plumbing across Zig and JS layers, but the changes are targeted and well-commented. The remaining inline bug comment about the misleading Windows ESRCH comment in the test file is a nit (comment inaccuracy, not a code defect).

Other factors

The PR includes a dedicated regression test, CI passes, and all previously-raised issues have been addressed.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/regression/issue/29001.test.ts`:
- Around line 40-54: The finally cleanup for the spawned `proc` must assert that
forced termination succeeded: after calling `proc.kill("SIGKILL")` and awaiting
`once(proc, "close")`, capture the close event results and assert that `signal`
equals "SIGKILL" and that `proc.killed` is true (and code is null if you want
explicit exit-code checking). Do this in the current alive-path test (the block
using `spawn("cat", ...)`, `proc.kill(0)` and the finally that calls
`proc.kill("SIGKILL")`) and apply the same assertions to the other alive-path
case referenced (the similar test around lines 78-90).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 24570455-6472-4b14-9679-b1ec7f7fbe37

📥 Commits

Reviewing files that changed from the base of the PR and between de06f16 and d162571.

📒 Files selected for processing (1)

• test/regression/issue/29001.test.ts

[claude]

LGTM — all previously flagged issues were addressed.

Extended reasoning...

Overview

This PR fixes ChildProcess.kill() to return false after the child has already exited, matching Node.js behavior. It touches 7 files: the core Process and Subprocess Zig implementations, the Node.js child_process TypeScript shim, the shell subprocess adapter, the ProcessAutoKiller, the bun.d.ts type declarations, and a new regression test file.

Security Risks

No security-sensitive code paths (auth, crypto, permissions) are touched. The change only affects the boolean return value of signal delivery APIs and the semantics of killed state tracking. No injection or privilege-escalation surface is introduced.

Level of Scrutiny

Moderate — this modifies a public API return type from void to boolean (a breaking change for TypeScript callers, but aligned with Node.js compatibility), and changes observable killed state behavior. The logic paths are well-defined (POSIX std.c.kill, Windows uv_process_kill, and the hasExited() fast-path), and the PR includes regression tests that confirm pass-before/fail-after on the real issue.

Other Factors

Four issues I raised in prior review rounds were all addressed in commit 3a2f98d and d162571: the .detached poller false-positive, the exitCode parameter rename, the kill(0) existence-probe not setting this.killed, and the inaccurate Windows ESRCH comment. The bug hunting system found no new issues. CodeRabbit's suggestions were also incorporated. Test coverage is solid for the fast-path across POSIX and Windows.

[claude]

All previously raised concerns have been addressed — LGTM.

Extended reasoning...

Overview

This PR changes the return type of the native Process.kill and Subprocess.tryKill from Maybe(void) to Maybe(bool), propagates that boolean up through the JS binding, and fixes ChildProcess.prototype.kill to mirror Node's semantics (returns false when the process is already gone, does not flip this.killed on a signal-0 existence probe).

Security risks

None. This is a return-value correctness fix; no privilege escalation, no injection surface, no credential exposure.

Level of scrutiny

Moderate. The change touches core spawn/kill infrastructure (process.zig, subprocess.zig), but each individual change is small and mechanical — switching a Maybe(void) to Maybe(bool) and updating every call site. The most complex aspect (the .detached branch now returning false instead of a spurious true) was flagged in a prior review and is now correctly handled.

Other factors

All four issues surfaced in previous review rounds were resolved: (1) the .detached poller returns .result = false; (2) the bun.d.ts parameter is renamed signal; (3) this.killed is guarded with signal \!== 0; (4) the comment about Windows ESRCH coverage is accurate. The shell subprocess retains its existing best-effort contract via a separate tryKill path. Regression tests cover the main cases on both the node:child_process and Bun.spawn paths, and existing Node parallel tests still pass per the PR description.

[claude]

LGTM — all previously flagged issues have been addressed.

Extended reasoning...

Overview

This PR fixes and returning after a child process has already exited, aligning Bun with Node.js semantics. Changes span , , , , , type declarations in , and a new regression test file.

Security risks

None. This is a bug fix for a boolean return value. No auth, crypto, or permission-sensitive code paths are touched.

Level of scrutiny

The changes are mechanical and well-scoped: return type changed from to , propagated through callers, with a new guard in the JS shim. Previous review rounds identified several correctness issues, all of which have now been fixed. The remaining inline comment (a JSDoc nit about the @returns parenthetical attributing every false to ESRCH) is documentation-only and has no runtime impact.

Other factors

• All four inline issues from prior reviews are resolved ( poller returns , guard prevents flipping , parameter renamed to , inaccurate Windows coverage comment corrected).
• Test coverage includes both the POSIX-only paths (node:child_process with cat/SIGQUIT) and the cross-platform hasExited() fast-path (Bun.spawn after-exit tests run on all platforms).
• CI failures in the timeline are unrelated (webview, valkey, azure tests on the initial commit).

[robobun]

@coderabbitai please stop auto-resolving merge conflicts on this branch. The merge commits you pushed pulled in a new zig compiler version (c031cbe → 365343a) that the test infrastructure cannot fetch, which breaks the gate build. I've force-reset the branch to 8c4cee3 — please leave it alone.

[claude]

LGTM — all previous feedback addressed.

Extended reasoning...

Overview

This PR fixes ChildProcess.kill() (and Bun.Subprocess#kill()) to return false when the child process has already exited, matching Node.js semantics. Changes span process.zig, subprocess.zig, child_process.ts, type definitions, and a regression test. The PR also cherry-picks cron codegen support needed to keep the build green.

Security risks

None. Changes are confined to signal delivery semantics and type annotations with no auth, crypto, or permission-sensitive paths touched.

Level of scrutiny

Moderate — the fix is targeted and the core logic (hasExited fast-path + ESRCH propagation + signal-0 guard) is straightforward. All edge cases I identified across three review rounds were addressed.

Other factors

The one outstanding inline comment (misleading error message when calling the 3-arg OS cron form with a missing title) is a minor DX nit that does not affect correctness — the call still throws, just with a less precise message. The inline comment captures it for follow-up.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 packages/bun-types/bun.d.ts:7227 — The type-test fixture test/integration/bun-types/fixture/spawn.ts calls proc.kill() at lines 122 and 125 without asserting the return type via tsd.expectType<boolean>, so a regression of the return type back to void would compile cleanly and go undetected. Additionally, the comment on line 125 reads // specify an exit code rather than // specify a signal, which is stale after the parameter rename in bun.d.ts. Fix: add tsd.expectType<boolean>(proc.kill()) and update the stale comment.

  Extended reasoning...

  What the bug is and how it manifests

  This PR changed Subprocess.kill()'s return type from void to boolean in packages/bun-types/bun.d.ts. The fixture file test/integration/bun-types/fixture/spawn.ts was not updated to assert this new return type. Specifically, lines 122 and 125 call proc.kill() and proc.kill() respectively, discarding the return value with no tsd.expectType assertion.

  The specific code path that triggers it

  TypeScript permits callers to silently discard any return value — whether void or boolean. So both of these calls compile identically whether kill() returns void or boolean:

  proc.kill();          // line 122 — return value discarded, no type assertion
  proc.kill();          // line 125 — return value discarded, no type assertion

  The bun-types integration suite runs tsd (or tsc) over all fixture files. Without a tsd.expectType<boolean>(proc.kill()) call, the suite would pass even if kill() regressed back to kill(): void.

  Why existing code does not prevent it

  The same fixture file already uses tsd.expectType for other Subprocess properties (e.g., proc.stdin, proc.stdout, proc.pid), establishing a pattern. The absence of a corresponding assertion for kill() is an oversight, not a deliberate choice. There is no other fixture or test that exercises the TypeScript-level return type of Subprocess.kill().

  What the impact would be

  If a future commit accidentally reverts the return type of kill() back to void in bun.d.ts, the bun-types integration suite would not catch it. The regression would only surface when an external user tried to use the return value in TypeScript and got a type error.

  How to fix it

  1. Wrap each proc.kill() call with tsd.expectType<boolean>:

     tsd.expectType<boolean>(proc.kill());
     tsd.expectType<boolean>(proc.kill(9));  // also exercises the signal parameter

  2. Update the stale comment on line 125 from // specify an exit code to // specify a signal (or similar), since the PR renamed the parameter from exitCode to signal in bun.d.ts.

  Step-by-step proof

  1. bun.d.ts is edited to revert kill(signal?: ...): boolean back to kill(exitCode?: ...): void.
  2. test/integration/bun-types/fixture/spawn.ts line 122: proc.kill(); — compiles fine because void return is discardable.
  3. test/integration/bun-types/fixture/spawn.ts line 125: proc.kill(); — same, no error.
  4. tsd / tsc over the fixture directory exits with code 0.
  5. CI passes; the regression ships undetected.

[claude]

LGTM — all substantive review concerns have been addressed and the bug hunting system found nothing in this revision.

Extended reasoning...

Overview

This PR fixes kill() returning true after a child process has already exited, matching Node.js semantics. The changes span 8 files: Zig process management (process.zig, subprocess.zig, ProcessAutoKiller.zig, subproc.zig), the Node.js child_process shim (child_process.ts), TypeScript type declarations (bun.d.ts), and tests.

Security risks

None. This is a behavioral correctness fix for a return-value contract — no auth, crypto, or permission-sensitive code is involved.

Level of scrutiny

The Zig changes are mechanical: Maybe(void) → Maybe(bool) with ESRCH mapped to false and successful delivery mapped to true. The .detached poller arm (pre-watch() or post-detach()) correctly returns false rather than claiming a signal was delivered. The child_process.ts change properly guards this.killed = true with signal \!== 0 to match Node.js behavior on existence probes. The shell subprocess retains best-effort semantics by discarding the boolean. All these were reviewed and corrected across multiple commits.

Other factors

• All bugs raised in previous review rounds (detached-poller false-positive, signal-0 killed flag, JSDoc inaccuracy, comment inaccuracy) were fixed by the author.
• Regression tests cover: kill() after exit returns false, kill(0) while alive returns true, proc.killed not set by existence probes, and exit code/signal assertions before the kill checks.
• The Bun.spawn alive-path test is missing a proc.killed === false assertion (raised in my last comment), but Subprocess.killed is purely computed from process.status so it cannot be wrong here — this is a documentation gap, not a defect.
• CI passed on the latest commit; no merge conflicts.

[claude]

🔴 The && signal !== 0 guard added here in response to earlier review comment 3052094521 is based on a factually incorrect claim about Node.js — Node's lib/internal/child_process.js sets this.killed = true unconditionally on err === 0, with no signal > 0 check. So in Node, proc.kill(0) on a live process sets proc.killed = true, while after this PR Bun leaves it false; and the assertion at test/regression/issue/29001.test.ts:35 (expect(proc.killed).toBe(false) after kill(0) on a live process) would fail on real Node, locking in the divergence. Drop the && signal !== 0 clause and flip/remove that test assertion.

Extended reasoning...

What the bug is and how it manifests

The guard at src/js/node/child_process.ts:1491 reads:

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 if (signal > 0) before setting killed" and quoted a snippet of Node source containing that guard. That claim — and the quoted snippet — is factually incorrect.

What Node.js actually does

Verified directly against Node.js source on both main (line ~525) and v20.x (line ~501) at lib/internal/child_process.js:

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 if (signal > 0) guard anywhere in Node's kill(). The earlier review comment fabricated a code snippet that does not exist in Node.

Why this is a regression introduced by this PR

The pre-PR Bun code (visible in the diff's - lines) set this.killed = true unconditionally on the success path:

handle.kill(signal);
this.killed = true;
return true;

That matched Node. This PR's added && signal !== 0 clause introduces a new divergence — in a PR whose explicit stated goal is "matching Node, which documents childprocess.killed as set to true after subprocess.kill() is used to successfully send a signal." And per kill(2), signal 0 is successfully sent (the syscall returns 0); Node treats it as such.

The test locks in the wrong behavior

test/regression/issue/29001.test.ts:35 asserts:

expect(proc.kill(0)).toBe(true);
expect(proc.killed).toBe(false);   // <-- would FAIL on real Node

Running that second assertion against real Node.js fails, because Node sets proc.killed = true after a successful kill(0). So this PR not only introduces a Node-compat divergence, it adds a regression test that enshrines the divergence.

Step-by-step proof

1. const proc = spawn('cat', [], { stdio: ['pipe', 'ignore', 'ignore'] }) — long-lived process.
2. proc.kill(0) is called. sig === 0 so signal = 0.
3. handle.kill(0) issues kill(pid, 0), which returns 0 (process alive) → delivered = true.
4. Node.js: err === 0 → this.killed = true, return true. So proc.killed === true.
5. Bun (this PR): delivered && signal !== 0 → true && false → this.killed stays false, return true. So proc.killed === false.
6. Bun (pre-PR): this.killed = true unconditionally → proc.killed === true (matched Node).

Impact

Any code that probes a live child with kill(0) and then reads proc.killed will observe different values in Bun vs. Node. This is a behavioral Node-compat regression in the node:child_process shim, introduced by this PR, based on incorrect review feedback.

How to fix

Drop the && signal !== 0 clause so the line reads:

if (delivered) this.killed = true;

and either remove the assertion at test/regression/issue/29001.test.ts:35 or flip it to expect(proc.killed).toBe(true) to match Node. The accompanying comment ("kill(0) is a POSIX existence probe, not a kill — don't mark killed") should also be removed.

[claude]

🟡 Nit: this spec citation (// Spec process.zig:550 — ".waiter_thread, .fd => kill(); else => {}") is now stale on both counts — this PR moved the Zig kill to line 587 and replaced the else => {} arm with .detached => return .{ .result = false }, and the Rust line immediately below was just changed to _ => return Ok(false). Consider updating the quote to the new .detached => false semantics or just dropping the line-number/else => {} part.

Extended reasoning...

What the issue is. The comment at src/spawn/process.rs:667 reads:

// Spec process.zig:550 — `.waiter_thread, .fd => kill(); else => {}`.

This is a porting cross-reference into the Zig sibling that, per src/CLAUDE.md, is "the original implementation kept as a porting reference for behavior." This PR (commit 2579c531, "spawn: apply kill() false-on-exit fix to Rust port too") modified both the cited Zig spec file and the Rust code immediately surrounding this comment, but left the spec citation untouched. It is now stale on two counts:

1. Line number — pub fn kill(this: *Process, …) now begins at process.zig:587, not :550 (verified via grep). The PR added an 11-line doc comment above the Zig function plus body changes, shifting it down.
2. Quoted semantics — the Zig else => {} arm no longer exists. This PR replaced it with .detached => return .{ .result = false } (see the process.zig diff hunk). And the Rust _ => {} line directly below this comment was changed in the same diff hunk to _ => return Ok(false). So the comment now quotes pre-PR semantics that exist in neither file.

Why this PR is the direct cause. The comment line itself appears as unchanged context in the diff, sandwiched between added/changed lines. The PR author updated the Zig spec, updated the Rust implementation to match, but did not update the one-line spec citation that bridges them. The line-number drift (550→587) might arguably have a pre-existing component, but the else => {} quote staleness was unambiguously introduced by this PR — that arm existed verbatim in both files until this change removed it from both.

Why existing code does not catch it. Comments are not type-checked or otherwise validated against their referents. The longer rationale block on lines 668–675 ("Detached is a deliberate no-op… Do not widen this match") remains substantively accurate — we still do not issue kill(2) when detached — so a maintainer reading the full comment would not be functionally misled. Only the one-line spec citation is wrong.

Impact. Zero runtime impact; comment-accuracy only. Per src/CLAUDE.md, the .zig sibling is the authoritative behavioral porting reference, so a stale cross-reference will send a future reader to the wrong line and show them semantics (else => {} falling through to Ok(())) that this PR explicitly removed. That is mildly misleading but easily corrected by reading the actual Zig file.

Step-by-step proof.

1. Before this PR, process.zig had else => {} in the POSIX poller switch and process.rs had _ => {} — the comment at line 667 accurately quoted both.
2. This PR's process.zig hunk replaces else => {} with .detached => return .{ .result = false }.
3. This PR's process.rs hunk (the very block containing line 667) replaces _ => {} with _ => return Ok(false) at line 698.
4. grep -n "pub fn kill(this: \*Process" on process.zig now returns line 587, not 550.
5. Line 667 still reads Spec process.zig:550 — ".waiter_thread, .fd => kill(); else => {}" — describing code that no longer exists in either file.

How to fix. Either update the citation to match the new semantics, e.g.:

// Spec process.zig:587 — `.waiter_thread, .fd => kill(); .detached => false`.

or drop the line-number + verbatim quote entirely (line numbers rot) and keep only the rationale block on lines 668–675, which already explains why detached returns "not delivered."