From 74f7286e16a0217aef91470f6bf3904ef8bf884a Mon Sep 17 00:00:00 2001 From: Dan Date: Thu, 6 Aug 2026 15:16:58 -0500 Subject: [PATCH] shpool kill: ignore missing target process If the shell is already gone, kill fails on ESRCH and the error stops the daemon from removing the session from its table, so the session can never be killed and lingers until the daemon restarts. Covered by kill::already_dead_shell. --- libshpool/src/daemon/shell.rs | 16 ++++++-- shpool/tests/kill.rs | 58 ++++++++++++++++++++++++++++ shpool/tests/support/line_matcher.rs | 19 ++++++--- 3 files changed, 84 insertions(+), 9 deletions(-) diff --git a/libshpool/src/daemon/shell.rs b/libshpool/src/daemon/shell.rs index ad65b0fb..9ab07b56 100644 --- a/libshpool/src/daemon/shell.rs +++ b/libshpool/src/daemon/shell.rs @@ -130,13 +130,21 @@ impl Session { // from a process. We can't use the normal SIGTERM graceful-shutdown // signal since shells just forward those to their child process, // but for shells SIGHUP serves as the graceful shutdown signal. - signal::kill(Pid::from_raw(self.child_pid), Some(signal::Signal::SIGHUP)) - .context("sending SIGHUP to child proc")?; + match signal::kill(Pid::from_raw(self.child_pid), Some(signal::Signal::SIGHUP)) { + // ESRCH means "no such process", so the child is already gone and + // there is nothing left to kill. + Err(nix::errno::Errno::ESRCH) => return Ok(()), + res => res.context("sending SIGHUP to child proc")?, + } if self.child_exit_notifier.wait(Some(SHELL_KILL_TIMEOUT)).is_none() { info!("child failed to exit within kill timeout, no longer being polite"); - signal::kill(Pid::from_raw(self.child_pid), Some(signal::Signal::SIGKILL)) - .context("sending SIGKILL to child proc")?; + match signal::kill(Pid::from_raw(self.child_pid), Some(signal::Signal::SIGKILL)) { + // ESRCH means "no such process", so the child exited on its own + // between the SIGHUP and now. + Err(nix::errno::Errno::ESRCH) => return Ok(()), + res => res.context("sending SIGKILL to child proc")?, + } } Ok(()) diff --git a/shpool/tests/kill.rs b/shpool/tests/kill.rs index b3b94ff9..1715f316 100644 --- a/shpool/tests/kill.rs +++ b/shpool/tests/kill.rs @@ -313,6 +313,64 @@ fn running_env_var() -> anyhow::Result<()> { Ok(()) } +/// A session whose shell has already died while nothing was attached still +/// sits in the daemon's table. Killing it must succeed and remove it, rather +/// than failing on ESRCH and leaving a session that can never be killed. +#[test] +#[timeout(30000)] +fn already_dead_shell() -> anyhow::Result<()> { + let mut daemon_proc = support::daemon::Proc::new("norc.toml", DaemonArgs::default()) + .context("starting daemon proc")?; + let bidi_done_w = daemon_proc.events.take().unwrap().waiter(["daemon-bidi-stream-done"]); + + let child_pid: i32; + { + let mut attach_proc = + daemon_proc.attach("sh1", Default::default()).context("starting attach proc")?; + let mut line_matcher = attach_proc.line_matcher()?; + + attach_proc.run_cmd("echo shellpid=$$")?; + let caps = line_matcher.scan_until_re_captures("shellpid=([0-9]+)$")?; + child_pid = caps[1].as_ref().ok_or(anyhow::anyhow!("no pid captured"))?.parse()?; + } + + // Let the daemon notice the client is gone, so the session is sitting in + // the table with no one attached. + daemon_proc.events = Some(bidi_done_w.wait_final_event("daemon-bidi-stream-done")?); + + // Kill the shell out from under the daemon, the way an OOM kill or a stray + // `kill -9` would. + nix::sys::signal::kill( + nix::unistd::Pid::from_raw(child_pid), + Some(nix::sys::signal::Signal::SIGKILL), + ) + .context("killing shell out from under the daemon")?; + + // Wait for the daemon's child watcher to reap it. Until it does the shell + // is a zombie, and signals to a zombie still succeed. + let start = std::time::Instant::now(); + while nix::sys::signal::kill(nix::unistd::Pid::from_raw(child_pid), None).is_ok() { + if start.elapsed() > std::time::Duration::from_secs(10) { + anyhow::bail!("shell {child_pid} was never reaped"); + } + std::thread::sleep(std::time::Duration::from_millis(20)); + } + + // On buggy code the SIGHUP fails with ESRCH, the error propagates, and the + // session is never removed from the table. + let out = daemon_proc.kill(vec![String::from("sh1")])?; + let stderr = String::from_utf8_lossy(&out.stderr[..]); + assert!(out.status.success(), "kill failed: {stderr}"); + assert!(stderr.is_empty(), "unexpected stderr: {stderr}"); + + // and the session should really be gone, not just reported as killed. + let list_out = daemon_proc.list()?; + let listing = String::from_utf8_lossy(&list_out.stdout[..]); + assert!(!listing.contains("sh1"), "session survived the kill: {listing}"); + + Ok(()) +} + #[test] #[timeout(30000)] fn missing() -> anyhow::Result<()> { diff --git a/shpool/tests/support/line_matcher.rs b/shpool/tests/support/line_matcher.rs index ceda30b7..45b72a8a 100644 --- a/shpool/tests/support/line_matcher.rs +++ b/shpool/tests/support/line_matcher.rs @@ -29,6 +29,13 @@ where /// Scan lines until one matches the given regex pub fn scan_until_re(&mut self, re: &str) -> anyhow::Result<()> { + self.scan_until_re_captures(re)?; + + Ok(()) + } + + /// Scan lines until one matches the given regex, returning its captures. + pub fn scan_until_re_captures(&mut self, re: &str) -> anyhow::Result>> { let compiled_re = Regex::new(re)?; let start = time::Instant::now(); let mut line = String::new(); @@ -62,13 +69,15 @@ where self.check_persistant_assertions(&line)?; eprint!("scanning for /{re}/... "); - if compiled_re.is_match(&line) { + if let Some(caps) = compiled_re.captures(&line) { eprintln!(" match"); - return Ok(()); - } else { - eprintln!(" no match"); - line.clear(); + return Ok(caps + .iter() + .map(|maybe_match| maybe_match.map(|m| String::from(m.as_str()))) + .collect()); } + eprintln!(" no match"); + line.clear(); } }