Skip to content

Unify process termination on POSIX & Windows (+ tests) #1044

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/mcp/client/stdio/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
from .win32 import (
create_windows_process,
get_windows_executable_command,
terminate_windows_process,
)

# Environment variables to inherit by default
Expand Down Expand Up @@ -180,10 +179,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(2.0):
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
Expand Down
69 changes: 44 additions & 25 deletions src/mcp/client/stdio/win32.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,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
Expand All @@ -133,6 +134,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(
Expand All @@ -159,24 +199,3 @@ async def create_windows_process(
bufsize=0,
)
return FallbackProcess(popen_obj)


async def terminate_windows_process(process: Process | FallbackProcess):
"""
Terminate a Windows process.

Note: On Windows, terminating a process with process.terminate() doesn't
always guarantee immediate process termination.
So we give it 2s to exit, or we call process.kill()
which sends a SIGKILL equivalent signal.

Args:
process: The process to terminate
"""
try:
process.terminate()
with anyio.fail_after(2.0):
await process.wait()
except TimeoutError:
# Force kill if it doesn't terminate
process.kill()
122 changes: 122 additions & 0 deletions tests/client/test_stdio.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import shutil
import sys
import textwrap
import time

import anyio
import pytest

from mcp.client.session import ClientSession
Expand Down Expand Up @@ -90,3 +94,121 @@ 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 < 5.0, (
f"stdio_client cleanup took {elapsed:.1f} seconds with SIGTERM-ignoring process. "
f"Expected < 5.0 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(
"stdio_client cleanup timed out after 5.0 seconds with SIGTERM-ignoring process. "
"This confirms the cleanup needs SIGINT/SIGKILL fallback for processes that ignore SIGTERM."
)
else:
raise
Loading