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