hermes-admin/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 4 Packages Projects Releases Wiki Activity

fix(env/mcp): tolerate start-time drift in the PGID kill guards

Browse Source 
Follow-up to #131910 (issue #43044): both new guards compared the
spawn-time start-time baseline with a later reading using exact
==/!= — tools/environments/local._leader_is_ours (terminal group
teardown, _kill_process_group_posix / _force_kill_process) 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 comparison; the "macOS has no baseline" premise was
also wrong (the psutil create_time fallback captures one). A
drifted reading made the guard refuse to kill live, legitimately
owned groups: timed-out terminal commands survived their teardown
and live stdio MCP servers leaked past the orphan reaper.

Route both comparisons through the shared tolerant comparator,
and keep the legacy best-effort kill when the current reading is
unreadable but the leader is still alive (a gone leader still
refuses the possibly-recycled number). Correct the stale
"macOS has no /proc baseline" comments.

Regression tests patch get_process_start_time to return
expected+100 (within tolerance) and assert the group is still
signalled; far-outside-tolerance readings are still refused.
This commit is contained in:
Brooklyn Nicholson
2026-10-02 21:19:27 -05:00
committed by brooklyn!
parent bb9a9fab6a
commit a5e7df27c7
3 changed files with 234 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

174
tests/tools/test_pgid_starttime_drift.py Normal file
View File

@@ -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 == []

31
tools/environments/local.py
View File

@@ -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):

55
tools/mcp_tool_lifecycle.py
View File

@@ -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/<pid>/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: