fix(core): properly cleanup MCP server subprocesses on exit - #1285
Conversation
|
Thank you for your contribution! I've already tested it and everything is fine. We will merge this PR. |
|
Thanks for the fix — wiring cleanup into the CLI exit flow via Config.shutdown looks solid. One small potential gap: ToolRegistry.discoverToolsForServer() currently uses connectAndDiscover() (not McpClientManager/McpClient), so any client/transport started via that path won’t be tracked by mcpClientManager.stop() and thus won’t be closed by ToolRegistry.stop()/Config.shutdown. Today this path is used by /mcp auth and likely targets network servers, but if a command-based server ever goes through this flow it could still leave a subprocess around until process exit. Might be worth a follow-up to route single-server re-discovery through McpClientManager (or otherwise track/stop those connections) so shutdown is comprehensive. |
|
I test this PR as well and it looks everything is fine. Thank you for your contribution. |
|
@liqiongyu good catch — I routed |
This reverts commit d7b003076e8a5f6b387b7313ecab0cd7ebea0997.
This reverts commit 61421e65872acebe20d88c2d40c9fea40fa3fefa.
|
@tt-a1i I've rebased this PR and updated some unit tests. All tests are passing now. Could you please take another look when you have a moment? If everything looks good, we're ready to merge. Thank you! |
fix(core): properly cleanup MCP server subprocesses on exit
Summary
Fixes #1213
MCP server subprocesses were not being terminated when qwen-code exits, causing orphaned processes to accumulate. Each startup/exit cycle left behind zombie processes still holding WS ports.
Root cause:
mcpClientManager.stop()was never called during the exit cleanup flow.Changes:
ToolRegistry.stop()to stop all MCP clientsConfig.shutdown()as a unified cleanup entry point (idempotent, handles uninitialized state)gemini.tsximmediately after config creationThe cleanup now covers all execution modes (interactive, non-interactive, stream-json) since it's registered at the CLI entry point before any mode-specific branching.
Test
Added unit tests for:
stop()disconnects all connected MCP clientsstop()is idempotent (can be called multiple times safely)