fix(mcp): clean up stdio test process trees (#368)

## Summary
- Move stdio MCP connection-test timeout handling inside the protocol
step so child cleanup always runs.
- Add a shared runtime helper for process-tree cleanup across Unix and
Windows.
- Add a Unix regression test that verifies stdio timeouts do not leave
the spawned process group alive.

## Test Plan
- `just push -u origin fix/mcp-stdio-timeout-cleanup`
  - includes `cargo fix --allow-dirty --allow-staged`
- includes `cargo clippy --fix --workspace --allow-dirty --allow-staged
-- -D warnings`
  - includes `cargo fmt --all`
  - includes `cargo nextest run --workspace` (5708 passed, 18 skipped)

Co-authored-by: zynx <>

Author: piorpua

Cargo.lock

@@ -4,10 +4,10 @@ use std::collections::HashMap;
 use std::time::Duration;
 
 use aionui_api_types::McpConnectionTestResult;
-use aionui_runtime::Builder as CmdBuilder;
+use aionui_runtime::{Builder as CmdBuilder, kill_process_tree};
 use serde::Serialize;
 use tokio::sync::mpsc;
-use tracing::debug;
+use tracing::{debug, warn};
 
 use crate::types::McpServerTransport;
 use protocol::{
@@ -72,10 +72,7 @@ impl McpConnectionTestService {
         args: &[String],
         env: &HashMap<String, String>,
     ) -> McpConnectionTestResult {
-        match tokio::time::timeout(self.timeout, self.test_stdio_inner(command, args, env)).await {
-            Ok(r) => r,
-            Err(_) => timeout_result(self.timeout),
-        }
+        self.test_stdio_inner(command, args, env).await
     }
 
     async fn test_stdio_inner(
@@ -98,8 +95,13 @@ impl McpConnectionTestService {
 
         let stdin = child.stdin.take().expect("stdin was piped");
         let stdout = child.stdout.take().expect("stdout was piped");
-        let result = run_stdio_protocol(stdin, stdout).await;
-        kill_child_tree(&mut child).await;
+        let result = match tokio::time::timeout(self.timeout, run_stdio_protocol(stdin, stdout)).await {
+            Ok(r) => r,
+            Err(_) => timeout_result(self.timeout),
+        };
+        if let Err(error) = kill_process_tree(&mut child).await {
+            warn!(%error, "failed to clean up MCP stdio connection test process tree");
+        }
         result
     }
 
@@ -316,36 +318,6 @@ struct HttpMcpResponse {
     session_id: Option<String>,
 }
 
-async fn kill_child_tree(child: &mut tokio::process::Child) {
-    if let Some(pid) = child.id() {
-        #[cfg(unix)]
-        {
-            let process_group = format!("-{pid}");
-            let _ = tokio::process::Command::new("kill")
-                .args(["-KILL", &process_group])
-                .status()
-                .await;
-        }
-
-        #[cfg(windows)]
-        {
-            let _ = tokio::process::Command::new("taskkill")
-                .args(["/PID", &pid.to_string(), "/T", "/F"])
-                .status()
-                .await;
-        }
-
-        #[cfg(not(any(unix, windows)))]
-        {
-            let _ = child.kill().await;
-        }
-    } else {
-        let _ = child.kill().await;
-    }
-
-    let _ = child.wait().await;
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -361,4 +333,82 @@ mod tests {
         let svc = McpConnectionTestService::new(reqwest::Client::new()).with_timeout(Duration::from_secs(5));
         assert_eq!(svc.timeout, Duration::from_secs(5));
     }
+
+    #[cfg(unix)]
+    #[tokio::test]
+    async fn stdio_timeout_cleans_up_process_group() {
+        let marker_path = std::env::temp_dir().join(format!(
+            "aionui-mcp-timeout-pid-{}-{}",
+            std::process::id(),
+            std::time::SystemTime::now()
+                .duration_since(std::time::UNIX_EPOCH)
+                .unwrap()
+                .as_nanos()
+        ));
+        let transport = McpServerTransport::Stdio {
+            command: "sh".into(),
+            args: vec![
+                "-c".into(),
+                "printf '%s\n' \"$$\" > \"$1\"; sleep 30".into(),
+                "mcp-timeout-child".into(),
+                marker_path.to_string_lossy().into_owned(),
+            ],
+            env: HashMap::new(),
+        };
+        let svc = McpConnectionTestService::new(reqwest::Client::new()).with_timeout(Duration::from_millis(100));
+
+        let result = svc.test_connection("timeout-cleanup", &transport).await;
+        assert!(!result.success);
+        assert!(
+            result.error.as_deref().unwrap_or_default().contains("timed out"),
+            "expected timeout result, got {result:?}"
+        );
+
+        let pid: i32 = std::fs::read_to_string(&marker_path)
+            .expect("stdio child should write its pid")
+            .trim()
+            .parse()
+            .expect("pid marker should be numeric");
+
+        let group_alive = wait_for_process_group_exit(pid, Duration::from_secs(1)).await;
+        if group_alive {
+            let _ = kill_process_group(pid, libc_sigkill());
+        }
+        let _ = std::fs::remove_file(marker_path);
+
+        assert!(
+            !group_alive,
+            "stdio timeout should terminate the spawned process group for pid={pid}"
+        );
+    }
+
+    #[cfg(unix)]
+    async fn wait_for_process_group_exit(pid: i32, timeout: Duration) -> bool {
+        let deadline = tokio::time::Instant::now() + timeout;
+        while tokio::time::Instant::now() < deadline {
+            if !is_process_group_alive(pid) {
+                return false;
+            }
+            tokio::time::sleep(Duration::from_millis(50)).await;
+        }
+        is_process_group_alive(pid)
+    }
+
+    #[cfg(unix)]
+    fn is_process_group_alive(pid: i32) -> bool {
+        kill_process_group(pid, 0)
+    }
+
+    #[cfg(unix)]
+    fn kill_process_group(pid: i32, signal: i32) -> bool {
+        unsafe extern "C" {
+            fn kill(pid: i32, sig: i32) -> i32;
+        }
+        unsafe { kill(-pid, signal) == 0 }
+    }
+
+    #[cfg(unix)]
+    fn libc_sigkill() -> i32 {
+        9
+    }
 }

crates/aionui-mcp/src/connection_test/mod.rs

@@ -39,4 +39,5 @@ sha2.workspace = true
 hex.workspace = true
 
 [target.'cfg(unix)'.dependencies]
+libc.workspace = true
 wait-timeout = "0.2"

crates/aionui-runtime/Cargo.toml

@@ -15,7 +15,7 @@ pub use cache::init;
 pub use resolver::{ResolveError, bun_bin_dir, resolve_bun, resolve_command_in, resolve_command_path};
 pub use shell_env::enhance_process_path;
 mod spawn;
-pub use spawn::Builder;
+pub use spawn::{Builder, kill_process_tree};
 
 #[cfg(test)]
 #[path = "../build_support.rs"]

crates/aionui-runtime/src/lib.rs

@@ -44,6 +44,25 @@ pub struct Builder {
     mode: Mode,
 }
 
+/// Force-kill a spawned child and wait for the direct child handle to exit.
+///
+/// On Unix, children spawned through [`Builder::new`] are process-group
+/// leaders, so this targets that group to clean up descendants as well. On
+/// Windows, this uses `taskkill /T` to terminate the process tree.
+pub async fn kill_process_tree(child: &mut Child) -> io::Result<()> {
+    let Some(pid) = child.id() else {
+        return child.kill().await;
+    };
+
+    #[cfg(unix)]
+    force_kill_process_tree(pid)?;
+    #[cfg(windows)]
+    kill_windows_process_tree(pid).await?;
+    #[cfg(not(any(unix, windows)))]
+    child.kill().await?;
+    child.wait().await.map(|_| ())
+}
+
 impl std::fmt::Debug for Builder {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.debug_struct("Builder")
@@ -191,6 +210,59 @@ fn configure_platform_spawn(cmd: &mut Command) {
 #[cfg(not(unix))]
 fn configure_platform_spawn(_cmd: &mut Command) {}
 
+#[cfg(unix)]
+fn force_kill_process_tree(pid: u32) -> io::Result<()> {
+    let pgid = unsafe { libc::getpgid(pid as i32) };
+    if pgid == -1 {
+        let err = io::Error::last_os_error();
+        if err.raw_os_error() == Some(libc::ESRCH) {
+            return Ok(());
+        }
+        return kill_unix_target(pid as i32);
+    }
+
+    if pgid > 1 && pgid as u32 == pid {
+        kill_unix_target(-pgid)
+    } else {
+        kill_unix_target(pid as i32)
+    }
+}
+
+#[cfg(unix)]
+fn kill_unix_target(target: i32) -> io::Result<()> {
+    let result = unsafe { libc::kill(target, libc::SIGKILL) };
+    if result == 0 {
+        return Ok(());
+    }
+
+    let err = io::Error::last_os_error();
+    if err.raw_os_error() == Some(libc::ESRCH) {
+        Ok(())
+    } else {
+        Err(err)
+    }
+}
+
+#[cfg(windows)]
+async fn kill_windows_process_tree(pid: u32) -> io::Result<()> {
+    let pid_arg = pid.to_string();
+    let mut cmd = Builder::clean_cli("taskkill");
+    cmd.args(["/F", "/T", "/PID", pid_arg.as_str()]);
+    let output = cmd.output().await?;
+    if output.status.success() || output.status.code() == Some(128) {
+        return Ok(());
+    }
+
+    Err(io::Error::new(
+        io::ErrorKind::Other,
+        format!(
+            "taskkill failed for pid {pid} (exit {:?}): {}",
+            output.status.code(),
+            String::from_utf8_lossy(&output.stderr)
+        ),
+    ))
+}
+
 /// Resolve `program` through `resolve_command_path` so callers don't have
 /// to. If the input already contains a path separator (relative or
 /// absolute) we leave it alone — only bare command names go through