fix(shell): corrected kill signal handling
Accepted numeric signal specifications, signaled every process operand, and restored SIGTERM as the default. Added process-level regressions for multi-target SIGKILL and graceful default termination. Fixes #6779
This commit is contained in:
@@ -2174,8 +2174,120 @@ 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, ¶ms)
|
||||
.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, ¶ms)
|
||||
.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));
|
||||
}
|
||||
|
||||
/// `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")]
|
||||
|
||||
+13
-21
@@ -32,8 +32,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,39 +67,31 @@ 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;
|
||||
// Look through the remaining args for process operands or a -sigspec option.
|
||||
let mut saw_pid_or_job_spec = 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) {
|
||||
// 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;
|
||||
} 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());
|
||||
saw_pid_or_job_spec = true;
|
||||
}
|
||||
}
|
||||
|
||||
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 !saw_pid_or_job_spec {
|
||||
writeln!(context.stderr(), "{}: invalid usage", context.command_name)?;
|
||||
return Ok(ExecutionExitCode::InvalidUsage.into());
|
||||
}
|
||||
|
||||
for pid_or_job_spec in self.args.iter().filter(|arg| !arg.starts_with('-')) {
|
||||
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) {
|
||||
|
||||
@@ -2,6 +2,10 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed the bash tool's `kill` builtin rejecting numeric signals and multiple process operands, and changed its default signal from `SIGKILL` to the standard `SIGTERM` ([#6779](https://github.com/can1357/oh-my-pi/issues/6779)).
|
||||
|
||||
## [17.1.5] - 2026-07-27
|
||||
|
||||
### Added
|
||||
|
||||
Reference in New Issue
Block a user