Skip to content

Commit d3d3448

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 8d9fb00 commit d3d3448

File tree

3 files changed

+363
-13
lines changed

3 files changed

+363
-13
lines changed

src/mcp/client/stdio/__init__.py

Lines changed: 81 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,14 @@
11
import os
2+
import signal
3+
import subprocess
24
import sys
35
from contextlib import asynccontextmanager
46
from pathlib import Path
57
from typing import Literal, TextIO
68

79
import anyio
810
import anyio.lowlevel
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,79 @@ 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_with_children(process: Process | FallbackProcess, timeout: float = 2.0) -> None:
256+
"""
257+
Terminate a process and all its children across platforms.
258+
259+
There's no cross-platform way in the stdlib to kill a process AND its children,
260+
so we need platform-specific handling:
261+
- POSIX: Use process groups and os.killpg()
262+
- Windows: Use taskkill with /T flag for tree termination
263+
264+
Args:
265+
process: The process to terminate
266+
timeout: Time to wait for graceful termination before force killing
267+
"""
268+
if sys.platform != "win32":
269+
# POSIX: Kill entire process group to avoid orphaning children
270+
pid = getattr(process, "pid", None)
271+
if not pid:
272+
return
273+
274+
try:
275+
# Try graceful termination of the group first
276+
pgid = os.getpgid(pid)
277+
os.killpg(pgid, signal.SIGTERM)
278+
279+
# Wait for termination
280+
with anyio.fail_after(timeout):
281+
await process.wait()
282+
except TimeoutError:
283+
# Force kill the process group
284+
try:
285+
pgid = os.getpgid(pid)
286+
os.killpg(pgid, signal.SIGKILL)
287+
except (OSError, ProcessLookupError):
288+
pass
289+
except (OSError, ProcessLookupError):
290+
# Process or group already dead
291+
pass
292+
else:
293+
# Windows: Extract PID from FallbackProcess if needed
294+
pid = getattr(process, "pid", None)
295+
if pid is None:
296+
popen = getattr(process, "popen", None)
297+
if popen:
298+
pid = getattr(popen, "pid", None)
299+
300+
if not pid:
301+
return
302+
303+
# Try graceful termination first
304+
try:
305+
process.terminate()
306+
with anyio.fail_after(timeout):
307+
await process.wait()
308+
except TimeoutError:
309+
# Force kill using taskkill for tree termination
310+
await anyio.to_thread.run_sync(
311+
subprocess.run,
312+
["taskkill", "/F", "/T", "/PID", str(pid)],
313+
capture_output=True,
314+
shell=False,
315+
check=False,
316+
)
317+
except ProcessLookupError:
318+
pass

src/mcp/client/stdio/win32.py

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

104+
@property
105+
def pid(self) -> int:
106+
"""Return the process ID."""
107+
return self.popen.pid
108+
104109

105110
# ------------------------
106111
# Updated function

0 commit comments

Comments
 (0)