fix: destroy descendant processes on StdioClientTransport.closeGracefully#966
Open
k-krawczyk wants to merge 1 commit into
Open
fix: destroy descendant processes on StdioClientTransport.closeGracefully#966k-krawczyk wants to merge 1 commit into
k-krawczyk wants to merge 1 commit into
Conversation
…ully When `closeGracefully()` is called, the previous implementation invoked only `process.destroy()` on the root process. With wrapper commands such as `npx` (POSIX) or `cmd.exe /c npx.cmd ...` (Windows), the wrapper has already forked child processes (`node.exe`, the MCP server itself); destroying only the wrapper leaves those descendants orphaned. On Windows this prevents the JVM from shutting down cleanly. Walk `ProcessHandle.descendants()` (JDK 9+, available since the repo's Java 17 baseline) and destroy each descendant before destroying the root. This avoids the OS-specific `taskkill /pid X /F /T` approach suggested on the issue: same effect, one cross-platform code path, no shelling out. The descendant snapshot is best-effort — processes forked concurrently with the destroy call may be missed, but the wider tree generally dies with its parent. Fixes modelcontextprotocol#496
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #496
Summary
StdioClientTransport.closeGracefully()previously calledprocess.destroy()on the root process only. When the configured command is a wrapper such asnpx(POSIX) orcmd.exe /c npx.cmd ...(Windows), the wrapper has already forked child processes (e.g.node.exe, the MCP server itself); destroying only the wrapper leaves those descendants orphaned. On Windows this prevents the JVM from shutting down cleanly — the symptom reported in the issue.Fix
Walk
ProcessHandle.descendants()(JDK 9+, available since the repo's Java 17 baseline) and destroy each descendant before destroying the root. This avoids the OS-specifictaskkill /pid X /F /Tapproach suggested on the issue: same effect, one cross-platform code path, no shelling out.The descendant snapshot is best-effort — processes forked concurrently with the destroy call may be missed, but the wider tree generally dies with its parent.
Extracted as a package-private static helper
destroyProcessTree(Process)for testability.Tests
New
mcp-core/.../StdioClientTransportDestroyTreeTestswith three cases:destroyProcessTreeTerminatesRootAndDescendants— spawnssh -c 'sleep 60 & sleep 60 & wait'(two child sleeps), captures descendantProcessHandles, calls the helper, then awaits!isAlive()on every captured handle. POSIX-only via@DisabledOnOs(WINDOWS)since CI runs on Ubuntu.destroyProcessTreeTerminatesRootWithNoDescendants— singlesleep 60, ensures the root path still works when there is nothing beneath it.destroyProcessTreeIsSafeOnAlreadyTerminatedProcess— runs a command that exits immediately, then calls the helper to assert it doesn't throw on a deadProcess/ProcessHandle.Verification
./mvnw verifygreen locally on macOS across all 11 modules (BUILD SUCCESS, 0 failures, 0 errors). This includesStdioMcpSyncClientTestsandStdioMcpAsyncClientTests, which exercise the close path through a realnpxsubprocess.Caveat
I'm on macOS, so the original Windows symptom is verified only by reasoning about the JDK API, not exercised end-to-end.
ProcessHandle.descendants()is a JDK-level abstraction that works across platforms, but I'd appreciate a reviewer with Windows confirming the report from #496 is resolved. Happy to switch to explicittaskkillinstead if maintainers prefer the explicit route.