{{ message }}
Unify process termination on POSIX & Windows (+ tests) - #1044
Merged
Merged
Conversation
felixweinberger
force-pushed
the
fweinberger/stream-cleanup-only-approach
branch
2 times, most recently
from
June 26, 2025 21:31
9f88ea7 to
a27fcef
Compare
9 tasks
felixweinberger
force-pushed
the
fweinberger/stream-cleanup-only-approach
branch
6 times, most recently
from
June 30, 2025 14:55
caa5628 to
1d495ca
Compare
felixweinberger
changed the title
felixweinberger
changed the title
felixweinberger
force-pushed
the
fweinberger/stream-cleanup-only-approach
branch
from
June 30, 2025 15:39
9e8d74b to
3f6c472
Compare
9 tasks
felixweinberger
changed the title
felixweinberger
force-pushed
the
fweinberger/stream-cleanup-only-approach
branch
15 times, most recently
from
July 1, 2025 17:45
cbaee2e to
e8954dd
Compare
felixweinberger
changed the title
felixweinberger
force-pushed
the
fweinberger/stream-cleanup-only-approach
branch
8 times, most recently
from
July 4, 2025 17:39
e817dfb to
7af9e65
Compare
Contributor
Author
felixweinberger
marked this pull request as ready for review
July 4, 2025 17:41
9 tasks
|
Any idea why |
The stdio cleanup was hanging indefinitely when processes ignored termination signals or took too long to exit. This caused the MCP client to freeze during shutdown, especially with servers that don't handle SIGTERM properly. This was already being handled on Windows, but not Unix systems. This Commit unifies the two approaches, removing special logic for windows process termination. The fix introduces a 2-second timeout for process termination. If a process doesn't exit gracefully within this window, it's forcefully killed. This ensures the client always completes cleanup in bounded time while still giving well-behaved servers a chance to exit cleanly. This resolves hanging issues reported when MCP servers ignore standard termination signals. resolves #555 Also adds regression tests for #559. Co-authored-by: Cristian Pufu <cristian.pufu@uipath.com>
This re-establishes behavior before #596 in the default case. - Attempt to use anyio's native open_process function on Windows - Fall back to subprocess.Popen only if NotImplementedError is raised - This improves compatibility with event loops that support async subprocesses - Extract fallback logic into separate function for clarity
felixweinberger
force-pushed
the
fweinberger/stream-cleanup-only-approach
branch
from
July 7, 2025 13:59
7af9e65 to
c9b43bc
Compare
Contributor
Author
felixweinberger
added a commit
that referenced
this pull request
Jul 7, 2025
This test shows that MCP server cleanup code in lifespan doesn't run when the process is terminated, but does run when stdin is closed first (as implemented in PR #1044). The test includes: - Demonstration of current broken behavior (cleanup doesn't run) - Verification that stdin closure allows graceful shutdown - Windows-specific ResourceWarning handling - Detailed documentation of the issue and solution Github-Issue:#1027
ihrpr
previously approved these changes
Jul 7, 2025
- Add SIGTERM_IGNORING_PROCESS_TIMEOUT constant in tests to document timeout behavior - Add PROCESS_TERMINATION_TIMEOUT constant to replace magic number in stdio client - Restore deprecated terminate_windows_process function with original functionality to maintain backward compatibility for external users The deprecated function is marked using @deprecated decorator following the codebase convention, while preserving its original terminate-wait-kill behavior.
felixweinberger
force-pushed
the
fweinberger/stream-cleanup-only-approach
branch
from
July 8, 2025 10:50
9fd111e to
5771abc
Compare
ihrpr
approved these changes
Jul 8, 2025
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
#555, #559 address important but different ways MCP servers can hang or fail to successfully terminate on Windows & POSIX systems.
This PR re-implements #555 by aligning process termination between Windows and Unix to make sure SIGTERM-ignoring processes don't inadvertently cause hangs.
How Has This Been Tested?
New regression tests added for #555 and #559. These were developed and tested on both POSIX and Windows systems.
Breaking Changes
None.
Types of changes
Checklist
Additional context