-
Notifications
You must be signed in to change notification settings - Fork 2k
Unify process termination on POSIX & Windows (+ tests) #1044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
9f88ea7
to
a27fcef
Compare
caa5628
to
1d495ca
Compare
9e8d74b
to
3f6c472
Compare
cbaee2e
to
e8954dd
Compare
Verification of Windows Process Tree Termination Fix Scenario: We observed the following process hierarchy:
Failure:
...as orphaned/zombie processes on Windows. Validation:
Conclusion: |
50559f7
to
84c4b0f
Compare
After receiving offline feedback from @dsp-ant I'm splitting this PR into 2:
FYI for @jingx8885 @surya-prakash-susarla, it's currently in draft and still needs work, I'll tag you once its ready |
@agronholm Should we have to create OSS specific logic for termination when using |
I'm not sure I follow. Can you give me more context? I'm very new to this project. |
84c4b0f
to
4d1804a
Compare
e817dfb
to
7af9e65
Compare
Hi @Kludex, @agronholm thanks for taking a look! For context, we actually don't spawn windows processes with However, your comment made me realize that we should probably explicitly guard for that case and still use If this could somehow be fixed upstream in |
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 <[email protected]>
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
7af9e65
to
c9b43bc
Compare
@agronholm Doing some research based on what @theailanguage raised in #596:
It seems that while the |
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
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