Skip to content

fix: Client.Stop does not terminate stdio MCP server processes, so direct callers leak subprocesses#755

Closed
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-9517d7bc
Closed

fix: Client.Stop does not terminate stdio MCP server processes, so direct callers leak subprocesses#755
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-9517d7bc

Conversation

@sam-saffron-jarvis

Copy link
Copy Markdown
Contributor

What changed

  • taught internal/mcp.Client to retain a stdio process cancel func when it starts command-based MCP servers
  • updated Client.Stop() to invoke that cancel func before closing the MCP session, so direct callers stop the stdio process group instead of only closing the protocol session
  • normalized expected shutdown errors from intentional stdio cancellation so Stop() still behaves like a clean stop
  • added a regression test that starts a real stdio MCP server through a wrapper shell which backgrounds a child process, then verifies Client.Stop() also terminates that leaked child

Why this is high-value

Direct client.Start(...); defer client.Stop() call sites in term-llm mcp ... and the MCP browser/test flows were relying on Stop() to clean up stdio servers, but Stop() previously only closed the MCP session. Because the command itself was tied to a cancelable context that Stop() did not own, direct callers could leave behind helper subprocesses or descendant daemons after returning.

This is high-value because leaked MCP helper processes accumulate over time, keep ports and file descriptors open, and can make later MCP interactions flaky or interfere with authenticated/background services.

Validation

  • gofmt -w internal/mcp/client.go internal/mcp/client_test.go
  • go build ./...
  • go test ./...
  • added TestClientStop_CancelsStdioProcessGroup covering the specific leak path

@SamSaffron

Copy link
Copy Markdown
Owner

Implemented the important changes from this PR directly in main worktree.

@SamSaffron SamSaffron closed this Jun 1, 2026
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