Improve child process termination on POSIX & Windows

[felixweinberger]

Motivation and Context

#729, and #850 address important failures to shut down childprocesses when MCP servers spawn subprocesses.

This is not a trivial thing to fix, as there's no great cross-platform library for terminating process-trees on all platforms so we have to fork between Unix (using os.killpg) and Windows (using JobObjects via pywin32 which is the best supported external library for this: https://github.com/mhammond/pywin32)

This PR adds regressions tests to clearly demonstrate the edge cases addressed + unifies the approaches from the PRs listed above.

How Has This Been Tested?

New regression tests added (each fails without the relevant fix). These were developed and tested on both POSIX and Windows systems.

For #729 and #850 (properly terminate processes and their children across platforms):

• Original authors: @jingx8885, @surya-prakash-susarla

Breaking Changes

None.

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

Additional context

[felixweinberger]

@dsp-ant as discussed platform native ways to terminate processes for Unix & Windows without race conditions between child enumeration and parent shutdown.

@jingx8885 I had to break this out of #1044 to isolate this particular problem. If you'd be willing to test whether this branch still resolves your issue like you did on #1044 that would be greatly appreciated! The tests still pass, but just to make sure there's nothing we missed.

[jingx8885]

@felixweinberger got it, I'll deal with it in a few days.

[felixweinberger]

@Kludex @agronholm given I've seen you both on Python process management related PRs, tagging y'all here in case you have any thoughts on whether this could or should be simpler with anyio 🙂

This is an edge case where terminating a process doesn't automatically terminate children via anyio's terminate(), but I'm also not sure whether it should in the first place as Python's asyncio doesn't do so either AFAIK.

For context, we need the subprocesses to be killed in the stdio case of MCP, because the MCP "server" is started as a subprocess of the "client". When spinning down the client, it should also spin down that server to free up resources.

[felixweinberger]

Aware that using "timeout" anywhere in a test is usually not great practice - however, cross platform challenges and the subject matter being tested (processes) make this a little trickier to resolve than usual, as these tests are fundamentally there to detect whether processes are being closed (which in these tests should take a negligible, but non-zero amount of time).

I ran a deep research on alternative approaches - here's @claude 's POV:

⏺ Deep Analysis: Is the Time-Based Testing Approach Justified?

After researching testing best practices and ultrathinking about this specific case, yes, the approach is justified. Here's why:

The Tests Are Actually Condition-Based, Not Time-Based

The tests check a condition: "Is the file size stable?" This indicates process termination. The sleep() calls create measurement windows, not synchronization points. This is fundamentally
different from hoping timing works out.

Why Alternative Approaches Don't Work Here

1. Process polling (poll()/wait()): Only works for known process handles. This PR tests termination of entire process trees including unknown grandchildren.
2. Mock-based testing: Process groups and Windows Job Objects require real OS interaction. The bugs being fixed are OS-level issues that mocks can't reproduce.
3. Event/signal-based coordination: Would require modifying child processes, but the goal is to test termination of arbitrary processes (like npx).
4. OS monitoring APIs: Platform-specific and still require polling. The file-based approach is simpler and more portable.

The Approach Is Well-Designed

• Black-box testing: Verifies actual behavior (processes stop working) rather than just process state
• Cross-platform: Works identically on Windows and Unix without platform-specific test code
• Real-world scenario: Tests continuous I/O patterns common in development servers
• Minimal timing: Uses 0.1s intervals and reasonable timeouts
• Comprehensive: Tests single children, nested trees, and race conditions

Critical Context

These tests provide regression coverage for production bugs where child processes weren't properly terminated. The ability to reliably test this behavior outweighs the theoretical concerns
about time-based tests.

Recommendations

1. Consider adding a comment explaining why the file-based approach is used:

# We use file writing as a cross-platform way to verify process liveness
# without needing to know child PIDs or use platform-specific APIs

2. The ResourceWarning filter is appropriately documented and justified.
3. Future enhancement: Consider adding a process count assertion on Unix using ps to supplement the file-based checks.

Verdict

This PR provides a robust, well-tested solution to critical process management bugs. The testing approach, while using timing windows, is actually condition-based and represents the most
pragmatic way to verify process tree termination across platforms. The implementation quality is high with proper platform abstractions and error handling.

[jingx8885]

@felixweinberger I saw you used win32api instead of psutil, and used win32job.TerminateJobObject in terminate_windows_process_tree function to kill all processes, which is more simple and elegance, I tested in local and it‘s worked, great job!

[felixweinberger]

Thank you for testing it, really appreciate you taking the time!

[ihrpr]

Thank you!

[dsp-ant]

This looks great. Thank you for working through this. My main comment here is about just improving the way we organize platform code. I would consider doing that before pushing the PR.

The base branch was changed.

[surya-prakash-susarla]

Thank you for pushing the fix!

[sungigant]

Uploading hello-vault-a8a65c85bbae57c879fa3c42e0320353623159ea.zip…