Skip to content

Commit 17765da

Browse files
Simplify child process cleanup tests and remove cruft
Remove unnecessary complexity from the child process termination tests introduced during development iterations: - Eliminate WindowsProcessWrapper class - anyio's process handling works correctly on both Unix and Windows without this wrapper - Remove escape_path_for_python() function - Python's repr() handles path escaping correctly across platforms - Unify process creation code paths - use anyio.open_process() consistently with platform-appropriate start_new_session flag - Remove [[tool.uv.index]] from pyproject.toml - this is the default PyPI index and doesn't need explicit configuration The tests remain functionally identical but are now simpler and more maintainable. All child process cleanup functionality continues to work correctly on both Unix and Windows.
1 parent da0d8c4 commit 17765da

File tree

4 files changed

+452
-61
lines changed

4 files changed

+452
-61
lines changed

pyproject.toml

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -111,9 +111,6 @@ members = ["examples/servers/*"]
111111
[tool.uv.sources]
112112
mcp = { workspace = true }
113113

114-
[[tool.uv.index]]
115-
url = "https://pypi.org/simple"
116-
117114
[tool.pytest.ini_options]
118115
log_cli = true
119116
xfail_strict = true

tests/client/test_stdio.py

Lines changed: 21 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -22,31 +22,6 @@
2222
python: str = shutil.which("python") # type: ignore
2323

2424

25-
def escape_path_for_python(path: str) -> str:
26-
"""Escape a file path for use in Python code strings."""
27-
# Use forward slashes which work on all platforms and don't need escaping
28-
return repr(path.replace("\\", "/"))
29-
30-
31-
class WindowsProcessWrapper:
32-
"""Minimal wrapper for subprocess.Popen to work with anyio-style process interface."""
33-
34-
def __init__(self, popen):
35-
self.pid = popen.pid
36-
self._popen = popen
37-
# Add popen attribute for compatibility with _terminate_process_with_children
38-
self.popen = popen
39-
40-
async def wait(self):
41-
while self._popen.poll() is None:
42-
await anyio.sleep(0.1)
43-
return self._popen.returncode
44-
45-
def terminate(self):
46-
self._popen.terminate()
47-
48-
def kill(self):
49-
self._popen.kill()
5025

5126

5227
@pytest.mark.anyio
@@ -399,13 +374,13 @@ async def test_stdio_client_child_process_cleanup():
399374
import os
400375
401376
# Mark that parent started
402-
with open({escape_path_for_python(parent_marker)}, 'w') as f:
377+
with open({repr(parent_marker)}, 'w') as f:
403378
f.write('parent started\\n')
404379
405380
# Child script that writes continuously
406381
child_script = f'''
407382
import time
408-
with open({escape_path_for_python(marker_file)}, 'a') as f:
383+
with open({repr(marker_file)}, 'a') as f:
409384
while True:
410385
f.write(f"{time.time()}")
411386
f.flush()
@@ -423,15 +398,11 @@ async def test_stdio_client_child_process_cleanup():
423398

424399
print("\nStarting child process termination test...")
425400

426-
# Start the parent process directly with process group
427-
if sys.platform == "win32":
428-
# Windows: Don't use CREATE_NEW_PROCESS_GROUP as it isolates the process
429-
# Instead, let it inherit the parent's console which allows taskkill /T to work
430-
popen = subprocess.Popen([sys.executable, "-c", parent_script])
431-
proc = WindowsProcessWrapper(popen)
432-
else:
433-
# Unix: Use start_new_session for process group creation
434-
proc = await anyio.open_process([sys.executable, "-c", parent_script], start_new_session=True)
401+
# Start the parent process using anyio (cross-platform)
402+
proc = await anyio.open_process(
403+
[sys.executable, "-c", parent_script],
404+
start_new_session=(sys.platform != "win32") # Only on Unix
405+
)
435406

436407
# Wait for processes to start
437408
await anyio.sleep(0.5)
@@ -509,7 +480,7 @@ async def test_stdio_client_nested_process_tree():
509480
510481
# Grandchild just writes to file
511482
grandchild_script = \"\"\"import time
512-
with open({escape_path_for_python(grandchild_file)}, 'a') as f:
483+
with open({repr(grandchild_file)}, 'a') as f:
513484
while True:
514485
f.write(f"gc {{time.time()}}")
515486
f.flush()
@@ -519,7 +490,7 @@ async def test_stdio_client_nested_process_tree():
519490
subprocess.Popen([sys.executable, '-c', grandchild_script])
520491
521492
# Child writes to its file
522-
with open({escape_path_for_python(child_file)}, 'a') as f:
493+
with open({repr(child_file)}, 'a') as f:
523494
while True:
524495
f.write(f"c {time.time()}")
525496
f.flush()
@@ -529,23 +500,19 @@ async def test_stdio_client_nested_process_tree():
529500
subprocess.Popen([sys.executable, '-c', child_script])
530501
531502
# Parent writes to its file
532-
with open({escape_path_for_python(parent_file)}, 'a') as f:
503+
with open({repr(parent_file)}, 'a') as f:
533504
while True:
534505
f.write(f"p {time.time()}")
535506
f.flush()
536507
time.sleep(0.1)
537508
"""
538509
)
539510

540-
# Start parent process directly
541-
if sys.platform == "win32":
542-
# Windows: Don't use CREATE_NEW_PROCESS_GROUP as it isolates the process
543-
# Instead, let it inherit the parent's console which allows taskkill /T to work
544-
popen = subprocess.Popen([sys.executable, "-c", parent_script])
545-
proc = WindowsProcessWrapper(popen)
546-
else:
547-
# Unix: Use start_new_session for process group creation
548-
proc = await anyio.open_process([sys.executable, "-c", parent_script], start_new_session=True)
511+
# Start parent process using anyio (cross-platform)
512+
proc = await anyio.open_process(
513+
[sys.executable, "-c", parent_script],
514+
start_new_session=(sys.platform != "win32") # Only on Unix
515+
)
549516

550517
# Let all processes start
551518
await anyio.sleep(1.0)
@@ -607,7 +574,7 @@ async def test_stdio_client_early_parent_exit():
607574
608575
# Child that continues running
609576
child_script = f'''import time
610-
with open({escape_path_for_python(marker_file)}, 'a') as f:
577+
with open({repr(marker_file)}, 'a') as f:
611578
while True:
612579
f.write(f"child {time.time()}")
613580
f.flush()
@@ -628,15 +595,11 @@ def handle_term(sig, frame):
628595
"""
629596
)
630597

631-
# Start the parent process
632-
if sys.platform == "win32":
633-
# Windows: Don't use CREATE_NEW_PROCESS_GROUP as it isolates the process
634-
# Instead, let it inherit the parent's console which allows taskkill /T to work
635-
popen = subprocess.Popen([sys.executable, "-c", parent_script])
636-
proc = WindowsProcessWrapper(popen)
637-
else:
638-
# Unix: Use start_new_session for process group creation
639-
proc = await anyio.open_process([sys.executable, "-c", parent_script], start_new_session=True)
598+
# Start the parent process using anyio (cross-platform)
599+
proc = await anyio.open_process(
600+
[sys.executable, "-c", parent_script],
601+
start_new_session=(sys.platform != "win32") # Only on Unix
602+
)
640603

641604
# Let child start writing
642605
await anyio.sleep(0.5)
Lines changed: 205 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,205 @@
1+
"""Refactored tests for stdio client child process cleanup."""
2+
import os
3+
import shutil
4+
import sys
5+
import tempfile
6+
import textwrap
7+
8+
import pytest
9+
import anyio
10+
11+
from mcp.client.stdio import _terminate_process_with_children
12+
13+
python: str = shutil.which("python") # type: ignore
14+
15+
16+
@pytest.mark.anyio
17+
async def test_stdio_client_child_process_cleanup():
18+
"""
19+
Test that child processes are properly terminated when the parent is killed.
20+
This addresses the issue where processes like npx spawn child processes
21+
that need to be cleaned up.
22+
"""
23+
# Create marker files
24+
parent_marker = tempfile.mktemp(suffix=".txt")
25+
child_marker = tempfile.mktemp(suffix=".txt")
26+
27+
try:
28+
# Parent script that spawns a child process
29+
parent_script = textwrap.dedent(
30+
f"""
31+
import subprocess
32+
import sys
33+
import time
34+
35+
# Mark that parent started
36+
with open({repr(parent_marker)}, 'w') as f:
37+
f.write('parent started\\n')
38+
39+
# Child script that writes continuously
40+
child_script = '''
41+
import time
42+
with open({repr(child_marker)}, 'a') as f:
43+
while True:
44+
f.write(str(time.time()) + "\\\\n")
45+
f.flush()
46+
time.sleep(0.1)
47+
'''
48+
49+
# Start the child process
50+
subprocess.Popen([sys.executable, '-c', child_script])
51+
52+
# Parent sleeps forever
53+
while True:
54+
time.sleep(0.1)
55+
"""
56+
).strip()
57+
58+
# Start the parent process using anyio (cross-platform)
59+
# Use start_new_session on Unix for process group, not needed on Windows
60+
proc = await anyio.open_process(
61+
[python, "-c", parent_script],
62+
start_new_session=(sys.platform != "win32"),
63+
stderr=None # Let stderr go to console for debugging
64+
)
65+
66+
# Wait for processes to start
67+
await anyio.sleep(1.0)
68+
69+
print(f"Parent marker path: {parent_marker}")
70+
print(f"Child marker path: {child_marker}")
71+
print(f"Parent marker exists: {os.path.exists(parent_marker)}")
72+
73+
# Verify parent started
74+
assert os.path.exists(parent_marker), "Parent process didn't start"
75+
76+
# Verify child is writing
77+
if os.path.exists(child_marker):
78+
initial_size = os.path.getsize(child_marker)
79+
await anyio.sleep(0.3)
80+
size_after_wait = os.path.getsize(child_marker)
81+
assert size_after_wait > initial_size, "Child process should be writing"
82+
83+
# Terminate using our function
84+
await _terminate_process_with_children(proc)
85+
86+
# Verify processes stopped
87+
await anyio.sleep(0.5)
88+
if os.path.exists(child_marker):
89+
size_after_cleanup = os.path.getsize(child_marker)
90+
await anyio.sleep(0.5)
91+
final_size = os.path.getsize(child_marker)
92+
assert final_size == size_after_cleanup, f"Child still running! File grew by {final_size - size_after_cleanup} bytes"
93+
94+
finally:
95+
# Clean up files
96+
for f in [parent_marker, child_marker]:
97+
if os.path.exists(f):
98+
try:
99+
os.unlink(f)
100+
except OSError:
101+
pass
102+
103+
104+
@pytest.mark.anyio
105+
async def test_stdio_client_nested_process_tree():
106+
"""
107+
Test that a nested process tree (parent → child → grandchild) is properly cleaned up.
108+
"""
109+
# Create marker files
110+
parent_file = tempfile.mktemp(suffix=".txt")
111+
child_file = tempfile.mktemp(suffix=".txt")
112+
grandchild_file = tempfile.mktemp(suffix=".txt")
113+
114+
try:
115+
# Create nested process tree
116+
parent_script = textwrap.dedent(
117+
f"""
118+
import subprocess
119+
import sys
120+
import time
121+
122+
# Child script that spawns grandchild
123+
child_script = f'''
124+
import subprocess
125+
import sys
126+
import time
127+
128+
# Grandchild script
129+
grandchild_script = \"\"\"
130+
import time
131+
with open({repr(grandchild_file)}, 'a') as f:
132+
while True:
133+
f.write(str(time.time()) + "\\\\n")
134+
f.flush()
135+
time.sleep(0.1)
136+
\"\"\"
137+
138+
# Spawn grandchild
139+
subprocess.Popen([sys.executable, '-c', grandchild_script])
140+
141+
# Child writes to its file
142+
with open({repr(child_file)}, 'a') as f:
143+
while True:
144+
f.write(str(time.time()) + "\\\\n")
145+
f.flush()
146+
time.sleep(0.1)
147+
'''
148+
149+
# Spawn child process
150+
subprocess.Popen([sys.executable, '-c', child_script])
151+
152+
# Parent writes to its file
153+
with open({repr(parent_file)}, 'a') as f:
154+
while True:
155+
f.write(str(time.time()) + "\\n")
156+
f.flush()
157+
time.sleep(0.1)
158+
"""
159+
).strip()
160+
161+
# Start parent process
162+
proc = await anyio.open_process(
163+
[python, "-c", parent_script],
164+
start_new_session=(sys.platform != "win32")
165+
)
166+
167+
# Let all processes start
168+
await anyio.sleep(1.0)
169+
170+
# Verify all are writing
171+
for file_path, name in [
172+
(parent_file, "parent"),
173+
(child_file, "child"),
174+
(grandchild_file, "grandchild"),
175+
]:
176+
if os.path.exists(file_path):
177+
initial_size = os.path.getsize(file_path)
178+
await anyio.sleep(0.3)
179+
new_size = os.path.getsize(file_path)
180+
assert new_size > initial_size, f"{name} process should be writing"
181+
182+
# Terminate all processes
183+
await _terminate_process_with_children(proc)
184+
185+
# Verify all stopped
186+
await anyio.sleep(0.5)
187+
for file_path, name in [
188+
(parent_file, "parent"),
189+
(child_file, "child"),
190+
(grandchild_file, "grandchild"),
191+
]:
192+
if os.path.exists(file_path):
193+
size_before = os.path.getsize(file_path)
194+
await anyio.sleep(0.5)
195+
size_after = os.path.getsize(file_path)
196+
assert size_after == size_before, f"{name} still running! File grew by {size_after - size_before} bytes"
197+
198+
finally:
199+
# Clean up files
200+
for f in [parent_file, child_file, grandchild_file]:
201+
if os.path.exists(f):
202+
try:
203+
os.unlink(f)
204+
except OSError:
205+
pass

0 commit comments

Comments
 (0)