Merge forge/main into atlas/5034-s4-remove-hive-model

hive-agent/src/web_ui/mod.rs

@@ -343,21 +343,40 @@ fn find_claude_child() -> Option<u32> {
 /// (best-effort). See [`find_claude_child`] for how it's identified —
 /// **not** a name-based `pkill claude`, which would also hit an unrelated
 /// `claude` process someone is running interactively in the container.
-async fn sigint_claude() -> SigintOutcome {
+fn sigint_claude() -> SigintOutcome {
     let Some(pid) = find_claude_child() else {
         return SigintOutcome::NoProcess;
     };
-    match tokio::process::Command::new("kill")
-        .args(["-INT", &pid.to_string()])
-        .status()
-        .await
-    {
-        Ok(status) if status.success() => SigintOutcome::Signalled,
-        // Non-zero from `kill` means "no such process" (ESRCH) — it already
-        // exited between the check above and the signal. Same as never
-        // having found it.
-        Ok(_) => SigintOutcome::NoProcess,
-        Err(e) => SigintOutcome::Failed(e),
+    sigint_turn(pid)
+}
+
+/// SIGINT `pid`, and its whole process group when it leads one, so the
+/// commands a turn's claude started stop with it. `hive-claude` spawns the
+/// turn's claude as the leader of its own group. A `claude` child that
+/// leads none (`plugins.rs`'s `claude plugin` calls) shares the harness's
+/// group, so it is signalled alone.
+fn sigint_turn(pid: u32) -> SigintOutcome {
+    // A child is never pid 0 or 1, and `-0` / `-1` would reach the
+    // harness's own group or every process it may signal.
+    let Some(pid) = i32::try_from(pid).ok().filter(|&pid| pid > 1) else {
+        return SigintOutcome::NoProcess;
+    };
+    // SAFETY: `getpgid(2)` only reads; a gone pid returns -1, never `pid`.
+    let target = if unsafe { libc::getpgid(pid) } == pid {
+        -pid
+    } else {
+        pid
+    };
+    // SAFETY: plain `kill(2)` on the child or the group it leads.
+    if unsafe { libc::kill(target, libc::SIGINT) } == 0 {
+        return SigintOutcome::Signalled;
+    }
+    let err = std::io::Error::last_os_error();
+    if err.raw_os_error() == Some(libc::ESRCH) {
+        // It exited between the scan and the signal: same as never found.
+        SigintOutcome::NoProcess
+    } else {
+        SigintOutcome::Failed(err)
     }
 }
 
@@ -425,14 +444,18 @@ fn broker_error_response(err: &BrokerError, action: &str) -> Response {
 
 #[cfg(test)]
 mod tests {
+    use std::os::unix::process::CommandExt as _;
+    use std::path::Path;
+    use std::process::{Command, Stdio};
     use std::sync::atomic::AtomicBool;
-    use std::sync::{Arc, Mutex, PoisonError};
+    use std::sync::{Arc, Mutex};
+    use std::time::Duration;
 
     use axum::body::Body;
     use axum::http::{Request, StatusCode, header};
     use tower::ServiceExt;
 
-    use super::{AppState, routes};
+    use super::{AppState, SigintOutcome, routes, sigint_turn};
     use crate::events::Bus;
     use crate::login::LoginState;
 
@@ -447,45 +470,127 @@ mod tests {
     /// either.
     #[test]
     fn neither_model_nor_effort_has_a_route() {
-        let dir = tempfile::tempdir().unwrap();
-        let _env = crate::paths::HARNESS_DIR_ENV
-            .lock()
-            .unwrap_or_else(PoisonError::into_inner);
-        let prev = std::env::var_os("HYPERHIVE_HARNESS_DIR");
-        // SAFETY: every test that sets this variable holds `HARNESS_DIR_ENV`.
-        unsafe { std::env::set_var("HYPERHIVE_HARNESS_DIR", dir.path()) };
-        let state = AppState {
-            label: "test".to_owned(),
-            login: Arc::new(Mutex::new(LoginState::Online)),
-            session: Arc::new(Mutex::new(None)),
-            bus: Bus::new(),
-            socket: dir.path().join("mcp.sock"),
-            gui_vnc_port: None,
-            interrupted: Arc::new(AtomicBool::new(false)),
-        };
-        let app = routes().with_state(state);
-        let (model, effort, compact) = tokio::runtime::Builder::new_current_thread()
-            .build()
-            .unwrap()
-            .block_on(async {
-                let model = app.clone().oneshot(form("/api/model", "model=opus"));
-                let effort = app.clone().oneshot(form("/api/effort", "effort=high"));
-                // A route the router does serve, so the 404s above are not
-                // the router refusing everything.
-                let compact = app.oneshot(form("/api/compact", ""));
-                (
-                    model.await.unwrap(),
-                    effort.await.unwrap(),
-                    compact.await.unwrap(),
-                )
-            });
-        match prev {
-            // SAFETY: as above.
-            Some(v) => unsafe { std::env::set_var("HYPERHIVE_HARNESS_DIR", v) },
-            None => unsafe { std::env::remove_var("HYPERHIVE_HARNESS_DIR") },
-        }
+        let (model, effort, compact) = crate::paths::with_harness_dir(|| {
+            let state = AppState {
+                label: "test".to_owned(),
+                login: Arc::new(Mutex::new(LoginState::Online)),
+                session: Arc::new(Mutex::new(None)),
+                bus: Bus::new(),
+                socket: crate::paths::harness_dir().join("mcp.sock"),
+                gui_vnc_port: None,
+                interrupted: Arc::new(AtomicBool::new(false)),
+            };
+            let app = routes().with_state(state);
+            tokio::runtime::Builder::new_current_thread()
+                .build()
+                .unwrap()
+                .block_on(async {
+                    let model = app.clone().oneshot(form("/api/model", "model=opus"));
+                    let effort = app.clone().oneshot(form("/api/effort", "effort=high"));
+                    // A route the router does serve, so the 404s above are not
+                    // the router refusing everything.
+                    let compact = app.oneshot(form("/api/compact", ""));
+                    (
+                        model.await.unwrap(),
+                        effort.await.unwrap(),
+                        compact.await.unwrap(),
+                    )
+                })
+        });
         assert_eq!(model.status(), StatusCode::NOT_FOUND);
         assert_eq!(effort.status(), StatusCode::NOT_FOUND);
         assert_eq!(compact.status(), StatusCode::OK);
     }
+
+    /// Poll `path` until it holds a complete, newline-terminated pid.
+    fn read_pid(path: &Path) -> i32 {
+        for _ in 0..250 {
+            if let Ok(text) = std::fs::read_to_string(path)
+                && text.ends_with('\n')
+            {
+                return text.trim().parse().expect("pid file holds a pid");
+            }
+            std::thread::sleep(Duration::from_millis(20));
+        }
+        unreachable!("no pid in {} within 5s", path.display());
+    }
+
+    /// Whether `pid` is still running. A zombie counts as gone: it has
+    /// exited, and `kill(pid, 0)` only keeps finding it until it is reaped.
+    fn is_running(pid: i32) -> bool {
+        // SAFETY: signal 0 delivers nothing; it only checks that `pid` exists.
+        if unsafe { libc::kill(pid, 0) } != 0 {
+            return false;
+        }
+        !std::fs::read_to_string(format!("/proc/{pid}/stat")).is_ok_and(|stat| {
+            stat.rsplit_once(") ")
+                .is_some_and(|(_, rest)| rest.starts_with('Z'))
+        })
+    }
+
+    #[test]
+    fn a_cancel_stops_the_commands_a_turn_started() {
+        let dir = tempfile::tempdir().expect("tempdir");
+        let pidfile = dir.path().join("grandchild.pid");
+        // A turn's claude as hive-claude spawns it: leading its own group,
+        // with a command of its own running in the foreground (a
+        // non-interactive shell starts `&` jobs with SIGINT ignored). The
+        // trailing `exit` keeps `sh -c` from exec'ing into that command.
+        let mut turn = Command::new("/bin/sh")
+            .args([
+                "-c",
+                r#"sh -c 'echo $$ >"$1"; exec sleep 300' sh "$1"; exit"#,
+                "sh",
+            ])
+            .arg(&pidfile)
+            .stdout(Stdio::null())
+            .stderr(Stdio::null())
+            .process_group(0)
+            .spawn()
+            .expect("spawn the stand-in turn");
+        let grandchild = read_pid(&pidfile);
+        assert_ne!(
+            i32::try_from(turn.id()).ok(),
+            Some(grandchild),
+            "the command must be a child of the turn, not the turn itself"
+        );
+
+        assert!(matches!(sigint_turn(turn.id()), SigintOutcome::Signalled));
+
+        let mut alive = is_running(grandchild);
+        for _ in 0..250 {
+            if !alive {
+                break;
+            }
+            std::thread::sleep(Duration::from_millis(20));
+            alive = is_running(grandchild);
+        }
+        if alive {
+            // SAFETY: `kill(2)` on a pid this test started; keeps a failed
+            // run from leaving a 300s `sleep` behind.
+            unsafe {
+                libc::kill(grandchild, libc::SIGKILL);
+            }
+        }
+        let _ = turn.wait();
+        assert!(!alive, "grandchild {grandchild} outlived the cancel");
+    }
+
+    #[test]
+    fn a_claude_sharing_the_harness_group_is_signalled_alone() {
+        // In this test process's own group, as `claude plugin` runs. A group
+        // signal here would SIGINT the test binary itself.
+        let mut child = Command::new("sleep")
+            .arg("300")
+            .spawn()
+            .expect("spawn sleep");
+
+        assert!(matches!(sigint_turn(child.id()), SigintOutcome::Signalled));
+
+        let status = child.wait().expect("reap sleep");
+        assert_eq!(
+            std::os::unix::process::ExitStatusExt::signal(&status),
+            Some(libc::SIGINT)
+        );
+    }
 }