diff --git a/src/mcp/client/stdio/__init__.py b/src/mcp/client/stdio/__init__.py index a75cfd764..0fd815ac7 100644 --- a/src/mcp/client/stdio/__init__.py +++ b/src/mcp/client/stdio/__init__.py @@ -16,7 +16,6 @@ from .win32 import ( create_windows_process, get_windows_executable_command, - terminate_windows_process, ) # Environment variables to inherit by default @@ -38,6 +37,9 @@ else ["HOME", "LOGNAME", "PATH", "SHELL", "TERM", "USER"] ) +# Timeout for process termination before falling back to force kill +PROCESS_TERMINATION_TIMEOUT = 2.0 + def get_default_environment() -> dict[str, str]: """ @@ -180,10 +182,12 @@ async def stdin_writer(): finally: # Clean up process to prevent any dangling orphaned processes try: - if sys.platform == "win32": - await terminate_windows_process(process) - else: - process.terminate() + process.terminate() + with anyio.fail_after(PROCESS_TERMINATION_TIMEOUT): + await process.wait() + except TimeoutError: + # If process doesn't terminate in time, force kill it + process.kill() except ProcessLookupError: # Process already exited, which is fine pass diff --git a/src/mcp/client/stdio/win32.py b/src/mcp/client/stdio/win32.py index 7246b9dec..d046084bb 100644 --- a/src/mcp/client/stdio/win32.py +++ b/src/mcp/client/stdio/win32.py @@ -12,6 +12,7 @@ from anyio import to_thread from anyio.abc import Process from anyio.streams.file import FileReadStream, FileWriteStream +from typing_extensions import deprecated def get_windows_executable_command(command: str) -> str: @@ -115,13 +116,14 @@ async def create_windows_process( env: dict[str, str] | None = None, errlog: TextIO | None = sys.stderr, cwd: Path | str | None = None, -) -> FallbackProcess: +) -> Process | FallbackProcess: """ Creates a subprocess in a Windows-compatible way. - On Windows, asyncio.create_subprocess_exec has incomplete support - (NotImplementedError when trying to open subprocesses). - Therefore, we fallback to subprocess.Popen and wrap it for async usage. + Attempt to use anyio's open_process for async subprocess creation. + In some cases this will throw NotImplementedError on Windows, e.g. + when using the SelectorEventLoop which does not support async subprocesses. + In that case, we fall back to using subprocess.Popen. Args: command (str): The executable to run @@ -133,6 +135,45 @@ async def create_windows_process( Returns: FallbackProcess: Async-compatible subprocess with stdin and stdout streams """ + try: + # First try using anyio with Windows-specific flags to hide console window + process = await anyio.open_process( + [command, *args], + env=env, + # Ensure we don't create console windows for each process + creationflags=subprocess.CREATE_NO_WINDOW # type: ignore + if hasattr(subprocess, "CREATE_NO_WINDOW") + else 0, + stderr=errlog, + cwd=cwd, + ) + return process + except NotImplementedError: + # Windows often doesn't support async subprocess creation, use fallback + return await _create_windows_fallback_process(command, args, env, errlog, cwd) + except Exception: + # Try again without creation flags + process = await anyio.open_process( + [command, *args], + env=env, + stderr=errlog, + cwd=cwd, + ) + return process + + +async def _create_windows_fallback_process( + command: str, + args: list[str], + env: dict[str, str] | None = None, + errlog: TextIO | None = sys.stderr, + cwd: Path | str | None = None, +) -> FallbackProcess: + """ + Create a subprocess using subprocess.Popen as a fallback when anyio fails. + + This function wraps the sync subprocess.Popen in an async-compatible interface. + """ try: # Try launching with creationflags to avoid opening a new console window popen_obj = subprocess.Popen( @@ -161,6 +202,10 @@ async def create_windows_process( return FallbackProcess(popen_obj) +@deprecated( + "terminate_windows_process is deprecated and will be removed in a future version. " + "Process termination is now handled internally by the stdio_client context manager." +) async def terminate_windows_process(process: Process | FallbackProcess): """ Terminate a Windows process. diff --git a/tests/client/test_stdio.py b/tests/client/test_stdio.py index c66a16ab9..585cb8eda 100644 --- a/tests/client/test_stdio.py +++ b/tests/client/test_stdio.py @@ -1,5 +1,9 @@ import shutil +import sys +import textwrap +import time +import anyio import pytest from mcp.client.session import ClientSession @@ -11,6 +15,11 @@ from mcp.shared.message import SessionMessage from mcp.types import CONNECTION_CLOSED, JSONRPCMessage, JSONRPCRequest, JSONRPCResponse +# Timeout for cleanup of processes that ignore SIGTERM +# This timeout ensures the test fails quickly if the cleanup logic doesn't have +# proper fallback mechanisms (SIGINT/SIGKILL) for processes that ignore SIGTERM +SIGTERM_IGNORING_PROCESS_TIMEOUT = 5.0 + tee: str = shutil.which("tee") # type: ignore python: str = shutil.which("python") # type: ignore @@ -90,3 +99,123 @@ async def test_stdio_client_nonexistent_command(): or "not found" in error_message.lower() or "cannot find the file" in error_message.lower() # Windows error message ) + + +@pytest.mark.anyio +async def test_stdio_client_universal_cleanup(): + """ + Test that stdio_client completes cleanup within reasonable time + even when connected to processes that exit slowly. + """ + + # Use a Python script that simulates a long-running process + # This ensures consistent behavior across platforms + long_running_script = textwrap.dedent( + """ + import time + import sys + + # Simulate a long-running process + for i in range(100): + time.sleep(0.1) + # Flush to ensure output is visible + sys.stdout.flush() + sys.stderr.flush() + """ + ) + + server_params = StdioServerParameters( + command=sys.executable, + args=["-c", long_running_script], + ) + + start_time = time.time() + + with anyio.move_on_after(8.0) as cancel_scope: + async with stdio_client(server_params) as (read_stream, write_stream): + # Immediately exit - this triggers cleanup while process is still running + pass + + end_time = time.time() + elapsed = end_time - start_time + + # On Windows: 2s (stdin wait) + 2s (terminate wait) + overhead = ~5s expected + assert elapsed < 6.0, ( + f"stdio_client cleanup took {elapsed:.1f} seconds, expected < 6.0 seconds. " + f"This suggests the timeout mechanism may not be working properly." + ) + + # Check if we timed out + if cancel_scope.cancelled_caught: + pytest.fail( + "stdio_client cleanup timed out after 8.0 seconds. " + "This indicates the cleanup mechanism is hanging and needs fixing." + ) + + +@pytest.mark.anyio +@pytest.mark.skipif(sys.platform == "win32", reason="Windows signal handling is different") +async def test_stdio_client_sigint_only_process(): + """ + Test cleanup with a process that ignores SIGTERM but responds to SIGINT. + """ + # Create a Python script that ignores SIGTERM but handles SIGINT + script_content = textwrap.dedent( + """ + import signal + import sys + import time + + # Ignore SIGTERM (what process.terminate() sends) + signal.signal(signal.SIGTERM, signal.SIG_IGN) + + # Handle SIGINT (Ctrl+C signal) by exiting cleanly + def sigint_handler(signum, frame): + sys.exit(0) + + signal.signal(signal.SIGINT, sigint_handler) + + # Keep running until SIGINT received + while True: + time.sleep(0.1) + """ + ) + + server_params = StdioServerParameters( + command=sys.executable, + args=["-c", script_content], + ) + + start_time = time.time() + + try: + # Use anyio timeout to prevent test from hanging forever + with anyio.move_on_after(5.0) as cancel_scope: + async with stdio_client(server_params) as (read_stream, write_stream): + # Let the process start and begin ignoring SIGTERM + await anyio.sleep(0.5) + # Exit context triggers cleanup - this should not hang + pass + + if cancel_scope.cancelled_caught: + raise TimeoutError("Test timed out") + + end_time = time.time() + elapsed = end_time - start_time + + # Should complete quickly even with SIGTERM-ignoring process + # This will fail if cleanup only uses process.terminate() without fallback + assert elapsed < SIGTERM_IGNORING_PROCESS_TIMEOUT, ( + f"stdio_client cleanup took {elapsed:.1f} seconds with SIGTERM-ignoring process. " + f"Expected < {SIGTERM_IGNORING_PROCESS_TIMEOUT} seconds. " + "This suggests the cleanup needs SIGINT/SIGKILL fallback." + ) + except (TimeoutError, Exception) as e: + if isinstance(e, TimeoutError) or "timed out" in str(e): + pytest.fail( + f"stdio_client cleanup timed out after {SIGTERM_IGNORING_PROCESS_TIMEOUT} seconds " + "with SIGTERM-ignoring process. " + "This confirms the cleanup needs SIGINT/SIGKILL fallback for processes that ignore SIGTERM." + ) + else: + raise