Skip to content

Commit 864e52b

Browse files
committed
fix: Improve Windows process termination timeouts
- Increase PROCESS_TERMINATION_TIMEOUT to 5s on Windows (vs 2s on Unix) - Increase _terminate_process_tree timeout to 4s on Windows (vs 2s on Unix) - Adjust test timeouts to be more generous on Windows: - stdin_close_ignored test: 12s timeout on Windows vs 7s on Unix - universal_cleanup test: 10s max time on Windows vs 6s on Unix - stdin_close_ignored assert: up to 8s on Windows vs 4.5s on Unix This addresses Windows-specific process termination delays that were causing test failures in CI.
1 parent 1ab73db commit 864e52b

File tree

2 files changed

+64
-26
lines changed

2 files changed

+64
-26
lines changed

src/mcp/client/stdio/__init__.py

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@
4444
)
4545

4646
# Timeout for process termination before falling back to force kill
47-
PROCESS_TERMINATION_TIMEOUT = 2.0
47+
# Windows needs more time for process termination
48+
PROCESS_TERMINATION_TIMEOUT = 5.0 if sys.platform == "win32" else 2.0
4849

4950

5051
def get_default_environment() -> dict[str, str]:
@@ -123,7 +124,11 @@ async def stdio_client(server: StdioServerParameters, errlog: TextIO = sys.stder
123124
process = await _create_platform_compatible_process(
124125
command=command,
125126
args=server.args,
126-
env=({**get_default_environment(), **server.env} if server.env is not None else get_default_environment()),
127+
env=(
128+
{**get_default_environment(), **server.env}
129+
if server.env is not None
130+
else get_default_environment()
131+
),
127132
errlog=errlog,
128133
cwd=server.cwd,
129134
)
@@ -167,7 +172,9 @@ async def stdin_writer():
167172
try:
168173
async with write_stream_reader:
169174
async for session_message in write_stream_reader:
170-
json = session_message.message.model_dump_json(by_alias=True, exclude_none=True)
175+
json = session_message.message.model_dump_json(
176+
by_alias=True, exclude_none=True
177+
)
171178
await process.stdin.send(
172179
(json + "\n").encode(
173180
encoding=server.encoding,
@@ -253,7 +260,9 @@ async def _create_platform_compatible_process(
253260
return process
254261

255262

256-
async def _terminate_process_tree(process: Process | FallbackProcess, timeout_seconds: float = 2.0) -> None:
263+
async def _terminate_process_tree(
264+
process: Process | FallbackProcess, timeout_seconds: float | None = None
265+
) -> None:
257266
"""
258267
Terminate a process and all its children using platform-specific methods.
259268
@@ -262,8 +271,10 @@ async def _terminate_process_tree(process: Process | FallbackProcess, timeout_se
262271
263272
Args:
264273
process: The process to terminate
265-
timeout_seconds: Timeout in seconds before force killing (default: 2.0)
274+
timeout_seconds: Timeout in seconds before force killing (default: platform-specific)
266275
"""
276+
if timeout_seconds is None:
277+
timeout_seconds = 4.0 if sys.platform == "win32" else 2.0
267278
if sys.platform == "win32":
268279
await terminate_windows_process_tree(process, timeout_seconds)
269280
else:

tests/client/test_stdio.py

Lines changed: 48 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -66,14 +66,20 @@ async def test_stdio_client():
6666
break
6767

6868
assert len(read_messages) == 2
69-
assert read_messages[0] == JSONRPCMessage(root=JSONRPCRequest(jsonrpc="2.0", id=1, method="ping"))
70-
assert read_messages[1] == JSONRPCMessage(root=JSONRPCResponse(jsonrpc="2.0", id=2, result={}))
69+
assert read_messages[0] == JSONRPCMessage(
70+
root=JSONRPCRequest(jsonrpc="2.0", id=1, method="ping")
71+
)
72+
assert read_messages[1] == JSONRPCMessage(
73+
root=JSONRPCResponse(jsonrpc="2.0", id=2, result={})
74+
)
7175

7276

7377
@pytest.mark.anyio
7478
async def test_stdio_client_bad_path():
7579
"""Check that the connection doesn't hang if process errors."""
76-
server_params = StdioServerParameters(command="python", args=["-c", "non-existent-file.py"])
80+
server_params = StdioServerParameters(
81+
command="python", args=["-c", "non-existent-file.py"]
82+
)
7783
async with stdio_client(server_params) as (read_stream, write_stream):
7884
async with ClientSession(read_stream, write_stream) as session:
7985
# The session should raise an error when the connection closes
@@ -147,8 +153,10 @@ async def test_stdio_client_universal_cleanup():
147153
elapsed = end_time - start_time
148154

149155
# On Windows: 2s (stdin wait) + 2s (terminate wait) + overhead = ~5s expected
150-
assert elapsed < 6.0, (
151-
f"stdio_client cleanup took {elapsed:.1f} seconds, expected < 6.0 seconds. "
156+
# Windows may need more time for process termination
157+
max_cleanup_time = 10.0 if sys.platform == "win32" else 6.0
158+
assert elapsed < max_cleanup_time, (
159+
f"stdio_client cleanup took {elapsed:.1f} seconds, expected < {max_cleanup_time} seconds. "
152160
f"This suggests the timeout mechanism may not be working properly."
153161
)
154162

@@ -161,7 +169,9 @@ async def test_stdio_client_universal_cleanup():
161169

162170

163171
@pytest.mark.anyio
164-
@pytest.mark.skipif(sys.platform == "win32", reason="Windows signal handling is different")
172+
@pytest.mark.skipif(
173+
sys.platform == "win32", reason="Windows signal handling is different"
174+
)
165175
async def test_stdio_client_sigint_only_process():
166176
"""
167177
Test cleanup with a process that ignores SIGTERM but responds to SIGINT.
@@ -254,7 +264,9 @@ class TestChildProcessCleanup:
254264
"""
255265

256266
@pytest.mark.anyio
257-
@pytest.mark.filterwarnings("ignore::ResourceWarning" if sys.platform == "win32" else "default")
267+
@pytest.mark.filterwarnings(
268+
"ignore::ResourceWarning" if sys.platform == "win32" else "default"
269+
)
258270
async def test_basic_child_process_cleanup(self):
259271
"""
260272
Test basic parent-child process cleanup.
@@ -303,7 +315,9 @@ async def test_basic_child_process_cleanup(self):
303315
print("\nStarting child process termination test...")
304316

305317
# Start the parent process
306-
proc = await _create_platform_compatible_process(sys.executable, ["-c", parent_script])
318+
proc = await _create_platform_compatible_process(
319+
sys.executable, ["-c", parent_script]
320+
)
307321

308322
# Wait for processes to start
309323
await anyio.sleep(0.5)
@@ -317,7 +331,9 @@ async def test_basic_child_process_cleanup(self):
317331
await anyio.sleep(0.3)
318332
size_after_wait = os.path.getsize(marker_file)
319333
assert size_after_wait > initial_size, "Child process should be writing"
320-
print(f"Child is writing (file grew from {initial_size} to {size_after_wait} bytes)")
334+
print(
335+
f"Child is writing (file grew from {initial_size} to {size_after_wait} bytes)"
336+
)
321337

322338
# Terminate using our function
323339
print("Terminating process and children...")
@@ -333,9 +349,9 @@ async def test_basic_child_process_cleanup(self):
333349
final_size = os.path.getsize(marker_file)
334350

335351
print(f"After cleanup: file size {size_after_cleanup} -> {final_size}")
336-
assert final_size == size_after_cleanup, (
337-
f"Child process still running! File grew by {final_size - size_after_cleanup} bytes"
338-
)
352+
assert (
353+
final_size == size_after_cleanup
354+
), f"Child process still running! File grew by {final_size - size_after_cleanup} bytes"
339355

340356
print("SUCCESS: Child process was properly terminated")
341357

@@ -348,7 +364,9 @@ async def test_basic_child_process_cleanup(self):
348364
pass
349365

350366
@pytest.mark.anyio
351-
@pytest.mark.filterwarnings("ignore::ResourceWarning" if sys.platform == "win32" else "default")
367+
@pytest.mark.filterwarnings(
368+
"ignore::ResourceWarning" if sys.platform == "win32" else "default"
369+
)
352370
async def test_nested_process_tree(self):
353371
"""
354372
Test nested process tree cleanup (parent → child → grandchild).
@@ -408,7 +426,9 @@ async def test_nested_process_tree(self):
408426
)
409427

410428
# Start the parent process
411-
proc = await _create_platform_compatible_process(sys.executable, ["-c", parent_script])
429+
proc = await _create_platform_compatible_process(
430+
sys.executable, ["-c", parent_script]
431+
)
412432

413433
# Let all processes start
414434
await anyio.sleep(1.0)
@@ -454,7 +474,9 @@ async def test_nested_process_tree(self):
454474
pass
455475

456476
@pytest.mark.anyio
457-
@pytest.mark.filterwarnings("ignore::ResourceWarning" if sys.platform == "win32" else "default")
477+
@pytest.mark.filterwarnings(
478+
"ignore::ResourceWarning" if sys.platform == "win32" else "default"
479+
)
458480
async def test_early_parent_exit(self):
459481
"""
460482
Test cleanup when parent exits during termination sequence.
@@ -498,7 +520,9 @@ def handle_term(sig, frame):
498520
)
499521

500522
# Start the parent process
501-
proc = await _create_platform_compatible_process(sys.executable, ["-c", parent_script])
523+
proc = await _create_platform_compatible_process(
524+
sys.executable, ["-c", parent_script]
525+
)
502526

503527
# Let child start writing
504528
await anyio.sleep(0.5)
@@ -625,7 +649,9 @@ def sigterm_handler(signum, frame):
625649
start_time = time.time()
626650

627651
# Use anyio timeout to prevent test from hanging forever
628-
with anyio.move_on_after(7.0) as cancel_scope:
652+
# Windows process termination can be slower, so give it more time
653+
timeout_seconds = 12.0 if sys.platform == "win32" else 7.0
654+
with anyio.move_on_after(timeout_seconds) as cancel_scope:
629655
async with stdio_client(server_params) as (read_stream, write_stream):
630656
# Let the process start
631657
await anyio.sleep(0.2)
@@ -634,16 +660,17 @@ def sigterm_handler(signum, frame):
634660

635661
if cancel_scope.cancelled_caught:
636662
pytest.fail(
637-
"stdio_client cleanup timed out after 7.0 seconds. "
663+
f"stdio_client cleanup timed out after {timeout_seconds} seconds. "
638664
"Process should have been terminated via SIGTERM escalation."
639665
)
640666

641667
end_time = time.time()
642668
elapsed = end_time - start_time
643669

644670
# Should take ~2 seconds (stdin close timeout) before SIGTERM is sent
645-
# Total time should be between 2-4 seconds
646-
assert 1.5 < elapsed < 4.5, (
671+
# Total time should be between 2-8 seconds (Windows needs more time)
672+
max_expected = 8.0 if sys.platform == "win32" else 4.5
673+
assert 1.5 < elapsed < max_expected, (
647674
f"stdio_client cleanup took {elapsed:.1f} seconds for stdin-ignoring process. "
648-
f"Expected between 2-4 seconds (2s stdin timeout + termination time)."
675+
f"Expected between 1.5-{max_expected} seconds (2s stdin timeout + termination time)."
649676
)

0 commit comments

Comments
 (0)