From d9e5b145543b63351b253ffbb558502d03b1dfd9 Mon Sep 17 00:00:00 2001 From: asteroida123 <264808420+asteroida123@users.noreply.github.com> Date: Thu, 10 Sep 2026 07:48:04 +0800 Subject: [PATCH 1/2] fix(acp): reap child when monitor future is never polled --- src-tauri/vendor/sacp-tokio/src/acp_agent.rs | 90 ++++++++++++++++---- 1 file changed, 72 insertions(+), 18 deletions(-) diff --git a/src-tauri/vendor/sacp-tokio/src/acp_agent.rs b/src-tauri/vendor/sacp-tokio/src/acp_agent.rs index 3178e5a81b..5019db4457 100644 --- a/src-tauri/vendor/sacp-tokio/src/acp_agent.rs +++ b/src-tauri/vendor/sacp-tokio/src/acp_agent.rs @@ -3,6 +3,7 @@ //! This module provides [`AcpAgent`], a convenient wrapper around [`sacp::schema::McpServer`] //! that can be parsed from either a command string or JSON configuration. +use std::future::Future; use std::path::PathBuf; use std::str::FromStr; use std::sync::Arc; @@ -656,35 +657,42 @@ fn append_limited_utf8(output: &mut String, chunk: &str, limit: usize) -> bool { /// /// The error message includes any stderr output collected by the background task. /// When dropped, the child process is killed. -async fn monitor_child( +fn monitor_child( child: Child, stderr_rx: tokio::sync::oneshot::Receiver, exit_callback: Option>, -) -> Result<(), sacp::Error> { - let mut guard = ChildGuard { +) -> impl Future> { + // Construct the guard before returning the future. `connect_to` races this + // future against the protocol driver; when that driver wins before the + // child monitor's first poll, dropping the future must still drop a guard + // that owns the child and starts the detached reap. + let guard = ChildGuard { child: Some(child), exit_callback, }; - // Wait for the child to exit - let status = guard - .wait() - .await - .map_err(|e| sacp::util::internal_error(format!("Failed to wait for process: {}", e)))?; + async move { + let mut guard = guard; - if status.success() { - Ok(()) - } else { - // Get stderr content if available - let stderr = stderr_rx.await.unwrap_or_default(); + // Wait for the child to exit + let status = guard.wait().await.map_err(|e| { + sacp::util::internal_error(format!("Failed to wait for process: {}", e)) + })?; - let message = if stderr.is_empty() { - format!("Process exited with {}", status) + if status.success() { + Ok(()) } else { - format!("Process exited with {}: {}", status, stderr) - }; + // Get stderr content if available + let stderr = stderr_rx.await.unwrap_or_default(); + + let message = if stderr.is_empty() { + format!("Process exited with {}", status) + } else { + format!("Process exited with {}: {}", status, stderr) + }; - Err(sacp::util::internal_error(message)) + Err(sacp::util::internal_error(message)) + } } } @@ -1100,6 +1108,52 @@ mod tests { assert_eq!(calls.load(Ordering::SeqCst), 1); } + /// Regression for the select race in `connect_to`: the protocol side may + /// finish before the child-monitor future receives its first poll. The + /// monitor must already own a `ChildGuard`, otherwise dropping that + /// unpolled future bypasses kill/reap and never fires `on_exit`. + #[cfg(unix)] + #[test] + fn dropping_an_unpolled_child_monitor_still_reaps_and_reports_exit() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + let calls = Arc::new(AtomicUsize::new(0)); + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("tokio runtime"); + rt.block_on(async { + let mut command = tokio::process::Command::new("/bin/sh"); + command.args(["-c", "sleep 30"]); + command.stderr(std::process::Stdio::piped()); + let mut child = command.spawn().expect("spawn sh"); + let stderr = child.stderr.take().expect("stderr"); + let (stderr_tx, stderr_rx) = tokio::sync::oneshot::channel(); + tokio::spawn(async move { + use tokio::io::AsyncReadExt; + let mut stderr = stderr; + let mut bytes = Vec::new(); + let _ = stderr.read_to_end(&mut bytes).await; + let _ = stderr_tx.send(String::from_utf8_lossy(&bytes).into_owned()); + }); + + let monitor = monitor_child(child, stderr_rx, Some(counting_callback(&calls))); + // Deliberately never poll it. + drop(monitor); + + let mut reported = false; + for _ in 0..200 { + if calls.load(Ordering::SeqCst) == 1 { + reported = true; + break; + } + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + } + assert!(reported, "unpolled monitor bypassed the reap callback"); + }); + assert_eq!(calls.load(Ordering::SeqCst), 1); + } + /// Dropping the guard kills the tree but does not wait, so the exit is not /// observed yet — and must not be reported yet. It has to be reported once /// the child actually dies, which only works because `drop` keeps owning the From 9005ded27a8576fa508887982941b76f7944b50d Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 14:45:05 +0800 Subject: [PATCH 2/2] docs(acp): pin monitor_child's unpolled-drop contract and Send bound State on the signature why monitor_child is deliberately not an `async fn`: the ChildGuard has to be armed when the future is created, because Tokio never kills a raw Child it drops and reaps it out of sight, so an unpolled drop leaves a live agent orphaned with its pid still published. Make the returned future's `Send` explicit instead of relying on RPIT auto-trait leakage, so a non-Send capture is reported against this function rather than against the `sacp::ConnectTo` impl that requires it. Repair the broken `connect_to` intra-doc link in `on_spawn`, which made `cargo doc --document-private-items` fail for the whole crate. --- src-tauri/vendor/sacp-tokio/src/acp_agent.rs | 29 +++++++++++++++++--- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/src-tauri/vendor/sacp-tokio/src/acp_agent.rs b/src-tauri/vendor/sacp-tokio/src/acp_agent.rs index 5019db4457..40c264d31c 100644 --- a/src-tauri/vendor/sacp-tokio/src/acp_agent.rs +++ b/src-tauri/vendor/sacp-tokio/src/acp_agent.rs @@ -377,8 +377,9 @@ impl AcpAgent { /// Register a callback invoked once with the OS process id (pid) of the /// spawned agent process, right after it launches. /// - /// The child is otherwise owned entirely by [`connect_to`]'s internal - /// `ChildGuard`, which kills the whole process tree on drop. But that drop + /// The child is otherwise owned entirely by + /// [`sacp::ConnectTo::connect_to`]'s internal `ChildGuard`, which kills the + /// whole process tree on drop. But that drop /// only runs when the driving future completes — during a host-process /// shutdown the driver may be torn down before it can, leaking the agent /// (and its own child processes) as orphans. Exposing the pid lets the host @@ -656,12 +657,32 @@ fn append_limited_utf8(output: &mut String, chunk: &str, limit: usize) -> bool { /// Waits for a child process and returns an error if it exits with non-zero status. /// /// The error message includes any stderr output collected by the background task. -/// When dropped, the child process is killed. +/// Dropping the returned future drops a [`ChildGuard`], which signals the +/// child's process tree and — given a runtime to reap on — keeps owning the +/// child until it is really gone. Neither half is unconditional: see +/// [`AcpAgent::on_exit`] for why the kill is only a signal, and +/// `ChildGuard::drop` for why a drop outside a runtime stays silent. +/// +/// That much has to survive a drop landing *before the first poll*, which is +/// why this is deliberately NOT an `async fn`: an `async fn` body does not run +/// until it is first polled, so the guard below would not exist yet and the +/// captured raw `Child` would be dropped on its own instead. Tokio never kills +/// on that path and reaps the child out of sight (at once if it has already +/// exited, via the orphan queue otherwise), so a live agent survives as an +/// orphan, `exit_callback` never fires, and the host is left publishing a pid +/// the OS may since have reassigned. The test +/// `dropping_an_unpolled_child_monitor_still_reaps_and_reports_exit` is what +/// catches a change back to `async fn`. +/// +/// `+ Send` is stated rather than left to auto-trait leakage because +/// `sacp::ConnectTo::connect_to` promises a `Send` future and holds this one +/// across an await: without the bound, a non-`Send` capture added here would be +/// reported against that impl rather than against this function. fn monitor_child( child: Child, stderr_rx: tokio::sync::oneshot::Receiver, exit_callback: Option>, -) -> impl Future> { +) -> impl Future> + Send { // Construct the guard before returning the future. `connect_to` races this // future against the protocol driver; when that driver wins before the // child monitor's first poll, dropping the future must still drop a guard