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.
This commit is contained in:
@@ -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):
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user