diff --git a/tests/tools/test_pgid_starttime_drift.py b/tests/tools/test_pgid_starttime_drift.py new file mode 100644 index 0000000000..0bbaae36f3 --- /dev/null +++ b/tests/tools/test_pgid_starttime_drift.py @@ -0,0 +1,174 @@ +"""PGID start-time guards tolerate same-host start-time drift (#117505; follow-up to #131910). + +The #43044 PID-reuse guards introduced by #131910 compared the spawn-time start-time +baseline with a later reading using exact ``==``/``!=``: +``tools.environments.local._leader_is_ours`` (terminal group teardown) and +``tools.mcp_tool_lifecycle._signal_mcp_process`` (stdio MCP orphan reaper). Same-host +readings drift ~1 s on macOS (``kern.boottime`` adjustment, #117505) and +``gateway.status`` ships ``start_time_fingerprints_match`` (tolerance 200 ticks ≈ 2 s) +for exactly that; a drifted reading therefore made the guard refuse to kill a live, +legitimately-owned process group — timed-out terminal commands survived their kill path +and live MCP stdio servers leaked past the reaper. Both comparisons now route through +the shared tolerant comparator, and an unreadable current value for a still-live leader +keeps the legacy best-effort kill instead of refusing. +""" + +import os +import signal +from types import SimpleNamespace + +import pytest + +pytestmark = pytest.mark.skipif(os.name == "nt", reason="POSIX-only process-group teardown") + +RECORDED = 178864182760 # spawn-time baseline (start ticks / psutil centiseconds — same scale) + + +# --- Local terminal teardown ------------------------------------------------ + +def _fake_proc(): + """A live wrapper whose spawn-time PGID/start-time baselines were captured.""" + return SimpleNamespace( + pid=12345, + _hermes_pgid=67890, + _hermes_pgid_start=RECORDED, + poll=lambda: 0, + wait=lambda timeout=None: 0, + kill=lambda: None, + ) + + +@pytest.fixture() +def killpg_calls(monkeypatch): + calls = [] + + def fake_getpgid(_pid): + return 67890 + + def fake_killpg(pgid, sig): + calls.append((pgid, sig)) + if sig == 0: # group-gone probe: _wait_for_group_exit returns immediately + raise ProcessLookupError + + monkeypatch.setattr(os, "getpgid", fake_getpgid) + monkeypatch.setattr(os, "killpg", fake_killpg) + return calls + + +def test_local_group_kill_survives_start_time_drift(monkeypatch, killpg_calls): + """~1 s drift between the recorded baseline and the later read must NOT make the + guard refuse to kill a live, legitimately-owned process group.""" + monkeypatch.setattr("gateway.status.get_process_start_time", lambda pid: RECORDED + 100) + from tools.environments.local import _kill_process_group_posix + + _kill_process_group_posix(_fake_proc()) + + assert (67890, signal.SIGTERM) in killpg_calls + + +def test_local_guard_still_refuses_recycled_pgid(monkeypatch, killpg_calls): + """A start time far outside the drift tolerance means the PID/PGID was recycled: + never signal the stale number (#43044).""" + monkeypatch.setattr("gateway.status.get_process_start_time", lambda pid: RECORDED + 500000) + from tools.environments.local import _kill_process_group_posix + + _kill_process_group_posix(_fake_proc()) + + assert killpg_calls == [] + + +def test_local_guard_unreadable_start_time_live_leader_is_best_effort(monkeypatch, killpg_calls): + """An unreadable current start time for a still-live leader keeps the legacy + best-effort kill; a gone leader still refuses the possibly-recycled number.""" + from tools.environments.local import _kill_process_group_posix, _leader_is_ours + + monkeypatch.setattr("gateway.status.get_process_start_time", lambda pid: None) + monkeypatch.setattr("gateway.status._pid_exists", lambda pid: True) + _kill_process_group_posix(_fake_proc()) + assert (67890, signal.SIGTERM) in killpg_calls + + monkeypatch.setattr("gateway.status._pid_exists", lambda pid: False) + assert _leader_is_ours(67890, RECORDED) is False + + +# --- stdio MCP orphan reaper ------------------------------------------------ + +def _reset_mcp_ledgers(): + from tools.mcp_tool_lifecycle import ( + _orphan_stdio_pid_servers, _orphan_stdio_pids, _stdio_pgids, _stdio_pids, _stdio_starttimes) + from tools.mcp_tool import _lock + with _lock: + _stdio_pids.clear() + _orphan_stdio_pids.clear() + _orphan_stdio_pid_servers.clear() + _stdio_pgids.clear() + _stdio_starttimes.clear() + + +def _stage_orphan(fake_pid: int): + from tools.mcp_tool_lifecycle import _orphan_stdio_pids, _stdio_pgids, _stdio_starttimes + from tools.mcp_tool import _lock + _reset_mcp_ledgers() + with _lock: + _orphan_stdio_pids.add(fake_pid) + _stdio_pgids[fake_pid] = fake_pid + _stdio_starttimes[fake_pid] = RECORDED + + +def test_mcp_orphan_reaper_survives_start_time_drift(monkeypatch): + """A live orphaned MCP stdio server whose start-time reading drifted ~1 s since + spawn (macOS boottime adjustment) must still be signalled, not skipped as + recycled — the group otherwise leaks past the reaper.""" + from tools.mcp_tool_lifecycle import _kill_orphaned_mcp_children + + fake_pid = 484848 + _stage_orphan(fake_pid) + killpg_calls = [] + monkeypatch.setattr("tools.mcp_tool_lifecycle._leader_start_time", + lambda pid: RECORDED + 100) + monkeypatch.setattr("tools.mcp_tool_lifecycle.os.killpg", + lambda pgid, sig: killpg_calls.append((pgid, sig))) + monkeypatch.setattr("gateway.status._pid_exists", lambda pid: False) # no SIGKILL pass + monkeypatch.setattr("tools.mcp_tool_lifecycle.time.sleep", lambda *_: None) + + _kill_orphaned_mcp_children() + + assert (fake_pid, signal.SIGTERM) in killpg_calls + + +def test_mcp_orphan_reaper_unreadable_live_leader_is_best_effort(monkeypatch): + """An unreadable leader start time on a still-live orphan keeps the legacy + best-effort signalling rather than skipping the kill.""" + from tools.mcp_tool_lifecycle import _kill_orphaned_mcp_children + + fake_pid = 494949 + _stage_orphan(fake_pid) + killpg_calls = [] + monkeypatch.setattr("tools.mcp_tool_lifecycle._leader_start_time", lambda pid: None) + monkeypatch.setattr("tools.mcp_tool_lifecycle.os.killpg", + lambda pgid, sig: killpg_calls.append((pgid, sig))) + monkeypatch.setattr("gateway.status._pid_exists", lambda pid: True) + monkeypatch.setattr("tools.mcp_tool_lifecycle.time.sleep", lambda *_: None) + + _kill_orphaned_mcp_children() + + assert (fake_pid, signal.SIGTERM) in killpg_calls + + +def test_mcp_orphan_reaper_still_refuses_recycled_pid(monkeypatch): + """A start time far outside the tolerance is a recycled PID: still skipped (#43044).""" + from tools.mcp_tool_lifecycle import _kill_orphaned_mcp_children + + fake_pid = 505050 + _stage_orphan(fake_pid) + killpg_calls = [] + monkeypatch.setattr("tools.mcp_tool_lifecycle._leader_start_time", + lambda pid: RECORDED + 500000) + monkeypatch.setattr("tools.mcp_tool_lifecycle.os.killpg", + lambda pgid, sig: killpg_calls.append((pgid, sig))) + monkeypatch.setattr("gateway.status._pid_exists", lambda pid: False) + monkeypatch.setattr("tools.mcp_tool_lifecycle.time.sleep", lambda *_: None) + + _kill_orphaned_mcp_children() + + assert killpg_calls == [] diff --git a/tools/environments/local.py b/tools/environments/local.py index 00e38941a3..e2f2bf3fa2 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -843,18 +843,29 @@ def _sweep_escaped_descendants(descendants: list, pgid: int) -> None: def _leader_is_ours(pgid, expected_start) -> bool: """The setsid group leader's PID == its PGID. Confirm it is still the process we spawned before signalling the whole group, so a recycled PID/PGID can never take - down an unrelated process group (#43044). When no start-time baseline was captured - (e.g. macOS, which has no /proc), fall back to the legacy best-effort behaviour - rather than refusing to kill.""" + down an unrelated process group (#43044). The baseline and the current reading + come from the same host at different times, so they go through the drift-tolerant + fingerprint comparator — same-host readings drift ~1 s on macOS (#117505), and + exact equality made the guard refuse to kill live, legitimately-owned groups. + When no baseline was captured, or the current reading is unreadable while the + leader is still alive, keep the legacy best-effort behaviour rather than + refusing to kill; an unreadable reading for a gone leader means the PID/PGID may + have been recycled, so refuse.""" if pgid is None: return False if expected_start is None: return True - from gateway.status import get_process_start_time + from gateway.status import _pid_exists, get_process_start_time, start_time_fingerprints_match try: - return get_process_start_time(pgid) == expected_start + current = get_process_start_time(pgid) except Exception: # noqa: BLE001 — the guard must never break signalling return True + if current is None: + return _pid_exists(pgid) + try: + return start_time_fingerprints_match(expected_start, current) + except (TypeError, ValueError): # junk fingerprints: best-effort, never break signalling + return True def _kill_process_group_posix(proc) -> None: @@ -1045,8 +1056,10 @@ class LocalEnvironment(BaseEnvironment): if not _IS_WINDOWS: with contextlib.suppress(ProcessLookupError): proc._hermes_pgid = os.getpgid(proc.pid) - # Record the group leader's start time so _kill_process can - # detect PID/PGID recycling before signalling the group (#43044). + # Record the group leader's start-time fingerprint so _kill_process can + # detect PID/PGID recycling before signalling the group (#43044). The + # psutil fallback in get_process_start_time captures a baseline on every + # platform, macOS included. from gateway.status import get_process_start_time proc._hermes_pgid_start = get_process_start_time(proc.pid) if stdin_data is not None: @@ -1069,8 +1082,8 @@ class LocalEnvironment(BaseEnvironment): pgid = getattr(proc, "_hermes_pgid", None) or os.getpgid(proc.pid) # PID-reuse guard (#43044): never SIGKILL a group whose leader's start time # no longer matches the spawn-time baseline — the PGID may have been recycled - # onto an unrelated process group. Without a baseline (macOS, no /proc) keep - # the legacy best-effort behaviour. + # onto an unrelated process group. Comparison is drift-tolerant (#117505); + # without a baseline keep the legacy best-effort behaviour. if pgid != os.getpgrp() and _leader_is_ours(pgid, getattr(proc, "_hermes_pgid_start", None)): os.killpg(pgid, signal.SIGKILL) # windows-footgun: ok — POSIX only (_IS_WINDOWS returned above) with contextlib.suppress(OSError): diff --git a/tools/mcp_tool_lifecycle.py b/tools/mcp_tool_lifecycle.py index 219033172a..4c0c801aa6 100644 --- a/tools/mcp_tool_lifecycle.py +++ b/tools/mcp_tool_lifecycle.py @@ -22,20 +22,23 @@ _orphan_stdio_pid_servers: Dict[int, str] = {} # grandchildren keep that PGID after the direct child exits, so killpg still reaches them. # Separate from _stdio_pids so the PGID survives the child's removal. Empty on Windows. _stdio_pgids: Dict[int, int] = {} -# Spawn-time start ticks (/proc//stat field 22) of each stdio child's pgroup leader, -# captured alongside the PGID. PIDs/PGIDs are recycled by the kernel once the original -# process exits and is reaped, so a long-lived tracker holding a bare PGID is unsafe: by -# the time a sweep runs, that number may name an unrelated process group (observed in the -# wild: a desktop browser whose session leader happened to reuse a dead MCP child's PID — -# #43044). We re-check the leader's start time before signalling so a recycled PGID is -# never killed. None entries are dropped: platforms without /proc (macOS) have no baseline -# and keep the legacy best-effort behaviour. +# Spawn-time start-time fingerprints of each stdio child's pgroup leader, captured +# alongside the PGID (the psutil fallback means every platform has a baseline, macOS +# included). PIDs/PGIDs are recycled by the kernel once the original process exits and +# is reaped, so a long-lived tracker holding a bare PGID is unsafe: by the time a sweep +# runs, that number may name an unrelated process group (observed in the wild: a +# desktop browser whose session leader happened to reuse a dead MCP child's PID — +# #43044). We re-check the leader's start time — drift-tolerantly, since same-host +# readings drift ~1 s on macOS (#117505) — before signalling so a recycled PGID is +# never killed. None entries are dropped: a capture that raced the child's exit keeps +# the legacy best-effort behaviour. _stdio_starttimes: Dict[int, int] = {} # pid -> leader start ticks def _leader_start_time(pid: int) -> Optional[int]: - """``/proc``-backed start time of the pgroup leader (PGID == leader PID on setsid spawn); - ``None`` on platforms without /proc (macOS/Windows) or for an already-reaped PID.""" + """Start-time fingerprint of the pgroup leader (PGID == leader PID on setsid spawn); + ``None`` only when the reading is genuinely unavailable (already-reaped PID, no + /proc AND no psutil) — the psutil fallback covers macOS/Windows.""" from gateway.status import get_process_start_time try: return get_process_start_time(pid) @@ -268,13 +271,31 @@ def _signal_mcp_process(pid: int, sig: int, server_name: str, pgid: Optional[int an MCP child exits and is reaped the kernel may recycle its PID/PGID onto an unrelated process group; signalling the stale number would kill a stranger (observed: a recycled PGID landing on a desktop browser's session leader). When ``expected_start`` was captured - at spawn and no longer matches, skip entirely. Without a baseline (no /proc — macOS — or - the capture raced the child's exit) fall through to the legacy best-effort path.""" - if expected_start is not None and _leader_start_time(pid) != expected_start: - logger.debug( - "Skip signalling MCP pid %d (%s): start-time mismatch — PID was recycled; " - "refusing to kill an unrelated process group.", pid, server_name) - return + at spawn and no longer matches — compared drift-tolerantly, because same-host readings + drift ~1 s on macOS (#117505) and exact equality skipped live, legitimately-owned servers + — skip entirely. Without a baseline (the capture raced the child's exit), or when the + current reading is unreadable while the PID is still alive, fall through to the legacy + best-effort path.""" + if expected_start is not None: + current = _leader_start_time(pid) + if current is None: + from gateway.status import _pid_exists + if not _pid_exists(pid): + logger.debug( + "Skip signalling MCP pid %d (%s): leader start time unreadable and the " + "PID is gone — it was likely recycled; refusing to kill an unrelated " + "process group.", pid, server_name) + return + else: + try: + from gateway.status import start_time_fingerprints_match + if not start_time_fingerprints_match(expected_start, current): + logger.debug( + "Skip signalling MCP pid %d (%s): start-time mismatch — PID was recycled; " + "refusing to kill an unrelated process group.", pid, server_name) + return + except (TypeError, ValueError): + pass # junk fingerprints: best-effort, never break signalling killpg = getattr(os, "killpg", None) if pgid is not None and killpg is not None: if my_pgid is not None and pgid == my_pgid: