From b5b2ab1bb0f3cb61e4cd3e411f69b1940daf697b Mon Sep 17 00:00:00 2001 From: John Paul Soliva Date: Mon, 24 Aug 2026 20:47:28 +0900 Subject: [PATCH] fix(mcp): forget supervised groups once nothing is left alive Review raised a real gap: we reap by pgid, so a registration is only as meaningful as the group's identity. A group we deliberately keep registered -- an orphan teardown failed to kill, such as the `node` mcp-remote leaves behind -- can later exit on its own, after which the kernel may hand that pgid to an unrelated process owned by the same user. An ungraceful Hermes death while the registration is stale would then signal a stranger. `_is_safe_target` cannot catch it, because the value is stale rather than invalid. Prune registrations whose group has no members left on every registration change, and tell the supervisor to forget them. Signal 0 is a pure existence question -- it cannot terminate anything -- so this is cheap and safe to run on the hot path. An ambiguous answer (EPERM: exists but not ours) keeps the registration, since dropping real coverage is the more expensive mistake. This narrows the window rather than closing it: a group can still die and its pgid be recycled between two probes. Closing it completely means proving group identity at reap time, e.g. stamping children with a boot-unique env marker and checking a member still carries it. That was judged not worth putting a `ps` parse into the one process whose job is to stay simple enough to always work, so the residual is now documented in the module docstring instead, along with the note that Hermes's existing killpg-based orphan cleanup already carries the same exposure (upstream #88350). Also records why supervisor recovery is deliberately two-step: a failed write drops the handle, and the next call rebuilds coverage from `_supervised_pgids`, which -- not the pipe -- is the record of what needs reaping. The protocol tests register synthetic pgids that were never real process groups, so they now state that precondition through an `all_groups_alive` fixture instead of depending on pid-space luck. 29 tests pass. Verified the change adds no failures: same selection, my changes stashed vs applied, 42 failed either way (pre-existing in a locally rebuilt venv) with passed going 617 -> 619 for the two new tests. Footgun linter clean. (cherry picked from commit 3a7d219620a8cb57a7fe2c2346ce3f055320e743) --- tests/tools/test_mcp_death_supervisor.py | 83 ++++++++++++++++++++++-- tools/mcp_death_supervisor.py | 24 +++++++ tools/mcp_tool.py | 45 ++++++++++++- 3 files changed, 145 insertions(+), 7 deletions(-) diff --git a/tests/tools/test_mcp_death_supervisor.py b/tests/tools/test_mcp_death_supervisor.py index ef08cdebec..6a8377a760 100644 --- a/tests/tools/test_mcp_death_supervisor.py +++ b/tests/tools/test_mcp_death_supervisor.py @@ -365,7 +365,19 @@ def _reset_client_state(): mcp_tool._supervised_pgids.clear() -def test_register_starts_the_supervisor_once_and_reuses_it(monkeypatch): +@pytest.fixture +def all_groups_alive(monkeypatch): + """Answer every liveness probe with "this group exists". + + The protocol tests below register synthetic pgids that were never real + process groups. Without this, the liveness prune correctly discards them + before the control stream can be asserted on -- so state the precondition + rather than letting these tests depend on pid-space luck. + """ + monkeypatch.setattr(mcp_tool.os, "killpg", lambda pgid, sig: None) + + +def test_register_starts_the_supervisor_once_and_reuses_it(monkeypatch, all_groups_alive): spawned = [] def _spawn(): @@ -382,7 +394,7 @@ def test_register_starts_the_supervisor_once_and_reuses_it(monkeypatch): assert spawned[0].lines() == ["register 111", "register 222"] -def test_unregister_is_forwarded(monkeypatch): +def test_unregister_is_forwarded(monkeypatch, all_groups_alive): fake = _FakeSupervisor() monkeypatch.setattr(mcp_tool, "_spawn_death_supervisor", lambda: fake) @@ -406,7 +418,7 @@ def test_unregister_alone_does_not_start_a_supervisor(monkeypatch): assert spawned == [] -def test_a_dead_supervisor_is_replaced_and_live_coverage_replayed(monkeypatch): +def test_a_dead_supervisor_is_replaced_and_live_coverage_replayed(monkeypatch, all_groups_alive): dead = _FakeSupervisor(exited=True) replacement = _FakeSupervisor() queue = [dead, replacement] @@ -420,7 +432,7 @@ def test_a_dead_supervisor_is_replaced_and_live_coverage_replayed(monkeypatch): assert set(replacement.lines()) == {"register 111", "register 222"} -def test_replay_does_not_resurrect_an_unregistered_group(monkeypatch): +def test_replay_does_not_resurrect_an_unregistered_group(monkeypatch, all_groups_alive): dead = _FakeSupervisor(exited=True) replacement = _FakeSupervisor() queue = [dead, replacement] @@ -434,7 +446,7 @@ def test_replay_does_not_resurrect_an_unregistered_group(monkeypatch): assert "register 111" not in replacement.lines()[-1:] -def test_a_broken_pipe_never_propagates_into_a_live_mcp_session(monkeypatch): +def test_a_broken_pipe_never_propagates_into_a_live_mcp_session(monkeypatch, all_groups_alive): class _BrokenPipe(_FakeSupervisor): def __init__(self): super().__init__() @@ -457,7 +469,7 @@ def test_a_broken_pipe_never_propagates_into_a_live_mcp_session(monkeypatch): assert mcp_tool._death_supervisor is None -def test_a_supervisor_that_cannot_start_is_not_fatal(monkeypatch): +def test_a_supervisor_that_cannot_start_is_not_fatal(monkeypatch, all_groups_alive): monkeypatch.setattr(mcp_tool, "_spawn_death_supervisor", lambda: None) mcp_tool._update_death_supervisor("register", [111]) # must not raise @@ -576,6 +588,65 @@ def test_a_server_that_survived_teardown_stays_registered(): child.wait(timeout=10) +@pytest.mark.live_system_guard_bypass +def test_a_group_with_nothing_left_alive_is_forgotten_and_unregistered(monkeypatch): + """A dead group must not stay registered: its pgid can be recycled. + + Uses a real process so the liveness probe is answered by the kernel rather + than a fixture -- the whole point is that we notice actual death. + """ + fake = _FakeSupervisor() + monkeypatch.setattr(mcp_tool, "_spawn_death_supervisor", lambda: fake) + + doomed = subprocess.Popen(_VICTIM, start_new_session=True) + doomed_pgid = os.getpgid(doomed.pid) + survivor = subprocess.Popen(_VICTIM, start_new_session=True) + survivor_pgid = os.getpgid(survivor.pid) + try: + mcp_tool._update_death_supervisor("register", [doomed_pgid, survivor_pgid]) + assert mcp_tool._supervised_pgids == {doomed_pgid, survivor_pgid} + + # Reap it fully so the group is genuinely empty, not a zombie. + doomed.kill() + doomed.wait(timeout=10) + + # Any later registration change is when we notice. + mcp_tool._update_death_supervisor("register", [survivor_pgid]) + + assert doomed_pgid not in mcp_tool._supervised_pgids, ( + "a group with no members left stayed registered, so a recycled " + "pgid could later be reaped as if it were an MCP server" + ) + assert survivor_pgid in mcp_tool._supervised_pgids, ( + "pruning dropped a group that is still alive" + ) + assert f"unregister {doomed_pgid}" in fake.lines(), ( + "the supervisor was never told to forget the dead group" + ) + finally: + _kill(survivor.pid) + survivor.wait(timeout=10) + _kill(doomed.pid) + + +def test_pruning_keeps_groups_it_cannot_prove_are_gone(monkeypatch): + # An ambiguous probe (EPERM: exists but not ours) must not drop coverage -- + # losing a real registration is worse than keeping a doubtful one. + monkeypatch.setattr(mcp_tool, "_supervised_pgids", {111, 222}, raising=False) + + def _probe(pgid, sig): + if pgid == 111: + raise PermissionError("exists, not ours") + raise ProcessLookupError("gone") + + monkeypatch.setattr(mcp_tool.os, "killpg", _probe) + + stale = mcp_tool._prune_dead_supervised_pgids() + + assert stale == {222} + assert mcp_tool._supervised_pgids == {111} + + def test_no_pgids_is_a_no_op(monkeypatch): spawned = [] monkeypatch.setattr( diff --git a/tools/mcp_death_supervisor.py b/tools/mcp_death_supervisor.py index 22ede0ca73..dc751fad2c 100644 --- a/tools/mcp_death_supervisor.py +++ b/tools/mcp_death_supervisor.py @@ -35,6 +35,30 @@ which unregisters as it tears each server down -- ends with nothing to kill. Unparseable lines are ignored rather than fatal: a corrupted byte on the control pipe must not cost us the reaping guarantee for every other server. + +Residual risk: process-group reuse +---------------------------------- +We reap by pgid, so a registration is only as meaningful as the group's +identity. A group we deliberately keep registered -- an orphan that teardown +failed to kill, such as the ``node`` ``mcp-remote`` leaves behind -- can +eventually exit on its own, after which the kernel is free to hand that pgid to +an unrelated process owned by the same user. If Hermes then dies ungracefully +while the registration is still stale, we would signal a stranger. +``_is_safe_target`` cannot catch this: the value is stale, not invalid. + +Two things narrow the window. Hermes prunes registrations whose group has no +members left (``_prune_dead_supervised_pgids``) on every registration change, +and the orphan sweep unregisters whatever it reaps. Neither closes it -- a +group can die and its pgid be recycled between two probes -- so the exposure is +real but bounded to that gap, and requires an ungraceful death inside it. + +Closing it completely means proving group identity at reap time, e.g. stamping +MCP children with a boot-unique env marker and checking that some member still +carries it before signalling. That was judged not worth putting a ``ps`` parse +into the one process whose job is to stay simple enough to always work; it is +the obvious next step if this class of bug ever actually bites. Note the same +exposure already exists in Hermes's own killpg-based orphan cleanup, which this +module did not introduce (see upstream issue #88350). """ from __future__ import annotations diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 130014ec57..58268ab945 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -1126,6 +1126,33 @@ def _spawn_death_supervisor(): return None +def _prune_dead_supervised_pgids() -> set: + """Forget supervised groups that have no members left; return what went. + + Caller must hold ``_death_supervisor_lock``. Probing with signal 0 is a + pure existence question -- it cannot terminate anything -- so this is safe + to run on every registration change. It narrows, but cannot close, the + window where a group dies and its pgid is recycled before we notice; see + the residual-risk note in ``tools/mcp_death_supervisor.py``. + """ + killpg = getattr(os, "killpg", None) + if killpg is None: # windows-footgun: ok - POSIX-only, guarded + return set() + stale = set() + for pgid in list(_supervised_pgids): + try: + killpg(pgid, 0) + except ProcessLookupError: + stale.add(pgid) + except (PermissionError, OSError): + # Exists but is not ours to signal, or the probe itself failed. + # Keep it: dropping coverage on an ambiguous answer is the more + # expensive mistake of the two. + pass + _supervised_pgids.difference_update(stale) + return stale + + def _update_death_supervisor(verb: str, pgids) -> None: """Register or unregister process groups with the shared supervisor. @@ -1145,6 +1172,16 @@ def _update_death_supervisor(verb: str, pgids) -> None: else: _supervised_pgids.difference_update(wanted) + # Drop groups with nothing left alive. A registration outlives the + # server only while some member survives -- e.g. an orphaned grandchild + # that teardown failed to kill, which we deliberately keep registered. + # Once that group is finally empty the pgid can be recycled by an + # unrelated process, and a stale registration would have us reap a + # stranger. The orphan sweep already unregisters what it reaps, but it + # is not guaranteed to run in a given process, so prune here too -- + # signal 0 cannot kill anything, it only asks whether the group exists. + stale = _prune_dead_supervised_pgids() + proc = _death_supervisor if proc is None or proc.poll() is not None: if verb == "unregister" and proc is None: @@ -1155,10 +1192,12 @@ def _update_death_supervisor(verb: str, pgids) -> None: if proc is None: return # A fresh supervisor knows nothing. Replay live coverage, which - # already reflects this call's mutation. + # already reflects this call's mutation and the prune above, so + # pruned groups simply never reach the replacement. payload = "".join(f"register {pgid}\n" for pgid in _supervised_pgids) else: payload = "".join(f"{verb} {pgid}\n" for pgid in wanted) + payload += "".join(f"unregister {pgid}\n" for pgid in stale) try: proc.stdin.write(payload) @@ -1166,6 +1205,10 @@ def _update_death_supervisor(verb: str, pgids) -> None: except (BrokenPipeError, ValueError, OSError): # It exited between poll() and write(). Drop it so the next call # respawns and replays, rather than writing into a dead pipe. + # Recovery is deliberately two-step: this call gives up, and the + # next one sees ``poll()`` non-None and rebuilds coverage from + # ``_supervised_pgids``. Nothing is lost in between because that + # set, not the pipe, is the record of what needs reaping. _death_supervisor = None