Skip to content

fix(client): wait for the whole server process tree to exit on stdio dispose - #1895

Open
iamAdarshh wants to merge 1 commit into
modelcontextprotocol:mainfrom
iamAdarshh:fix/1894-wait-for-process-tree
Open

iamAdarshh wants to merge 1 commit into
modelcontextprotocol:mainfrom
iamAdarshh:fix/1894-wait-for-process-tree

Conversation

@iamAdarshh

Copy link
Copy Markdown

Addresses problem 1 of #1894. Problem 2 (requests sent during dispose never completing) is left for a separate PR because it touches session disposal ordering.

Problem

ProcessHelper.KillTree kills the entire process tree, then waits only on the root process. On Windows the root is the cmd.exe /c wrapper, so DisposeAsync can return while the real server and its conhost.exe are still terminating. They still hold the working directory and open files, so deleting or reusing those right after dispose fails with "being used by another process". The netstandard2.0 taskkill /T /F path has the same gap.

Fix

  • Before the tree is killed, take a snapshot of the root's descendants, since the parent/child links are gone once it's torn down.
  • After the kill, wait on the root and then on each descendant. They all share the existing ShutdownTimeout budget, so the worst-case dispose time doesn't grow.
  • Descendants are found the same way the runtime's own Process.Kill(entireProcessTree: true) finds them:
    • Windows: a CreateToolhelp32Snapshot / Process32FirstW / Process32NextW snapshot. This works on netstandard2.0 / .NET Framework too.
    • Linux: parse the ppid from /proc/*/stat.
    • macOS: proc_listchildpids from libproc.
    • Other platforms: no descendants are found, so the behavior is the same as before (wait on the root only).
  • Guarding against reused PIDs:
    • A candidate is only accepted if it started no earlier than its parent, the same check the runtime uses.
    • On Windows, the process handle is opened when the snapshot is taken, so the later wait is tied to the original process.
  • Discovery is best effort. Any failure falls back to waiting on whatever was found.
  • The P/Invokes use only blittable types, so no marshalling stubs are generated and they stay AOT-friendly. AllowUnsafeBlocks is now enabled for every Core target, not just netstandard2.0, because the PROCESSENTRY32W struct needs a fixed buffer.

The diff for ProcessHelper.cs is easier to read with whitespace hidden: the existing kill code is only re-indented inside a new try/finally that disposes the snapshotted Process objects.

Using a job object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, as the issue suggests, would be more robust, and it could also replace the cmd.exe wrapper (#1751). It's a bigger change, though, so I've left it as a possible follow-up.

Testing

  • New test StdioClientTransportTests.DisposeAsync_WaitsForDescendantProcessesToExit. The server starts a long-running descendant (powershell under the cmd.exe wrapper on Windows, sleep under sh on Unix) that writes its PID to a file. The test disposes the session and asserts that the descendant has already exited.
    • On Unix the killed descendant is cleaned up by the OS almost instantly, so this test also passes without the fix there. The Windows CI leg is the one that exercises the bug.
  • Full ModelContextProtocol.Tests run on net10.0 (macOS): 2,395 total, 0 failed, 3 skipped.
  • Checked descendant discovery by hand with a nested tree (sh → sleep, sh → sleep, sleep):
    • macOS: all 4 descendants found, none alive after KillTree.
    • Linux (in the dotnet/sdk:10.0 container): same result.
  • I don't have a Windows machine, so the Toolhelp32 path has only been checked by building it and will be exercised by the Windows CI leg.

…dispose

KillTree killed the entire process tree but only waited on the root process.
On Windows the root is the cmd.exe /c wrapper, so DisposeAsync could return
while the actual server (and conhost.exe) were still terminating and holding
the working directory and open files.

Snapshot the root's descendants before killing the tree, then wait on each of
them within the same ShutdownTimeout. Descendants are discovered with a
Toolhelp32 snapshot on Windows, /proc on Linux and proc_listchildpids on macOS.
Start times guard against stale parent IDs from reused PIDs.

Fixes part 1 of modelcontextprotocol#1894.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant