acp: Ensure connection subprocess gets killed on drop (#37858)

Agus Zubiaga created 4 months ago

It appears that in macOS, the `AcpConnection._wait_task` doesn't always
get dropped when quitting the app. In these cases, the subprocess would
be kept alive because we move the `child` into it.

Instead, we will now explicitly kill it when `AcpConnection` is dropped.
It's ok to do this because when the connection is dropped, the thread is
also dropped, so there's no need to report the exit status to it.

Closes #37741

Release Notes:

- Claude Code: Fix subprocess leak on app quit

Change summary

crates/agent_servers/src/acp.rs | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)

Detailed changes

crates/agent_servers/src/acp.rs

@@ -9,6 +9,7 @@ use futures::AsyncBufReadExt as _;
 use futures::io::BufReader;
 use project::Project;
 use serde::Deserialize;
+use util::ResultExt as _;
 
 use std::{any::Any, cell::RefCell};
 use std::{path::Path, rc::Rc};
@@ -29,6 +30,11 @@ pub struct AcpConnection {
     sessions: Rc<RefCell<HashMap<acp::SessionId, AcpSession>>>,
     auth_methods: Vec<acp::AuthMethod>,
     agent_capabilities: acp::AgentCapabilities,
+    default_mode: Option<acp::SessionModeId>,
+    root_dir: PathBuf,
+    // NB: Don't move this into the wait_task, since we need to ensure the process is
+    // killed on drop (setting kill_on_drop on the command seems to not always work).
+    child: smol::process::Child,
     _io_task: Task<Result<()>>,
     _wait_task: Task<Result<()>>,
     _stderr_task: Task<Result<()>>,
@@ -64,9 +70,11 @@ impl AcpConnection {
             .current_dir(root_dir)
             .stdin(std::process::Stdio::piped())
             .stdout(std::process::Stdio::piped())
-            .stderr(std::process::Stdio::piped())
-            .kill_on_drop(true)
-            .spawn()?;
+            .stderr(std::process::Stdio::piped());
+        if !is_remote {
+            child.current_dir(root_dir);
+        }
+        let mut child = child.spawn()?;
 
         let stdout = child.stdout.take().context("Failed to take stdout")?;
         let stdin = child.stdin.take().context("Failed to take stdin")?;
@@ -102,8 +110,9 @@ impl AcpConnection {
 
         let wait_task = cx.spawn({
             let sessions = sessions.clone();
+            let status_fut = child.status();
             async move |cx| {
-                let status = child.status().await?;
+                let status = status_fut.await?;
 
                 for session in sessions.borrow().values() {
                     session
@@ -152,6 +161,7 @@ impl AcpConnection {
             _io_task: io_task,
             _wait_task: wait_task,
             _stderr_task: stderr_task,
+            child,
         })
     }
 
@@ -160,6 +170,13 @@ impl AcpConnection {
     }
 }
 
+impl Drop for AcpConnection {
+    fn drop(&mut self) {
+        // See the comment on the child field.
+        self.child.kill().log_err();
+    }
+}
+
 impl AgentConnection for AcpConnection {
     fn new_thread(
         self: Rc<Self>,