fix(debug): wait for Windows debuggee trees to exit (#2349)

crates/ide-core/src/process.rs

@@ -104,6 +104,9 @@ fn signal_group(child: &Child, signal: libc::c_int) {
 #[cfg(windows)]
 const TREE_EXIT_WAIT: Duration = Duration::from_secs(5);
 
+#[cfg(windows)]
+pub use windows_tree::ProcessTree;
+
 /// `taskkill /T /F` from `pid`, then waits (up to [`TREE_EXIT_WAIT`]) until what the child
 /// started is gone: termination is asynchronous, and until a process is gone the files it
 /// holds open can't be opened or deleted. The child itself is left to its owner to reap.
@@ -136,13 +139,48 @@ mod windows_tree {
         CreateToolhelp32Snapshot, PROCESSENTRY32W, Process32FirstW, Process32NextW, TH32CS_SNAPPROCESS,
     };
     use windows_sys::Win32::System::Threading::{
-        GetProcessTimes, OpenProcess, PROCESS_QUERY_LIMITED_INFORMATION, PROCESS_SYNCHRONIZE, WaitForSingleObject,
+        GetProcessTimes, OpenProcess, PROCESS_QUERY_LIMITED_INFORMATION, PROCESS_SYNCHRONIZE, PROCESS_TERMINATE,
+        TerminateProcess, WaitForSingleObject,
     };
 
+    /// Owned handles to a program and its current descendants, retained even if its launcher exits.
+    pub struct ProcessTree {
+        processes: Vec<OwnedHandle>,
+    }
+
+    impl ProcessTree {
+        pub fn capture(pid: u32) -> Self {
+            let Some(root) = open_with(pid, PROCESS_TERMINATE) else { return Self { processes: Vec::new() } };
+            let children = descendants_with(pid, PROCESS_TERMINATE);
+            Self { processes: std::iter::once(root).chain(children).collect() }
+        }
+
+        pub fn is_running(&self) -> bool {
+            self.processes.iter().any(|process| {
+                // SAFETY: an owned process handle opened with SYNCHRONIZE access.
+                unsafe {
+                    WaitForSingleObject(process.as_raw_handle(), 0) == windows_sys::Win32::Foundation::WAIT_TIMEOUT
+                }
+            })
+        }
+
+        /// Requests termination children first; poll `is_running` until termination completes.
+        pub fn terminate(&self) {
+            for process in self.processes.iter().rev() {
+                // SAFETY: an owned process handle opened with TERMINATE access, never a reused pid.
+                unsafe { TerminateProcess(process.as_raw_handle(), 1) };
+            }
+        }
+    }
+
     /// Every process `root` started, directly or not, opened so it can be waited for. A
     /// parent's id is reused once it is gone, so a process counts only if it started after
     /// its parent.
     pub(super) fn descendants(root: u32) -> Vec<OwnedHandle> {
+        descendants_with(root, 0)
+    }
+
+    fn descendants_with(root: u32, access: u32) -> Vec<OwnedHandle> {
         let Some(root_started) = open(root).as_ref().and_then(started) else { return Vec::new() };
         let processes = processes();
         let mut seen = HashSet::from([root]);
@@ -153,7 +191,7 @@ mod windows_tree {
                 if ppid != parent || !seen.insert(pid) {
                     continue;
                 }
-                let Some(handle) = open(pid) else { continue };
+                let Some(handle) = open_with(pid, access) else { continue };
                 if let Some(at) = started(&handle).filter(|&at| at >= parent_started) {
                     parents.push((pid, at));
                     found.push(handle);
@@ -193,8 +231,12 @@ mod windows_tree {
     }
 
     fn open(pid: u32) -> Option<OwnedHandle> {
+        open_with(pid, 0)
+    }
+
+    fn open_with(pid: u32, access: u32) -> Option<OwnedHandle> {
         // SAFETY: plain call; a non-null result is a process handle owned by the caller.
-        let handle = unsafe { OpenProcess(PROCESS_SYNCHRONIZE | PROCESS_QUERY_LIMITED_INFORMATION, 0, pid) };
+        let handle = unsafe { OpenProcess(PROCESS_SYNCHRONIZE | PROCESS_QUERY_LIMITED_INFORMATION | access, 0, pid) };
         // SAFETY: as above.
         (!handle.is_null()).then(|| unsafe { OwnedHandle::from_raw_handle(handle) })
     }

crates/ide-dap/src/lib.rs

@@ -1130,6 +1130,9 @@ impl DebugManager {
 
     /// Ends a session: `terminate` (when asked and supported) → `disconnect` → kill.
     pub(crate) async fn shutdown(&self, s: &Arc<Session>, terminate: bool) {
+        if terminate {
+            crate::session::remember_debuggee(s).await;
+        }
         // java-debug runs the program itself and pipes its stdout over DAP. A `disconnect`
         // ends that DAP session, so every line the program writes while shutting down is lost.
         // Ask the program to exit ourselves and keep the adapter attached until it has drained

crates/ide-dap/src/session.rs

@@ -59,6 +59,8 @@ pub(crate) struct Session {
     /// adapter may report the launcher shell, whose child is the program).
     #[cfg_attr(not(unix), allow(dead_code))]
     pub stop_pids: parking_lot::Mutex<Vec<i32>>,
+    #[cfg(windows)]
+    pub stop_tree: Mutex<Option<ide_core::process::ProcessTree>>,
     /// Breakpoints replaced for run to cursor / force stepping, restored on the next stop.
     pub pending_restore: Mutex<PendingRestore>,
     /// Adapter breakpoint id → (file, index in the stored list).
@@ -111,6 +113,8 @@ impl Session {
             stop_force: AtomicBool::new(false),
             debuggee_pid: AtomicI64::new(0),
             stop_pids: parking_lot::Mutex::new(Vec::new()),
+            #[cfg(windows)]
+            stop_tree: Mutex::new(None),
             pending_restore: Mutex::default(),
             breakpoint_ids: Mutex::new(HashMap::new()),
             resolved_paths: Mutex::new(HashMap::new()),
@@ -298,6 +302,21 @@ impl Session {
 /// How long Stop waits for a launched program to exit before reporting the session ended.
 pub(crate) const DEBUGGEE_EXIT_WAIT: Duration = Duration::from_secs(10);
 
+pub(crate) async fn remember_debuggee(s: &Arc<Session>) {
+    #[cfg(windows)]
+    {
+        let Ok(pid) = u32::try_from(s.debuggee_pid.load(Ordering::Acquire)) else { return };
+        if s.request != "launch" || pid == 0 || s.stop_tree.lock().is_some() {
+            return;
+        }
+        if let Ok(tree) = tokio::task::spawn_blocking(move || ide_core::process::ProcessTree::capture(pid)).await {
+            s.stop_tree.lock().get_or_insert(tree);
+        }
+    }
+    #[cfg(not(windows))]
+    let _ = s;
+}
+
 /// The program and everything it started: an adapter may report the launcher shell, whose
 /// child is the debugged program, so stopping only the reported pid leaves the program alive.
 #[cfg(unix)]
@@ -349,12 +368,10 @@ pub(crate) fn signal_debuggee(s: &Arc<Session>, force: bool) {
     }
     #[cfg(windows)]
     {
-        let mut cmd = std::process::Command::new("taskkill");
-        cmd.args(["/PID", &pid.to_string(), "/T"]);
-        if force {
-            cmd.arg("/F");
+        // Console programs have no external graceful signal; the adapter gets its chance first.
+        if force && let Some(tree) = s.stop_tree.lock().as_ref() {
+            tree.terminate();
         }
-        let _ = cmd.status();
     }
     #[cfg(not(any(unix, windows)))]
     let _ = (pid, force);
@@ -391,7 +408,18 @@ pub(crate) async fn wait_for_debuggee(s: &Arc<Session>, terminate: bool) {
             tokio::time::sleep(Duration::from_millis(50)).await;
         }
     }
-    #[cfg(not(unix))]
+    #[cfg(windows)]
+    {
+        remember_debuggee(s).await;
+        let deadline = std::time::Instant::now() + DEBUGGEE_EXIT_WAIT;
+        while s.stop_tree.lock().as_ref().is_some_and(|tree| tree.is_running()) {
+            if s.stop_force.swap(false, Ordering::AcqRel) || std::time::Instant::now() >= deadline {
+                signal_debuggee(s, true);
+            }
+            tokio::time::sleep(Duration::from_millis(50)).await;
+        }
+    }
+    #[cfg(not(any(unix, windows)))]
     {
         let _ = pid;
     }

crates/ide-dap/src/tests.rs

@@ -14,6 +14,93 @@ use tokio::io::{AsyncRead, AsyncWrite, AsyncWriteExt, BufReader};
 
 type Log = Arc<Mutex<Vec<Value>>>;
 
+#[cfg(windows)]
+#[test]
+#[ignore = "child process for Windows stop tests"]
+fn windows_debuggee_process() {
+    if std::env::var_os("OPENIDEA_TEST_DEBUGGEE").is_none() {
+        return;
+    }
+    if std::env::var("OPENIDEA_TEST_DEBUGGEE").unwrap() == "wrapper" {
+        let mut command = std::process::Command::new(std::env::current_exe().unwrap());
+        let mut child = ide_core::process::prepare(&mut command)
+            .args(["--exact", "tests::windows_debuggee_process", "--ignored", "--nocapture"])
+            .env("OPENIDEA_TEST_DEBUGGEE", "leaf")
+            .spawn()
+            .unwrap();
+        let _ = child.wait();
+        return;
+    }
+    let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap();
+    println!("LISTENING {}", listener.local_addr().unwrap().port());
+    std::thread::sleep(Duration::from_secs(60));
+}
+
+#[cfg(windows)]
+#[tokio::test]
+async fn windows_stop_releases_the_debuggees_port_before_returning() {
+    use std::io::BufRead;
+    use std::sync::atomic::Ordering;
+
+    struct Child(std::process::Child, Option<ide_core::process::ProcessTree>);
+    impl Drop for Child {
+        fn drop(&mut self) {
+            if let Some(tree) = &self.1 {
+                tree.terminate();
+            }
+            let _ = self.0.kill();
+            let _ = self.0.wait();
+        }
+    }
+
+    for (second_stop, wrapper, supports_terminate) in
+        [(false, false, false), (true, false, false), (true, true, false), (true, true, true)]
+    {
+        let mut command = std::process::Command::new(std::env::current_exe().unwrap());
+        ide_core::process::prepare(&mut command)
+            .args(["--exact", "tests::windows_debuggee_process", "--ignored", "--nocapture"])
+            .env("OPENIDEA_TEST_DEBUGGEE", if wrapper { "wrapper" } else { "leaf" })
+            .stdout(std::process::Stdio::piped());
+        let mut child = Child(command.spawn().unwrap(), None);
+        let port: u16 = std::io::BufReader::new(child.0.stdout.take().unwrap())
+            .lines()
+            .map(Result::unwrap)
+            .find_map(|line| line.split("LISTENING ").nth(1).and_then(|p| p.parse().ok()))
+            .unwrap();
+        let pid = child.0.id();
+        child.1 = Some(ide_core::process::ProcessTree::capture(pid));
+        let h = harness(Script { debuggee_pid: Some(pid), supports_terminate, ..Default::default() });
+        let id = h.start(config()).await.unwrap();
+        let session = h.manager.session(id).unwrap();
+        while session.debuggee_pid.load(Ordering::Acquire) != i64::from(pid) {
+            tokio::time::sleep(Duration::from_millis(10)).await;
+        }
+        let stop = tokio::spawn({
+            let manager = h.manager.clone();
+            async move { manager.stop(id).await }
+        });
+        let deadline = std::time::Instant::now() + Duration::from_secs(5);
+        while session.stop_tree.lock().is_none() && !stop.is_finished() {
+            assert!(std::time::Instant::now() < deadline, "Stop did not capture the program tree");
+            tokio::time::sleep(Duration::from_millis(10)).await;
+        }
+        assert!(!stop.is_finished(), "Stop must wait for the live program");
+        if wrapper {
+            assert!(session.stop_tree.lock().is_some());
+            child.0.kill().unwrap();
+            child.0.wait().unwrap();
+            assert!(std::net::TcpListener::bind(("127.0.0.1", port)).is_err(), "the child still owns its port");
+        }
+        if second_stop {
+            h.manager.stop(id).await.unwrap();
+        }
+        let limit = if second_stop { 10 } else { 25 };
+        tokio::time::timeout(Duration::from_secs(limit), stop).await.unwrap().unwrap().unwrap();
+        assert!(child.0.try_wait().unwrap().is_some(), "Stop returned with the program alive");
+        let _listener = std::net::TcpListener::bind(("127.0.0.1", port)).expect("Stop returned with its port occupied");
+    }
+}
+
 #[derive(Clone)]
 struct Script {
     /// Send a `runInTerminal` reverse request when `launch` arrives.