Skip to content

Commit 903cc56

Browse files
committed
Reap and close spawned processes inside the child cleanup tests
The cleanup tests killed the spawned process tree but never awaited the process or closed its pipe streams. The asyncio subprocess transports stayed referenced by the per-test event loop, became garbage once that loop closed, and were finalized during a later test on the same worker. Under filterwarnings=error the resulting ResourceWarning fails whichever test happens to be running; on Windows the warning itself can die inside the transport repr with 'I/O operation on closed pipe'. A gc.collect() inside the test cannot help because the transports are still referenced by the live event loop at that point. After the stop-of-writes check confirms the tree is dead, each test now waits on the process (bounded, tolerant of the already-exited process), closes stdin, drains stdout to EOF so the event loop observes the pipe closure and can close the subprocess transport, then closes stdout. The same disposal runs in the failure-path cleanup after its kill. Draining works for both process flavors used on the platforms (anyio Process and the Windows FallbackProcess, which has no aclose), and EOF is guaranteed because every process that inherited the pipe handle is already dead.
1 parent 8fe1f69 commit 903cc56

1 file changed

Lines changed: 47 additions & 0 deletions

File tree

tests/client/test_stdio.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
import anyio
1111
import pytest
12+
from anyio.abc import Process
1213

1314
from mcp.client.session import ClientSession
1415
from mcp.client.stdio import (
@@ -17,6 +18,7 @@
1718
_terminate_process_tree,
1819
stdio_client,
1920
)
21+
from mcp.os.win32.utilities import FallbackProcess
2022
from mcp.shared.exceptions import McpError
2123
from mcp.shared.message import SessionMessage
2224
from mcp.types import CONNECTION_CLOSED, JSONRPCMessage, JSONRPCRequest, JSONRPCResponse
@@ -265,6 +267,39 @@ async def _wait_for_writes_to_stop(path: str) -> None:
265267
await anyio.sleep(0.3)
266268

267269

270+
async def _dispose_process(proc: Process | FallbackProcess) -> None:
271+
"""Reap a dead process and close its pipe streams inside the test that spawned it.
272+
273+
Without this, the subprocess transports stay referenced by the per-test event
274+
loop, become garbage only after that loop closes, and their GC-time
275+
ResourceWarnings fire during a later test on the same worker (on Windows
276+
proactor the warning can itself die in __repr__ on a closed pipe). An in-test
277+
gc.collect() cannot catch that, so the process is reaped and closed
278+
deterministically here. Draining stdout to EOF guarantees the event loop has
279+
observed the pipe closure (anyio's reader aclose alone does not close the
280+
underlying transport), which lets asyncio close the subprocess transport
281+
before the test returns.
282+
283+
Precondition: the WHOLE process tree must already be confirmed dead. wait()
284+
tolerates an already-exited process and returns promptly, but on the Windows
285+
fallback path it runs popen.wait in a thread and is effectively uncancellable,
286+
and stdout only reaches EOF once every tree member that inherited the pipe
287+
handle is gone. The timeout fails the test rather than hanging it if that
288+
precondition is ever violated.
289+
"""
290+
with anyio.fail_after(15):
291+
await proc.wait()
292+
assert proc.stdin is not None
293+
await proc.stdin.aclose()
294+
assert proc.stdout is not None
295+
while True:
296+
try:
297+
await proc.stdout.receive()
298+
except anyio.EndOfStream:
299+
break
300+
await proc.stdout.aclose()
301+
302+
268303
class TestChildProcessCleanup:
269304
"""
270305
Tests for child process cleanup functionality using _terminate_process_tree.
@@ -354,9 +389,13 @@ async def test_basic_child_process_cleanup(self):
354389

355390
# Verify the child stopped writing; a survivor times out and fails the test
356391
await _wait_for_writes_to_stop(marker_file)
392+
393+
# Tree is dead: reap and close the process so nothing leaks into later tests
394+
await _dispose_process(proc)
357395
finally:
358396
if not tree_killed: # pragma: no cover - cleanup only reached when the test failed mid-flight
359397
await _terminate_process_tree(proc)
398+
await _dispose_process(proc)
360399
# Clean up files
361400
for f in [marker_file, parent_marker]:
362401
try:
@@ -443,9 +482,13 @@ async def test_nested_process_tree(self):
443482
# Verify every level stopped writing; a survivor times out and fails the test
444483
for file_path in (parent_file, child_file, grandchild_file):
445484
await _wait_for_writes_to_stop(file_path)
485+
486+
# Tree is dead: reap and close the process so nothing leaks into later tests
487+
await _dispose_process(proc)
446488
finally:
447489
if not tree_killed: # pragma: no cover - cleanup only reached when the test failed mid-flight
448490
await _terminate_process_tree(proc)
491+
await _dispose_process(proc)
449492
# Clean up all marker files
450493
for f in [parent_file, child_file, grandchild_file]:
451494
try:
@@ -514,9 +557,13 @@ def handle_term(sig, frame):
514557

515558
# Verify the child stopped writing; a survivor times out and fails the test
516559
await _wait_for_writes_to_stop(marker_file)
560+
561+
# Tree is dead: reap and close the process so nothing leaks into later tests
562+
await _dispose_process(proc)
517563
finally:
518564
if not tree_killed: # pragma: no cover - cleanup only reached when the test failed mid-flight
519565
await _terminate_process_tree(proc)
566+
await _dispose_process(proc)
520567
# Clean up marker file
521568
try:
522569
os.unlink(marker_file)

0 commit comments

Comments
 (0)