fix: force kill MCP server processes on dispose to prevent orphan processes - #7424
fix: force kill MCP server processes on dispose to prevent orphan processes#7424hendem wants to merge 4 commits into
Conversation
|
The following comment was made by an LLM, it may be inaccurate: No duplicate PRs found |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This pull request introduces force-kill process cleanup for MCP (Model Context Protocol) server processes to prevent orphan processes from accumulating when graceful shutdown fails. The change tracks local MCP server transports and forcefully terminates their processes after attempting graceful closure, addressing a memory leak issue where ~300MB+ processes could persist after OpenCode exits.
Key changes:
- Added forceKillProcess() helper with platform-specific process termination (taskkill on Windows, SIGKILL on Unix)
- Introduced LocalTransportInfo type to track StdioClientTransport instances alongside MCP clients
- Modified dispose and disconnect logic to force-kill processes after 2-second graceful close timeout
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/opencode/src/mcp/index.ts | Core implementation: adds LocalTransportInfo tracking, forceKillProcess helper, and force-kill logic in dispose/disconnect flows |
| packages/opencode/test/mcp/process-cleanup.test.ts | Unit tests verifying PID tracking for local transports and client.close() invocation during disconnect |
packages/opencode/src/mcp/index.ts:529
- When listTools fails during client creation, the code closes the MCP client but doesn't force kill the local process. If the graceful close fails (as described in the PR description), this could leave orphan processes. Consider adding force kill logic here as well, similar to the disconnect function, by checking if localTransport is defined and calling forceKillProcess after the failed close attempt.
if (!result) {
await mcpClient.close().catch((error) => {
log.error("Failed to close MCP client", {
error,
})
})
status = {
status: "failed",
error: "Failed to get tools",
}
return {
mcpClient: undefined,
localTransport: undefined,
status: {
status: "failed" as const,
error: "Failed to get tools",
},
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
Sorry, something went wrong.
…cesses Track StdioClientTransport instances for local MCP servers to enable forceful process termination on dispose. The MCP SDK's client.close() relies on AbortController signals which don't work for all processes (e.g., docker containers without --init, hung processes). Changes: - Track LocalTransportInfo (transport + name) alongside MCP clients - Add forceKillProcess() helper for cross-platform process termination - Use taskkill on Windows, SIGKILL on Unix for forceful termination - Add timeout to client.close() calls to prevent hanging on dispose - Update connect() and disconnect() to track/cleanup local transports - Add tests for transport tracking and client lifecycle This prevents orphan MCP server processes from accumulating across sessions, which was causing significant memory usage (~300MB+ per orphan process). Related: anomalyco#7261
|
This is a good defensive fix for MCP processes that don't respond to graceful close. Worth noting that a separate root cause also contributes to orphaned MCP processes: the parent opencode process itself doesn't handle SIGHUP (terminal closure signal). When the terminal is closed, opencode stays alive as an orphan under launchd/PID 1, and its MCP children stay alive with it — not because MCP cleanup failed, but because cleanup was never triggered. Adding SIGHUP handlers to the main entry points (tui, attach, serve) would complement this PR by ensuring Instance.disposeAll() actually runs on terminal close, which then triggers the MCP client cleanup path this PR improves. |
Sorry, something went wrong.
|
Closing this pull request because it has had no updates for more than 60 days. If you plan to continue working on it, feel free to reopen or open a new PR. |
Sorry, something went wrong.
Relates to #3013, #7261
Summary
Problem
The MCP SDK's client.close() uses AbortController.abort() which may not work reliably for:
Solution
Defensive fallback that only triggers if graceful close doesn't work:
Testing
Added tests in test/mcp/process-cleanup.test.ts.