Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions experiments/issue-15/graceful-child.sh
Original file line number Diff line number Diff line change
@@ -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
39 changes: 39 additions & 0 deletions experiments/issue-15/js-escalation-debug.mjs
Original file line number Diff line number Diff line change
@@ -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()}`);
}
32 changes: 32 additions & 0 deletions experiments/issue-15/js-kill-grace.mjs
Original file line number Diff line number Diff line change
@@ -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)));
}
36 changes: 36 additions & 0 deletions experiments/issue-15/js-orphan-grandchild.mjs
Original file line number Diff line number Diff line change
@@ -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);
54 changes: 54 additions & 0 deletions experiments/issue-15/macos-zombie-getpgid.sh
Original file line number Diff line number Diff line change
@@ -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/<pid>/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<u32> {
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
64 changes: 64 additions & 0 deletions experiments/issue-15/rust_kill_grace.rs
Original file line number Diff line number Diff line change
@@ -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() }
);
}
76 changes: 76 additions & 0 deletions experiments/issue-15/sh-trap-race.py
Original file line number Diff line number Diff line change
@@ -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"
)
15 changes: 15 additions & 0 deletions js/.changeset/issue-15-signal-handling.md
Original file line number Diff line number Diff line change
@@ -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`.
Loading
Loading