Fix stdio_client kill process after timeout #555

cristipufu · 2025-04-21T18:01:45Z

Motivation and Context

Be consistent across platforms (Windows, Linux, etc). Some MCP Servers don't react to SIGTERM, so stdio_client will hang indefinitely when exiting the context manager.

Reference: #372

How Has This Been Tested?

Yes

Breaking Changes

No

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

ihrpr

Thank you @cristipufu for working on this!

Do you have steps to reproduce? How this changes were tested?

Kludex · 2025-05-28T12:30:32Z

src/mcp/client/stdio/__init__.py

+                    await process.wait()
+            except TimeoutError:
+                # Force kill if it doesn't terminate
+                process.kill()


I think the same TerminateProcess is called on Windows... Did this worked locally?

https://anyio.readthedocs.io/en/stable/api.html#anyio.abc.Process.kill

surya-prakash-susarla · 2025-05-30T07:45:37Z

Slightly orthogonal but this does not handle child process spawns which can cause the parent to leave ghost processes which are being re-parented to the root process. Addressed it here: #850

Add comprehensive tests that validate stdio client cleanup behavior: - Universal timeout test ensures cleanup completes within reasonable time even with long-running processes - Immediate completion test ensures fast processes remain fast and timeout mechanism doesn't introduce delays - Cross-platform compatibility with different commands for Windows vs Unix These tests will help ensure PR #555's timeout mechanism works correctly without regressing normal operation. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

@cristipufu

Changes from PR #555 by @cristipufu: - Remove Windows-specific terminate_windows_process function - Apply uniform 2-second timeout mechanism across all platforms: - Try process.terminate() first (SIGTERM on Unix, similar on Windows) - Wait up to 2 seconds for graceful termination - If timeout occurs, force process.kill() (SIGKILL equivalent) This ensures consistent behavior between Windows and Unix systems when cleaning up stdio client processes, preventing hanging when MCP servers don't respond to SIGTERM. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

This change demonstrates a simpler approach than PR #555's timeout mechanism: 1. **Evidence**: Extensive testing shows PR #559 stream cleanup alone prevents hanging, even with servers that ignore SIGTERM and keep streams open. 2. **Simplification**: Removes all timeout logic and Windows-specific termination function in favor of unified `process.terminate()` + stream cleanup. 3. **Benefits**: - Less code complexity (no timeout handling, no platform branching) - Preserves proven stream cleanup protection from PR #559 - Makes behavior consistent across all platforms - All existing timeout tests still pass 4. **Risk reduction**: Avoids changing process termination semantics while maintaining hanging protection through stream cleanup. The core insight: process hanging was caused by stream management issues (solved by PR #559), not termination timing issues (targeted by PR #555).

Copy of #555 Testing shows PR #559 stream cleanup alone prevents hanging, even with servers that ignore SIGTERM and keep streams open. Removes all timeout logic and Windows-specific termination function in favor of unified `process.terminate()`.

NOTE: this test FAIL without #555 This test verifies that stdio_client properly handles processes that ignore SIGTERM but respond to SIGINT, preventing hanging issues with Node.js servers and other interactive tools that expect Ctrl+C signals.

**NOTE: These tests FAIL without the changes introduced in #559** - test_stdio_client_universal_timeout - test_stdio_client_immediate_completion await read_stream.aclose() await write_stream.aclose() await read_stream_writer.aclose() await write_stream_reader.aclose() These tests verify that stdio_client completes cleanup within reasonable time for both slow-terminating and fast-exiting processes, preventing the hanging issues reported in #559. **NOTE: This test FAILS without the changes introduced in #555** - test_stdio_client_sigint_only_process try: process.terminate() with anyio.fail_after(2.0): await process.wait() except TimeoutError: # If process doesn't terminate in time, force kill it process.kill()

**NOTE: These tests FAIL without the changes introduced in #559** - test_stdio_client_universal_timeout - test_stdio_client_immediate_completion await read_stream.aclose() await write_stream.aclose() await read_stream_writer.aclose() await write_stream_reader.aclose() These tests verify that stdio_client completes cleanup within reasonable time for both slow-terminating and fast-exiting processes, preventing the hanging issues reported in #559. **NOTE: This test FAILS without the changes introduced in #555** - test_stdio_client_sigint_only_process try: process.terminate() with anyio.fail_after(2.0): await process.wait() except TimeoutError: # If process doesn't terminate in time, force kill it process.kill() This test verifies that on UNIX systems MCP servers that don't respect SIGTERM but e.g. SIGINT still get terminated after a grace period.

cherry-pick of #555

**NOTE: These tests FAIL without the changes introduced in #559** - test_stdio_client_universal_timeout - test_stdio_client_immediate_completion await read_stream.aclose() await write_stream.aclose() await read_stream_writer.aclose() await write_stream_reader.aclose() These tests verify that stdio_client completes cleanup within reasonable time for both slow-terminating and fast-exiting processes, preventing the hanging issues reported in #559. **NOTE: This test FAILS without the changes introduced in #555** - test_stdio_client_sigint_only_process try: process.terminate() with anyio.fail_after(2.0): await process.wait() except TimeoutError: # If process doesn't terminate in time, force kill it process.kill() This test verifies that on UNIX systems MCP servers that don't respect SIGTERM but e.g. SIGINT still get terminated after a grace period.

felixweinberger

Hi @cristipufu, thank you for working on this and for submitting this PR!

Having looked into this in some detail I think your simplification makes sense:

There shouldn't be any change in behavior on Windows from this change, so we're good on that end
Fix hanging on streams when stdio_client exiting #559 already made some changes to stream cleanup and I believe addresses SIGTERM hangs already on UNIX systems
As you mention on Fixes to stdio_client to support Windows more robustly #372 though, MCP servers that ignore SIGTERM may still be an issue on UNIX systems
I was able to create a test (see below) that simulates a service that expects SIGINT and ignores SIGTERM - this PR fixes that case.

I created a draft PR #1044 with 3 new regression tests in an attempt to actually reproduce the hanging resolved by both this change (#555) and #559.

Fix hanging on streams when stdio_client exiting #559 resolves test_stdio_client_universal_cleanup and test_stdio_client_immediate_completion
Fix stdio_client kill process after timeout #555 (this PR) resolves test_stdio_client_sigint_only_process

Please take a look if that reflects what you were going for here - if yes could you rebase your PR here on latest main?

**NOTE: These tests FAIL without the changes introduced in #559** - test_stdio_client_universal_timeout - test_stdio_client_immediate_completion await read_stream.aclose() await write_stream.aclose() await read_stream_writer.aclose() await write_stream_reader.aclose() These tests verify that stdio_client completes cleanup within reasonable time for both slow-terminating and fast-exiting processes, preventing the hanging issues reported in #559. **NOTE: This test FAILS without the changes introduced in #555** - test_stdio_client_sigint_only_process try: process.terminate() with anyio.fail_after(2.0): await process.wait() except TimeoutError: # If process doesn't terminate in time, force kill it process.kill() This test verifies that on UNIX systems MCP servers that don't respect SIGTERM but e.g. SIGINT still get terminated after a grace period.

Fix stdio_client kill process after timeout

12f2f50

This was referenced Apr 21, 2025

Fixes to stdio_client to support Windows more robustly #372

Merged

Fix process termination in stdio client #496

Closed

BenMawnMahlauNBTC mentioned this pull request Apr 23, 2025

On Windows 11 when you initialize an mcp client it hangs indefinitely #552

Open

ihrpr added this to the r-05-25 milestone Apr 29, 2025

This was referenced May 7, 2025

Fix Windows subprocess NotImplementedError (STDIO clients) #596

Merged

MCPToolset.from_server fails, raising NotImplementedError google/adk-python#611

Closed

pkduongsu mentioned this pull request May 8, 2025

MCP_Python Issue preventing me from integrating Notion MCP with GoogleADK via StdioStandardParameters pkduongsu/notion_agent#1

Open

ihrpr mentioned this pull request May 28, 2025

Align stdio shutdown sequence with spec #765

Open

9 tasks

ihrpr requested changes May 28, 2025

View reviewed changes

ihrpr modified the milestones: r-05-25, stdio shutdown May 28, 2025

Kludex reviewed May 28, 2025

View reviewed changes

felixweinberger mentioned this pull request Jun 26, 2025

Add regression tests for #559 and #555 #1044

Draft

9 tasks

felixweinberger added a commit that referenced this pull request Jun 26, 2025

Simplify stdio cleanup: unified stream cleanup across platforms

a60d852

cherry-pick of #555

felixweinberger requested changes Jun 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix stdio_client kill process after timeout #555

Fix stdio_client kill process after timeout #555

cristipufu commented Apr 21, 2025 •

edited

Loading

Uh oh!

ihrpr left a comment

Uh oh!

Kludex May 28, 2025

Uh oh!

surya-prakash-susarla commented May 30, 2025

Uh oh!

felixweinberger left a comment •

edited

Loading

Uh oh!

Uh oh!

Fix stdio_client kill process after timeout #555

Are you sure you want to change the base?

Fix stdio_client kill process after timeout #555

Conversation

cristipufu commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

Checklist

Uh oh!

ihrpr left a comment

Choose a reason for hiding this comment

Uh oh!

Kludex May 28, 2025

Choose a reason for hiding this comment

Uh oh!

surya-prakash-susarla commented May 30, 2025

Uh oh!

felixweinberger left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cristipufu commented Apr 21, 2025 •

edited

Loading

felixweinberger left a comment •

edited

Loading