Fix actix worker wedge on session teardown; grant CAP_KILL; bump to 0.3.12

Terminal::stopping did child.kill() (error ignored) and then a blocking
child.wait() on the actix worker thread. Since the 0.3.11 privilege drop,
sessions run as `demo` and the server had no CAP_KILL, so kill() failed
with EPERM and wait() blocked forever. With two workers, exactly every
other request to :8082 then timed out — 5 days of readiness/liveness
flapping on socktop.io and orphaned restricted-shell/socktop processes
piling up in the pods.

- Signal the session's whole process group (portable-pty setsid()s the
  child), not just the shell: a non-interactive bash defers signals while
  a foreground command runs, so the old SIGHUP/SIGKILL to the shell alone
  orphaned socktop anyway.
- Reap on a dedicated thread: SIGHUP, 2 s grace, SIGKILL, 10 s deadline,
  then a blocking wait on that thread only. Signal failures are logged.
- Drop the pty handles before handing off the child.
- kubernetes/03-deployment.yaml: add CAP_KILL (sessions still lose every
  cap via setpriv). CI never applies the manifest — it was patched live.
- tests/reaper_tests.rs pins the non-blocking return, process-group kill,
  and SIGHUP→SIGKILL escalation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Cargo.lock

@@ -2609,7 +2609,7 @@ dependencies = [
 
 [[package]]
 name = "webterm"
-version = "0.3.11"
+version = "0.3.12"
 dependencies = [
  "actix",
  "actix-files",

Cargo.toml

@@ -6,7 +6,7 @@ documentation = "https://docs.rs/webterm"
 readme = "README.md"
 categories = ["web-programming", "web-programming::websocket", "web-programming::http-server", "command-line-utilities"]
 keywords = ["terminal", "xterm", "websocket", "terminus", "console"]
-version = "0.3.11"
+version = "0.3.12"
 authors = ["fabian.freyer@physik.tu-berlin.de","jasonpwitty+socktop@proton.me"]
 edition = "2021"
 license = "BSD-3-Clause"

kubernetes/03-deployment.yaml

@@ -85,7 +85,7 @@ spec:
 
       containers:
         - name: webterm
-          image: gt.wittyoneoff.com/jason/socktop-webterm:0.3.11
+          image: gt.wittyoneoff.com/jason/socktop-webterm:0.3.12
           imagePullPolicy: Always
 
           command: ["/docker-entrypoint.sh"]
@@ -165,13 +165,13 @@ spec:
             - name: socktop-home
               mountPath: /var/lib/socktop
 
-          # webterm-server runs as in-container root holding ONLY
+          # webterm-server runs as in-container root holding only the caps
-          # CAP_SETUID/CAP_SETGID (everything else dropped, no privilege
+          # listed below (everything else dropped, no privilege escalation),
-          # escalation), so it can drop each websocket session to the
+          # so it can drop each websocket session to the unprivileged `demo`
-          # unprivileged `demo` user via session-shell.sh. The kernel then
+          # user via session-shell.sh. The kernel then refuses any signal a
-          # refuses any signal a session aims at the server, the agent
+          # session aims at the server, the agent (running as `socktop`), or
-          # (running as `socktop`), or another session's UID — the kill
+          # another session's UID — the kill feature's UI gating stops being
-          # feature's UI gating stops being the only line of defense.
+          # the only line of defense.
           securityContext:
             allowPrivilegeEscalation: false
             capabilities:
@@ -180,6 +180,14 @@ spec:
               add:
                 - SETUID
                 - SETGID
+                # The server must be able to signal the sessions it spawned,
+                # which run as `demo` — a different uid — so root needs
+                # CAP_KILL for that. Without it every idle-timeout teardown
+                # got EPERM, the session lived on, and (0.3.11) the actix
+                # worker blocked in wait() on it: half of all requests hung.
+                # Sessions still cannot signal anything: setpriv drops every
+                # cap (including this one) before the restricted shell runs.
+                - KILL
                 # prepare_demo_home (entrypoint.sh) writes into and re-owns
                 # /home/demo, which the image ships as demo-owned 700. With
                 # ALL dropped, uid 0 has no implicit file privilege, so the

src/lib.rs

@@ -218,6 +218,113 @@ impl Handler<ChildDied> for Websocket {
     }
 }
 
+/// Grace the session gets to exit on SIGHUP before the reaper sends SIGKILL.
+const CHILD_EXIT_GRACE: Duration = Duration::from_secs(2);
+/// Upper bound on how long the reaper polls after SIGKILL before it logs the
+/// survivor and falls back to a blocking wait on its own thread.
+const CHILD_REAP_DEADLINE: Duration = Duration::from_secs(10);
+const CHILD_POLL_INTERVAL: Duration = Duration::from_millis(100);
+
+/// Send `signal` to the child's whole process group.
+///
+/// portable-pty spawns the child with `setsid()`, so its pid is also its pgid
+/// and `kill(-pid)` reaches every process in the session — the shell *and*
+/// whatever it is running. Signalling only the shell is not enough: a
+/// non-interactive bash waiting on a foreground command defers signal handling
+/// until that command exits, so the shell dies on SIGKILL and its child is
+/// orphaned still holding the pty.
+fn signal_process_group(pid: u32, signal: libc::c_int) -> std::io::Result<()> {
+    let pgid = -(pid as libc::pid_t);
+    // SAFETY: plain libc call with a pgid we own; no memory is touched.
+    if unsafe { libc::kill(pgid, signal) } == 0 {
+        Ok(())
+    } else {
+        Err(std::io::Error::last_os_error())
+    }
+}
+
+/// Terminate and reap a session's child process without blocking the caller.
+///
+/// SIGHUP goes to the process group immediately; a detached thread then polls
+/// for exit, escalates to SIGKILL after [`CHILD_EXIT_GRACE`], and only after
+/// [`CHILD_REAP_DEADLINE`] gives up polling and parks in a blocking `wait()` —
+/// on its own thread, so a session the server is not permitted to signal can
+/// never wedge an actix worker again. Signal failures are logged, not
+/// swallowed: `EPERM` here means the container lacks `CAP_KILL` while sessions
+/// run as a different uid.
+///
+/// Returns the reaper's `JoinHandle`; callers normally drop it.
+pub fn reap_child(mut child: Box<dyn portable_pty::Child + Send>) -> std::thread::JoinHandle<()> {
+    let pid = child.process_id();
+
+    if let Some(pid) = pid {
+        match signal_process_group(pid, libc::SIGHUP) {
+            Ok(()) => log::debug!("Sent SIGHUP to session process group {}", pid),
+            Err(e) => log::error!(
+                "Cannot SIGHUP session process group {}: {} \
+                 (EPERM means webterm-server lacks CAP_KILL for cross-uid sessions)",
+                pid,
+                e
+            ),
+        }
+    }
+
+    std::thread::Builder::new()
+        .name(format!("reap-{}", pid.unwrap_or(0)))
+        .spawn(move || {
+            let started = Instant::now();
+            let mut killed = false;
+
+            loop {
+                match child.try_wait() {
+                    Ok(Some(status)) => {
+                        log::info!(
+                            "Session {} exited with {:?} after {:?}",
+                            pid.unwrap_or(0),
+                            status,
+                            started.elapsed()
+                        );
+                        return;
+                    }
+                    Ok(None) => {}
+                    Err(e) => {
+                        log::error!("try_wait on session {} failed: {}", pid.unwrap_or(0), e);
+                        return;
+                    }
+                }
+
+                let elapsed = started.elapsed();
+                if !killed && elapsed >= CHILD_EXIT_GRACE {
+                    killed = true;
+                    if let Some(pid) = pid {
+                        match signal_process_group(pid, libc::SIGKILL) {
+                            Ok(()) => log::warn!(
+                                "Session {} ignored SIGHUP for {:?}; sent SIGKILL",
+                                pid,
+                                elapsed
+                            ),
+                            Err(e) => {
+                                log::error!("Cannot SIGKILL session process group {}: {}", pid, e)
+                            }
+                        }
+                    }
+                }
+                if elapsed >= CHILD_REAP_DEADLINE {
+                    log::error!(
+                        "Session {} still alive {:?} after SIGKILL; reaper parking in wait()",
+                        pid.unwrap_or(0),
+                        elapsed
+                    );
+                    let _ = child.wait();
+                    return;
+                }
+
+                std::thread::sleep(CHILD_POLL_INTERVAL);
+            }
+        })
+        .expect("failed to spawn session reaper thread")
+}
+
 /// Represents a PTY backend with attached child
 pub struct Terminal {
     pty_master: Option<Box<dyn portable_pty::MasterPty + Send>>,
@@ -367,9 +474,17 @@ impl Actor for Terminal {
     fn stopping(&mut self, _ctx: &mut Self::Context) -> Running {
         log::info!("Stopping Terminal");
 
-        if let Some(mut child) = self.child.take() {
+        // Release our side of the pty first so the reader thread sees EOF once
-            let _ = child.kill();
+        // the session is gone, then hand the child to the reaper. This used to
-            let _ = child.wait();
+        // be `child.kill(); child.wait();` inline — a blocking waitpid on the
+        // actix worker thread. When the kill was refused (the session runs as
+        // another uid and the server had no CAP_KILL) the child lived on and
+        // the worker hung forever, taking half the server's connections with
+        // it. Nothing here may block.
+        self.pty_writer = None;
+        self.pty_master = None;
+        if let Some(child) = self.child.take() {
+            reap_child(child);
         }
 
         // Notify the websocket that the child died.

tests/reaper_tests.rs

@@ -0,0 +1,144 @@
+// Copyright (c) 2024 Jason Witty <jasonpwitty+socktop@proton.me>.
+// All rights reserved.
+//
+// Regression tests for session teardown (`webterm::reap_child`).
+//
+// Background: `Terminal::stopping` used to call `child.kill()` then a blocking
+// `child.wait()` on the actix worker thread. When the signal was refused
+// (sessions run as another uid, server without CAP_KILL) or only reached the
+// shell (a non-interactive bash defers signals while a foreground command
+// runs), the child survived and the worker hung forever — every other request
+// to the server then timed out. These tests pin the fixed behaviour: the whole
+// process group is signalled, SIGHUP escalates to SIGKILL, and the caller is
+// never blocked.
+
+use std::io::Read;
+use std::time::{Duration, Instant};
+
+use portable_pty::{native_pty_system, CommandBuilder, PtySize};
+
+/// True while `pid` exists and is not a zombie (kill(pid, 0) also succeeds on
+/// zombies, so read /proc directly).
+fn alive(pid: i32) -> bool {
+    match std::fs::read_to_string(format!("/proc/{pid}/status")) {
+        Ok(s) => !s
+            .lines()
+            .any(|l| l.starts_with("State:") && l.contains("Z (zombie)")),
+        Err(_) => false,
+    }
+}
+
+fn wait_gone(pid: i32, timeout: Duration) -> bool {
+    let start = Instant::now();
+    while start.elapsed() < timeout {
+        if !alive(pid) {
+            return true;
+        }
+        std::thread::sleep(Duration::from_millis(50));
+    }
+    !alive(pid)
+}
+
+/// Spawn `script` under `sh -c` in a fresh pty. The script must print the pid
+/// of its long-running grandchild as its first line; that pid is returned along
+/// with the pty child handle.
+fn spawn_in_pty(script: &str) -> (Box<dyn portable_pty::Child + Send>, i32) {
+    let pty = native_pty_system();
+    let pair = pty
+        .openpty(PtySize {
+            rows: 24,
+            cols: 80,
+            pixel_width: 0,
+            pixel_height: 0,
+        })
+        .expect("openpty");
+
+    let mut cmd = CommandBuilder::new("sh");
+    cmd.arg("-c");
+    cmd.arg(script);
+    let child = pair.slave.spawn_command(cmd).expect("spawn");
+    drop(pair.slave);
+
+    let mut reader = pair.master.try_clone_reader().expect("reader");
+    let mut buf = Vec::new();
+    let mut byte = [0u8; 1];
+    let deadline = Instant::now() + Duration::from_secs(5);
+    while Instant::now() < deadline {
+        match reader.read(&mut byte) {
+            Ok(1) if byte[0] == b'\n' => break,
+            Ok(1) => buf.push(byte[0]),
+            _ => break,
+        }
+    }
+    let grandchild: i32 = String::from_utf8_lossy(&buf)
+        .trim()
+        .parse()
+        .expect("grandchild pid line");
+    assert!(
+        alive(grandchild),
+        "grandchild {grandchild} should be running"
+    );
+
+    // Keep the master alive for the duration of the test (mirrors the
+    // server's reader thread holding a cloned fd), so the kernel does not
+    // hang up the pty for us and the reaper alone has to do the work.
+    std::mem::forget(pair.master);
+    std::mem::forget(reader);
+
+    (child, grandchild)
+}
+
+#[test]
+fn reap_child_returns_immediately_and_kills_whole_process_group() {
+    // sh waits on a foreground sleep: signalling sh alone would be deferred.
+    let (child, sleeper) = spawn_in_pty("sleep 30 & echo $!; wait");
+    let shell = child.process_id().expect("pid") as i32;
+
+    let t0 = Instant::now();
+    let reaper = webterm::reap_child(child);
+    assert!(
+        t0.elapsed() < Duration::from_millis(500),
+        "reap_child must not block the caller (took {:?})",
+        t0.elapsed()
+    );
+
+    reaper.join().expect("reaper thread");
+    assert!(
+        wait_gone(shell, Duration::from_secs(2)),
+        "shell {shell} survived"
+    );
+    assert!(
+        wait_gone(sleeper, Duration::from_secs(2)),
+        "grandchild {sleeper} was orphaned instead of killed with its group"
+    );
+}
+
+#[test]
+fn reap_child_escalates_to_sigkill_when_sighup_is_ignored() {
+    // `trap '' HUP` makes sh ignore SIGHUP and the ignored disposition is
+    // inherited across exec, so sleep ignores it too — exactly the shape of a
+    // TUI that swallows HUP.
+    let (child, sleeper) = spawn_in_pty("trap '' HUP; sleep 30 & echo $!; wait");
+    let shell = child.process_id().expect("pid") as i32;
+
+    let t0 = Instant::now();
+    webterm::reap_child(child).join().expect("reaper thread");
+    let took = t0.elapsed();
+
+    assert!(
+        took >= Duration::from_secs(2),
+        "should have waited out the SIGHUP grace period (took {took:?})"
+    );
+    assert!(
+        took < Duration::from_secs(8),
+        "SIGKILL escalation should finish well inside the deadline (took {took:?})"
+    );
+    assert!(
+        wait_gone(shell, Duration::from_secs(2)),
+        "shell {shell} survived SIGKILL"
+    );
+    assert!(
+        wait_gone(sleeper, Duration::from_secs(2)),
+        "grandchild {sleeper} survived SIGKILL"
+    );
+}