diff --git a/docs/client.md b/docs/client.md index c2ec9342d..5b5c3e031 100644 --- a/docs/client.md +++ b/docs/client.md @@ -146,6 +146,15 @@ ServerParameters params = ServerParameters.builder("npx") McpTransport transport = new StdioClientTransport(params, McpJsonDefaults.getMapper()); ``` +On `closeGracefully()`, the transport asks the server process to terminate and waits up to `processTerminationTimeout` (10 seconds by default) for it to exit before destroying it forcibly: + +```java +ServerParameters params = ServerParameters.builder("npx") + .args("-y", "@modelcontextprotocol/server-everything", "dir") + .processTerminationTimeout(Duration.ofSeconds(5)) + .build(); +``` + ### Streamable HTTP === "Streamable HttpClient" diff --git a/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/ServerParameters.java b/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/ServerParameters.java index 094bc73a6..f75b45db1 100644 --- a/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/ServerParameters.java +++ b/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/ServerParameters.java @@ -4,6 +4,7 @@ package io.modelcontextprotocol.client.transport; +import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; @@ -30,13 +31,20 @@ public class ServerParameters { "SYSTEMDRIVE", "SYSTEMROOT", "TEMP", "USERNAME", "USERPROFILE") : Arrays.asList("HOME", "LOGNAME", "PATH", "SHELL", "TERM", "USER"); + private static final Duration DEFAULT_PROCESS_TERMINATION_TIMEOUT = Duration.ofSeconds(10); + + private static final Duration MAX_PROCESS_TERMINATION_TIMEOUT = Duration.ofNanos(Long.MAX_VALUE); + private String command; private List args = new ArrayList<>(); private Map env; - private ServerParameters(String command, List args, Map env) { + private Duration processTerminationTimeout; + + private ServerParameters(String command, List args, Map env, + Duration processTerminationTimeout) { Assert.notNull(command, "The command can not be null"); Assert.notNull(args, "The args can not be null"); @@ -46,6 +54,7 @@ private ServerParameters(String command, List args, Map if (env != null && !env.isEmpty()) { this.env.putAll(env); } + this.processTerminationTimeout = processTerminationTimeout; } public String getCommand() { @@ -60,6 +69,15 @@ public Map getEnv() { return this.env; } + /** + * How long {@link StdioClientTransport#closeGracefully()} waits for the server + * process to exit after the termination request before destroying it forcibly. + * @return the termination timeout, 10 seconds by default + */ + public Duration getProcessTerminationTimeout() { + return this.processTerminationTimeout; + } + public static Builder builder(String command) { return new Builder(command); } @@ -72,6 +90,8 @@ public static class Builder { private Map env = new HashMap<>(); + private Duration processTerminationTimeout = DEFAULT_PROCESS_TERMINATION_TIMEOUT; + public Builder(String command) { Assert.notNull(command, "The command can not be null"); this.command = command; @@ -109,8 +129,28 @@ public Builder addEnvVar(String key, String value) { return this; } + /** + * Sets how long {@link StdioClientTransport#closeGracefully()} waits for the + * server process to exit after the termination request before destroying it + * forcibly. Defaults to 10 seconds. + * @param processTerminationTimeout the timeout, must be positive and fit in + * {@code Long.MAX_VALUE} nanoseconds (about 292 years) + * @return this builder + */ + public Builder processTerminationTimeout(Duration processTerminationTimeout) { + Assert.notNull(processTerminationTimeout, "The processTerminationTimeout can not be null"); + Assert.isTrue(!processTerminationTimeout.isNegative() && !processTerminationTimeout.isZero(), + "The processTerminationTimeout must be positive"); + // The timeout is scheduled in nanoseconds; a longer Duration would overflow + // when closeGracefully() runs + Assert.isTrue(processTerminationTimeout.compareTo(MAX_PROCESS_TERMINATION_TIMEOUT) <= 0, + "The processTerminationTimeout must not exceed Long.MAX_VALUE nanoseconds"); + this.processTerminationTimeout = processTerminationTimeout; + return this; + } + public ServerParameters build() { - return new ServerParameters(command, args, env); + return new ServerParameters(command, args, env, processTerminationTimeout); } } 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 60a8d7c13..0474384a2 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 @@ -393,7 +393,8 @@ protected void handleOutbound(Function, Flux closeGracefully() { })).then(Mono.defer(() -> { logger.debug("Sending TERM to process"); if (this.process != null) { - this.process.destroy(); - return Mono.fromFuture(process.onExit()); + Process serverProcess = this.process; + serverProcess.destroy(); + Duration timeout = params.getProcessTerminationTimeout(); + // A process that ignores TERM would otherwise keep closeGracefully() + // from completing + return Mono.fromFuture(serverProcess::onExit).timeout(timeout, Mono.defer(() -> { + logger.warn("MCP server did not exit within {} after TERM, destroying it forcibly", timeout); + serverProcess.destroyForcibly(); + return Mono.fromFuture(serverProcess::onExit); + })); } else { logger.warn("Process not started"); diff --git a/mcp-test/src/test/java/io/modelcontextprotocol/client/transport/StdioClientTransportTerminationTests.java b/mcp-test/src/test/java/io/modelcontextprotocol/client/transport/StdioClientTransportTerminationTests.java new file mode 100644 index 000000000..ba2136ca6 --- /dev/null +++ b/mcp-test/src/test/java/io/modelcontextprotocol/client/transport/StdioClientTransportTerminationTests.java @@ -0,0 +1,97 @@ +/* + * Copyright 2026-2026 the original author or authors. + */ + +package io.modelcontextprotocol.client.transport; + +import java.time.Duration; +import java.util.concurrent.atomic.AtomicReference; + +import org.awaitility.Awaitility; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; +import org.junit.jupiter.api.condition.OS; +import reactor.test.StepVerifier; + +import static io.modelcontextprotocol.util.McpJsonMapperUtils.JSON_MAPPER; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; + +/** + * Tests for the bounded server process termination in + * {@link StdioClientTransport#closeGracefully()} (#937). + */ +class StdioClientTransportTerminationTests { + + @Test + @DisabledOnOs(OS.WINDOWS) // Process#destroy is TerminateProcess there, it cannot be + // ignored + void closeGracefullyDestroysForciblyWhenServerIgnoresSigterm() { + ServerParameters params = ServerParameters.builder("sh") + .args("-c", "trap '' TERM; echo ready >&2; while :; do sleep 1; done") + .processTerminationTimeout(Duration.ofSeconds(1)) + .build(); + + assertClosesWithin(params, Duration.ofSeconds(10)); + } + + @Test + @DisabledOnOs(OS.WINDOWS) + void closeGracefullyDoesNotWaitForTimeoutWhenServerExitsOnSigterm() { + ServerParameters params = ServerParameters.builder("sh") + .args("-c", "echo ready >&2; while :; do sleep 1; done") + .processTerminationTimeout(Duration.ofSeconds(30)) + .build(); + + assertClosesWithin(params, Duration.ofSeconds(10)); + } + + @Test + void processTerminationTimeoutDefaultsToTenSeconds() { + assertThat(ServerParameters.builder("sh").build().getProcessTerminationTimeout()) + .isEqualTo(Duration.ofSeconds(10)); + } + + @Test + void processTerminationTimeoutMustBePositive() { + ServerParameters.Builder builder = ServerParameters.builder("sh"); + assertThatIllegalArgumentException().isThrownBy(() -> builder.processTerminationTimeout(null)); + assertThatIllegalArgumentException().isThrownBy(() -> builder.processTerminationTimeout(Duration.ZERO)); + assertThatIllegalArgumentException() + .isThrownBy(() -> builder.processTerminationTimeout(Duration.ofSeconds(-1))); + } + + @Test + void processTerminationTimeoutMustFitInNanoseconds() { + ServerParameters.Builder builder = ServerParameters.builder("sh"); + assertThatIllegalArgumentException() + .isThrownBy(() -> builder.processTerminationTimeout(Duration.ofSeconds(Long.MAX_VALUE))); + assertThat(builder.processTerminationTimeout(Duration.ofNanos(Long.MAX_VALUE)) + .build() + .getProcessTerminationTimeout()).isEqualTo(Duration.ofNanos(Long.MAX_VALUE)); + } + + private static void assertClosesWithin(ServerParameters params, Duration limit) { + StdioClientTransport transport = new StdioClientTransport(params, JSON_MAPPER); + AtomicReference stderr = new AtomicReference<>(); + transport.setStdErrorHandler(stderr::set); + + StepVerifier.create(transport.connect(msg -> msg)).verifyComplete(); + Awaitility.await().atMost(Duration.ofSeconds(5)).until(() -> "ready".equals(stderr.get())); + ProcessHandle server = ProcessHandle.current() + .children() + .filter(p -> p.info().command().map(c -> c.endsWith("sh")).orElse(false)) + .findFirst() + .orElseThrow(); + + try { + StepVerifier.create(transport.closeGracefully()).expectComplete().verify(limit); + } + finally { + boolean alive = server.isAlive(); + server.destroyForcibly(); + assertThat(alive).as("server process still alive after closeGracefully()").isFalse(); + } + } + +}