Slim comments and docstrings across the stdio surface

Adopt a uniform comment standard for the stdio transport and its tests:
PEP 257 one-line summaries, docstring bodies only for contracts or for
warning-of-consequences notes (why the obvious alternative is wrong),
inline comments at most two lines, no Args/Returns boilerplate on private
helpers, ASCII punctuation only. Facts guarding non-obvious OS behavior
(killpg/EPERM semantics, the pipe-gated process.wait(), the Windows
stdin-pipe interpreter freeze, CPython gh-106749) are kept in compressed
form; pure narration is deleted.

No code changes outside comments and docstrings.

Author: maxisbey

src/mcp/client/stdio.py

@@ -10,67 +10,54 @@
 
 logger = logging.getLogger(__name__)
 
-# How often to probe for surviving process-group members between SIGTERM and SIGKILL.
+# How often to probe for surviving group members between SIGTERM and SIGKILL.
 _GROUP_POLL_INTERVAL = 0.01
 
 
 async def terminate_posix_process_tree(process: Process, timeout_seconds: float = 2.0) -> None:
-    """Terminate a process and all its descendants on POSIX systems.
-
-    The process was spawned with `start_new_session=True`, so it leads its own
-    process group and `os.killpg` reaches every descendant in one atomic call,
-    even those whose parent (including the leader itself) has already exited.
-    Sends SIGTERM to the group, waits up to `timeout_seconds` for the group to
-    disappear, then SIGKILLs whatever remains.
-
-    Descendants that move themselves into a new session or process group
-    (daemonizers) escape a group kill by design. A group only disappears once
-    every member is dead *and reaped*: a client running as PID 1 without reaping
-    orphans keeps zombie descendants in the group and makes the wait below run
-    its full timeout (run such clients under an init shim, e.g. `docker run
-    --init`).
+    """SIGTERM the process group, wait up to timeout_seconds for it to
+    disappear, then SIGKILL whatever remains.
+
+    killpg reaches every descendant atomically, even ones whose parent already
+    exited; daemonizers that left the group escape by design. A group only
+    disappears once every member is dead and reaped, so a client running as
+    PID 1 should reap orphans (e.g. docker run --init) or the wait below runs
+    its full timeout.
     """
-    # start_new_session=True makes the leader's pid the pgid. Never ask via
-    # getpgid(): it fails once the leader is reaped, even with live members left.
+    # The leader's pid is the pgid (start_new_session). Never use getpgid():
+    # it fails once the leader is reaped, even with live members left.
     pgid = process.pid
 
     try:
         os.killpg(pgid, signal.SIGTERM)
     except ProcessLookupError:
-        return  # the entire group is already gone
+        return  # the whole group is already gone
     except PermissionError:
-        # EPERM never proves the group is gone (Linux: every member denied but
-        # alive; macOS: one foreign-euid or zombie member is enough, the rest
-        # were signalled), so keep waiting and escalating — both tolerate it.
+        # EPERM never proves the group is gone (macOS raises it for zombie or
+        # foreign-euid members), so keep waiting and escalating.
         logger.warning(
             "No permission to signal some of process group %d; waiting for it to exit anyway", pgid, exc_info=True
         )
 
     with anyio.move_on_after(timeout_seconds):
         while _group_alive(pgid):
-            # Reading `returncode` reaps the leader on trio (the property calls
-            # Popen.poll()); without it the leader's zombie would keep the group
-            # alive for the full timeout. On asyncio it is a cheap attribute read.
+            # Reading returncode reaps the leader on trio; a zombie leader would
+            # otherwise keep the group alive for the full timeout.
             _ = process.returncode
             await anyio.sleep(_GROUP_POLL_INTERVAL)
         return
 
+    # ESRCH: died since the last probe. EPERM: we killed what we were allowed to.
     with suppress(ProcessLookupError, PermissionError):
-        # ESRCH: the group died between the last probe and now. EPERM: whatever
-        # the platform let us signal has been KILLed; the rest is not ours to touch.
         os.killpg(pgid, signal.SIGKILL)
 
 
 def _group_alive(pgid: int) -> bool:
-    """Probe the group with signal 0. Only ESRCH proves it is gone: the probe
-    keeps succeeding while live members or unreaped zombies remain, and EPERM
-    is ambiguous on every platform."""
+    """Probe the group with signal 0; only ESRCH proves it is gone."""
     try:
         os.killpg(pgid, 0)
     except ProcessLookupError:
         return False
     except PermissionError:
-        # Unsignalable survivors or zombies; keep waiting — reaping turns an
-        # all-zombie group into ESRCH, and survivors may yet exit on their own.
-        pass
+        pass  # unsignalable survivors or unreaped zombies; EPERM is ambiguous
     return True

src/mcp/os/posix/utilities.py

@@ -28,59 +28,36 @@
     win32job = None
     pywintypes = None
 
-# How often FallbackProcess polls the underlying Popen for exit. Polling keeps the
-# wait cancellable: a thread blocked in Popen.wait() cannot be cancelled by anyio,
-# which would make every timeout around it ineffective.
+# How often FallbackProcess polls the underlying Popen for exit.
 _EXIT_POLL_INTERVAL = 0.01
 
-# The Job Object each spawned process was assigned to, so its process tree can be
-# terminated through it later. Values must stay the pywin32 `PyHANDLE` returned by
-# `CreateJobObject`, never a detached int: if neither pop site runs (an abandoned
-# shutdown), the dying weak entry drops the last reference and the `PyHANDLE`
-# destructor closes the OS handle, which is what makes `KILL_ON_JOB_CLOSE` reap
-# the orphaned tree.
+# Job Object handle per spawned process, for tree termination at shutdown.
+# Values stay pywin32 PyHANDLEs: if no pop site ever runs, the dying weak entry
+# drops the last reference and the PyHANDLE destructor closes the handle, which
+# is what makes KILL_ON_JOB_CLOSE reap an abandoned tree.
 _process_jobs: "weakref.WeakKeyDictionary[Process | FallbackProcess, object]" = weakref.WeakKeyDictionary()
 
 
 def get_windows_executable_command(command: str) -> str:
-    """Get the correct executable command normalized for Windows.
-
-    On Windows, commands might exist with specific extensions (.exe, .cmd, etc.)
-    that need to be located for proper execution.
-
-    Args:
-        command: Base command (e.g., 'uvx', 'npx')
-
-    Returns:
-        str: Windows-appropriate command path
-    """
+    """Resolve the command to a Windows executable path, trying the bare name
+    first and then the common script extensions (.cmd, .bat, .exe, .ps1)."""
     try:
-        # First check if command exists in PATH as-is
         if command_path := shutil.which(command):
             return command_path
 
-        # Check for Windows-specific extensions
         for ext in [".cmd", ".bat", ".exe", ".ps1"]:
             ext_version = f"{command}{ext}"
             if ext_path := shutil.which(ext_version):
                 return ext_path
 
-        # For regular commands or if we couldn't find special versions
         return command
     except OSError:
-        # Handle file system errors during path resolution
-        # (permissions, broken symlinks, etc.)
-        return command
+        return command  # path probing failed (permissions, broken symlinks)
 
 
 class FallbackProcess:
-    """A fallback process wrapper for Windows to handle async I/O
-    when using subprocess.Popen, which provides sync-only FileIO objects.
-
-    This wraps stdin and stdout into async-compatible
-    streams (FileReadStream, FileWriteStream),
-    so that MCP clients expecting async streams can work properly.
-    """
+    """Async wrapper around subprocess.Popen for Windows event loops without
+    async subprocess support (SelectorEventLoop)."""
 
     def __init__(self, popen_obj: subprocess.Popen[bytes]) -> None:
         self.popen: subprocess.Popen[bytes] = popen_obj
@@ -91,11 +68,8 @@ def __init__(self, popen_obj: subprocess.Popen[bytes]) -> None:
         self.stdout = FileReadStream(cast(BinaryIO, stdout)) if stdout else None
 
     async def wait(self) -> int:
-        """Wait for process exit by polling.
-
-        `Popen.wait()` in a worker thread cannot be cancelled by anyio, which would
-        defeat every timeout placed around this call; polling keeps it cancellable.
-        """
+        """Wait for exit by polling; a thread blocked in Popen.wait() cannot be
+        cancelled by anyio, which would defeat every timeout around this call."""
         while (returncode := self.popen.poll()) is None:
             await anyio.sleep(_EXIT_POLL_INTERVAL)
         return returncode
@@ -105,7 +79,7 @@ def terminate(self) -> None:
         self.popen.terminate()
 
     def kill(self) -> None:
-        """Kill the subprocess (on Windows this is the same hard kill as terminate)."""
+        """Kill the subprocess (same hard kill as terminate on Windows)."""
         self.popen.kill()
 
     @property
@@ -115,11 +89,8 @@ def pid(self) -> int:
 
     @property
     def returncode(self) -> int | None:
-        """Return the exit code, or `None` if the process has not yet terminated.
-
-        Polls the underlying `Popen` so the value updates as soon as the process
-        dies, without anyone having to call `wait()`.
-        """
+        """Exit code, or None while running; polls Popen so death is observable
+        without anyone calling wait()."""
         return self.popen.poll()
 
 
@@ -135,27 +106,9 @@ async def create_windows_process(
     errlog: TextIO | None = sys.stderr,
     cwd: Path | str | None = None,
 ) -> Process | FallbackProcess:
-    """Creates a subprocess in a Windows-compatible way with Job Object support.
-
-    Attempts to use anyio's open_process for async subprocess creation.
-    In some cases this will throw NotImplementedError on Windows, e.g.,
-    when using the SelectorEventLoop, which does not support async subprocesses.
-    In that case, we fall back to using subprocess.Popen.
-
-    The process is added to a Job Object so that child processes are terminated
-    with it; children spawned before the assignment completes are not captured
-    (see the inline note below).
-
-    Args:
-        command (str): The executable to run
-        args (list[str]): List of command line arguments
-        env (dict[str, str] | None): Environment variables
-        errlog (TextIO | None): Where to send stderr output (defaults to sys.stderr)
-        cwd (Path | str | None): Working directory for the subprocess
-
-    Returns:
-        Process | FallbackProcess: Async-compatible subprocess with stdin and stdout streams
-    """
+    """Spawn the server inside a Job Object so its children can be terminated
+    with it; falls back to subprocess.Popen on event loops without async
+    subprocess support."""
     try:
         process = await anyio.open_process(
             [command, *args],
@@ -169,11 +122,9 @@ async def create_windows_process(
         # Windows event loops without async subprocess support (SelectorEventLoop)
         process = await _create_windows_fallback_process(command, args, env, errlog, cwd)
 
-    # Created only after a successful spawn (a failed spawn raises before any job
-    # exists, so no handle can leak). Children the server spawns before
-    # AssignProcessToJobObject completes land outside the job — membership is
-    # inherited at CreateProcess, never acquired retroactively. If that window
-    # ever bites, the fix is a CREATE_SUSPENDED spawn -> assign -> resume.
+    # Children spawned before the assignment completes land outside the job
+    # (membership is inherited at CreateProcess, never acquired retroactively);
+    # if that ever bites, the fix is a CREATE_SUSPENDED spawn -> assign -> resume.
     job = _create_job_object()
     _maybe_assign_process_to_job(process, job)
     return process
@@ -186,10 +137,7 @@ async def _create_windows_fallback_process(
     errlog: TextIO | None = sys.stderr,
     cwd: Path | str | None = None,
 ) -> FallbackProcess:
-    """Create a subprocess using subprocess.Popen as a fallback when anyio fails.
-
-    This function wraps the sync subprocess.Popen in an async-compatible interface.
-    """
+    """Spawn via subprocess.Popen and wrap it in FallbackProcess."""
     popen_obj = subprocess.Popen(
         [command, *args],
         stdin=subprocess.PIPE,
@@ -218,20 +166,15 @@ def _create_job_object() -> object | None:
         return job
     except pywintypes.error:
         logger.warning("Failed to create Job Object for process tree management", exc_info=True)
-        # If creation succeeded but configuration failed, close the handle rather
-        # than leaving it to be reclaimed whenever the GC gets to it.
+        # If creation succeeded but configuration failed, close the handle now.
         if job is not None:
             _close_job_handle(job)
         return None
 
 
 def _maybe_assign_process_to_job(process: Process | FallbackProcess, job: object | None) -> None:
-    """Try to assign a process to a job object.
-
-    On success the job is recorded for the process so that
-    `terminate_windows_process_tree` can terminate the whole tree through it.
-    If assignment fails for any reason, the job handle is closed.
-    """
+    """Assign the process to the job and record it for tree termination; on
+    any failure the job handle is closed instead."""
     if job is None:
         return
 
@@ -249,25 +192,18 @@ def _maybe_assign_process_to_job(process: Process | FallbackProcess, job: object
             win32job.AssignProcessToJobObject(job, process_handle)
         finally:
             win32api.CloseHandle(process_handle)
-        # Recorded only after the CloseHandle above succeeded: had it failed, the
-        # except below would close the job and KILL_ON_JOB_CLOSE would take the
-        # server with it.
+        # Record only after the CloseHandle above succeeded: had it failed, the
+        # except below would close the job and KILL_ON_JOB_CLOSE takes the server.
         _process_jobs[process] = job
     except pywintypes.error:
         logger.warning("Failed to assign process %d to Job Object", process.pid, exc_info=True)
         _close_job_handle(job)
 
 
 def close_process_job(process: Process | FallbackProcess) -> None:
-    """Close the process's Job Object handle, if it still has one.
-
-    The job is created with `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`, so closing the
-    handle also kills any job members still alive — deterministically, instead of
-    whenever the GC would have collected the handle. This is a deliberate
-    divergence from POSIX, where a well-behaved server's surviving children are
-    left alive. No-op on POSIX and when no job was assigned (or tree termination
-    already closed it).
-    """
+    """Close the process's Job Object handle, killing any members still alive
+    (KILL_ON_JOB_CLOSE) deterministically rather than at GC time; a deliberate
+    divergence from POSIX, where a graceful server's children are left alive."""
     if sys.platform != "win32":
         return
 
@@ -277,14 +213,9 @@ def close_process_job(process: Process | FallbackProcess) -> None:
 
 
 async def terminate_windows_process_tree(process: Process | FallbackProcess) -> None:
-    """Terminate a process and all its children on Windows.
-
-    If the process was assigned to a Job Object at spawn, the job is terminated,
-    which kills every process in it immediately. Otherwise only the process itself
-    is terminated. Both are immediate hard kills: Windows offers no portable
-    equivalent of SIGTERM for a whole tree, so the stdin-close grace period in the
-    client shutdown is the server's opportunity to exit cleanly.
-    """
+    """Terminate the job (an immediate hard kill of every member), or just the
+    process if it has no job. Windows has no tree-wide SIGTERM; the stdin-close
+    grace period is the server's chance to exit cleanly."""
     if sys.platform != "win32":
         return
 
@@ -296,8 +227,7 @@ async def terminate_windows_process_tree(process: Process | FallbackProcess) ->
         finally:
             _close_job_handle(job)
 
-    # Terminate the process directly too: it may have no job (creation or
-    # assignment failed), in which case the job path above did nothing.
+    # The process may have no job (creation or assignment failed); kill it directly too.
     try:
         process.terminate()
     except OSError: