Skip to content

Commit 0f69a95

Browse files
committed
fix: destroy descendant processes on StdioClientTransport.closeGracefully
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 #496
1 parent c09ee67 commit 0f69a95

2 files changed

Lines changed: 98 additions & 5 deletions

File tree

mcp-core/src/main/java/io/modelcontextprotocol/client/transport/StdioClientTransport.java

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -327,9 +327,11 @@ protected void handleOutbound(Function<Flux<JSONRPCMessage>, Flux<JSONRPCMessage
327327
}
328328

329329
/**
330-
* Gracefully closes the transport by destroying the process and disposing of the
331-
* schedulers. This method sends a TERM signal to the process and waits for it to exit
332-
* before cleaning up resources.
330+
* Gracefully closes the transport by destroying the server process tree and disposing
331+
* of the schedulers. This method sends a TERM signal to every process in the tree
332+
* (descendants first, then the root) and waits for the root to exit before cleaning
333+
* up resources. Tree-aware termination prevents leaked child processes when a wrapper
334+
* command such as {@code npx} or {@code cmd.exe /c npx.cmd} is used.
333335
* @return A Mono that completes when the transport is closed
334336
*/
335337
@Override
@@ -346,9 +348,9 @@ public Mono<Void> closeGracefully() {
346348
// Give a short time for any pending messages to be processed
347349
return Mono.delay(Duration.ofMillis(100)).then();
348350
})).then(Mono.defer(() -> {
349-
logger.debug("Sending TERM to process");
351+
logger.debug("Sending TERM to process tree");
350352
if (this.process != null) {
351-
this.process.destroy();
353+
destroyProcessTree(this.process);
352354
return Mono.fromFuture(process.onExit());
353355
}
354356
else {
@@ -387,4 +389,21 @@ public <T> T unmarshalFrom(Object data, TypeRef<T> typeRef) {
387389
return this.jsonMapper.convertValue(data, typeRef);
388390
}
389391

392+
/**
393+
* Destroys {@code process} together with any descendant processes spawned beneath it.
394+
* Wrapper invocations such as {@code cmd.exe /c npx.cmd ...} on Windows or
395+
* {@code npx} on POSIX systems spawn additional child processes; destroying only the
396+
* wrapper leaves those descendants orphaned and prevents graceful JVM shutdown.
397+
* <p>
398+
* The descendant snapshot from {@link ProcessHandle#descendants()} is best-effort:
399+
* processes that fork concurrently with this call may not be signalled. The wider
400+
* tree typically dies anyway once its parent is gone.
401+
* <p>
402+
* Visible for tests.
403+
*/
404+
static void destroyProcessTree(Process process) {
405+
process.descendants().forEach(ProcessHandle::destroy);
406+
process.destroy();
407+
}
408+
390409
}
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
/*
2+
* Copyright 2026-2026 the original author or authors.
3+
*/
4+
package io.modelcontextprotocol.client.transport;
5+
6+
import java.time.Duration;
7+
import java.util.List;
8+
import java.util.stream.Collectors;
9+
10+
import org.junit.jupiter.api.Test;
11+
import org.junit.jupiter.api.condition.DisabledOnOs;
12+
import org.junit.jupiter.api.condition.OS;
13+
14+
import static org.assertj.core.api.Assertions.assertThat;
15+
import static org.assertj.core.api.Assertions.assertThatCode;
16+
import static org.awaitility.Awaitility.await;
17+
18+
/**
19+
* Tests for {@link StdioClientTransport#destroyProcessTree(Process)}.
20+
*
21+
* <p>
22+
* Regression coverage for the case where {@code closeGracefully()} previously left
23+
* orphaned descendant processes (e.g. {@code node} spawned beneath {@code npx} on
24+
* Windows). The tree-aware destroy must terminate the entire descendant chain, not just
25+
* the root.
26+
*/
27+
class StdioClientTransportDestroyTreeTests {
28+
29+
@Test
30+
@DisabledOnOs(OS.WINDOWS) // sh + & + wait is POSIX; CI is ubuntu-latest
31+
void destroyProcessTreeTerminatesRootAndDescendants() throws Exception {
32+
// `sh` forks two long-sleeping children. Killing only `sh` would leave both
33+
// children alive — exactly the bug reported in #496 (but reproduced portably).
34+
Process root = new ProcessBuilder("sh", "-c", "sleep 60 & sleep 60 & wait").start();
35+
36+
await().atMost(Duration.ofSeconds(3)).until(() -> root.descendants().count() >= 2);
37+
List<ProcessHandle> descendants = root.descendants().collect(Collectors.toList());
38+
assertThat(descendants).hasSizeGreaterThanOrEqualTo(2);
39+
40+
StdioClientTransport.destroyProcessTree(root);
41+
42+
await().atMost(Duration.ofSeconds(5)).untilAsserted(() -> {
43+
assertThat(root.isAlive()).as("root sh process").isFalse();
44+
for (ProcessHandle d : descendants) {
45+
assertThat(d.isAlive()).as("descendant pid=%s", d.pid()).isFalse();
46+
}
47+
});
48+
}
49+
50+
@Test
51+
@DisabledOnOs(OS.WINDOWS)
52+
void destroyProcessTreeTerminatesRootWithNoDescendants() throws Exception {
53+
Process root = new ProcessBuilder("sleep", "60").start();
54+
55+
assertThat(root.isAlive()).isTrue();
56+
57+
StdioClientTransport.destroyProcessTree(root);
58+
59+
await().atMost(Duration.ofSeconds(5)).until(() -> !root.isAlive());
60+
}
61+
62+
@Test
63+
void destroyProcessTreeIsSafeOnAlreadyTerminatedProcess() throws Exception {
64+
// Pick a command that exists on every supported OS and exits immediately.
65+
String[] cmd = System.getProperty("os.name").toLowerCase().contains("win")
66+
? new String[] { "cmd.exe", "/c", "exit", "0" } : new String[] { "sh", "-c", "exit 0" };
67+
Process root = new ProcessBuilder(cmd).start();
68+
root.waitFor();
69+
assertThat(root.isAlive()).isFalse();
70+
71+
assertThatCode(() -> StdioClientTransport.destroyProcessTree(root)).doesNotThrowAnyException();
72+
}
73+
74+
}

0 commit comments

Comments
 (0)