diff --git a/python/copilot/client.py b/python/copilot/client.py index c800e2158..ccfe68a2b 100644 --- a/python/copilot/client.py +++ b/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 diff --git a/python/pyproject.toml b/python/pyproject.toml index 6e805c250..66b002a77 100644 --- a/python/pyproject.toml +++ b/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] diff --git a/python/test_process_cleanup.py b/python/test_process_cleanup.py new file mode 100644 index 000000000..75e4dc2ba --- /dev/null +++ b/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