diff --git a/experiments/issue-15/graceful-child.sh b/experiments/issue-15/graceful-child.sh new file mode 100755 index 00000000..eae17b17 --- /dev/null +++ b/experiments/issue-15/graceful-child.sh @@ -0,0 +1,11 @@ +#!/bin/sh +# A child that handles SIGTERM gracefully: it traps the signal, writes a marker +# file proving the handler ran, and exits. Used to check whether +# command-stream's kill() actually gives the child a chance to clean up. +marker="$1" +trap 'echo "SIGTERM handler ran" >> "$marker"; exit 0' TERM +trap 'echo "SIGINT handler ran" >> "$marker"; exit 0' INT +echo ready +while true; do + sleep 0.05 +done diff --git a/experiments/issue-15/js-escalation-debug.mjs b/experiments/issue-15/js-escalation-debug.mjs new file mode 100644 index 00000000..0afe4cc4 --- /dev/null +++ b/experiments/issue-15/js-escalation-debug.mjs @@ -0,0 +1,39 @@ +// Why does a child that ignores SIGTERM survive the SIGKILL escalation? +import { $ } from '../../js/src/$.mjs'; +import { unlinkSync, statSync } from 'fs'; + +const hb = '/tmp/hb.txt'; +try { + unlinkSync(hb); +} catch {} + +const command = `trap '' TERM INT; echo ready; while true; do echo tick >> ${hb}; sleep 0.05; done`; +const cmd = $({ mirror: false, killGrace: 50 })`sh -c ${command}`; +cmd.start(); +await new Promise((r) => setTimeout(r, 300)); + +const pid = cmd.child?.pid; +console.log('options.killGrace =', JSON.stringify(cmd.options?.killGrace)); +console.log('child.pid =', pid); + +const alive = (p) => { + try { + process.kill(p, 0); + return true; + } catch { + return false; + } +}; +const size = () => { + try { + return statSync(hb).size; + } catch { + return 0; + } +}; + +cmd.kill(); +for (const ms of [100, 300, 600, 1000]) { + await new Promise((r) => setTimeout(r, ms === 100 ? 100 : 200)); + console.log(`t+${ms}ms direct-child-alive=${alive(pid)} heartbeat=${size()}`); +} diff --git a/experiments/issue-15/js-kill-grace.mjs b/experiments/issue-15/js-kill-grace.mjs new file mode 100644 index 00000000..6c297a7c --- /dev/null +++ b/experiments/issue-15/js-kill-grace.mjs @@ -0,0 +1,32 @@ +// Does command-stream's JS kill() let a child handle the signal it was sent? +// +// The child traps SIGTERM/SIGINT and appends a line to a marker file. If the +// marker stays empty, the child never got to run its handler -- meaning the +// signal was immediately followed by an unsurvivable SIGKILL. +import { $ } from '../../js/src/$.mjs'; +import { mkdtempSync, readFileSync, existsSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +const dir = mkdtempSync(join(tmpdir(), 'cs-signal-')); +const child = new URL('./graceful-child.sh', import.meta.url).pathname; + +async function probe(signal) { + const marker = join(dir, `marker-${signal}`); + const cmd = $({ mirror: false })`sh ${child} ${marker}`; + const running = cmd.start(); + await new Promise((r) => setTimeout(r, 400)); // let the trap install + cmd.kill(signal); + const result = await running; + await new Promise((r) => setTimeout(r, 300)); // let the handler flush + const handled = existsSync(marker) ? readFileSync(marker, 'utf8').trim() : ''; + return { + signal, + code: result.code, + handled: handled || '(handler never ran)', + }; +} + +for (const signal of ['SIGTERM', 'SIGINT']) { + console.log(JSON.stringify(await probe(signal))); +} diff --git a/experiments/issue-15/js-orphan-grandchild.mjs b/experiments/issue-15/js-orphan-grandchild.mjs new file mode 100644 index 00000000..a642f5fc --- /dev/null +++ b/experiments/issue-15/js-orphan-grandchild.mjs @@ -0,0 +1,36 @@ +// Why does killing a command whose shell already exited leave the grandchild +// running in JavaScript? Prints the runner state at kill time. +import { $ } from '../../js/src/$.mjs'; +import { mkdtempSync, statSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; + +const dir = mkdtempSync(join(tmpdir(), 'orphan-')); +const beat = join(dir, 'heartbeat'); +const sleep = (ms) => new Promise((r) => setTimeout(r, ms)); +const size = (p) => { + try { + return statSync(p).size; + } catch { + return 0; + } +}; + +const command = `sh -c 'while true; do echo tick >> ${beat}; sleep 0.05; done' & echo ready`; +const cmd = $({ mirror: false, killGrace: 50 })`sh -c ${command}`; +cmd.start(); +await sleep(400); + +console.log('before kill:', { + finished: cmd.finished, + hasChild: Boolean(cmd.child), + pid: cmd.child?.pid ?? null, + beat: size(beat), +}); + +cmd.kill(); +await sleep(400); +const afterKill = size(beat); +await sleep(400); +console.log('heartbeat after kill:', afterKill, '-> later:', size(beat)); +process.exit(0); diff --git a/experiments/issue-15/macos-zombie-getpgid.sh b/experiments/issue-15/macos-zombie-getpgid.sh new file mode 100755 index 00000000..01ef15c5 --- /dev/null +++ b/experiments/issue-15/macos-zombie-getpgid.sh @@ -0,0 +1,54 @@ +#!/usr/bin/env bash +# Show that the grandchild regression tests catch the macOS failure mode. +# +# macOS resolves `getpgid(pid)` through XNU's `proc_find`, which skips zombie +# processes, so the lookup fails with ESRCH once the `sh` wrapper has exited - +# and the code that used it then skipped the group delivery entirely, leaving +# the grandchild running. Linux answers `getpgid` for a zombie, so the bug is +# invisible here. +# +# This script restores the `getpgid` guard *and* emulates the macOS behaviour by +# treating a zombie as "not found" (via /proc//stat), then runs the signal +# tests. Expected: the orphaned-grandchild test fails. It reverts the patch on +# exit, so it can be run repeatedly. +set -uo pipefail +cd "$(dirname "$0")/../../rust" || exit 1 + +backup="$(mktemp)" + +# Restore from a copy rather than with `git checkout`, which would also discard +# any uncommitted work in this file. +cp src/signal.rs "$backup" +trap 'cp "$backup" src/signal.rs; rm -f "$backup"' EXIT + +python3 - <<'PY' +import pathlib +p = pathlib.Path('src/signal.rs') +s = p.read_text() +old = """ // Signal the whole process group (negative pid) first, so grandchildren are + // reached even if the group leader dies on the signal we send it next. + if delivery == Delivery::ProcessAndGroup {""" +new = """ // Signal the whole process group (negative pid) first, so grandchildren are + // reached even if the group leader dies on the signal we send it next. + if delivery == Delivery::ProcessAndGroup && macos_style_getpgid(pid) == Some(pid) {""" +assert s.count(old) == 1, "anchor not found - has send_signal_to_process changed?" +s = s.replace(old, new) +s += ''' +/// `getpgid` as macOS answers it: ESRCH for a process that is already a zombie. +#[cfg(unix)] +fn macos_style_getpgid(pid: u32) -> Option { + use nix::unistd::{getpgid, Pid}; + + let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).ok()?; + let state = stat.rsplit_once(')')?.1.split_whitespace().next()?.to_string(); + if state == "Z" { + return None; + } + getpgid(Some(Pid::from_raw(pid as i32))).ok().map(|p| p.as_raw() as u32) +} +''' +p.write_text(s) +print("patched: group delivery gated behind a macOS-style getpgid") +PY + +cargo test --test signals --all-features 2>&1 | tail -20 diff --git a/experiments/issue-15/rust_kill_grace.rs b/experiments/issue-15/rust_kill_grace.rs new file mode 100644 index 00000000..def160db --- /dev/null +++ b/experiments/issue-15/rust_kill_grace.rs @@ -0,0 +1,64 @@ +//! Does the Rust side let a child handle the signal it was sent? +//! +//! Mirror of experiments/issue-15/js-kill-grace.mjs. The child traps SIGTERM +//! and appends to a marker file; an empty marker means the handler never ran. +//! +//! Run with: cargo test --test rust_kill_grace -- --nocapture +use command_stream::{OutputChunk, ProcessRunner, RunOptions, StreamingRunner}; +use std::time::Duration; + +fn child_script(marker: &std::path::Path) -> String { + format!( + "trap 'echo handled >> {marker}; exit 0' TERM INT; echo ready; while true; do sleep 0.05; done", + marker = marker.display() + ) +} + +async fn probe_stream(signal: &str) -> (i32, String) { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("marker"); + let runner = StreamingRunner::new(child_script(&marker)).kill_signal(signal); + let mut stream = runner.stream(); + let mut code = -1; + let mut killed = false; + while let Some(chunk) = stream.next().await { + match chunk { + OutputChunk::Stdout(_) if !killed => { + killed = true; + stream.kill(); + } + OutputChunk::Exit(c) => code = c, + _ => {} + } + } + tokio::time::sleep(Duration::from_millis(300)).await; + let handled = std::fs::read_to_string(&marker).unwrap_or_default(); + (code, if handled.trim().is_empty() { "(handler never ran)".into() } else { handled.trim().into() }) +} + +#[tokio::test] +async fn stream_kill_grace() { + for signal in ["SIGTERM", "SIGINT"] { + let (code, handled) = probe_stream(signal).await; + println!("STREAM signal={signal} code={code} handled={handled}"); + } +} + +#[tokio::test] +async fn process_runner_kill_grace() { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("marker"); + let mut runner = ProcessRunner::new( + child_script(&marker), + RunOptions { mirror: false, ..Default::default() }, + ); + runner.start().await.unwrap(); + tokio::time::sleep(Duration::from_millis(400)).await; + runner.kill().unwrap(); + tokio::time::sleep(Duration::from_millis(400)).await; + let handled = std::fs::read_to_string(&marker).unwrap_or_default(); + println!( + "PROCESS_RUNNER kill() handled={}", + if handled.trim().is_empty() { "(handler never ran)" } else { handled.trim() } + ); +} diff --git a/experiments/issue-15/sh-trap-race.py b/experiments/issue-15/sh-trap-race.py new file mode 100755 index 00000000..798939fd --- /dev/null +++ b/experiments/issue-15/sh-trap-race.py @@ -0,0 +1,76 @@ +#!/usr/bin/env python3 +"""Does a POSIX shell reliably run its trap when the signal reaches the group? + +The Rust signal tests failed intermittently for SIGINT only (~13% of runs), +which points at shell semantics rather than at the library. This reproduces the +library's exact delivery - spawn `sh -c` in its own process group, then signal +both the pid and the group - without any command-stream code in the picture. + +The second half adds the library's SIGKILL escalation, to tell "the shell never +runs its trap" apart from "the escalation arrived before the trap could". + +Usage: experiments/issue-15/sh-trap-race.py [ATTEMPTS] +""" +import os +import signal +import subprocess +import sys +import tempfile +import time + +ATTEMPTS = int(sys.argv[1]) if len(sys.argv) > 1 else 30 + +SCRIPT = ( + "trap 'echo handled >> {marker}; exit 0' {name}; " + "echo ready; while true; do sleep 0.05; done" +) + + +def attempt(name, sig, group, escalate_ms=None): + """Run one child, signal it, and report whether its trap ran.""" + fd, marker = tempfile.mkstemp() + os.close(fd) + try: + # start_new_session=True is what the library does with process_group(0): + # the shell is the group leader, its `sleep` is in the same group. + child = subprocess.Popen( + ["sh", "-c", SCRIPT.format(marker=marker, name=name)], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + start_new_session=True, + ) + time.sleep(0.3) + os.kill(child.pid, sig) + if group: + os.kill(-child.pid, sig) + if escalate_ms is not None: + time.sleep(escalate_ms / 1000) + for target in (child.pid, -child.pid): + try: + os.kill(target, signal.SIGKILL) + except ProcessLookupError: + pass + time.sleep(0.4) + try: + os.kill(-child.pid, signal.SIGKILL) + except ProcessLookupError: + pass + child.wait() + with open(marker) as handle: + return "handled" in handle.read() + finally: + os.unlink(marker) + + +for name, sig in (("TERM", signal.SIGTERM), ("INT", signal.SIGINT)): + for group in (True, False): + for escalate_ms in (None, 100): + ran = sum( + attempt(name, sig, group, escalate_ms) for _ in range(ATTEMPTS) + ) + target = "pid + group" if group else "pid only" + escalation = "no escalation" if escalate_ms is None else f"SIGKILL +{escalate_ms}ms" + print( + f"SIG{name:<4} to {target:<11} ({escalation:<15}): " + f"trap ran in {ran}/{ATTEMPTS} runs" + ) diff --git a/js/.changeset/issue-15-signal-handling.md b/js/.changeset/issue-15-signal-handling.md new file mode 100644 index 00000000..eef1d12f --- /dev/null +++ b/js/.changeset/issue-15-signal-handling.md @@ -0,0 +1,15 @@ +--- +'command-stream': minor +--- + +Deliver signals gracefully when stopping a running command. `kill()` used to +send the requested signal and `SIGKILL` in the same tick, so a child that +trapped `SIGTERM` never got to run its handler even though the reported exit +code claimed it had. The signal is now delivered to the whole process group, +followed by a grace period, and only then by `SIGKILL`. The new `killGrace` +option controls that window (default `100` ms; `0` escalates immediately), and +`killSignal` sets the default signal for `kill()`, `break`, and `AbortSignal`. + +Documents the behavior in "Sending Signals to a Running Command" with the +`128 + signal` exit-code table, and adds a runnable +`examples/signals-graceful-shutdown.mjs`. diff --git a/js/README.md b/js/README.md index 3b09f708..a1cb3d5b 100644 --- a/js/README.md +++ b/js/README.md @@ -926,37 +926,12 @@ for await (const chunk of $`some-endless-stream`.stream()) { ##### Choosing the stop signal -`kill()` defaults to `SIGTERM`, but you can stop with any signal. Pass it -explicitly, or configure a default via the `killSignal` option so that an -argument-less `kill()`, a `break`, or an `AbortSignal` all use it: - -```javascript -// Explicit per-call signal: -cmd.kill('SIGINT'); // exit code 130 - -// Configured default — used by kill(), break, and AbortSignal cancellation: -const cmd = $({ killSignal: 'SIGINT' })`some-endless-stream`; -for await (const chunk of cmd.stream()) { - if (chunk.type === 'stdout' && done(chunk)) - cmd.kill(); // sends SIGINT - else if (chunk.type === 'exit') console.log(chunk.code); // 130 -} - -// AbortSignal style also honors killSignal — awaiting resolves promptly when -// the signal fires (it does not hang) with the configured signal's exit code: -const ac = new AbortController(); -const running = $({ - signal: ac.signal, - killSignal: 'SIGINT', -})`some-endless-stream`; -setTimeout(() => ac.abort(), 1000); // stops with SIGINT -const result = await running; -console.log(result.code); // 130 -``` - -command-stream still escalates to `SIGKILL` after delivering the chosen signal -so a process that ignores it is guaranteed to terminate; the reported exit code -reflects the signal you configured. +`kill()` defaults to `SIGTERM`, but you can stop with any signal, either per +call (`cmd.kill('SIGINT')`) or by configuring a default with the `killSignal` +option. The child is given a grace period to handle the signal before SIGKILL +follows. See +[Sending Signals to a Running Command](#sending-signals-to-a-running-command) +for the full model, the `killGrace` option, and the exit-code table. ### EventEmitter Pattern (Event-driven) @@ -1642,6 +1617,7 @@ As with any shell-enabled process, pass only trusted `file` and `args` values; s - `env: object` - Environment variables - `exitPumpGrace: number` - Milliseconds to wait for buffered output to drain after the process exits before aborting stdio reads held open by a grandchild (default `100`; see [Async Iteration](#async-iteration-real-time-streaming)) - `killSignal: string` - Signal used to stop the process when it is killed without an explicit signal — i.e. `kill()` with no argument, `break`ing out of a `stream()` loop, or an external `AbortSignal` firing (default `'SIGTERM'`). An explicit `kill(signal)` argument always overrides this. The reported exit code follows the conventional `128 + signal` mapping (e.g. `SIGTERM` → 143, `SIGINT` → 130, `SIGKILL` → 137) +- `killGrace: number` - Milliseconds to wait after delivering `killSignal` before escalating to `SIGKILL`, giving the child a chance to run its own signal handler and shut down cleanly (default `100`). Set to `0` to escalate immediately, with no chance to clean up (only `SIGKILL` is delivered). `SIGKILL` itself is never delayed. See [Sending Signals to a Running Command](#sending-signals-to-a-running-command) **Override defaults:** @@ -1923,9 +1899,151 @@ The library provides **advanced CTRL+C handling** that properly manages signals 2. **User Handler Preservation**: When no children are running, your custom SIGINT handlers work normally 3. **Process Groups**: Child processes use detached spawning for proper signal isolation 4. **TTY Mode Support**: Raw TTY mode is properly managed and restored on interruption -5. **Graceful Termination**: Uses SIGTERM → SIGKILL escalation for robust process cleanup +5. **Graceful Termination**: Sends SIGTERM, waits a grace period so the child can handle it, then escalates to SIGKILL 6. **Exit Code Standards**: Proper signal exit codes (130 for SIGINT, 143 for SIGTERM) +### Sending Signals to a Running Command + +The behavior above is about signals arriving _at your script_. This section is +the other direction: sending a signal _to the command you launched_. + +`kill()` stops a running command. It defaults to `SIGTERM`, and accepts any +signal name: + +```javascript +const cmd = $`ping 8.8.8.8`; +cmd.start(); + +cmd.kill(); // SIGTERM (the default) +cmd.kill('SIGINT'); // what CTRL+C sends +cmd.kill('SIGHUP'); // any signal name works +``` + +#### What `kill()` actually does + +Stopping a process is not a single signal. Every `kill()` runs the same four +steps: + +1. The requested signal is delivered to the child **and its process group**, so + a grandchild behind a shell wrapper is reached too (see + [Grandchildren and process groups](#grandchildren-and-process-groups)). +2. The child is given a grace period (`killGrace`, default `100`ms) to run its + own signal handler and exit on its own terms. +3. If it is still alive when the grace period expires, `SIGKILL` follows, so a + process that ignores the signal is still guaranteed to terminate. +4. The reported exit code is the conventional `128 + signal` value. + +Step 2 is what makes a shutdown _graceful_: without it, a child that traps +SIGTERM to flush output, release a lock, or stop its own workers is destroyed +before its handler can run. + +```javascript +// A child that cleans up when asked to stop: +const cmd = $`sh -c 'trap "echo cleaning up; exit 0" TERM; while true; do sleep 1; done'`; +cmd.start(); +cmd.kill(); // the trap runs, prints "cleaning up", and exits + +const result = await cmd; +console.log(result.code); // 143 +``` + +#### Choosing the stop signal + +Pass a signal explicitly, or configure a default with the `killSignal` option so +that an argument-less `kill()`, a `break` out of a stream loop, and an +`AbortSignal` all use it: + +```javascript +// Explicit per-call signal — overrides killSignal for this call only: +cmd.kill('SIGINT'); // exit code 130 + +// Configured default — used by kill(), break, and AbortSignal cancellation: +const cmd = $({ killSignal: 'SIGINT' })`some-endless-stream`; +for await (const chunk of cmd.stream()) { + if (chunk.type === 'stdout' && done(chunk)) + cmd.kill(); // sends SIGINT + else if (chunk.type === 'exit') console.log(chunk.code); // 130 +} + +// AbortSignal style also honors killSignal — awaiting resolves promptly when +// the signal fires (it does not hang) with the configured signal's exit code: +const ac = new AbortController(); +const running = $({ + signal: ac.signal, + killSignal: 'SIGINT', +})`some-endless-stream`; +setTimeout(() => ac.abort(), 1000); // stops with SIGINT +console.log((await running).code); // 130 +``` + +#### Tuning the grace period + +`killGrace` is the number of milliseconds between the requested signal and the +SIGKILL escalation: + +```javascript +// Give a slow shutdown more room: +const cmd = $({ killGrace: 5000 })`./server --graceful-shutdown`; + +// Opt out entirely — SIGKILL is sent immediately, with no chance to clean up: +const cmd = $({ killGrace: 0 })`stuck-process`; +``` + +With `killGrace: 0` the requested signal is not delivered at all — only +`SIGKILL` is. Delivering it first and then killing would leave a window the +child can be scheduled in, which makes "no grace" a race rather than a +guarantee. The reported exit code still reflects the signal you requested. + +`SIGKILL` is never delayed: it cannot be caught, so `kill('SIGKILL')` skips the +grace period regardless of the `killGrace` value. + +#### Signal exit codes + +A process stopped by a signal reports `128 + signal`, the same convention POSIX +shells use: + +| Signal | Number | Exit code | Typical meaning | +| --------- | ------ | --------- | ------------------------------------ | +| `SIGHUP` | 1 | `129` | Terminal closed / reload config | +| `SIGINT` | 2 | `130` | CTRL+C | +| `SIGQUIT` | 3 | `131` | Quit from keyboard | +| `SIGKILL` | 9 | `137` | Forced termination, cannot be caught | +| `SIGUSR1` | 10 | `138` | Application-defined | +| `SIGUSR2` | 12 | `140` | Application-defined | +| `SIGTERM` | 15 | `143` | Polite request to stop (the default) | + +The code reflects the signal **you requested**, even when the SIGKILL escalation +is what ultimately stopped the process — `kill('SIGTERM')` on a child that +ignores SIGTERM still reports `143`, not `137`. + +#### Grandchildren and process groups + +Commands run through a shell, so `$\`sh -c '...'\`` is usually a shell wrapper +with the real work in a grandchild. Signals are delivered to the whole process +group rather than just the direct child, so the grandchild is stopped too — +including the common case where the wrapper dies on the first signal and the +grandchild is reparented to init. + +The one exception is interactive mode, where the command shares your terminal +and is spawned into the caller's process group so that CTRL+C keeps reaching it. +There `kill()` signals the direct child alone — but CTRL+C from the terminal +already reaches the whole group anyway. + +There is a limit to this. If the shell itself exits and leaves a background +worker behind, the command is finished as far as the runner is concerned, and +Node and Bun have already reaped the shell — which frees its pid, and with it +the group id, for reuse. A later `kill()` therefore signals nothing rather than +risk signalling an unrelated process group, and the orphaned worker keeps +running, exactly as it would if you had started it from your own shell. Keep the +worker in the foreground (`... & wait`, or no `&` at all) if you want `kill()` +to reach it. + +#### Rust parity + +The Rust crate exposes the same model with `kill_signal` / `kill_with(signal)` / +`kill_grace_ms`, the same 100ms default, and the same exit codes. See +[the Rust signal documentation](../rust/README.md#signals). + ### Advanced Signal Behavior ```javascript diff --git a/js/examples/signals-graceful-shutdown.mjs b/js/examples/signals-graceful-shutdown.mjs new file mode 100644 index 00000000..a3f355dd --- /dev/null +++ b/js/examples/signals-graceful-shutdown.mjs @@ -0,0 +1,95 @@ +#!/usr/bin/env node +// Sending signals to a running command (issue #15). +// +// Run it: node js/examples/signals-graceful-shutdown.mjs +// +// The worker below traps SIGTERM and SIGINT the way a real service does: it +// gets a chance to flush state and release resources before exiting. Each +// scenario prints whether that cleanup actually ran, which is the difference +// between a graceful stop and a process that was simply destroyed. +import { $ } from '../src/$.mjs'; + +// A stand-in for a service that must clean up before it stops. +const worker = ` + trap 'echo "[worker] SIGTERM received, flushing state"; exit 0' TERM + trap 'echo "[worker] SIGINT received, flushing state"; exit 0' INT + echo "[worker] started" + while true; do sleep 0.1; done +`; + +// A worker that refuses to stop, to show the SIGKILL escalation. +const stubbornWorker = ` + trap '' TERM INT + echo "[worker] started, ignoring TERM and INT" + while true; do sleep 0.1; done +`; + +const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + +async function scenario(title, command, options, stop) { + console.log(`\n=== ${title} ===`); + const cmd = $({ mirror: true, ...options })`sh -c ${command}`; + cmd.start(); + await sleep(300); // let the worker install its traps + stop(cmd); + const result = await cmd; + console.log(`exit code: ${result.code}`); +} + +// 1. The default: SIGTERM, with a grace period so the worker can clean up. +await scenario('Default kill() sends SIGTERM', worker, {}, (cmd) => cmd.kill()); + +// 2. SIGINT is exactly what CTRL+C sends, delivered programmatically. +await scenario('kill("SIGINT") — the signal CTRL+C sends', worker, {}, (cmd) => + cmd.kill('SIGINT') +); + +// 3. killSignal makes SIGINT the default for kill(), break, and AbortSignal. +await scenario( + 'killSignal option configures the default', + worker, + { killSignal: 'SIGINT' }, + (cmd) => cmd.kill() +); + +// 4. A slow shutdown needs a larger window than the 100ms default. +await scenario( + 'killGrace gives a slow shutdown more room', + worker, + { killGrace: 2000 }, + (cmd) => cmd.kill() +); + +// 5. A process that ignores the signal is still guaranteed to terminate: +// the grace period expires and SIGKILL follows. Note that no cleanup +// message appears — there was no cleanup to run. +await scenario( + 'SIGKILL escalation stops a process that ignores the signal', + stubbornWorker, + { killGrace: 200 }, + (cmd) => cmd.kill() +); + +// 6. killGrace: 0 opts out of graceful shutdown entirely. The worker traps +// SIGTERM, but is destroyed before the handler can run — so no cleanup +// message is printed, even though the exit code is still 143. +await scenario( + 'killGrace: 0 escalates immediately, skipping cleanup', + worker, + { killGrace: 0 }, + (cmd) => cmd.kill() +); + +// 7. An AbortSignal stops the command with the configured killSignal. +console.log('\n=== AbortController stops the command ==='); +const controller = new AbortController(); +const running = $({ + mirror: true, + signal: controller.signal, + killSignal: 'SIGTERM', +})`sh -c ${worker}`; +setTimeout(() => controller.abort(), 300); +console.log(`exit code: ${(await running).code}`); + +console.log('\nExit codes follow the 128 + signal convention:'); +console.log(' SIGINT (2) => 130, SIGTERM (15) => 143, SIGKILL (9) => 137'); diff --git a/js/src/$.process-runner-base.mjs b/js/src/$.process-runner-base.mjs index a8c5718f..548379f8 100644 --- a/js/src/$.process-runner-base.mjs +++ b/js/src/$.process-runner-base.mjs @@ -216,6 +216,7 @@ class ProcessRunner extends StreamEmitter { interactive: false, shellOperators: true, killSignal: 'SIGTERM', + killGrace: 100, ...options, }; diff --git a/js/src/$.process-runner-stream-kill.mjs b/js/src/$.process-runner-stream-kill.mjs index 4d3201c1..0e951e52 100644 --- a/js/src/$.process-runner-stream-kill.mjs +++ b/js/src/$.process-runner-stream-kill.mjs @@ -7,6 +7,12 @@ import { createResult } from './$.result.mjs'; const isBun = typeof globalThis.Bun !== 'undefined'; +/** + * Default milliseconds a child is given to handle the kill signal before + * SIGKILL follows. Mirrors the Rust `kill_grace_ms` default. + */ +const DEFAULT_KILL_GRACE = 100; + /** * Send a signal to a process and its group * @param {number} pid - Process ID @@ -46,51 +52,131 @@ function sendSignalToProcess(pid, sig, runtime) { return operations; } +/** + * Check whether a process still exists, without signalling it. + * @param {number} pid - Process ID + * @returns {boolean} True when the process is still alive + */ +function processIsAlive(pid) { + try { + // Signal 0 performs the permission and existence checks without delivery. + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +/** + * Is anything from the original command still running? + * + * The check must cover the same target the signal was delivered to: the direct + * child *and* its process group. A shell wrapper frequently dies on the first + * signal while the grandchild doing the real work survives and is reparented to + * init; looking only at the direct child would report "already gone" and skip + * the escalation, leaving that grandchild running forever. + * + * @param {number} pid - Process ID of the direct child / group leader + * @returns {boolean} True when the process or any group member is still alive + */ +function processTreeIsAlive(pid) { + return processIsAlive(pid) || processIsAlive(-pid); +} + +/** + * Schedule the forceful SIGKILL escalation that guarantees termination. + * + * The escalation is deliberately deferred: sending SIGKILL in the same tick as + * the requested signal means a child that handles SIGTERM (to flush output, + * remove a lock file, stop its own children) is destroyed before its handler + * can run, so a "graceful" stop was never actually graceful. + * + * @param {number} pid - Process ID + * @param {number} graceMilliseconds - Time to wait before SIGKILL + * @param {string} runtime - Runtime identifier for logging + */ +function scheduleForcefulEscalation(pid, graceMilliseconds, runtime) { + const timer = setTimeout(() => { + if (!processTreeIsAlive(pid)) { + trace( + 'ProcessRunner', + () => `Process ${pid} exited within the grace period; no SIGKILL needed` + ); + return; + } + trace( + 'ProcessRunner', + () => `Grace period elapsed, escalating to SIGKILL for process ${pid}` + ); + sendSignalToProcess(pid, 'SIGKILL', runtime); + }, graceMilliseconds); + + // The escalation must never be the reason the process stays alive: an + // unref'd timer lets the runtime exit as soon as everything else is done. + timer.unref?.(); +} + /** * Kill a child process with escalating signals * @param {object} child - Child process object * @param {string} [signal] - Signal to send first (default 'SIGTERM') + * @param {number} [graceMilliseconds] - Time the child is given to handle the + * signal before SIGKILL follows (default 100) */ -function killChildProcess(child, signal = 'SIGTERM') { +function killChildProcess( + child, + signal = 'SIGTERM', + graceMilliseconds = DEFAULT_KILL_GRACE +) { if (!child || !child.pid) { return; } const runtime = isBun ? 'Bun' : 'Node'; + const pid = child.pid; trace( 'ProcessRunner', () => - `Killing ${runtime} process | ${JSON.stringify({ pid: child.pid, signal }, null, 2)}` + `Killing ${runtime} process | ${JSON.stringify({ pid, signal, graceMilliseconds }, null, 2)}` ); - // Send the configured signal first, then escalate to SIGKILL to guarantee - // termination even if the process ignores or handles the first signal. - // When the configured signal already is SIGKILL we skip the redundant second - // delivery. - const killOperations = []; - killOperations.push(...sendSignalToProcess(child.pid, signal, runtime)); - if (signal !== 'SIGKILL') { - killOperations.push(...sendSignalToProcess(child.pid, 'SIGKILL', runtime)); - } + // SIGKILL cannot be handled, so there is nothing to wait for. A zero grace + // period means the child is given no opportunity to handle the signal + // either, so the requested signal is not delivered at all: anything done + // between it and SIGKILL is a window the child can be scheduled in, which + // would make "no grace" a race the child can win rather than a guarantee. + // The reported exit code still comes from the signal that was requested. + const forceful = signal === 'SIGKILL' || !(graceMilliseconds > 0); + const killOperations = sendSignalToProcess( + pid, + forceful ? 'SIGKILL' : signal, + runtime + ); trace( 'ProcessRunner', () => `${runtime} kill operations attempted: ${killOperations.join(', ')}` ); - if (isBun) { - try { - child.kill(); - trace( - 'ProcessRunner', - () => `Called child.kill() for Bun process ${child.pid}` - ); - } catch (err) { - trace( - 'ProcessRunner', - () => `Error calling child.kill(): ${err.message}` - ); + if (forceful) { + if (isBun) { + try { + child.kill(); + trace( + 'ProcessRunner', + () => `Called child.kill() for Bun process ${pid}` + ); + } catch (err) { + trace( + 'ProcessRunner', + () => `Error calling child.kill(): ${err.message}` + ); + } } + } else { + // Otherwise escalate to SIGKILL once the grace period has passed, so + // termination is still guaranteed for a process that ignores the signal. + scheduleForcefulEscalation(pid, graceMilliseconds, runtime); } child.removeAllListeners?.(); @@ -236,7 +322,7 @@ function killRunner(runner, signal) { if (runner.child && !runner.finished) { trace('ProcessRunner', () => `Killing child process ${runner.child.pid}`); try { - killChildProcess(runner.child, signal); + killChildProcess(runner.child, signal, runner.options?.killGrace); runner.child = null; } catch (err) { trace('ProcessRunner', () => `Error killing process: ${err.message}`); diff --git a/js/tests/signal-handling.test.mjs b/js/tests/signal-handling.test.mjs new file mode 100644 index 00000000..5691d266 --- /dev/null +++ b/js/tests/signal-handling.test.mjs @@ -0,0 +1,213 @@ +/** + * Signal handling tests (issue #15). + * + * These cover the documented contract for stopping a running command: which + * signal is delivered, that the child gets a chance to handle it, that a + * process ignoring it is still terminated, and which exit code is reported. + * + * The Rust counterpart is `rust/tests/signals.rs`; both suites assert the same + * behavior so the two implementations stay in parity. + */ +import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; +import { beforeTestCleanup, afterTestCleanup } from './test-cleanup.mjs'; +import { $ } from '../src/$.mjs'; +import { mkdtempSync, rmSync, readFileSync, statSync } from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; + +// Signals are a Unix concept; Windows terminates processes by other means. +const isWindows = process.platform === 'win32'; + +const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + +describe.skipIf(isWindows)('Signal handling', () => { + let workDir; + + beforeEach(async () => { + await beforeTestCleanup(); + workDir = mkdtempSync(join(tmpdir(), 'cs-signals-')); + }); + + afterEach(async () => { + rmSync(workDir, { recursive: true, force: true }); + await afterTestCleanup(); + }); + + /** + * A command that traps a signal, records that its handler ran, and exits. + * + * Writing to a marker file is what distinguishes "the child handled the + * signal" from "the child was destroyed before it could": an exit code alone + * cannot tell the two apart, because the reported code is derived from the + * signal that was requested either way. + */ + const gracefulChild = (marker, signals = 'TERM INT') => + `trap 'echo handled >> ${marker}; exit 0' ${signals}; ` + + `echo ready; while true; do sleep 0.05; done`; + + const handlerRan = (marker) => { + try { + return readFileSync(marker, 'utf8').includes('handled'); + } catch { + return false; + } + }; + + const fileSize = (path) => { + try { + return statSync(path).size; + } catch { + return 0; + } + }; + + // Start a command, wait for it to be running, then stop it. + const startAndKill = async (command, options, kill) => { + const cmd = $({ mirror: false, ...options })`sh -c ${command}`; + cmd.start(); + await sleep(300); + kill(cmd); + const result = await cmd; + await sleep(300); + return result; + }; + + describe('graceful termination', () => { + it('lets the child run its SIGTERM handler before exiting', async () => { + const marker = join(workDir, 'marker'); + + const result = await startAndKill(gracefulChild(marker), {}, (cmd) => + cmd.kill() + ); + + expect(handlerRan(marker)).toBe(true); + expect(result.code).toBe(143); // 128 + SIGTERM(15) + }); + + it('delivers an explicit per-call signal override', async () => { + const marker = join(workDir, 'marker'); + + // SIGINT is the signal CTRL+C sends. + const result = await startAndKill(gracefulChild(marker), {}, (cmd) => + cmd.kill('SIGINT') + ); + + expect(handlerRan(marker)).toBe(true); + expect(result.code).toBe(130); // 128 + SIGINT(2) + }); + + it('delivers the configured killSignal when kill() takes no argument', async () => { + const marker = join(workDir, 'marker'); + + // Only INT is trapped, so the marker proves SIGINT (not the SIGTERM + // default) was the signal actually delivered. + const result = await startAndKill( + gracefulChild(marker, 'INT'), + { killSignal: 'SIGINT' }, + (cmd) => cmd.kill() + ); + + expect(handlerRan(marker)).toBe(true); + expect(result.code).toBe(130); + }); + }); + + describe('forceful escalation', () => { + it('escalates to SIGKILL when the child ignores the signal', async () => { + const heartbeat = join(workDir, 'heartbeat'); + // Ignores TERM and INT, and appends while it runs. Whether the heartbeat + // keeps growing is the evidence that the process is still executing. + const command = + `trap '' TERM INT; echo ready; ` + + `while true; do echo tick >> ${heartbeat}; sleep 0.05; done`; + + const cmd = $({ mirror: false, killGrace: 50 })`sh -c ${command}`; + cmd.start(); + await sleep(300); + + expect(fileSize(heartbeat)).toBeGreaterThan(0); + + cmd.kill(); + // Wait out the grace period plus the SIGKILL escalation. + await sleep(500); + const afterKill = fileSize(heartbeat); + // If the process were still alive it would keep appending here. + await sleep(500); + + expect(fileSize(heartbeat)).toBe(afterKill); + }); + + it('killGrace: 0 escalates immediately without waiting', async () => { + const marker = join(workDir, 'marker'); + + const result = await startAndKill( + gracefulChild(marker), + { killGrace: 0 }, + (cmd) => cmd.kill() + ); + + // The reported code still reflects the requested signal, even though the + // process was actually stopped by the SIGKILL escalation. + expect(result.code).toBe(143); + // With no grace period the child never gets to run its handler. + expect(handlerRan(marker)).toBe(false); + }); + }); + + describe('process group', () => { + it('reaches grandchildren, not just the direct child', async () => { + const heartbeat = join(workDir, 'heartbeat'); + // The real work runs in a grandchild behind a shell that waits, so + // signalling only the direct child would leave the worker running. + // Delivering to the process group is what reaches it. + const command = + `sh -c 'while true; do echo tick >> ${heartbeat}; sleep 0.05; done' & ` + + `echo ready; wait`; + + const cmd = $({ mirror: false, killGrace: 50 })`sh -c ${command}`; + cmd.start(); + await sleep(400); + + expect(fileSize(heartbeat)).toBeGreaterThan(0); + + cmd.kill(); + // Past the grace period, so the escalation has been delivered too. + await sleep(400); + const afterKill = fileSize(heartbeat); + // A surviving grandchild would keep appending here. + await sleep(400); + + expect(fileSize(heartbeat)).toBe(afterKill); + }); + + // There is deliberately no counterpart to the Rust + // `process_runner_kill_reaches_a_grandchild_whose_parent_already_exited` + // test here: once the shell exits, Node and Bun reap it and the runner is + // finished, so its pid - and with it the group id - can be reused by an + // unrelated process. Signalling that group would be worse than leaving the + // grandchild running. Rust can make the guarantee because its runner keeps + // the unreaped child, which holds the group id reserved. + }); + + describe('exit codes', () => { + it('follows the 128 + signal convention', async () => { + // A child that ignores nothing, stopped with a range of signals. + const cases = [ + ['SIGHUP', 129], + ['SIGINT', 130], + ['SIGQUIT', 131], + ['SIGKILL', 137], + ['SIGTERM', 143], + ]; + + for (const [signal, expected] of cases) { + const result = await startAndKill( + 'echo ready; while true; do sleep 0.05; done', + {}, + (cmd) => cmd.kill(signal) + ); + expect(result.code).toBe(expected); + } + }); + }); +}); diff --git a/rust/README.md b/rust/README.md index a9dc1466..28c8e151 100644 --- a/rust/README.md +++ b/rust/README.md @@ -137,6 +137,152 @@ The exact-argv form bypasses `/bin/sh -c` and `cmd.exe /c`, so it does not require shell-specific quoting. It also accepts OS-native executable and argument values such as `PathBuf` and `OsString`. +## Signals + +`kill()` stops a running command. It defaults to `SIGTERM` and works the same way +for both runners, matching the JavaScript implementation +([JS signal documentation](../js/README.md#sending-signals-to-a-running-command)). + +### What `kill()` actually does + +Stopping a process is not a single signal. Every kill runs the same four steps: + +1. The requested signal is delivered to the child **and its process group**, so a + grandchild behind a shell wrapper is reached too. +2. The child is given a grace period (`kill_grace_ms`, default `100`) to run its + own signal handler and exit on its own terms. +3. If it is still alive when the grace period expires, `SIGKILL` follows, so a + process that ignores the signal is still guaranteed to terminate. +4. The reported exit code is the conventional `128 + signal` value. + +Step 2 is what makes a shutdown _graceful_: without it, a child that traps +SIGTERM to flush output, release a lock, or stop its own workers is destroyed +before its handler can run. + +### Grandchildren and process groups + +Commands run through a shell, so the real work is usually a grandchild of the +`sh` that was spawned. Both runners therefore start the child in its own process +group and signal the group, not just the direct child — including the common +case where the wrapper dies on the first signal and the grandchild is reparented +to init. + +The one exception is a command that shares your terminal: when `interactive` is +set, or when stdin is inherited and is a tty, `ProcessRunner` leaves the child in +the caller's process group. It has to, because the terminal delivers CTRL+C to +its foreground group only, and a background child that read from the terminal +would be stopped with SIGTTIN. For those commands the signal reaches the direct +child alone — and CTRL+C from the terminal already reaches the whole group +anyway. Set `stdin` to `StdinOption::Null` or `StdinOption::Pipe` if you need +group delivery from `kill()`. + +Group membership is recorded when the child is spawned rather than looked up +when it is signalled, because by then the group leader is usually dead: the +first signal kills the `sh` wrapper, and the escalation follows a grace period +later. macOS refuses to answer `getpgid` for a process in that state, which +would silently skip the delivery and leave the grandchild running. + +This is one place where `ProcessRunner` can promise slightly more than the +JavaScript implementation. Because it holds the child until you await it, the +group id stays reserved even after the shell exits, so `kill()` still reaches a +worker the shell left behind. Node and Bun reap the shell immediately, so +JavaScript cannot address that group safely and leaves such a worker running. + +### ProcessRunner + +`kill()` sends the configured signal; `kill_with(signal)` overrides it for a +single call: + +```rust,no_run +use command_stream::{ProcessRunner, RunOptions}; + +#[tokio::main] +async fn main() -> command_stream::Result<()> { + let mut runner = ProcessRunner::new( + "sh -c 'trap \"echo cleaning up; exit 0\" TERM; while true; do sleep 1; done'", + RunOptions { + // The signal an argument-less kill() delivers (default SIGTERM). + kill_signal: "SIGTERM".to_string(), + // Time the child gets to handle it before SIGKILL (default 100ms). + kill_grace_ms: 100, + ..Default::default() + }, + ); + + runner.start().await?; + runner.kill()?; // the trap runs, prints "cleaning up", and exits + runner.kill_with("SIGINT")?; // explicit per-call override + + Ok(()) +} +``` + +### StreamingRunner + +`stream.kill()` and `stream.kill_with(signal)` stop the process from inside the +loop; dropping the stream (e.g. `break`) stops it too: + +```rust,no_run +use command_stream::{OutputChunk, StreamingRunner}; + +#[tokio::main] +async fn main() { + let mut stream = StreamingRunner::new("sh -c 'while true; do echo tick; sleep 0.1; done'") + .kill_signal("SIGINT") // default for kill() and for dropping the stream + .kill_grace_ms(100) // grace before the SIGKILL escalation + .stream(); + + while let Some(chunk) = stream.next().await { + match chunk { + OutputChunk::Stdout(_) => stream.kill(), // sends SIGINT + OutputChunk::Exit(code) => println!("exit: {code}"), // 130 + OutputChunk::Stderr(_) => {} + } + } +} +``` + +### Tuning the grace period + +`kill_grace_ms` is the number of milliseconds between the requested signal and +the SIGKILL escalation. Set it to `0` to escalate immediately, with no chance to +clean up: the requested signal is then not delivered at all, only `SIGKILL`. +Delivering it first and then killing would leave a window the child can be +scheduled in, which makes "no grace" a race rather than a guarantee. The +reported exit code still reflects the signal you requested. + +`SIGKILL` is never delayed: it cannot be caught, so `kill_with("SIGKILL")` skips +the grace period regardless of the configured value. + +### Signal exit codes + +A process stopped by a signal reports `128 + signal`, the same convention POSIX +shells use. `signal_number` and `signal_exit_code` expose the mapping: + +```rust +use command_stream::{signal_exit_code, signal_number}; + +assert_eq!(signal_number("SIGINT"), 2); +assert_eq!(signal_exit_code("SIGINT"), 130); // CTRL+C +assert_eq!(signal_exit_code("SIGTERM"), 143); +assert_eq!(signal_exit_code("SIGKILL"), 137); +``` + +| Signal | Number | Exit code | Typical meaning | +| --------- | ------ | --------- | ------------------------------------ | +| `SIGHUP` | 1 | `129` | Terminal closed / reload config | +| `SIGINT` | 2 | `130` | CTRL+C | +| `SIGQUIT` | 3 | `131` | Quit from keyboard | +| `SIGKILL` | 9 | `137` | Forced termination, cannot be caught | +| `SIGUSR1` | 10 | `138` | Application-defined | +| `SIGUSR2` | 12 | `140` | Application-defined | +| `SIGTERM` | 15 | `143` | Polite request to stop (the default) | + +The code reflects the signal **you requested**, even when the SIGKILL escalation +is what ultimately stopped the process. Unknown signal names fall back to +`SIGTERM`. Signals are a Unix concept; on Windows the escalation terminates the +process directly. + ## Multiline Text and Exact Output The command macros treat an interpolated multiline string as one literal diff --git a/rust/changelog.d/20260916_050000_signal_handling.md b/rust/changelog.d/20260916_050000_signal_handling.md new file mode 100644 index 00000000..a1def461 --- /dev/null +++ b/rust/changelog.d/20260916_050000_signal_handling.md @@ -0,0 +1,32 @@ +--- +bump: minor +--- + +### Added + +- `ProcessRunner::kill_with(signal)` to stop a running command with an explicit + signal, matching `OutputStream::kill_with` and the JavaScript `kill(signal)`. +- A `## Signals` section in the README covering the delivery model, the + `kill_signal` / `kill_grace_ms` options and the `128 + signal` exit codes, + plus a runnable `examples/signals_graceful_shutdown.rs`. + +### Fixed + +- `ProcessRunner::kill()` sent `SIGKILL` unconditionally and ignored the + configured `kill_signal`, so a child that trapped `SIGTERM` was destroyed + before its handler could run. It now delivers the configured signal to the + process group, waits `kill_grace_ms`, and escalates to `SIGKILL` only if the + process is still running. +- `ProcessRunner` did not start its child in its own process group, so killing + it never reached grandchildren: the worker behind a `sh -c` wrapper kept + running. The child now leads its own group, as `StreamingRunner` already did. + A command sharing the caller's terminal is deliberately left in the caller's + group so that CTRL+C keeps reaching it. +- `kill_grace_ms: 0` still let the child run its handler: awaiting a zero-length + timeout yields to the runtime, and that gap was enough for a shell to run its + trap. `SIGKILL` now follows in the same step as the signal. +- Killing a command left grandchildren running on macOS. Group membership was + looked up with `getpgid` at signal time, by which point the `sh` wrapper had + usually exited; macOS reports `ESRCH` for a process in that state, so the + group - including the still-running worker - was never signalled. Whether the + child leads its own group is now recorded when it is spawned. diff --git a/rust/examples/signals_graceful_shutdown.rs b/rust/examples/signals_graceful_shutdown.rs new file mode 100644 index 00000000..5ba06f16 --- /dev/null +++ b/rust/examples/signals_graceful_shutdown.rs @@ -0,0 +1,115 @@ +//! Sending signals to a running command (issue #15). +//! +//! Run it: `cargo run --example signals_graceful_shutdown` +//! +//! The worker below traps SIGTERM and SIGINT the way a real service does: it +//! gets a chance to flush state and release resources before exiting. Each +//! scenario prints whether that cleanup actually ran, which is the difference +//! between a graceful stop and a process that was simply destroyed. +//! +//! `StreamingRunner` is used throughout because it surfaces the child's output +//! while the process is still running, which is what makes the cleanup message +//! visible. `ProcessRunner` takes the same `kill_signal` / `kill_grace_ms` +//! options and has the same `kill()` / `kill_with()` pair, but it pumps output +//! inside `run()`, so a start-then-kill example there would print nothing. +use command_stream::{OutputChunk, StreamingRunner}; + +/// A stand-in for a service that must clean up before it stops. +const WORKER: &str = r#" + trap 'echo "[worker] SIGTERM received, flushing state"; exit 0' TERM + trap 'echo "[worker] SIGINT received, flushing state"; exit 0' INT + echo "[worker] started" + while true; do sleep 0.1; done +"#; + +/// A worker that refuses to stop, to show the SIGKILL escalation. +const STUBBORN_WORKER: &str = r#" + trap '' TERM INT + echo "[worker] started, ignoring TERM and INT" + while true; do sleep 0.1; done +"#; + +/// Stream a command until it produces output, stop it, and report the exit code. +async fn scenario(title: &str, runner: StreamingRunner, explicit_signal: Option<&str>) { + println!("\n=== {title} ==="); + let mut stream = runner.stream(); + + let mut stopped = false; + while let Some(chunk) = stream.next().await { + match chunk { + OutputChunk::Stdout(data) => { + print!("{}", String::from_utf8_lossy(&data)); + // Stop once the worker has installed its traps and said so. + if !stopped { + stopped = true; + match explicit_signal { + // An explicit per-call override. + Some(signal) => stream.kill_with(signal), + // The configured kill_signal (SIGTERM unless changed). + None => stream.kill(), + } + } + } + OutputChunk::Stderr(data) => eprint!("{}", String::from_utf8_lossy(&data)), + OutputChunk::Exit(code) => println!("exit code: {code}"), + } + } +} + +#[tokio::main] +async fn main() { + // 1. The default: SIGTERM, with a grace period so the worker can clean up. + scenario( + "Default kill() sends SIGTERM", + StreamingRunner::new(WORKER), + None, + ) + .await; + + // 2. SIGINT is exactly what CTRL+C sends, delivered programmatically. + scenario( + "kill_with(\"SIGINT\") - the signal CTRL+C sends", + StreamingRunner::new(WORKER), + Some("SIGINT"), + ) + .await; + + // 3. kill_signal makes SIGINT the default for an argument-less kill(). + scenario( + "kill_signal option configures the default", + StreamingRunner::new(WORKER).kill_signal("SIGINT"), + None, + ) + .await; + + // 4. A slow shutdown needs a larger window than the 100ms default. + scenario( + "kill_grace_ms gives a slow shutdown more room", + StreamingRunner::new(WORKER).kill_grace_ms(2000), + None, + ) + .await; + + // 5. A process that ignores the signal is still guaranteed to terminate: + // the grace period expires and SIGKILL follows. No cleanup message + // appears here - there was no cleanup to run. + scenario( + "SIGKILL escalation stops a process that ignores the signal", + StreamingRunner::new(STUBBORN_WORKER).kill_grace_ms(200), + None, + ) + .await; + + // 6. kill_grace_ms(0) opts out of graceful shutdown entirely. The worker + // traps SIGTERM, but is destroyed before the handler can run - so no + // cleanup message is printed, even though the exit code is still 143. + scenario( + "kill_grace_ms(0) escalates immediately, skipping cleanup", + StreamingRunner::new(WORKER).kill_grace_ms(0), + None, + ) + .await; + + println!("\nExit codes follow the 128 + signal convention:"); + println!(" SIGINT (2) => 130, SIGTERM (15) => 143, SIGKILL (9) => 137"); +} diff --git a/rust/src/lib.rs b/rust/src/lib.rs index be7b50e9..367e7b3d 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -68,6 +68,7 @@ pub mod events; pub mod macros; pub mod pipeline; pub mod quote; +pub mod signal; pub mod state; pub mod stream; pub mod terminal; @@ -98,6 +99,7 @@ pub use quote::{ is_pre_quoted_passthrough_enabled, is_quote_context_enabled, quote, quote_for_context, scan_quote_context, QuoteContext, }; +pub use signal::{signal_exit_code, signal_number, DEFAULT_KILL_GRACE_MS, DEFAULT_KILL_SIGNAL}; pub use state::{ get_shell_settings, global_state, reset_global_state, set_shell_option, unset_shell_option, GlobalState, ShellSettings, @@ -283,6 +285,18 @@ pub struct RunOptions { pub shell_operators: bool, /// Enable tracing for this command pub trace: bool, + /// Signal used to stop the process when it is killed without an explicit + /// signal, i.e. [`ProcessRunner::kill`] (default `SIGTERM`). + /// + /// Mirrors the JavaScript `killSignal` option. An explicit + /// [`ProcessRunner::kill_with`] argument always overrides it. + pub kill_signal: String, + /// Milliseconds the child is given to handle the kill signal before + /// `SIGKILL` is sent (default 100). + /// + /// Mirrors the JavaScript `killGrace` option. This is the window in which a + /// child running its own signal handler can shut down on its own terms. + pub kill_grace_ms: u64, } impl Default for RunOptions { @@ -296,6 +310,8 @@ impl Default for RunOptions { interactive: false, shell_operators: true, trace: true, + kill_signal: signal::DEFAULT_KILL_SIGNAL.to_string(), + kill_grace_ms: signal::DEFAULT_KILL_GRACE_MS, } } } @@ -322,6 +338,12 @@ pub struct ProcessRunner { started: bool, finished: bool, cancelled: bool, + /// Whether the child was spawned into a process group of its own, and so + /// can be signalled as a group. Recorded at spawn time because it cannot be + /// discovered later: by the time the group is signalled the leader is + /// usually a zombie, which macOS refuses to answer `getpgid` for. + #[cfg(unix)] + own_process_group: bool, output_tx: Option>, // Held, never read: dropping the receiver would close the channel, and // streaming virtual commands treat a closed channel as "stop now" (see @@ -343,11 +365,28 @@ impl ProcessRunner { started: false, finished: false, cancelled: false, + #[cfg(unix)] + own_process_group: false, output_tx: Some(tx), output_rx: Some(rx), } } + /// Whether the child will read from the caller's terminal. + /// + /// Only an *inherited* stdin that is actually a tty counts: a pipe, a null + /// stdin, or inherited stdin that has been redirected to a file carries no + /// terminal, and neither does output-only inheritance. This is the one case + /// where the child must stay in the caller's process group. + #[cfg(unix)] + fn shares_the_terminal(&self) -> bool { + use std::io::IsTerminal; + + self.options.interactive + || (matches!(self.options.stdin, StdinOption::Inherit) + && std::io::stdin().is_terminal()) + } + /// Start the process pub async fn start(&mut self) -> Result<()> { if self.started { @@ -431,6 +470,23 @@ impl ProcessRunner { } } + // Run the child in its own process group so that killing it can signal + // the whole group (parent + grandchildren), matching `StreamingRunner` + // and the JavaScript implementation's `detached` spawn. + // + // A child that shares the terminal is deliberately left in the caller's + // group. The tty delivers CTRL+C to its foreground group only, so + // moving such a child out would both hide CTRL+C from it and stop it + // with SIGTTIN the moment it read from the terminal. JavaScript draws + // the same line, spawning interactive commands without `detached`. + #[cfg(unix)] + { + self.own_process_group = !self.shares_the_terminal(); + if self.own_process_group { + cmd.process_group(0); + } + } + // Spawn the process let child = cmd.spawn()?; self.child = Some(child); @@ -556,13 +612,104 @@ impl ProcessRunner { } } - /// Kill the process + /// Stop the process using the configured kill signal + /// ([`RunOptions::kill_signal`], default `SIGTERM`). + /// + /// Mirrors the JavaScript `kill()` with no argument. pub fn kill(&mut self) -> Result<()> { + let signal = self.options.kill_signal.clone(); + self.kill_with(&signal) + } + + /// Stop the process using an explicit signal, overriding + /// [`RunOptions::kill_signal`] for this call. + /// + /// Mirrors the JavaScript `kill(signal)`. The signal is delivered to the + /// child and its process group, so grandchildren behind a `sh -c` wrapper + /// are stopped too - except for a child sharing the caller's terminal, + /// which stays in the caller's group so CTRL+C keeps reaching it. The child + /// then has [`RunOptions::kill_grace_ms`] to run its own handler before + /// `SIGKILL` follows, so a process that ignores the signal still + /// terminates. + /// + /// ```no_run + /// use command_stream::{ProcessRunner, RunOptions}; + /// + /// # #[tokio::main] + /// # async fn main() -> command_stream::Result<()> { + /// let mut runner = ProcessRunner::new("sleep 30", RunOptions::default()); + /// runner.start().await?; + /// runner.kill_with("SIGINT")?; // the CTRL+C signal + /// # Ok(()) + /// # } + /// ``` + pub fn kill_with(&mut self, signal: &str) -> Result<()> { self.cancelled = true; - if let Some(ref mut child) = self.child { + utils::trace_lazy("ProcessRunner", || format!("kill | signal={signal}")); + + let Some(child) = self.child.as_mut() else { + return Ok(()); + }; + + // Windows has no signals to deliver and no handler for the child to + // run, so there is nothing to grant a grace period to: the forceful + // stop is the only way to end the process. + // The `#[cfg(unix)]` block below is stripped on Windows, which leaves + // this one as the function's tail expression - hence no `return`. + #[cfg(not(unix))] + { + let _ = signal; child.start_kill()?; + Ok(()) + } + + // Without a pid the process never spawned (or was already reaped); + // fall back to the forceful stop so `kill()` still terminates it. + #[cfg(unix)] + { + let Some(pid) = child.id() else { + child.start_kill()?; + return Ok(()); + }; + + // `SIGKILL` cannot be handled, so there is nothing to wait for. + // + // A zero grace period means the child is given no opportunity to + // handle the signal either, so the requested signal is not + // delivered at all. Anything done between it and `SIGKILL` - even a + // single syscall - is a window the child can be scheduled in, which + // made "no grace" a race the child occasionally won rather than a + // guarantee. The reported exit code still comes from the signal + // that was requested. + let grace = self.options.kill_grace_ms; + let delivery = if self.own_process_group { + signal::Delivery::ProcessAndGroup + } else { + signal::Delivery::ProcessOnly + }; + if grace == 0 || signal == "SIGKILL" { + signal::send_signal_to_process(pid, "SIGKILL", delivery); + let _ = child.start_kill(); + return Ok(()); + } + + signal::send_signal_to_process(pid, signal, delivery); + + // Otherwise escalate in the background so the child keeps its grace + // period without blocking the caller, which may not be inside an + // await point. + tokio::spawn(async move { + tokio::time::sleep(std::time::Duration::from_millis(grace)).await; + // Best effort: if the child already exited on the first signal + // this delivery simply fails, and the pid has not been reused + // because the `Child` handle above has not reaped it yet. That + // unreaped leader is also what keeps the group id alive, so the + // group delivery still reaches a grandchild that outlived it. + signal::send_signal_to_process(pid, "SIGKILL", delivery); + }); + + Ok(()) } - Ok(()) } /// Check if the process is finished diff --git a/rust/src/signal.rs b/rust/src/signal.rs new file mode 100644 index 00000000..b4a3d348 --- /dev/null +++ b/rust/src/signal.rs @@ -0,0 +1,145 @@ +//! Signal delivery and the signal exit-code convention. +//! +//! Both runners stop processes the same way, so the signal vocabulary lives in +//! one place instead of being duplicated per runner: +//! +//! * [`ProcessRunner::kill`](crate::ProcessRunner::kill) / +//! [`ProcessRunner::kill_with`](crate::ProcessRunner::kill_with) +//! * [`OutputStream::kill`](crate::OutputStream::kill) / +//! [`OutputStream::kill_with`](crate::OutputStream::kill_with) +//! +//! The model mirrors the JavaScript implementation: +//! +//! 1. The requested signal is delivered to the child **and** its process +//! group, so grandchildren spawned by a shell are stopped too. The group +//! is skipped for a child that shares the caller's terminal, which stays +//! in the caller's process group by design so that CTRL+C keeps reaching +//! it. +//! 2. The child is given a grace period ([`DEFAULT_KILL_GRACE_MS`]) to run its +//! own signal handler and exit on its own terms. +//! 3. If it is still alive when the grace period expires, `SIGKILL` follows, +//! so a process that ignores the signal still terminates. +//! 4. The reported exit code is the conventional `128 + signal` value +//! ([`signal_exit_code`]). +//! +//! A grace period of zero collapses steps 1 to 3 into `SIGKILL` alone: any work +//! between the requested signal and the escalation is a window the child can be +//! scheduled in, so delivering it first would make "no grace" a race rather +//! than a guarantee. The exit code still reflects the signal that was asked +//! for. + +/// Default signal used to stop a process when no explicit signal is given. +/// +/// Mirrors the JavaScript `killSignal` default. +pub const DEFAULT_KILL_SIGNAL: &str = "SIGTERM"; + +/// Default grace period (in milliseconds) between the requested signal and the +/// forceful `SIGKILL` escalation. +/// +/// Mirrors the JavaScript `killGrace` default. It is what makes a graceful +/// shutdown possible: without it the child is killed before its own handler +/// gets to run. +pub const DEFAULT_KILL_GRACE_MS: u64 = 100; + +/// Map a signal name to its numeric value. +/// +/// Unknown names fall back to `SIGTERM`, matching the JavaScript +/// implementation's behavior for unrecognized signals. +/// +/// ``` +/// use command_stream::signal::signal_number; +/// +/// assert_eq!(signal_number("SIGINT"), 2); +/// assert_eq!(signal_number("SIGKILL"), 9); +/// assert_eq!(signal_number("SIGTERM"), 15); +/// ``` +pub fn signal_number(signal: &str) -> i32 { + match signal { + "SIGHUP" => 1, + "SIGINT" => 2, + "SIGQUIT" => 3, + "SIGKILL" => 9, + "SIGUSR1" => 10, + "SIGUSR2" => 12, + "SIGTERM" => 15, + _ => 15, + } +} + +/// The exit code reported for a process stopped with `signal`, following the +/// conventional `128 + signal` mapping used by POSIX shells. +/// +/// ``` +/// use command_stream::signal::signal_exit_code; +/// +/// assert_eq!(signal_exit_code("SIGINT"), 130); // CTRL+C +/// assert_eq!(signal_exit_code("SIGTERM"), 143); +/// assert_eq!(signal_exit_code("SIGKILL"), 137); +/// ``` +pub fn signal_exit_code(signal: &str) -> i32 { + 128 + signal_number(signal) +} + +/// Who a signal is delivered to. +/// +/// Only the runner that spawned the child knows which of these applies, so it +/// is stated rather than discovered: a child spawned with `process_group(0)` +/// leads its own group, and a child left in the caller's group does not. +// Windows has neither signals nor process groups, so the distinction only ever +// narrows to `ProcessAndGroup` there and the other variant is genuinely unused. +#[cfg_attr(not(unix), allow(dead_code))] +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub(crate) enum Delivery { + /// The process alone, for a child sharing the caller's process group. + ProcessOnly, + /// The process and the group it leads, which is what reaches grandchildren. + ProcessAndGroup, +} + +/// Send a signal to a process and, when it leads one, its process group. +/// +/// Delivery to the group (negative pid) is what reaches grandchildren, e.g. the +/// real command behind a `sh -c` wrapper. It must not be attempted for a child +/// left in the caller's group, where `-pid` would name a group we do not own - +/// at best a non-existent one, at worst an unrelated group that reused the +/// number. +/// +/// Group leadership is passed in rather than looked up with `getpgid` because +/// the leader is usually dead by the time the group is signalled: the first +/// signal kills the `sh` wrapper, and the escalation follows a grace period +/// later. Linux answers `getpgid` for a zombie, but macOS does not - its +/// `proc_find` skips zombies, so the lookup failed with `ESRCH` and the group, +/// including the still-running grandchild, was never signalled at all. +/// +/// Both deliveries are best effort: the process may already have exited, which +/// is not an error for a caller that only wants it stopped. +#[cfg(unix)] +pub(crate) fn send_signal_to_process(pid: u32, signal: &str, delivery: Delivery) { + use nix::sys::signal::{kill, Signal}; + use nix::unistd::Pid; + + let sig = match signal { + "SIGHUP" => Signal::SIGHUP, + "SIGINT" => Signal::SIGINT, + "SIGQUIT" => Signal::SIGQUIT, + "SIGKILL" => Signal::SIGKILL, + "SIGUSR1" => Signal::SIGUSR1, + "SIGUSR2" => Signal::SIGUSR2, + "SIGTERM" => Signal::SIGTERM, + _ => Signal::SIGTERM, + }; + + // Signal the whole process group (negative pid) first, so grandchildren are + // reached even if the group leader dies on the signal we send it next. + if delivery == Delivery::ProcessAndGroup { + let _ = kill(Pid::from_raw(-(pid as i32)), sig); + } + // Signal the process itself. + let _ = kill(Pid::from_raw(pid as i32), sig); +} + +/// On non-Unix platforms there is no signal delivery, and no process groups to +/// deliver to; the forceful `start_kill()` escalation in the caller handles +/// termination. +#[cfg(not(unix))] +pub(crate) fn send_signal_to_process(_pid: u32, _signal: &str, _delivery: Delivery) {} diff --git a/rust/src/stream.rs b/rust/src/stream.rs index 7c01bcad..f72c27e0 100644 --- a/rust/src/stream.rs +++ b/rust/src/stream.rs @@ -64,6 +64,9 @@ use tokio::process::Command; use tokio::sync::mpsc; use tokio::task::JoinHandle; +use crate::signal::{ + send_signal_to_process, signal_exit_code, Delivery, DEFAULT_KILL_GRACE_MS, DEFAULT_KILL_SIGNAL, +}; use crate::trace::trace_lazy; use crate::{CommandResult, Result}; @@ -72,9 +75,6 @@ use crate::{CommandResult, Result}; /// the JavaScript `exitPumpGrace` default. const DEFAULT_EXIT_PUMP_GRACE_MS: u64 = 100; -/// Default signal used to stop a process when no explicit signal is given. -const DEFAULT_KILL_SIGNAL: &str = "SIGTERM"; - /// A chunk of output from a streaming process #[derive(Debug, Clone)] pub enum OutputChunk { @@ -93,6 +93,7 @@ pub struct StreamingRunner { env: Option>, stdin_content: Option, kill_signal: String, + kill_grace_ms: u64, exit_pump_grace_ms: u64, } @@ -136,6 +137,7 @@ impl StreamingRunner { env: None, stdin_content: None, kill_signal: DEFAULT_KILL_SIGNAL.to_string(), + kill_grace_ms: DEFAULT_KILL_GRACE_MS, exit_pump_grace_ms: DEFAULT_EXIT_PUMP_GRACE_MS, } } @@ -169,6 +171,17 @@ impl StreamingRunner { self } + /// Configure how long (in milliseconds) the child is given to handle the + /// kill signal before `SIGKILL` is sent. Mirrors the JavaScript `killGrace` + /// option (default 100ms). + /// + /// This is the window in which a child running its own `SIGTERM` handler + /// can shut down on its own terms. Set it to `0` to escalate immediately. + pub fn kill_grace_ms(mut self, ms: u64) -> Self { + self.kill_grace_ms = ms; + self + } + /// Configure the grace period (in milliseconds) to keep draining the stdio /// pipes after the process exits before aborting lingering readers. Mirrors /// the JavaScript `exitPumpGrace` option (default 100ms). @@ -187,7 +200,10 @@ impl StreamingRunner { let cwd = self.cwd.take(); let env = self.env.take(); let stdin_content = self.stdin_content.take(); - let grace = self.exit_pump_grace_ms; + let grace = GraceWindows { + exit_pump_ms: self.exit_pump_grace_ms, + kill_ms: self.kill_grace_ms, + }; let kill_signal = self.kill_signal.clone(); let task = tokio::spawn(async move { @@ -321,13 +337,25 @@ impl Drop for OutputStream { } } +/// How long the runner waits, in milliseconds, at the two points where it gives +/// something a chance to finish on its own before forcing the issue. +#[derive(Debug, Clone, Copy)] +struct GraceWindows { + /// Time allowed for the readers to drain buffered output after the child + /// exits, before the `Exit` chunk is emitted. + exit_pump_ms: u64, + /// Time allowed for the child to handle the delivered signal, before the + /// escalation to `SIGKILL`. + kill_ms: u64, +} + /// Run a streaming process and send output to the channel async fn run_streaming_process( command: StreamingCommand, cwd: Option, env: Option>, stdin_content: Option, - exit_pump_grace_ms: u64, + grace: GraceWindows, tx: mpsc::Sender, mut kill_rx: mpsc::UnboundedReceiver, ) -> Result<()> { @@ -457,21 +485,38 @@ async fn run_streaming_process( // being dropped). Stop the process group with the requested signal. let signal = maybe_signal.unwrap_or_else(|| DEFAULT_KILL_SIGNAL.to_string()); trace_lazy("StreamingRunner", || format!("Kill requested | signal={}", signal)); - if let Some(pid) = pid { - send_signal_to_process(pid, &signal); - } - // Give it a brief moment to exit on the requested signal, then - // escalate to a forceful kill so it always terminates. - if tokio::time::timeout(Duration::from_millis(exit_pump_grace_ms), child.wait()) - .await - .is_err() - { + // Give the child its grace period to run its own handler and exit + // on its own terms, then escalate to a forceful kill so a process + // that ignores the signal still terminates. + // + // A zero grace period means the child is given no opportunity to + // handle the signal, so the requested signal is not delivered at + // all. Anything done between it and the forceful kill - a syscall, + // or awaiting a zero-length timeout, which yields to the runtime - + // is a window the child can be scheduled in, which made "no grace" + // a race the child occasionally won rather than a guarantee. + let survived_grace = if grace.kill_ms == 0 { + true + } else { + if let Some(pid) = pid { + // The child is always spawned with `process_group(0)` + // above, so it leads the group named by its own pid. + send_signal_to_process(pid, &signal, Delivery::ProcessAndGroup); + } + tokio::time::timeout(Duration::from_millis(grace.kill_ms), child.wait()) + .await + .is_err() + }; + if survived_grace { + if let Some(pid) = pid { + send_signal_to_process(pid, "SIGKILL", Delivery::ProcessAndGroup); + } let _ = child.start_kill(); let _ = child.wait().await; } // Report the conventional 128 + signal code for the requested // signal, matching the JavaScript implementation. - code = 128 + signal_number(&signal); + code = signal_exit_code(&signal); } } @@ -488,7 +533,7 @@ async fn run_streaming_process( let _ = handle.await; } }; - if tokio::time::timeout(Duration::from_millis(exit_pump_grace_ms), drain) + if tokio::time::timeout(Duration::from_millis(grace.exit_pump_ms), drain) .await .is_err() { @@ -526,49 +571,6 @@ fn status_to_code(status: std::process::ExitStatus) -> i32 { -1 } -/// Map a signal name to its numeric value for the `128 + signal` exit-code -/// convention. Unknown names fall back to `SIGTERM`. -fn signal_number(signal: &str) -> i32 { - match signal { - "SIGHUP" => 1, - "SIGINT" => 2, - "SIGQUIT" => 3, - "SIGKILL" => 9, - "SIGUSR1" => 10, - "SIGUSR2" => 12, - "SIGTERM" => 15, - _ => 15, - } -} - -/// Send a signal to a process and its process group (best effort). -#[cfg(unix)] -fn send_signal_to_process(pid: u32, signal: &str) { - use nix::sys::signal::{kill, Signal}; - use nix::unistd::Pid; - - let sig = match signal { - "SIGHUP" => Signal::SIGHUP, - "SIGINT" => Signal::SIGINT, - "SIGQUIT" => Signal::SIGQUIT, - "SIGKILL" => Signal::SIGKILL, - "SIGUSR1" => Signal::SIGUSR1, - "SIGUSR2" => Signal::SIGUSR2, - "SIGTERM" => Signal::SIGTERM, - _ => Signal::SIGTERM, - }; - - // Signal the process itself. - let _ = kill(Pid::from_raw(pid as i32), sig); - // Signal the whole process group (negative pid) to reach grandchildren. - let _ = kill(Pid::from_raw(-(pid as i32)), sig); -} - -/// On non-Unix platforms there is no signal delivery; the forceful -/// `start_kill()` escalation in the caller handles termination. -#[cfg(not(unix))] -fn send_signal_to_process(_pid: u32, _signal: &str) {} - /// Shell configuration #[derive(Debug, Clone)] struct ShellConfig { diff --git a/rust/tests/signals.rs b/rust/tests/signals.rs new file mode 100644 index 00000000..d4e78d74 --- /dev/null +++ b/rust/tests/signals.rs @@ -0,0 +1,487 @@ +//! Signal handling tests (issue #15). +//! +//! These cover the documented contract for stopping a running command: which +//! signal is delivered, that the child gets a chance to handle it, that a +//! process ignoring it is still terminated, and which exit code is reported. +//! +//! The JavaScript counterpart is `js/tests/signal-handling.test.mjs`; both +//! suites assert the same behavior so the two implementations stay in parity. +use command_stream::signal::{signal_exit_code, signal_number}; +// Only the exit-code mapping is portable; everything that actually delivers a +// signal is Unix-only, and so are the imports it needs. +#[cfg(unix)] +use command_stream::{OutputChunk, ProcessRunner, RunOptions, StdinOption, StreamingRunner}; +#[cfg(unix)] +use std::time::Duration; + +/// A command that traps a signal, records that its handler ran, and exits. +/// +/// Writing to a marker file is what distinguishes "the child handled the +/// signal" from "the child was destroyed before it could": an exit code alone +/// cannot tell the two apart, because the reported code is derived from the +/// signal that was requested either way. +#[cfg(unix)] +fn graceful_child(marker: &std::path::Path) -> String { + format!( + "trap 'echo handled >> {marker}; exit 0' TERM INT; \ + echo ready; \ + while true; do sleep 0.05; done", + marker = marker.display() + ) +} + +/// A command that ignores TERM and INT outright, so only SIGKILL stops it. +#[cfg(unix)] +fn stubborn_child() -> String { + "trap '' TERM INT; echo ready; while true; do sleep 0.05; done".to_string() +} + +/// A stubborn child that also appends to a heartbeat file while it runs. +/// +/// Whether the heartbeat keeps growing is the evidence that the process is +/// still executing. Checking the pid with signal 0 would not work here: after +/// `kill()` nobody awaits the child, so it lingers as a zombie and still +/// answers signal 0 long after it stopped running. +#[cfg(unix)] +fn stubborn_heartbeat_child(heartbeat: &std::path::Path) -> String { + format!( + "trap '' TERM INT; \ + echo ready; \ + while true; do echo tick >> {heartbeat}; sleep 0.05; done", + heartbeat = heartbeat.display() + ) +} + +/// A command whose real work runs in a *grandchild*, behind a shell that waits. +/// +/// This is the shape both READMEs promise to handle: `sh` stays alive as the +/// parent, so signalling only the direct child leaves the actual worker running +/// and the group delivery is what has to reach it. +#[cfg(unix)] +fn grandchild_heartbeat_command(heartbeat: &std::path::Path) -> String { + format!( + "sh -c 'while true; do echo tick >> {beat}; sleep 0.05; done' & \ + echo ready; \ + wait", + beat = heartbeat.display() + ) +} + +/// The same, but the shell exits immediately instead of waiting. +/// +/// By the time the command is killed the direct child is long gone - a zombie, +/// because the runner holds its handle and has not reaped it. Only the group is +/// left to signal, and its leader being dead must not stand in the way: looking +/// the group up with `getpgid` at that point fails with `ESRCH` on macOS, which +/// silently skipped the delivery and left the grandchild running. +#[cfg(unix)] +fn orphaned_grandchild_heartbeat_command(heartbeat: &std::path::Path) -> String { + format!( + "sh -c 'while true; do echo tick >> {beat}; sleep 0.05; done' & \ + echo ready", + beat = heartbeat.display() + ) +} + +#[cfg(unix)] +fn heartbeat_len(path: &std::path::Path) -> u64 { + std::fs::metadata(path).map(|meta| meta.len()).unwrap_or(0) +} + +/// Grace period used by the tests that assert a signal handler actually ran. +/// +/// The 100ms default is plenty in isolation, but these tests share a machine +/// with the rest of the binary, and under that contention the child's trap can +/// be scheduled after the escalation deadline - which failed the assertion in +/// roughly one run out of eight. A generous window removes the race without +/// weakening the test: the bug being guarded against sent SIGKILL in the same +/// step as the signal, so no grace period would have saved the handler. +#[cfg(unix)] +const GRACEFUL_KILL_GRACE_MS: u64 = 2000; + +#[cfg(unix)] +fn handler_ran(marker: &std::path::Path) -> bool { + std::fs::read_to_string(marker) + .map(|text| text.contains("handled")) + .unwrap_or(false) +} + +// ============================================================================ +// Exit-code convention +// ============================================================================ + +#[test] +fn signal_numbers_follow_the_posix_names() { + assert_eq!(signal_number("SIGHUP"), 1); + assert_eq!(signal_number("SIGINT"), 2); + assert_eq!(signal_number("SIGQUIT"), 3); + assert_eq!(signal_number("SIGKILL"), 9); + assert_eq!(signal_number("SIGUSR1"), 10); + assert_eq!(signal_number("SIGUSR2"), 12); + assert_eq!(signal_number("SIGTERM"), 15); +} + +#[test] +fn unknown_signal_names_fall_back_to_sigterm() { + assert_eq!(signal_number("NOT-A-SIGNAL"), signal_number("SIGTERM")); + assert_eq!(signal_exit_code("NOT-A-SIGNAL"), 143); +} + +#[test] +fn exit_codes_follow_the_128_plus_signal_convention() { + // The table documented in both READMEs. + assert_eq!(signal_exit_code("SIGHUP"), 129); + assert_eq!(signal_exit_code("SIGINT"), 130); // CTRL+C + assert_eq!(signal_exit_code("SIGQUIT"), 131); + assert_eq!(signal_exit_code("SIGKILL"), 137); + assert_eq!(signal_exit_code("SIGUSR1"), 138); + assert_eq!(signal_exit_code("SIGUSR2"), 140); + assert_eq!(signal_exit_code("SIGTERM"), 143); +} + +// ============================================================================ +// ProcessRunner +// ============================================================================ + +/// The default stop signal is SIGTERM, and the child gets to handle it. +#[cfg(unix)] +#[tokio::test] +async fn process_runner_kill_lets_the_child_handle_sigterm() { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("marker"); + + let mut runner = ProcessRunner::new( + graceful_child(&marker), + RunOptions { + mirror: false, + kill_grace_ms: GRACEFUL_KILL_GRACE_MS, + ..Default::default() + }, + ); + runner.start().await.unwrap(); + tokio::time::sleep(Duration::from_millis(300)).await; + runner.kill().unwrap(); + tokio::time::sleep(Duration::from_millis(400)).await; + + assert!( + handler_ran(&marker), + "the child's SIGTERM handler never ran: kill() destroyed it before it could clean up" + ); +} + +/// `kill_with` overrides the configured signal for a single call. +#[cfg(unix)] +#[tokio::test] +async fn process_runner_kill_with_sends_the_requested_signal() { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("marker"); + + let mut runner = ProcessRunner::new( + graceful_child(&marker), + RunOptions { + mirror: false, + kill_grace_ms: GRACEFUL_KILL_GRACE_MS, + ..Default::default() + }, + ); + runner.start().await.unwrap(); + tokio::time::sleep(Duration::from_millis(300)).await; + // SIGINT is the signal CTRL+C sends. + runner.kill_with("SIGINT").unwrap(); + tokio::time::sleep(Duration::from_millis(400)).await; + + assert!(handler_ran(&marker), "the child's SIGINT handler never ran"); +} + +/// A configured `kill_signal` is what an argument-less `kill()` delivers. +#[cfg(unix)] +#[tokio::test] +async fn process_runner_honors_the_configured_kill_signal() { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("marker"); + + let mut runner = ProcessRunner::new( + // Only INT is trapped, so the marker proves SIGINT (not the SIGTERM + // default) was the signal actually delivered. + format!( + "trap 'echo handled >> {marker}; exit 0' INT; echo ready; while true; do sleep 0.05; done", + marker = marker.display() + ), + RunOptions { + mirror: false, + kill_signal: "SIGINT".to_string(), + kill_grace_ms: GRACEFUL_KILL_GRACE_MS, + ..Default::default() + }, + ); + runner.start().await.unwrap(); + tokio::time::sleep(Duration::from_millis(300)).await; + runner.kill().unwrap(); + tokio::time::sleep(Duration::from_millis(400)).await; + + assert!( + handler_ran(&marker), + "kill() did not deliver the configured SIGINT" + ); +} + +/// A process that ignores the signal is still terminated by the escalation. +#[cfg(unix)] +#[tokio::test] +async fn process_runner_escalates_to_sigkill_when_the_signal_is_ignored() { + let dir = tempfile::tempdir().unwrap(); + let heartbeat = dir.path().join("heartbeat"); + + let mut runner = ProcessRunner::new( + stubborn_heartbeat_child(&heartbeat), + RunOptions { + mirror: false, + kill_grace_ms: 50, + ..Default::default() + }, + ); + runner.start().await.unwrap(); + tokio::time::sleep(Duration::from_millis(300)).await; + + let while_running = heartbeat_len(&heartbeat); + assert!( + while_running > 0, + "the child never started: no heartbeat was written" + ); + + runner.kill().unwrap(); + // Wait out the grace period plus the SIGKILL escalation. + tokio::time::sleep(Duration::from_millis(500)).await; + let after_kill = heartbeat_len(&heartbeat); + // If the process were still alive it would keep appending during this window. + tokio::time::sleep(Duration::from_millis(500)).await; + + assert_eq!( + heartbeat_len(&heartbeat), + after_kill, + "a process ignoring SIGTERM kept running: it was never escalated to SIGKILL" + ); +} + +/// Killing the runner also stops grandchildren, not just the direct child. +/// +/// Without the child in its own process group, `kill(-pid, ...)` names a group +/// the runner does not own, so the worker behind the shell kept running and +/// kept writing its heartbeat long after the command was stopped. +#[cfg(unix)] +#[tokio::test] +async fn process_runner_kill_reaches_grandchildren() { + let dir = tempfile::tempdir().unwrap(); + let heartbeat = dir.path().join("heartbeat"); + + let mut runner = ProcessRunner::new( + grandchild_heartbeat_command(&heartbeat), + RunOptions { + mirror: false, + kill_grace_ms: 50, + // Explicit, so the result does not depend on whether the test + // harness happened to be given a terminal: a child sharing the + // caller's terminal stays in its process group by design. + stdin: StdinOption::Null, + ..Default::default() + }, + ); + runner.start().await.unwrap(); + tokio::time::sleep(Duration::from_millis(400)).await; + assert!( + heartbeat_len(&heartbeat) > 0, + "the grandchild never started: no heartbeat was written" + ); + + runner.kill().unwrap(); + // Past the grace period, so the escalation has been delivered too. + tokio::time::sleep(Duration::from_millis(400)).await; + let after_kill = heartbeat_len(&heartbeat); + tokio::time::sleep(Duration::from_millis(400)).await; + + assert_eq!( + heartbeat_len(&heartbeat), + after_kill, + "the grandchild survived the kill and kept writing its heartbeat" + ); +} + +/// The group is still reached when its leader has already exited. +/// +/// The shell that started the worker exits straight away, so at kill time the +/// direct child is a zombie and the grandchild is all that is left to stop. +/// Asking the operating system which group the child leads is not an option +/// then: Linux answers for a zombie, macOS does not, and there the grandchild +/// was never signalled. Group leadership is therefore recorded when the child +/// is spawned. +/// +/// This one guarantee has no JavaScript counterpart: Node and Bun reap the +/// shell as soon as it exits, which frees its pid - and with it the group id - +/// for reuse, so the group can no longer be signalled safely. Here the runner +/// still holds the unreaped child, which keeps the group id reserved. +#[cfg(unix)] +#[tokio::test] +async fn process_runner_kill_reaches_a_grandchild_whose_parent_already_exited() { + let dir = tempfile::tempdir().unwrap(); + let heartbeat = dir.path().join("heartbeat"); + + let mut runner = ProcessRunner::new( + orphaned_grandchild_heartbeat_command(&heartbeat), + RunOptions { + mirror: false, + kill_grace_ms: 50, + stdin: StdinOption::Null, + ..Default::default() + }, + ); + runner.start().await.unwrap(); + // Long enough for the shell to have exited and the worker to be ticking. + tokio::time::sleep(Duration::from_millis(400)).await; + assert!( + heartbeat_len(&heartbeat) > 0, + "the grandchild never started: no heartbeat was written" + ); + + runner.kill().unwrap(); + // Past the grace period, so the escalation has been delivered too. + tokio::time::sleep(Duration::from_millis(400)).await; + let after_kill = heartbeat_len(&heartbeat); + tokio::time::sleep(Duration::from_millis(400)).await; + + assert_eq!( + heartbeat_len(&heartbeat), + after_kill, + "the orphaned grandchild survived the kill and kept writing its heartbeat" + ); +} + +// ============================================================================ +// StreamingRunner +// ============================================================================ + +/// The streaming runner gives the child the same grace period, and reports the +/// `128 + signal` exit code for the signal that was requested. +#[cfg(unix)] +#[tokio::test] +async fn stream_kill_lets_the_child_handle_the_signal() { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("marker"); + + let mut stream = StreamingRunner::new(graceful_child(&marker)) + .kill_grace_ms(GRACEFUL_KILL_GRACE_MS) + .stream(); + + let mut exit_code = None; + let mut killed = false; + while let Some(chunk) = stream.next().await { + match chunk { + OutputChunk::Stdout(_) if !killed => { + killed = true; + stream.kill(); + } + OutputChunk::Exit(code) => exit_code = Some(code), + _ => {} + } + } + tokio::time::sleep(Duration::from_millis(300)).await; + + assert_eq!(exit_code, Some(143), "expected the SIGTERM exit code"); + assert!( + handler_ran(&marker), + "the child's SIGTERM handler never ran" + ); +} + +/// `kill_grace_ms(0)` opts out of the grace period entirely. +#[cfg(unix)] +#[tokio::test] +async fn stream_zero_grace_escalates_immediately() { + let mut stream = StreamingRunner::new(stubborn_child()) + .kill_grace_ms(0) + .stream(); + + let mut exit_code = None; + let mut killed = false; + while let Some(chunk) = stream.next().await { + match chunk { + OutputChunk::Stdout(_) if !killed => { + killed = true; + stream.kill(); + } + OutputChunk::Exit(code) => exit_code = Some(code), + _ => {} + } + } + + // The reported code still reflects the requested signal, even though the + // process was actually stopped by the SIGKILL escalation. + assert_eq!(exit_code, Some(143)); +} + +/// Number of times the zero-grace tests repeat their scenario. +/// +/// The bug they guard against is a lost race, not a constant failure: awaiting a +/// zero-length timeout yields to the runtime, and the child wins that gap only +/// sometimes. A single attempt caught the old behavior in roughly one run out of +/// three, so the scenario is repeated to turn a coin flip into a reliable signal. +#[cfg(unix)] +const ZERO_GRACE_ATTEMPTS: usize = 10; + +/// With no grace period the child never gets to run its handler, even though it +/// traps the signal. The escalation must therefore happen in the same step as +/// the signal, leaving no scheduling gap for the shell to run its trap in. +#[cfg(unix)] +#[tokio::test] +async fn stream_zero_grace_leaves_no_room_for_the_handler() { + for attempt in 0..ZERO_GRACE_ATTEMPTS { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("marker"); + + let mut stream = StreamingRunner::new(graceful_child(&marker)) + .kill_grace_ms(0) + .stream(); + + let mut killed = false; + while let Some(chunk) = stream.next().await { + if matches!(chunk, OutputChunk::Stdout(_)) && !killed { + killed = true; + stream.kill(); + } + } + tokio::time::sleep(Duration::from_millis(200)).await; + + assert!( + !handler_ran(&marker), + "attempt {attempt}: kill_grace_ms(0) still left the child time to run its SIGTERM handler" + ); + } +} + +/// The same guarantee for `ProcessRunner`, whose escalation runs in a spawned +/// task: with `kill_grace_ms: 0` it must not wait for that task to be polled. +#[cfg(unix)] +#[tokio::test] +async fn process_runner_zero_grace_leaves_no_room_for_the_handler() { + for attempt in 0..ZERO_GRACE_ATTEMPTS { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("marker"); + + let mut runner = ProcessRunner::new( + graceful_child(&marker), + RunOptions { + mirror: false, + kill_grace_ms: 0, + ..Default::default() + }, + ); + runner.start().await.unwrap(); + tokio::time::sleep(Duration::from_millis(200)).await; + runner.kill().unwrap(); + tokio::time::sleep(Duration::from_millis(200)).await; + + assert!( + !handler_ran(&marker), + "attempt {attempt}: kill_grace_ms: 0 still left the child time to run its SIGTERM handler" + ); + } +}