diff --git a/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/StdioClientTransport.java b/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/StdioClientTransport.java index 1b4eaca97..802a1d2fe 100644 --- a/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/StdioClientTransport.java +++ b/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/StdioClientTransport.java @@ -327,9 +327,11 @@ protected void handleOutbound(Function, Flux closeGracefully() { // Give a short time for any pending messages to be processed return Mono.delay(Duration.ofMillis(100)).then(); })).then(Mono.defer(() -> { - logger.debug("Sending TERM to process"); + logger.debug("Sending TERM to process tree"); if (this.process != null) { - this.process.destroy(); + destroyProcessTree(this.process); return Mono.fromFuture(process.onExit()); } else { @@ -387,4 +389,21 @@ public T unmarshalFrom(Object data, TypeRef typeRef) { return this.jsonMapper.convertValue(data, typeRef); } + /** + * Destroys {@code process} together with any descendant processes spawned beneath it. + * Wrapper invocations such as {@code cmd.exe /c npx.cmd ...} on Windows or + * {@code npx} on POSIX systems spawn additional child processes; destroying only the + * wrapper leaves those descendants orphaned and prevents graceful JVM shutdown. + *

+ * The descendant snapshot from {@link ProcessHandle#descendants()} is best-effort: + * processes that fork concurrently with this call may not be signalled. The wider + * tree typically dies anyway once its parent is gone. + *

+ * Visible for tests. + */ + static void destroyProcessTree(Process process) { + process.descendants().forEach(ProcessHandle::destroy); + process.destroy(); + } + } diff --git a/mcp-core/src/test/java/io/modelcontextprotocol/client/transport/StdioClientTransportDestroyTreeTests.java b/mcp-core/src/test/java/io/modelcontextprotocol/client/transport/StdioClientTransportDestroyTreeTests.java new file mode 100644 index 000000000..a84f21d93 --- /dev/null +++ b/mcp-core/src/test/java/io/modelcontextprotocol/client/transport/StdioClientTransportDestroyTreeTests.java @@ -0,0 +1,74 @@ +/* + * Copyright 2026-2026 the original author or authors. + */ +package io.modelcontextprotocol.client.transport; + +import java.time.Duration; +import java.util.List; +import java.util.stream.Collectors; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; +import org.junit.jupiter.api.condition.OS; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.awaitility.Awaitility.await; + +/** + * Tests for {@link StdioClientTransport#destroyProcessTree(Process)}. + * + *

+ * Regression coverage for the case where {@code closeGracefully()} previously left + * orphaned descendant processes (e.g. {@code node} spawned beneath {@code npx} on + * Windows). The tree-aware destroy must terminate the entire descendant chain, not just + * the root. + */ +class StdioClientTransportDestroyTreeTests { + + @Test + @DisabledOnOs(OS.WINDOWS) // sh + & + wait is POSIX; CI is ubuntu-latest + void destroyProcessTreeTerminatesRootAndDescendants() throws Exception { + // `sh` forks two long-sleeping children. Killing only `sh` would leave both + // children alive — exactly the bug reported in #496 (but reproduced portably). + Process root = new ProcessBuilder("sh", "-c", "sleep 60 & sleep 60 & wait").start(); + + await().atMost(Duration.ofSeconds(3)).until(() -> root.descendants().count() >= 2); + List descendants = root.descendants().collect(Collectors.toList()); + assertThat(descendants).hasSizeGreaterThanOrEqualTo(2); + + StdioClientTransport.destroyProcessTree(root); + + await().atMost(Duration.ofSeconds(5)).untilAsserted(() -> { + assertThat(root.isAlive()).as("root sh process").isFalse(); + for (ProcessHandle d : descendants) { + assertThat(d.isAlive()).as("descendant pid=%s", d.pid()).isFalse(); + } + }); + } + + @Test + @DisabledOnOs(OS.WINDOWS) + void destroyProcessTreeTerminatesRootWithNoDescendants() throws Exception { + Process root = new ProcessBuilder("sleep", "60").start(); + + assertThat(root.isAlive()).isTrue(); + + StdioClientTransport.destroyProcessTree(root); + + await().atMost(Duration.ofSeconds(5)).until(() -> !root.isAlive()); + } + + @Test + void destroyProcessTreeIsSafeOnAlreadyTerminatedProcess() throws Exception { + // Pick a command that exists on every supported OS and exits immediately. + String[] cmd = System.getProperty("os.name").toLowerCase().contains("win") + ? new String[] { "cmd.exe", "/c", "exit", "0" } : new String[] { "sh", "-c", "exit 0" }; + Process root = new ProcessBuilder(cmd).start(); + root.waitFor(); + assertThat(root.isAlive()).isFalse(); + + assertThatCode(() -> StdioClientTransport.destroyProcessTree(root)).doesNotThrowAnyException(); + } + +}