Skip to content

StdioClientTransport should support bounded child process termination #937

Description

@nquinquenel

StdioClientTransport starts and manages a child MCP server process. During graceful shutdown, the SDK transport currently calls process.destroy() and waits for process.onExit().

That works when the child process responds to SIGTERM, but it can hang indefinitely if the child process ignores termination or becomes stuck. Downstream, we had to implement a custom McpClientTransport to ensure proxied MCP server processes are always cleaned up when the parent shuts down.

Current behavior

The current shutdown flow is effectively:

if (this.process != null) {
  this.process.destroy();
  return Mono.fromFuture(process.onExit());
}

There is no configurable timeout and no fallback to destroyForcibly().

Expected behavior

StdioClientTransport.closeGracefully() should support bounded process termination:

  1. Stop accepting new messages.
  2. Complete the transport sinks.
  3. Send graceful termination with process.destroy().
  4. Wait up to a configurable timeout.
  5. If the process is still alive, call process.destroyForcibly().
  6. Dispose the transport schedulers.

This guarantees that child MCP server processes do not survive parent shutdown indefinitely.

Proposed fix

Add a configurable process termination timeout to StdioClientTransport, with a sensible default.

Conceptually:

process.destroy();

return Mono.fromFuture(process.onExit())
  .timeout(processTerminationTimeout, Mono.defer(() -> {
    if (process.isAlive()) {
      process.destroyForcibly();
    }
    return Mono.fromFuture(process.onExit());
  }));

This could be exposed through the existing ServerParameters builder or a dedicated StdioClientTransport constructor/builder option.

Suggested regression test

Add a test with a child process that ignores SIGTERM or does not exit promptly.

The test should verify that:

  1. closeGracefully() completes within the configured timeout plus a small buffer.
  2. The child process is no longer alive after closeGracefully() completes.
  3. The transport schedulers and sinks are cleaned up.

Activity

  1. added
    enhancementNew feature or request
    ready for workThe goal is clear and work towards it can be commenced
    P2Moderate issues affecting some users, edge cases, potentially valuable feature
    and removed on Aug 17, 2026
  2. ZYZ666-RGB commented on Aug 17, 2026

    @ZYZ666-RGB

    Hi maintainers, I'd like to work on this issue.

    I verified that the current StdioClientTransport.closeGracefully() path can wait indefinitely on process.onExit() after process.destroy(), with no forced-termination fallback.

    I plan to:

    • add a configurable process-termination timeout while preserving source compatibility;
    • attempt graceful termination first, then call destroyForcibly() only if the child is still alive;
    • ensure sinks and schedulers are disposed on both normal and forced-exit paths;
    • add deterministic regression tests for promptly exiting and non-cooperative child processes;
    • account for platform-specific process behavior.

    I will follow the repository's existing builder and configuration conventions and keep the public API change as small as possible. Please let me know if you prefer a particular API location or default timeout.

  3. ZYZ666-RGB commented on Aug 17, 2026

    @ZYZ666-RGB

    Thanks for the context. I need to step back from this issue for now so it remains available to other contributors.

  4. Jellyzzzz commented on Aug 23, 2026

    @Jellyzzzz

    Hi maintainers, I’d like to work on this issue. I’ll first reproduce the current StdioClientTransport.closeGracefully() behavior on main, then keep the change focused on bounded process termination and regression coverage for both cooperative and non-cooperative child processes. I’ll share a PR once I’ve validated the approach.

  5. Kamenalpha commented on Sep 17, 2026

    @Kamenalpha

    Hi maintainers 👋 — I'd like to pick this up (the previous offer in August doesn't seem to have resulted in a PR; happy to coordinate if that's changed).

    I have a focused implementation ready on top of current main:

    • add a configurable processTerminationTimeout to ServerParameters (default 10s), exposed via the builder;
    • in StdioClientTransport.closeGracefully(), after destroy(), wait bounded by the timeout and fall back to destroyForcibly() when the process is still alive;
    • keep existing public constructors/methods source-compatible (only an additive builder method/getter);
    • add two POSIX regression tests in StdioClientTransportTests: cooperative exit, and forced termination when SIGTERM is ignored.

    One scope question before I open the PR: would you prefer the knob on ServerParameters as proposed, or a constructor/builder option directly on StdioClientTransport? I'll open the PR once you confirm the preferred surface.

  6. caiocv18 commented on Oct 19, 2026

    @caiocv18

    Reproduced on current main (7b9b000, Linux, JDK 25): with a server started as sh -c "trap '' TERM; while :; do sleep 1; done", closeGracefully() does not complete within 15 s and the child process stays alive. JUnit reproduction (Windows excluded, since destroy() is TerminateProcess there):

    StepVerifier.create(transport.closeGracefully()).expectComplete().verify(Duration.ofSeconds(15));
    // fails, and the server ProcessHandle is still alive afterwards

    @Kamenalpha are you still planning to open the PR you mentioned on 17/09? If not, I can take it: destroy(), wait up to a configurable timeout, then destroyForcibly(), with this test turned into a regression test. It would also need to stay compatible with #966, which changes the same method to destroy descendants first.

    Maintainers: any preference on where the timeout lives, ServerParameters.Builder or a StdioClientTransport constructor/builder option?

  7. caiocv18 commented on Oct 28, 2026

    @caiocv18

    Opened #1257 for this, following the surface @Kamenalpha proposed above (processTerminationTimeout on ServerParameters.Builder, default 10 s, then destroyForcibly()). It includes the SIGTERM-ignoring reproduction from my earlier comment as a test. @Kamenalpha, if you had a different take on this, happy to compare or fold your changes in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    P2Moderate issues affecting some users, edge cases, potentially valuable featureenhancementNew feature or requestready for workThe goal is clear and work towards it can be commenced

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions