fix: shpool kill: ignore missing target process

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(())

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<()> {

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<Vec<Option<String>>> {
         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();
         }
     }