fix: Windows process tree cleanup in client.stop() (#1132)

python/copilot/client.py

@@ -15,6 +15,7 @@
 from __future__ import annotations
 
 import asyncio
+import importlib.util
 import inspect
 import os
 import re
@@ -69,6 +70,9 @@
 
 ConnectionState = Literal["disconnected", "connecting", "connected", "error"]
 
+# Check if psutil is available for process tree cleanup on Windows
+HAS_PSUTIL = importlib.util.find_spec("psutil") is not None
+
 LogLevel = Literal["none", "error", "warning", "info", "debug", "all"]
 
 
@@ -803,6 +807,114 @@ def _extract_transform_callbacks(
     return wire_payload, callbacks
 
 
+def _kill_process_tree(process: subprocess.Popen) -> None:
+    """
+    Gracefully terminate a process and all its children.
+
+    Sends SIGTERM to all children, waits up to 3 seconds for clean exit,
+    then force-kills any survivors. Falls back to simple terminate() if
+    psutil is unavailable.
+
+    On Windows, subprocess.Popen.terminate() only kills the parent process,
+    leaving child processes (like MCP servers) running as orphans. This function
+    ensures all descendants are terminated gracefully.
+
+    Args:
+        process: The subprocess.Popen instance to terminate.
+
+    Note:
+        If psutil is not available, falls back to simple terminate() which may
+        leave child processes running on Windows.
+    """
+    if not process or not process.pid:
+        return
+
+    if HAS_PSUTIL and sys.platform == "win32":
+        try:
+            import psutil as ps  # Import here to get the real exception classes
+
+            parent = ps.Process(process.pid)
+            # Get all child processes recursively
+            children = parent.children(recursive=True)
+
+            # Graceful termination: terminate children first (bottom-up)
+            for child in children:
+                try:
+                    child.terminate()
+                except ps.NoSuchProcess:
+                    pass
+
+            # Give children a chance to terminate gracefully
+            _, alive = ps.wait_procs(children, timeout=3)
+
+            # Force kill any that didn't terminate
+            for child in alive:
+                try:
+                    child.kill()
+                except ps.NoSuchProcess:
+                    pass
+
+            # Finally terminate the parent
+            try:
+                parent.terminate()
+            except ps.NoSuchProcess:
+                pass
+
+        except Exception:
+            # If psutil fails for any reason, fall back to simple terminate
+            process.terminate()
+    else:
+        # On non-Windows or without psutil, use simple terminate
+        # (child processes typically die with parent on Unix-like systems)
+        process.terminate()
+
+
+def _kill_process_tree_force(process: subprocess.Popen) -> None:
+    """
+    Immediately kill a process and all its children. No grace period.
+
+    Intended for force_stop() — skips SIGTERM and wait entirely,
+    sends SIGKILL straight away to every process in the tree.
+
+    Args:
+        process: The subprocess.Popen instance to kill.
+
+    Note:
+        If psutil is not available, falls back to simple kill() which may
+        leave child processes running on Windows.
+    """
+    if not process or not process.pid:
+        return
+
+    if HAS_PSUTIL and sys.platform == "win32":
+        try:
+            import psutil as ps  # Import here to get the real exception classes
+
+            parent = ps.Process(process.pid)
+            # Get all child processes recursively
+            children = parent.children(recursive=True)
+
+            # SIGKILL immediately, no wait
+            for child in children:
+                try:
+                    child.kill()
+                except ps.NoSuchProcess:
+                    pass
+
+            try:
+                parent.kill()
+            except ps.NoSuchProcess:
+                pass
+
+        except Exception:
+            # If psutil fails for any reason, fall back to simple kill
+            process.kill()
+    else:
+        # On non-Windows or without psutil, use simple kill
+        # (child processes typically die with parent on Unix-like systems)
+        process.kill()
+
+
 class CopilotClient:
     """
     Main client for interacting with the Copilot CLI.
@@ -1124,7 +1236,7 @@ async def stop(self) -> None:
 
         # Kill CLI process (only if we spawned it)
         if self._process and not self._is_external_server:
-            self._process.terminate()
+            _kill_process_tree(self._process)
             try:
                 self._process.wait(timeout=5)
             except subprocess.TimeoutExpired:
@@ -1166,7 +1278,7 @@ async def force_stop(self) -> None:
                 if self._is_external_server:
                     self._process.terminate()  # closes the TCP socket
                 else:
-                    self._process.kill()
+                    _kill_process_tree_force(self._process)
                     self._process = None
             except Exception:
                 pass

python/pyproject.toml

@@ -25,6 +25,7 @@ classifiers = [
 dependencies = [
     "python-dateutil>=2.9.0.post0",
     "pydantic>=2.0",
+    "psutil>=5.9.0; sys_platform == 'win32'",
 ]
 
 [project.urls]

python/test_process_cleanup.py

@@ -0,0 +1,134 @@
+"""
+Process Cleanup Tests
+
+Tests for ensuring child processes (like MCP servers) are properly terminated
+when the client stops, especially on Windows.
+
+Related to issue #1132: https://github.com/github/copilot-sdk/issues/1132
+"""
+
+import subprocess
+import sys
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from copilot.client import _kill_process_tree, _kill_process_tree_force
+
+
+class TestKillProcessTree:
+    """Test the _kill_process_tree helper function."""
+
+    def test_kill_process_tree_with_none_process(self):
+        """Should handle None process gracefully."""
+        _kill_process_tree(None)  # Should not raise
+
+    def test_kill_process_tree_with_no_pid(self):
+        """Should handle process with no PID gracefully."""
+        mock_process = MagicMock(spec=subprocess.Popen)
+        mock_process.pid = None
+        _kill_process_tree(mock_process)  # Should not raise
+
+    @pytest.mark.skipif(sys.platform != "win32", reason="Windows-specific test")
+    @patch("copilot.client.HAS_PSUTIL", True)
+    def test_kill_process_tree_on_windows_with_psutil(self):
+        """On Windows with psutil, should kill children recursively."""
+        # We can't easily mock the import inside the function,
+        # so this test verifies the function doesn't crash when psutil is available
+        mock_process = MagicMock(spec=subprocess.Popen)
+        mock_process.pid = 99999999  # Use a PID that doesn't exist
+
+        # Should not raise even with non-existent PID
+        _kill_process_tree(mock_process)
+
+        # Verify terminate was called as fallback
+        mock_process.terminate.assert_called_once()
+
+    @pytest.mark.skipif(sys.platform != "win32", reason="Windows-specific test")
+    @patch("copilot.client.HAS_PSUTIL", False)
+    def test_kill_process_tree_on_windows_without_psutil(self):
+        """On Windows without psutil, should fall back to simple terminate."""
+        mock_process = MagicMock(spec=subprocess.Popen)
+        mock_process.pid = 1234
+
+        _kill_process_tree(mock_process)
+
+        mock_process.terminate.assert_called_once()
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="Non-Windows test")
+    @patch("copilot.client.HAS_PSUTIL", True)
+    def test_kill_process_tree_on_unix(self):
+        """On Unix-like systems, should use simple terminate (children die with parent)."""
+        mock_process = MagicMock(spec=subprocess.Popen)
+        mock_process.pid = 1234
+
+        _kill_process_tree(mock_process)
+
+        mock_process.terminate.assert_called_once()
+
+
+class TestKillProcessTreeForce:
+    """Test the _kill_process_tree_force helper function."""
+
+    def test_kill_process_tree_force_with_none_process(self):
+        """Should handle None process gracefully."""
+        _kill_process_tree_force(None)  # Should not raise
+
+    def test_kill_process_tree_force_with_no_pid(self):
+        """Should handle process with no PID gracefully."""
+        mock_process = MagicMock(spec=subprocess.Popen)
+        mock_process.pid = None
+        _kill_process_tree_force(mock_process)  # Should not raise
+
+    @pytest.mark.skipif(sys.platform != "win32", reason="Windows-specific test")
+    @patch("copilot.client.HAS_PSUTIL", True)
+    def test_kill_process_tree_force_on_windows_with_psutil(self):
+        """On Windows with psutil, should kill children immediately with no wait."""
+        mock_process = MagicMock(spec=subprocess.Popen)
+        mock_process.pid = 99999999  # Use a PID that doesn't exist
+
+        # Should not raise even with non-existent PID
+        _kill_process_tree_force(mock_process)
+
+        # Verify kill was called as fallback (not terminate)
+        mock_process.kill.assert_called_once()
+        mock_process.terminate.assert_not_called()
+
+    @pytest.mark.skipif(sys.platform != "win32", reason="Windows-specific test")
+    @patch("copilot.client.HAS_PSUTIL", False)
+    def test_kill_process_tree_force_on_windows_without_psutil(self):
+        """On Windows without psutil, should fall back to simple kill."""
+        mock_process = MagicMock(spec=subprocess.Popen)
+        mock_process.pid = 1234
+
+        _kill_process_tree_force(mock_process)
+
+        mock_process.kill.assert_called_once()
+        mock_process.terminate.assert_not_called()
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="Non-Windows test")
+    @patch("copilot.client.HAS_PSUTIL", True)
+    def test_kill_process_tree_force_on_unix(self):
+        """On Unix-like systems, should use simple kill (children die with parent)."""
+        mock_process = MagicMock(spec=subprocess.Popen)
+        mock_process.pid = 1234
+
+        _kill_process_tree_force(mock_process)
+
+        mock_process.kill.assert_called_once()
+        mock_process.terminate.assert_not_called()
+
+    @patch("copilot.client.HAS_PSUTIL", False)
+    def test_kill_process_tree_force_without_psutil(self):
+        """Without psutil, should use kill() immediately."""
+        mock_process = MagicMock(spec=subprocess.Popen)
+        mock_process.pid = 1234
+
+        _kill_process_tree_force(mock_process)
+
+        mock_process.kill.assert_called_once()
+        mock_process.terminate.assert_not_called()
+
+
+# Note: Integration tests for actual MCP server cleanup would go in e2e/
+# This file focuses on unit testing the helper function