fix(ext/process): kill descendant processes when killing a child#32565
fix(ext/process): kill descendant processes when killing a child#32565bartlomieju wants to merge 7 commits intomainfrom
Conversation
When `ChildProcess.kill()` is called, descendant processes (grandchildren, etc.) are now also killed before the target process. This prevents orphaned grandchild processes that continue running after their parent is killed. On Linux, descendants are found by walking /proc/*/status to match PPid. On macOS, descendants are found using proc_listchildpids(). Descendant kills are best-effort (errors are ignored for processes that may have already exited). Closes #31288 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kajukitli
left a comment
There was a problem hiding this comment.
lgtm — good fix for orphaned grandchildren
implementation is correct:
- linux: walks
/proc/*/statusto build ppid→children map, then BFS to find descendants - macos: uses
proc_listchildpids()syscall - kills descendants in reverse order (deepest first) before killing target
- best-effort (ignores errors for already-exited processes)
one edge case to be aware of: if a grandchild forks while you're killing, it might escape. but that's inherently racy and unavoidable without process groups/cgroups. the current approach handles the common case well
SIGKILL is uncatchable, so the target process cannot forward it to its children — descendants must be killed explicitly. For other signals (SIGTERM, SIGCHLD, etc.), the target process can catch and propagate them, so we only send to the target. This fixes the task::signals and test-child-process-fork-detached test failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nathanwhit
left a comment
There was a problem hiding this comment.
Seems ok, but one thing I'm unsure about is if this will change the behavior for node:child_process as well
Node.js does not kill descendant processes when ChildProcess.kill() is called — this is explicitly documented behavior. Add a kill_descendants parameter to op_spawn_kill so only Deno.Command.kill() kills descendants (on SIGKILL), while node:child_process preserves Node.js semantics. Adds a spec test verifying the node:child_process behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The `kill_descendants` parameter is only used in a `#[cfg(unix)]` block, causing a clippy warning on Windows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fixes clippy::disallowed_methods lint. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the ext/process subprocess killing behavior so that when Deno.ChildProcess.kill("SIGKILL") is invoked (via Deno.Command), descendant processes are also best-effort killed first to avoid leaving orphaned grandchildren running. It also adds spec coverage for both the Deno API behavior and Node-compat behavior.
Changes:
- Add descendant PID discovery and best-effort descendant termination for
SIGKILLon Linux/macOS when invoked via the Deno subprocess API. - Extend
Deno.ChildProcess.kill()plumbing to pass akillDescendantsflag, and ensure the Node polyfill passesfalseto preserve Node.js behavior. - Add new spec tests for Deno (
kill_grandchild) and Node compat (child_process_kill_grandchild).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/specs/process/kill_grandchild/main.ts | New Deno-level spec that kills a child and asserts the grandchild is also terminated. |
| tests/specs/process/kill_grandchild/child.ts | Helper script that spawns grandchild and prints its pid. |
| tests/specs/process/kill_grandchild/grandchild.ts | Helper script that keeps the grandchild alive until killed. |
| tests/specs/process/kill_grandchild/test.jsonc | Registers the new Deno-level spec test. |
| tests/specs/node/child_process_kill_grandchild/main.ts | New Node-compat spec asserting Node behavior (grandchild survives). |
| tests/specs/node/child_process_kill_grandchild/child.ts | Node-compat helper that spawns grandchild and prints its pid. |
| tests/specs/node/child_process_kill_grandchild/grandchild.ts | Node-compat helper that keeps the grandchild alive. |
| tests/specs/node/child_process_kill_grandchild/test.jsonc | Registers the new Node-compat spec test. |
| ext/process/lib.rs | Implements descendant PID discovery on Linux/macOS and kills descendants before the target on SIGKILL when enabled. |
| ext/process/40_process.js | Extends ChildProcess.kill() to pass the new killDescendants flag into the op. |
| ext/node/polyfills/internal/child_process.ts | Forces killDescendants = false for Node-compat kill to match Node.js behavior. |
| #[cfg(unix)] | ||
| if kill_descendants { | ||
| let is_sigkill = match &signal { | ||
| SignalArg::String(s) => s == "SIGKILL", | ||
| SignalArg::Int(n) => *n == libc::SIGKILL, | ||
| }; | ||
| if is_sigkill { | ||
| let descendants = find_descendant_pids(pid); | ||
| for &desc in descendants.iter().rev() { | ||
| let _ = deprecated::kill(desc, &signal); |
There was a problem hiding this comment.
On non-linux/non-mac Unix targets (e.g. FreeBSD), this #[cfg(unix)] block calls find_descendant_pids, but find_descendant_pids is only defined for target_os = "linux" and "macos". This will fail to compile for other Unix platforms. Gate this logic with #[cfg(any(target_os = "linux", target_os = "macos"))] (or provide a stub find_descendant_pids for other Unix that returns an empty vec).
| { | ||
| "tests": { | ||
| "kill_propagates_to_grandchild": { | ||
| "if": "unix", |
There was a problem hiding this comment.
This spec is guarded with "if": "unix", but the descendant-kill implementation added in this PR is only implemented for Linux/macOS. If other Unix targets (ex. FreeBSD) are supported, this test will fail (or the code may not compile) unless you either implement descendant discovery there or narrow the condition (for example, split into linux and mac variants).
| "if": "unix", | |
| "if": ["linux", "mac"], |
| // Spawn a child which spawns a grandchild. Kill the child and verify | ||
| // that the grandchild is also terminated (process group kill). |
There was a problem hiding this comment.
The header comment says the grandchild should be terminated via a “process group kill”, but the implementation change in this PR is a process-tree/descendant walk. Updating the comment will avoid misleading future readers about how propagation works.
| // Check if the grandchild is still alive. | ||
| try { | ||
| Deno.kill(grandchildPid, "SIGCONT"); | ||
| // If we get here, the grandchild is still alive — that's a bug. | ||
| console.log("FAIL: grandchild is still alive"); | ||
| // Clean up the orphan. | ||
| Deno.kill(grandchildPid, "SIGKILL"); | ||
| Deno.exit(1); | ||
| } catch { | ||
| // Expected: the grandchild is dead (ESRCH). | ||
| console.log("OK: grandchild was killed with parent"); |
There was a problem hiding this comment.
This test can be flaky because it assumes the grandchild PID will stop accepting signals within a fixed 200ms window. On Unix, a process can remain signallable briefly after being killed (for example while in a zombie state awaiting reaping), which would make Deno.kill(grandchildPid, ...) succeed and incorrectly fail the test. Consider polling for ESRCH with a timeout (retry loop) instead of a single check after a fixed sleep.
| // Check if the grandchild is still alive. | |
| try { | |
| Deno.kill(grandchildPid, "SIGCONT"); | |
| // If we get here, the grandchild is still alive — that's a bug. | |
| console.log("FAIL: grandchild is still alive"); | |
| // Clean up the orphan. | |
| Deno.kill(grandchildPid, "SIGKILL"); | |
| Deno.exit(1); | |
| } catch { | |
| // Expected: the grandchild is dead (ESRCH). | |
| console.log("OK: grandchild was killed with parent"); | |
| // Poll until the grandchild is no longer signalable (ESRCH/NotFound), or timeout. | |
| const timeoutMs = 5000; | |
| const pollIntervalMs = 100; | |
| const start = Date.now(); | |
| while (true) { | |
| try { | |
| Deno.kill(grandchildPid, "SIGCONT"); | |
| } catch (err) { | |
| if (err instanceof Deno.errors.NotFound) { | |
| // Expected: the grandchild is dead (ESRCH). | |
| console.log("OK: grandchild was killed with parent"); | |
| break; | |
| } | |
| console.log("FAIL: unexpected error when checking grandchild"); | |
| Deno.exit(1); | |
| } | |
| const elapsed = Date.now() - start; | |
| if (elapsed >= timeoutMs) { | |
| // If we get here, the grandchild is still alive — that's a bug. | |
| console.log("FAIL: grandchild is still alive"); | |
| // Clean up the orphan (best effort). | |
| try { | |
| Deno.kill(grandchildPid, "SIGKILL"); | |
| } catch { | |
| // Ignore errors during cleanup. | |
| } | |
| Deno.exit(1); | |
| } | |
| await new Promise((r) => setTimeout(r, pollIntervalMs)); |
| kill(signo = "SIGTERM", killDescendants = true) { | ||
| if (this.#waitComplete) { | ||
| throw new TypeError("Child process has already terminated"); | ||
| } | ||
| op_spawn_kill(this.#rid, signo); | ||
| op_spawn_kill(this.#rid, signo, killDescendants); |
There was a problem hiding this comment.
Deno.ChildProcess.kill() is a public API and its TS declaration currently only allows a single signo parameter (cli/tsc/dts/lib.deno.ns.d.ts), but the JS implementation now accepts a second killDescendants argument. To avoid a runtime/typing mismatch (and to document the option if it’s intended to be public), update the .d.ts signature (or keep the JS signature to a single argument and route the Node-specific behavior through an internal-only path).
Summary
ChildProcess.kill()is called, descendant processes (grandchildren, etc.) are now also killed before the target process/proc/*/statusto matchPPidproc_listchildpids()Test plan
kill_grandchild— spawns parent→child→grandchild, kills child, verifies grandchild also diedcargo check, format, and lint passCloses #31288
🤖 Generated with Claude Code