smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge PR #6780: fix(shell): correct kill signal handling (@roboomp)

Browse Source
This commit is contained in:
can1357
2026-07-27 15:57:47 +02:00
parent 7d864fd4e6 0c1fd7f5d8
commit 8acd667005
3 changed files with 381 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/pi-shell/src/shell.rs
+304
View File
@@ -2174,8 +2174,312 @@ fn uutils_env_disabled(config: &ShellConfig, key: &str) -> bool {
#[cfg(test)]
mod tests {
#[cfg(unix)]
use std::os::unix::process::ExitStatusExt as _;
#[cfg(unix)]
use tokio::process::Command;
use super::*;
#[cfg(unix)]
async fn kill_test_context() -> (ShellSessionCore, ExecutionParameters) {
let config = ShellConfig { session_env: None, snapshot_path: None, minimizer: None };
let mut session = create_session(&config).await.expect("create_session");
let mut params = session.shell.default_exec_params();
params.set_fd(OpenFiles::STDIN_FD, null_file().expect("null stdin"));
params.set_fd(OpenFiles::STDOUT_FD, null_file().expect("null stdout"));
params.set_fd(OpenFiles::STDERR_FD, null_file().expect("null stderr"));
(session, params)
}
/// The kill builtin accepts a numeric signal and applies it to every process
/// operand.
#[cfg(unix)]
#[tokio::test(flavor = "multi_thread")]
async fn kill_builtin_accepts_numeric_signal_for_multiple_processes() {
let mut first = Command::new("sleep")
.arg("30")
.spawn()
.expect("first sleep");
let mut second = Command::new("sleep")
.arg("30")
.spawn()
.expect("second sleep");
let first_pid = first.id().expect("first pid");
let second_pid = second.id().expect("second pid");
let (mut session, params) = kill_test_context().await;
let source_info = SourceInfo::from("pi-natives:test");
let result = session
.shell
.run_string(format!("kill -9 {first_pid} {second_pid}"), &source_info, &params)
.await
.expect("kill command");
let code = exit_code(&result);
if code != 0 {
let _ = first.kill().await;
let _ = second.kill().await;
let _ = first.wait().await;
let _ = second.wait().await;
assert_eq!(code, 0, "numeric multi-process kill should succeed");
}
let statuses =
time::timeout(Duration::from_secs(5), async { tokio::join!(first.wait(), second.wait()) })
.await;
if statuses.is_err() {
let _ = first.kill().await;
let _ = second.kill().await;
let _ = first.wait().await;
let _ = second.wait().await;
panic!("kill must signal every process operand");
}
let (first_status, second_status) = statuses.expect("checked timeout");
assert_eq!(first_status.expect("first wait").signal(), Some(libc::SIGKILL));
assert_eq!(second_status.expect("second wait").signal(), Some(libc::SIGKILL));
}
/// The kill builtin defaults to SIGTERM so processes can shut down
/// gracefully.
#[cfg(unix)]
#[tokio::test(flavor = "multi_thread")]
async fn kill_builtin_defaults_to_sigterm() {
let dir = tempfile::tempdir().expect("temp dir");
let ready = dir.path().join("ready");
let mut child = Command::new("sh")
.args([
"-c",
"trap 'exit 42' TERM; : > \"$1\"; while :; do :; done",
"sh",
ready.to_str().expect("utf8 path"),
])
.spawn()
.expect("trapping child");
let pid = child.id().expect("child pid");
let ready_result = time::timeout(Duration::from_secs(5), async {
while !ready.exists() {
time::sleep(Duration::from_millis(10)).await;
}
})
.await;
if ready_result.is_err() {
let _ = child.kill().await;
let _ = child.wait().await;
panic!("child did not install its SIGTERM trap");
}
let (mut session, params) = kill_test_context().await;
let source_info = SourceInfo::from("pi-natives:test");
let result = session
.shell
.run_string(format!("kill {pid}"), &source_info, &params)
.await
.expect("kill command");
let code = exit_code(&result);
let status = time::timeout(Duration::from_secs(5), child.wait()).await;
if status.is_err() {
let _ = child.kill().await;
let _ = child.wait().await;
panic!("default kill signal did not terminate the child");
}
assert_eq!(code, 0, "default kill should succeed");
assert_eq!(status.expect("checked timeout").expect("child wait").code(), Some(42));
}
/// A negative PID after `--` targets a process group per `kill(2)` instead
/// of being parsed as a numeric signal, and a plain PID in the same command
/// is still signaled.
#[cfg(unix)]
#[tokio::test(flavor = "multi_thread")]
async fn kill_builtin_preserves_negative_pid_process_group_operands() {
let dir = tempfile::tempdir().expect("temp dir");
let group_ready = dir.path().join("group-ready");
let plain_ready = dir.path().join("plain-ready");
let spawn_trapping = |ready: &std::path::Path, own_group: bool| {
let mut cmd = Command::new("sh");
cmd.args([
"-c",
"trap 'exit 42' TERM; : > \"$1\"; while :; do sleep 0.05; done",
"sh",
ready.to_str().expect("utf8 path"),
]);
if own_group {
// pgid becomes this child's own pid, so `-pid` addresses the group.
cmd.process_group(0);
}
cmd.spawn().expect("trapping child")
};
let mut group_child = spawn_trapping(&group_ready, true);
let mut plain_child = spawn_trapping(&plain_ready, false);
let group_pid = group_child.id().expect("group pid");
let plain_pid = plain_child.id().expect("plain pid");
let ready_result = time::timeout(Duration::from_secs(5), async {
while !group_ready.exists() || !plain_ready.exists() {
time::sleep(Duration::from_millis(10)).await;
}
})
.await;
if ready_result.is_err() {
let _ = group_child.start_kill();
let _ = plain_child.start_kill();
let _ = group_child.wait().await;
let _ = plain_child.wait().await;
panic!("children did not install their SIGTERM traps");
}
let (mut session, params) = kill_test_context().await;
let source_info = SourceInfo::from("pi-natives:test");
let result = session
.shell
.run_string(format!("kill -TERM -- -{group_pid} {plain_pid}"), &source_info, &params)
.await
.expect("kill command");
let code = exit_code(&result);
let statuses = time::timeout(Duration::from_secs(5), async {
tokio::join!(group_child.wait(), plain_child.wait())
})
.await;
if statuses.is_err() {
let _ = group_child.start_kill();
let _ = plain_child.start_kill();
let _ = group_child.wait().await;
let _ = plain_child.wait().await;
panic!("negative-PID kill must signal the process group and the plain PID");
}
let (group_status, plain_status) = statuses.expect("checked timeout");
assert_eq!(code, 0, "negative-PID kill should succeed");
assert_eq!(group_status.expect("group wait").code(), Some(42));
assert_eq!(plain_status.expect("plain wait").code(), Some(42));
}
/// When clap consumes the `--` marker before `execute` (the default-signal
/// and `-s SIG` forms), a following negative PID is still an operand, not a
/// signal: `kill -- -<pgid>` defaults to SIGTERM for the group, and
/// `kill -s TERM -- -<pgid>` sends the named signal to the group.
#[cfg(unix)]
#[tokio::test(flavor = "multi_thread")]
async fn kill_builtin_signals_group_when_marker_precedes_negative_pid() {
let dir = tempfile::tempdir().expect("temp dir");
let default_ready = dir.path().join("default-ready");
let named_ready = dir.path().join("named-ready");
let spawn_group_leader = |ready: &std::path::Path| {
Command::new("sh")
.args([
"-c",
"trap 'exit 42' TERM; : > \"$1\"; while :; do sleep 0.05; done",
"sh",
ready.to_str().expect("utf8 path"),
])
.process_group(0)
.spawn()
.expect("group leader")
};
let mut default_child = spawn_group_leader(&default_ready);
let mut named_child = spawn_group_leader(&named_ready);
let default_pid = default_child.id().expect("default pid");
let named_pid = named_child.id().expect("named pid");
let ready_result = time::timeout(Duration::from_secs(5), async {
while !default_ready.exists() || !named_ready.exists() {
time::sleep(Duration::from_millis(10)).await;
}
})
.await;
if ready_result.is_err() {
let _ = default_child.start_kill();
let _ = named_child.start_kill();
let _ = default_child.wait().await;
let _ = named_child.wait().await;
panic!("group leaders did not install their SIGTERM traps");
}
let (mut session, params) = kill_test_context().await;
let source_info = SourceInfo::from("pi-natives:test");
// Default signal (SIGTERM) with the marker consumed by clap.
let default_result = session
.shell
.run_string(format!("kill -- -{default_pid}"), &source_info, &params)
.await
.expect("default kill command");
// Named signal via -s, marker consumed by clap.
let named_result = session
.shell
.run_string(format!("kill -s TERM -- -{named_pid}"), &source_info, &params)
.await
.expect("named kill command");
let statuses = time::timeout(Duration::from_secs(5), async {
tokio::join!(default_child.wait(), named_child.wait())
})
.await;
if statuses.is_err() {
let _ = default_child.start_kill();
let _ = named_child.start_kill();
let _ = default_child.wait().await;
let _ = named_child.wait().await;
panic!("marker-preceded negative PID must signal the process group");
}
let (default_status, named_status) = statuses.expect("checked timeout");
assert_eq!(exit_code(&default_result), 0, "`kill -- -<pgid>` should succeed");
assert_eq!(exit_code(&named_result), 0, "`kill -s TERM -- -<pgid>` should succeed");
assert_eq!(default_status.expect("default wait").code(), Some(42));
assert_eq!(named_status.expect("named wait").code(), Some(42));
}
/// A failed target makes `kill` return non-zero without preventing later
/// process operands from receiving the selected signal.
#[cfg(unix)]
#[tokio::test(flavor = "multi_thread")]
async fn kill_builtin_continues_after_target_failure() {
let mut first = Command::new("sleep")
.arg("30")
.spawn()
.expect("first sleep");
let mut second = Command::new("sleep")
.arg("30")
.spawn()
.expect("second sleep");
let mut stale = Command::new("true").spawn().expect("stale process");
let first_pid = first.id().expect("first pid");
let second_pid = second.id().expect("second pid");
let stale_pid = stale.id().expect("stale pid");
stale.wait().await.expect("stale wait");
let (mut session, params) = kill_test_context().await;
let source_info = SourceInfo::from("pi-natives:test");
let result = session
.shell
.run_string(
format!("kill -KILL {first_pid} {stale_pid} {second_pid}"),
&source_info,
&params,
)
.await
.expect("kill command");
let code = exit_code(&result);
let statuses =
time::timeout(Duration::from_secs(5), async { tokio::join!(first.wait(), second.wait()) })
.await;
if statuses.is_err() {
let _ = first.start_kill();
let _ = second.start_kill();
let _ = first.wait().await;
let _ = second.wait().await;
panic!("kill must continue signaling after an intermediate target fails");
}
let (first_status, second_status) = statuses.expect("checked timeout");
assert_ne!(code, 0, "a failed target should make kill return non-zero");
assert_eq!(first_status.expect("first wait").signal(), Some(libc::SIGKILL));
assert_eq!(second_status.expect("second wait").signal(), Some(libc::SIGKILL));
}
/// `cmp` remains available with no executable search path, proving the shell
/// dispatches the in-process builtin rather than a platform binary.
#[tokio::test(flavor = "multi_thread")]
crates/vendor/brush-builtins/src/kill.rs
Vendored
+74 -35
View File
@@ -23,6 +23,12 @@ pub(crate) struct KillCommand {
// Interpretation of these depends on whether -l is present.
#[arg(allow_hyphen_values = true)]
args: Vec<String>,
/// Process/job operands given after the `--` end-of-options marker. clap
/// consumes `--` before `execute`, so these are captured separately and are
/// always operands — never signal specifications (preserves negative PIDs).
#[arg(last = true, allow_hyphen_values = true)]
post_marker_args: Vec<String>,
}
impl builtins::Command for KillCommand {
@@ -32,8 +38,8 @@ impl builtins::Command for KillCommand {
&self,
context: brush_core::ExecutionContext<'_, SE>,
) -> Result<brush_core::ExecutionResult, Self::Error> {
// Default signal is SIGKILL.
let mut trap_signal = TrapSignal::Signal(nix::sys::signal::Signal::SIGKILL);
// Match shell and POSIX defaults by allowing graceful termination.
let mut trap_signal = TrapSignal::Signal(nix::sys::signal::Signal::SIGTERM);
// Try parsing the signal name (if specified).
if let Some(signal_name) = &self.signal_name {
@@ -67,43 +73,61 @@ impl builtins::Command for KillCommand {
}
}
// Look through the remaining args for a pid/job spec or a -sigspec style
// option.
let mut pid_or_job_spec = None;
// Interpret the pre-`--` args: an optional leading `-sigspec` in the
// option position, then pid/jobspec operands. A hyphen leads a sigspec
// only in the option position — once a signal is chosen (via `-s`/`-n`, a
// leading `-sigspec`, or the `--` marker) or an operand is seen, later
// hyphen-led args are operands, so negative PIDs (process groups per
// `kill(2)`) survive: `kill -TERM -- -10 123` signals process group 10 and
// PID 123, and `kill -s TERM -- -10` signals process group 10.
let mut operands: Vec<&String> = Vec::new();
let mut options_done = self.signal_name.is_some() || self.signal_number.is_some();
let mut consumed_marker = false;
for arg in &self.args {
// See if this is -sigspec syntax.
if let Some(possible_sigspec) = arg.strip_prefix("-") {
// See if this is -sigspec syntax.
if let Ok(parsed_trap_signal) = TrapSignal::try_from(possible_sigspec) {
trap_signal = parsed_trap_signal;
} else {
writeln!(context.stderr(), "{}: invalid signal name", context.command_name)?;
return Ok(ExecutionExitCode::InvalidUsage.into());
}
} else if pid_or_job_spec.is_none() {
pid_or_job_spec = Some(arg);
} else {
writeln!(
context.stderr(),
"{}: too many jobs or processes specified",
context.command_name
)?;
return Ok(ExecutionExitCode::InvalidUsage.into());
// Whether clap leaves the `--` marker here depends on whether a
// positional value was already collected; consume the first one and
// close the option position either way.
if !consumed_marker && arg == "--" {
consumed_marker = true;
options_done = true;
continue;
}
if !options_done {
if let Some(possible_sigspec) = arg.strip_prefix('-') {
if !possible_sigspec.is_empty() {
// Option position: interpret as a signal specification. The
// sigspec may be a signal name (e.g. -TERM) or number (e.g. -9).
if let Ok(parsed_trap_signal) = possible_sigspec.parse::<TrapSignal>() {
trap_signal = parsed_trap_signal;
options_done = true;
continue;
}
writeln!(context.stderr(), "{}: invalid signal name", context.command_name)?;
return Ok(ExecutionExitCode::InvalidUsage.into());
}
}
// The first operand ends the option position.
options_done = true;
}
operands.push(arg);
}
// Operands after the `--` marker are always process/job specs.
operands.extend(self.post_marker_args.iter());
if self.list_signals {
return print_signals(&context, self.args.as_ref());
} else {
let Some(pid_or_job_spec) = pid_or_job_spec else {
writeln!(context.stderr(), "{}: invalid usage", context.command_name)?;
return Ok(ExecutionExitCode::InvalidUsage.into());
};
}
if operands.is_empty() {
writeln!(context.stderr(), "{}: invalid usage", context.command_name)?;
return Ok(ExecutionExitCode::InvalidUsage.into());
}
if pid_or_job_spec.starts_with('%') {
let mut had_failure = false;
for pid_or_job_spec in operands {
let signal_result = if pid_or_job_spec.starts_with('%') {
// It's a job spec.
if let Some(job) = context.shell.jobs_mut().resolve_job_spec(pid_or_job_spec) {
job.kill(trap_signal)?;
job.kill(trap_signal)
} else {
writeln!(
context.stderr(),
@@ -111,16 +135,31 @@ impl builtins::Command for KillCommand {
context.command_name,
pid_or_job_spec
)?;
return Ok(ExecutionResult::general_error());
had_failure = true;
continue;
}
} else {
let pid = brush_core::int_utils::parse(pid_or_job_spec.as_str(), 10)?;
brush_core::int_utils::parse(pid_or_job_spec.as_str(), 10)
.and_then(|pid| sys::signal::kill_process(pid, trap_signal))
};
// It's a pid.
sys::signal::kill_process(pid, trap_signal)?;
if let Err(err) = signal_result {
writeln!(
context.stderr(),
"{}: {}: {}",
context.command_name,
pid_or_job_spec,
err
)?;
had_failure = true;
}
}
Ok(ExecutionResult::success())
if had_failure {
Ok(ExecutionResult::general_error())
} else {
Ok(ExecutionResult::success())
}
}
}
packages/coding-agent/CHANGELOG.md
+3
View File
@@ -32,6 +32,9 @@
### Fixed
- Fixed `/live` sideband WebSockets ignoring standard proxy environment variables and `NO_PROXY`, which left proxied sessions stuck while the rest of the Codex connection succeeded ([#6770](https://github.com/can1357/oh-my-pi/issues/6770)).
### Fixed
- Fixed the bash tool's `kill` builtin rejecting numeric signals and multiple process operands, stopping after the first failed target, and defaulting to `SIGKILL` instead of the standard `SIGTERM`. Negative PID operands (process groups per `kill(2)`) and the `--` end-of-options marker are now handled instead of being misparsed as signals ([#6779](https://github.com/can1357/oh-my-pi/issues/6779)).
## [17.1.5] - 2026-07-27