Subprocess kill win32
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
- [x] 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
- [x] I have read the MCP Documentation
- [x] My code follows the repository's style guidelines
- [x] New and existing tests pass locally
- [ ] I have added appropriate error handling
- [ ] I have added or updated documentation as needed
Additional context
@dsp-ant require code review
@dsp-ant require code review, thinks.
-
PR test is passed.
-
This issue is difficult to write test cases for, but I can describe the specific problem. During development of a Python MCP server using the standard uv-pyproject structure, I conducted integration testing with the mcp-agent project. When configuring the MCP server in stdio mode and launching it via uv run --env-file=xxx mcp_server.py, the service initialization creates a Python subprocess that spawns a uv.exe process, which in turn launches the actual MCP stdio service process. The issue occurs when terminating the parent subprocess - it fails to kill the child processes, leaving both the uv.exe and MCP stdio services as zombie processes. @felixweinberger
Thank you again @jingx8885 for submitting this PR and for your detailed responses on what failures you were seeing - extremely helpful.
I spent yesterday and today trying to unify all the different approaches and fixes we have pending in this process termination space at the moment, as there are several interrelated fixes that either conflict or depend on each other - specifically #555, #729, #765, and #850.
I've added your change to #1044 as a draft with you as a co-author + added extensive regression testing. Would you be OK with consolidating this change into #1044 for the comprehensive testing & process handling introduced there?
If you're able to, I'd also love your feedback whether the unified approach in #1044 successfully addresses the issue you're seeing in this PR.
@felixweinberger Thank you again for consolidating these changes and including me as a co-author on #1044. I've now had a chance to review the draft #1044 code in detail.
Great news: I can confirm that my specific changes from this PR have been correctly integrated into the unified approach of #1044. More importantly, the logic addressing the process termination issue I encountered is fully present.
Critical Validation: I want to highlight that this exact logic (or its direct equivalent now in #1044) has been running successfully in both our production environment and daily development/testing cycles for a month. It has proven to be robust and reliable on Windows 11 under our workload conditions. This gives me high confidence that #1044 effectively resolves the core issue I originally aimed to fix.
Therefore, I fully support merging these changes via #1044 and agree to close this PR in favor of it.
I remain very keen to collaborate on finalizing #1044. Please let me know how I can best contribute next – whether it's further testing on specific platforms/scenarios, reviewing recent updates, or assisting with documentation.
Looking forward to getting this comprehensive fix merged!
Best regards,
Close this merge, support #1044 to fix this bug.