From 126668c72d9e319fb80cfc0b06551736fcdb10e7 Mon Sep 17 00:00:00 2001
From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com>
Date: Thu, 3 Sep 2026 02:13:01 +0530
Subject: [PATCH] fix(mcp): wait() the released supervisor so it does not
 linger as a zombie

Phase-2c finding on #101598: the empty-set release closed the pipe and
dropped the Popen without reaping it, so an idle gateway that once
connected a stdio server held one zombie until the next Popen in the
process. wait(timeout=5) after close -- the supervisor exits on EOF
immediately with nothing registered (E2E: ps shows no entry at all).

Also: the replay test asserted against the last line only, which could
not catch the regression it names; assert against the full stream and a
post-unregister replay. Module docstring now states the stdlib-only /
no tools/ import constraint and why _reap's sweep is a deliberate copy.
---
 tests/tools/test_mcp_death_supervisor.py | 14 +++++++++++++-
 tools/mcp_death_supervisor.py            |  6 ++++++
 tools/mcp_tool.py                        |  8 ++++++++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/tests/tools/test_mcp_death_supervisor.py b/tests/tools/test_mcp_death_supervisor.py
index 75c1e418e0..f89ff64b46 100644
--- a/tests/tools/test_mcp_death_supervisor.py
+++ b/tests/tools/test_mcp_death_supervisor.py
@@ -366,6 +366,10 @@ class _FakeSupervisor:
     def poll(self):
         return 1 if self._exited else None
 
+    def wait(self, timeout=None):
+        self.waited = True
+        return 0
+
     def lines(self):
         if self.closed:
             return self._sent.splitlines()
@@ -443,6 +447,7 @@ def test_supervisor_is_released_once_nothing_is_left_to_reap(monkeypatch, all_gr
 
     mcp_tool._update_death_supervisor("unregister", [222])
     assert spawned[0].closed, "supervisor kept resident with nothing left to reap"
+    assert getattr(spawned[0], "waited", False), "released supervisor was never wait()ed -> zombie until the next Popen"
     assert spawned[0].lines()[-1] == "unregister 222", "release happened before the last unregister was sent"
     assert mcp_tool._death_supervisor is None
 
@@ -506,7 +511,14 @@ def test_replay_does_not_resurrect_an_unregistered_group(monkeypatch, all_groups
     mcp_tool._update_death_supervisor("unregister", [111])
 
     assert mcp_tool._supervised_pgids == {222}
-    assert "register 111" not in replacement.lines()[-1:]
+    # 111 was legitimately replayed to the replacement (it was live when the
+    # dead supervisor was swapped out), then unregistered. What must never
+    # happen is a replay AFTER the unregister bringing it back.
+    lines = replacement.lines()
+    assert lines.index("unregister 111") > lines.index("register 111")
+    assert "register 111" not in lines[lines.index("unregister 111") :]
+    mcp_tool._update_death_supervisor("register", [333])  # any later replay/append
+    assert "register 111" not in replacement.lines()[len(lines) :]
 
 
 def test_a_broken_pipe_never_propagates_into_a_live_mcp_session(monkeypatch, all_groups_alive):
diff --git a/tools/mcp_death_supervisor.py b/tools/mcp_death_supervisor.py
index dc751fad2c..87302ae685 100644
--- a/tools/mcp_death_supervisor.py
+++ b/tools/mcp_death_supervisor.py
@@ -8,6 +8,12 @@ crash), stdio MCP servers it spawned are reparented to init and keep running
 forever.  macOS has no ``PR_SET_PDEATHSIG``, so something has to outlive Hermes
 and reap them.
 
+This module is deliberately standard-library-only and must not import anything
+from ``tools/``: it runs after Hermes may already be dead, and pulling in
+``mcp_tool`` would drag the whole agent with it. The TERM -> grace -> KILL
+``killpg`` sweep in ``_reap`` therefore duplicates similar sweeps elsewhere in
+the tree on purpose.
+
 The predecessor (``mcp_stdio_watchdog.py``) solved this with one CPython
 *per MCP server*, wrapping each server command and polling ``getppid()`` every
 two seconds.  That costs ~10 MB of resident memory per server and detects death
diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py
index d483269213..ee33d3a1cb 100644
--- a/tools/mcp_tool.py
+++ b/tools/mcp_tool.py
@@ -1228,6 +1228,14 @@ def _update_death_supervisor(verb: str, pgids) -> None:
                 proc.stdin.close()
             except (BrokenPipeError, ValueError, OSError):
                 pass
+            # Reap it, or the exited supervisor stays a zombie until the next
+            # Popen in this process (CPython only collects abandoned children
+            # opportunistically). It exits on EOF with nothing to do, so this
+            # returns promptly; the timeout keeps a wedged one from stalling us.
+            try:
+                proc.wait(timeout=5)
+            except Exception:  # noqa: BLE001 - timeout or already gone; either way we drop it
+                pass
             _death_supervisor = None