Skip to content

Fix kill on drop on Windows #28

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

Closed
wants to merge 1 commit into from
Closed

Conversation

ozaner
Copy link

@ozaner ozaner commented May 6, 2025

📌 Summary

When a StdioTransport is dropped, it's child process is meant to be killed along with it. However this does not work on Windows, allowing for zombie stdio processes to stay alive in the background.

✨ Changes Made

Seems the code had separate sections for spawning in the child process on both Windows and Unix.

Fix was simple, instead of spawning a "cmd.exe" process and running the command on that:

let command = "cmd.exe".to_string();
let mut command_args = vec!["/c".to_string(), self.command.clone().unwrap_or_default()];
command_args.extend(self.args.clone().unwrap_or_default());

Just run the command directly (as in the Unix case):

let command = self.command.clone().unwrap_or_default();
let command_args = self.args.clone().unwrap_or_default();

In practice, this just meant removing the Windows specific code, and using the Unix specific code for both.

🛠️ Testing Steps

Try running a stdio mcp server that does not terminate upon the server closing (e.g. if it's a Typescript server add a setInterval). The Rust program will hang because of the zombie process.

💡 Additional Notes

I imagine this bug is a result of development/testing happening on Unix rather than Windows. I am currently developing on Windows so I was able to catch it. Other than this bug, everything else seems to work perfectly. 👍

@hashemix
Copy link
Member

Hey @ozaner
Thanks for your contribution and detailed PR description!

I noticed that your suggested change removes the cmd.exe wrapper on Windows.

Just wanted to clarify why that was intentionally included for cross-platform compatibility.

On Windows, certain commands, particularly shell commands or scripts like npx, npm, or other tools that rely on shell resolution, don't behave the same way when executed directly via std::process::Command. (Unlike Unix-based systems, where commands like npx are typically shell-resolved correctly)
Using cmd was a workaround to avoid that issue.

The change you suggested will break functionality on Windows. you can verify this by running the following command in your branch on a Windows machine:

 cargo run -p simple-mcp-client

you will likely get a program not found error:

image

That being said, I tried to replicate the issue you observed regarding zombie stdio processes to stay alive in the background.
I noticed stdio process remains alive for a while, sometimes few seconds, sometimes about a minute and then windows terminates it.

As this PR breaks the expected functionality , I am going to close it for now but we can re-open it once we are aligned on a proper fix.

If you're still interested in helping fix this , please open an issue first, that way, we can chat about the solution before moving ahead with a pull request.

Here are my thoughts on finding a working solution:
Wrapping in cmd.exe may not be the best solution on windows, there are alternatives we can look into like

  • look into reason why after tokio::process is finished, windows keeps the shell open for a while
  • use which crate to resolve the command first before starting it
  • Maybe powershell is a better option

@hashemix hashemix closed this May 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants