Skip to content

Fix child process kill error with npx based servers #850

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

surya-prakash-susarla
Copy link

Motivation and Context

When running npx based servers in stdio mode, the npx process spawns a sub-process. During exit, the main process does not propagate the SIGTERM to the child process which leads to a failed kill signal and the stdin & stdout of the root process getting mangled.

The current change creates a new process group when launching the fork for the server and ensures that a kill signal is sent for the process group is the process does not shutdown cleanly. This would ensure that the process group (which includes all the child process) would be killed successfully to avoid any potential issues.

How Has This Been Tested?

Yes, a simple script like this:

from fastmcp import Client
import asyncio

async def test():
    config = {
        "mcpServers": {
            "everything": {
                "command": "npx",
                "args": ["-y", "@modelcontextprotocol/server-everything"]
            }
        }
    }

    async with Client(config) as client:
      tools = await client.list_tools()
      print("TOOlS: {tools}".format(tools=tools))

asyncio.run(test())

The above script will currently block the stdio of the parent server when run using as: python test.py
After the change the npx root & the node child process die succesfully allowing the control to flow back to the parent process ensuring a clean shutdown.

Breaking Changes

No, this should not be a breaking change since the process group flag is ignored in non-POSIX systems, and the cleanup via process group functionality is only applied for POSIX style launches.

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
  • [ x ] I have added appropriate error handling
  • [ x ] I have added or updated documentation as needed

Additional context

Please refer to the issue here (slightly different context):
#547

@surya-prakash-susarla
Copy link
Author

Failing test seems to be a streamable http test which was not modified in this change. Please let me know in case it's a side-effect of this change.

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.

1 participant