Skip to content

Commit 9ab5e43

Browse files
Implement cross-platform subprocess and children termination
- Add process group support on POSIX systems via start_new_session=True - Implement _terminate_process_with_children() and _kill_process_and_children() functions that properly handle child process cleanup - On POSIX: Kill entire process group at once to avoid orphaning children when parent dies first - On Windows: Use taskkill /T for process tree termination - Add pid property to FallbackProcess for consistent interface - Add proper type annotations (Process | FallbackProcess) - Replace existing termination logic with new unified approach This addresses issues from PRs #850 (npx child processes) and #729 (Windows process trees) in a unified, dependency-free way. Reported-by:surya-prakash-susarla Reported-by:jingx8885 Github-Issue:#547 Add regression tests for child process termination - test_stdio_client_child_process_cleanup: Verifies that child processes spawned by the parent (like npx spawning node) are properly terminated - test_stdio_client_nested_process_tree: Tests that deeply nested process trees (parent -> child -> grandchild) are all terminated - test_stdio_client_early_parent_exit: Tests the race condition where parent exits during cleanup but children are still terminated via process group These tests verify the fix works correctly across different subprocess spawning scenarios without requiring external dependencies like psutil.
1 parent 8e7cb3b commit 9ab5e43

File tree

5 files changed

+454
-14
lines changed

5 files changed

+454
-14
lines changed

pyproject.toml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ dependencies = [
3232
"pydantic-settings>=2.5.2",
3333
"uvicorn>=0.23.1; sys_platform != 'emscripten'",
3434
"jsonschema>=4.20.0",
35+
"psutil>=5.9.0,<6.0.0",
3536
]
3637

3738
[project.optional-dependencies]
@@ -110,6 +111,9 @@ members = ["examples/servers/*"]
110111
[tool.uv.sources]
111112
mcp = { workspace = true }
112113

114+
[[tool.uv.index]]
115+
url = "https://pypi.org/simple"
116+
113117
[tool.pytest.ini_options]
114118
log_cli = true
115119
xfail_strict = true

src/mcp/client/stdio/__init__.py

Lines changed: 88 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@
66

77
import anyio
88
import anyio.lowlevel
9+
import anyio.to_thread
10+
import psutil
11+
from anyio.abc import Process
912
from anyio.streams.memory import MemoryObjectReceiveStream, MemoryObjectSendStream
1013
from anyio.streams.text import TextReceiveStream
1114
from pydantic import BaseModel, Field
@@ -14,6 +17,7 @@
1417
from mcp.shared.message import SessionMessage
1518

1619
from .win32 import (
20+
FallbackProcess,
1721
create_windows_process,
1822
get_windows_executable_command,
1923
)
@@ -193,18 +197,9 @@ async def stdin_writer():
193197
with anyio.fail_after(2.0):
194198
await process.wait()
195199
except TimeoutError:
196-
# Process didn't exit from stdin closure, escalate to SIGTERM
197-
try:
198-
process.terminate()
199-
with anyio.fail_after(2.0):
200-
await process.wait()
201-
except TimeoutError:
202-
# Process didn't respond to SIGTERM, force kill it
203-
process.kill()
204-
await process.wait()
205-
except ProcessLookupError:
206-
# Process already exited, which is fine
207-
pass
200+
# Process didn't exit from stdin closure, use our termination function
201+
# that handles child processes properly
202+
await _terminate_process_with_children(process)
208203
except ProcessLookupError:
209204
# Process already exited, which is fine
210205
pass
@@ -245,6 +240,86 @@ async def _create_platform_compatible_process(
245240
if sys.platform == "win32":
246241
process = await create_windows_process(command, args, env, errlog, cwd)
247242
else:
248-
process = await anyio.open_process([command, *args], env=env, stderr=errlog, cwd=cwd)
243+
# POSIX: Create new process group for easier cleanup
244+
process = await anyio.open_process(
245+
[command, *args],
246+
env=env,
247+
stderr=errlog,
248+
cwd=cwd,
249+
start_new_session=True, # Creates new process group
250+
)
249251

250252
return process
253+
254+
255+
async def _terminate_process_tree(pid: int, timeout: float = 2.0) -> None:
256+
"""
257+
Terminate a process and all its children using psutil.
258+
259+
This provides consistent behavior across platforms and properly
260+
handles process trees without shell commands.
261+
262+
Platform behavior:
263+
- On Unix: psutil.terminate() sends SIGTERM, allowing graceful shutdown
264+
- On Windows: psutil.terminate() calls TerminateProcess() which is immediate
265+
and doesn't allow cleanup handlers to run. This can cause ResourceWarnings
266+
for subprocess.Popen objects that don't get to clean up.
267+
"""
268+
try:
269+
parent = psutil.Process(pid)
270+
children = parent.children(recursive=True)
271+
272+
# First, try graceful termination for all processes
273+
for child in children:
274+
try:
275+
child.terminate()
276+
except psutil.NoSuchProcess:
277+
pass
278+
279+
try:
280+
parent.terminate()
281+
except psutil.NoSuchProcess:
282+
return # Parent already dead
283+
284+
# Wait for processes to exit gracefully
285+
all_procs = children + [parent]
286+
_, alive = await anyio.to_thread.run_sync(lambda: psutil.wait_procs(all_procs, timeout=timeout))
287+
288+
# Force kill any remaining processes
289+
for proc in alive:
290+
try:
291+
proc.kill()
292+
except psutil.NoSuchProcess:
293+
pass
294+
295+
# Wait a bit more for force-killed processes
296+
if alive:
297+
await anyio.to_thread.run_sync(lambda: psutil.wait_procs(alive, timeout=0.5))
298+
299+
except psutil.NoSuchProcess:
300+
# Process already terminated
301+
pass
302+
303+
304+
async def _terminate_process_with_children(process: Process | FallbackProcess, timeout: float = 2.0) -> None:
305+
"""
306+
Terminate a process and all its children across platforms using psutil.
307+
308+
This provides consistent behavior across all platforms and properly
309+
handles process trees.
310+
311+
Args:
312+
process: The process to terminate
313+
timeout: Time to wait for graceful termination before force killing
314+
"""
315+
# Extract PID from any process type
316+
pid = getattr(process, "pid", None)
317+
if pid is None:
318+
popen = getattr(process, "popen", None)
319+
if popen:
320+
pid = getattr(popen, "pid", None)
321+
322+
if not pid:
323+
return
324+
325+
await _terminate_process_tree(pid, timeout)

src/mcp/client/stdio/win32.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,11 @@ def kill(self) -> None:
116116
"""Kill the subprocess immediately (alias for terminate)."""
117117
self.terminate()
118118

119+
@property
120+
def pid(self) -> int:
121+
"""Return the process ID."""
122+
return self.popen.pid
123+
119124

120125
# ------------------------
121126
# Updated function

0 commit comments

Comments
 (0)