mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix(mcp): reap stdio orphans before reconnect
This commit is contained in:
parent
086596ca2b
commit
f99e9f0d27
2 changed files with 85 additions and 6 deletions
|
|
@ -109,6 +109,7 @@ class TestStdioPidTracking:
|
|||
"""_kill_orphaned_mcp_children does nothing when no PIDs tracked."""
|
||||
from tools.mcp_tool import (
|
||||
_kill_orphaned_mcp_children,
|
||||
_orphan_stdio_pid_servers,
|
||||
_orphan_stdio_pids,
|
||||
_stdio_pids,
|
||||
_lock,
|
||||
|
|
@ -117,6 +118,7 @@ class TestStdioPidTracking:
|
|||
with _lock:
|
||||
_stdio_pids.clear()
|
||||
_orphan_stdio_pids.clear()
|
||||
_orphan_stdio_pid_servers.clear()
|
||||
|
||||
# Should not raise
|
||||
_kill_orphaned_mcp_children()
|
||||
|
|
@ -125,6 +127,7 @@ class TestStdioPidTracking:
|
|||
"""_kill_orphaned_mcp_children gracefully handles already-dead PIDs."""
|
||||
from tools.mcp_tool import (
|
||||
_kill_orphaned_mcp_children,
|
||||
_orphan_stdio_pid_servers,
|
||||
_orphan_stdio_pids,
|
||||
_lock,
|
||||
)
|
||||
|
|
@ -133,6 +136,7 @@ class TestStdioPidTracking:
|
|||
fake_pid = 999999999
|
||||
with _lock:
|
||||
_orphan_stdio_pids.add(fake_pid)
|
||||
_orphan_stdio_pid_servers[fake_pid] = "orphan"
|
||||
|
||||
# Should not raise (ProcessLookupError is caught)
|
||||
_kill_orphaned_mcp_children()
|
||||
|
|
@ -144,6 +148,7 @@ class TestStdioPidTracking:
|
|||
"""SIGTERM-first then SIGKILL after 2s for orphan cleanup."""
|
||||
from tools.mcp_tool import (
|
||||
_kill_orphaned_mcp_children,
|
||||
_orphan_stdio_pid_servers,
|
||||
_orphan_stdio_pids,
|
||||
_lock,
|
||||
)
|
||||
|
|
@ -151,7 +156,9 @@ class TestStdioPidTracking:
|
|||
fake_pid = 424242
|
||||
with _lock:
|
||||
_orphan_stdio_pids.clear()
|
||||
_orphan_stdio_pid_servers.clear()
|
||||
_orphan_stdio_pids.add(fake_pid)
|
||||
_orphan_stdio_pid_servers[fake_pid] = "orphan"
|
||||
|
||||
fake_sigkill = 9
|
||||
monkeypatch.setattr(signal, "SIGKILL", fake_sigkill, raising=False)
|
||||
|
|
@ -177,6 +184,7 @@ class TestStdioPidTracking:
|
|||
"""Without SIGKILL, SIGTERM is used for both phases."""
|
||||
from tools.mcp_tool import (
|
||||
_kill_orphaned_mcp_children,
|
||||
_orphan_stdio_pid_servers,
|
||||
_orphan_stdio_pids,
|
||||
_lock,
|
||||
)
|
||||
|
|
@ -184,7 +192,9 @@ class TestStdioPidTracking:
|
|||
fake_pid = 434343
|
||||
with _lock:
|
||||
_orphan_stdio_pids.clear()
|
||||
_orphan_stdio_pid_servers.clear()
|
||||
_orphan_stdio_pids.add(fake_pid)
|
||||
_orphan_stdio_pid_servers[fake_pid] = "orphan"
|
||||
|
||||
monkeypatch.delattr(signal, "SIGKILL", raising=False)
|
||||
|
||||
|
|
@ -261,6 +271,37 @@ class TestStdioPidTracking:
|
|||
with _lock:
|
||||
assert fake_pid not in _orphan_stdio_pids
|
||||
|
||||
def test_kill_orphaned_can_filter_by_server_name(self):
|
||||
"""Reconnect cleanup reaps only the orphan owned by that MCP server."""
|
||||
from tools.mcp_tool import (
|
||||
_kill_orphaned_mcp_children,
|
||||
_orphan_stdio_pid_servers,
|
||||
_orphan_stdio_pids,
|
||||
_lock,
|
||||
)
|
||||
|
||||
target_pid = 454545
|
||||
other_pid = 464646
|
||||
with _lock:
|
||||
_orphan_stdio_pids.clear()
|
||||
_orphan_stdio_pid_servers.clear()
|
||||
_orphan_stdio_pids.update({target_pid, other_pid})
|
||||
_orphan_stdio_pid_servers[target_pid] = "feishu"
|
||||
_orphan_stdio_pid_servers[other_pid] = "mimir"
|
||||
|
||||
with patch("tools.mcp_tool.os.kill") as mock_kill, \
|
||||
patch("gateway.status._pid_exists", return_value=False), \
|
||||
patch("tools.mcp_tool.time.sleep") as mock_sleep:
|
||||
_kill_orphaned_mcp_children(server_name="feishu")
|
||||
|
||||
mock_kill.assert_called_once_with(target_pid, signal.SIGTERM)
|
||||
mock_sleep.assert_called_once_with(2)
|
||||
with _lock:
|
||||
assert target_pid not in _orphan_stdio_pids
|
||||
assert target_pid not in _orphan_stdio_pid_servers
|
||||
assert other_pid in _orphan_stdio_pids
|
||||
assert _orphan_stdio_pid_servers[other_pid] == "mimir"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Fix 2b: stdio descendant reaping via process group (issue #23799)
|
||||
|
|
@ -276,10 +317,17 @@ class TestStdioPgroupReaping:
|
|||
"""_kill_orphaned_mcp_children reaps via killpg when a pgid is tracked."""
|
||||
|
||||
def _reset_state(self):
|
||||
from tools.mcp_tool import _stdio_pids, _orphan_stdio_pids, _stdio_pgids, _lock
|
||||
from tools.mcp_tool import (
|
||||
_orphan_stdio_pid_servers,
|
||||
_orphan_stdio_pids,
|
||||
_stdio_pgids,
|
||||
_stdio_pids,
|
||||
_lock,
|
||||
)
|
||||
with _lock:
|
||||
_stdio_pids.clear()
|
||||
_orphan_stdio_pids.clear()
|
||||
_orphan_stdio_pid_servers.clear()
|
||||
_stdio_pgids.clear()
|
||||
|
||||
def test_killpg_used_when_pgid_tracked(self, monkeypatch):
|
||||
|
|
@ -505,6 +553,7 @@ class TestStdioPgroupReaping:
|
|||
# Drive the reaper: register the parent pid + pgid as an orphan.
|
||||
from tools.mcp_tool import (
|
||||
_kill_orphaned_mcp_children,
|
||||
_orphan_stdio_pid_servers,
|
||||
_orphan_stdio_pids,
|
||||
_stdio_pgids,
|
||||
_stdio_pids,
|
||||
|
|
@ -513,8 +562,10 @@ class TestStdioPgroupReaping:
|
|||
with _lock:
|
||||
_stdio_pids.clear()
|
||||
_orphan_stdio_pids.clear()
|
||||
_orphan_stdio_pid_servers.clear()
|
||||
_stdio_pgids.clear()
|
||||
_orphan_stdio_pids.add(parent.pid)
|
||||
_orphan_stdio_pid_servers[parent.pid] = "orphan"
|
||||
_stdio_pgids[parent.pid] = parent_pgid
|
||||
try:
|
||||
_kill_orphaned_mcp_children()
|
||||
|
|
|
|||
|
|
@ -1958,6 +1958,12 @@ class MCPServerTask:
|
|||
# accumulation (see #57355).
|
||||
_kill_orphaned_mcp_children()
|
||||
|
||||
# A previous stdio transport for this same server may have survived
|
||||
# SDK context teardown and been marked orphaned in the prior
|
||||
# _run_stdio() finally block. Reap it before spawning the replacement
|
||||
# so reconnect churn cannot accumulate one child per attempt.
|
||||
_kill_orphaned_mcp_children(server_name=self.name)
|
||||
|
||||
# Snapshot child PIDs before spawning so we can track the new one.
|
||||
pids_before = _snapshot_child_pids()
|
||||
new_pids: set = set()
|
||||
|
|
@ -2048,6 +2054,7 @@ class MCPServerTask:
|
|||
pgroup_alive = False
|
||||
if pid_alive or pgroup_alive:
|
||||
_orphan_stdio_pids.add(pid)
|
||||
_orphan_stdio_pid_servers[pid] = self.name
|
||||
else:
|
||||
# Nothing left to reap — drop the pgid entry so
|
||||
# PID-reuse can't surface stale pgroup state later.
|
||||
|
|
@ -3241,6 +3248,7 @@ _stdio_pids: Dict[int, str] = {} # pid -> server_name
|
|||
# Separate from _stdio_pids so cleanup sweeps never race with active
|
||||
# sessions (e.g. concurrent cron jobs or live user chats).
|
||||
_orphan_stdio_pids: set = set()
|
||||
_orphan_stdio_pid_servers: Dict[int, str] = {}
|
||||
|
||||
# Process-group IDs of stdio MCP subprocesses, captured at spawn time.
|
||||
# The MCP SDK spawns stdio children with ``start_new_session=True`` so each
|
||||
|
|
@ -5168,7 +5176,10 @@ def shutdown_mcp_servers():
|
|||
_stop_mcp_loop()
|
||||
|
||||
|
||||
def _kill_orphaned_mcp_children(include_active: bool = False) -> None:
|
||||
def _kill_orphaned_mcp_children(
|
||||
include_active: bool = False,
|
||||
server_name: Optional[str] = None,
|
||||
) -> None:
|
||||
"""Best-effort graceful shutdown of stdio MCP subprocesses to reap orphans.
|
||||
|
||||
Orphans are PIDs that survived their session context exit (SDK teardown
|
||||
|
|
@ -5187,6 +5198,10 @@ def _kill_orphaned_mcp_children(include_active: bool = False) -> None:
|
|||
first) are reaped alongside the direct child. Falls back to ``os.kill``
|
||||
on Windows and when no pgid is recorded.
|
||||
|
||||
When ``server_name`` is set, only orphaned PIDs known to belong to that
|
||||
MCP server are reaped. This lets stdio reconnects clean up their previous
|
||||
transport without touching unrelated servers.
|
||||
|
||||
With ``include_active=True`` also kills every PID in ``_stdio_pids`` —
|
||||
used only at final shutdown, after the MCP event loop has stopped and no
|
||||
sessions can still be in flight.
|
||||
|
|
@ -5196,11 +5211,24 @@ def _kill_orphaned_mcp_children(include_active: bool = False) -> None:
|
|||
with _lock:
|
||||
pids: Dict[int, str] = {}
|
||||
for opid in _orphan_stdio_pids:
|
||||
pids[opid] = "orphan"
|
||||
_orphan_stdio_pids.clear()
|
||||
owner = _orphan_stdio_pid_servers.get(opid, "orphan")
|
||||
if server_name is not None and owner != server_name:
|
||||
continue
|
||||
pids[opid] = owner
|
||||
for opid in pids:
|
||||
_orphan_stdio_pids.discard(opid)
|
||||
_orphan_stdio_pid_servers.pop(opid, None)
|
||||
if include_active:
|
||||
pids.update(dict(_stdio_pids))
|
||||
_stdio_pids.clear()
|
||||
active = dict(_stdio_pids)
|
||||
if server_name is not None:
|
||||
active = {
|
||||
pid: owner
|
||||
for pid, owner in active.items()
|
||||
if owner == server_name
|
||||
}
|
||||
pids.update(active)
|
||||
for pid in active:
|
||||
_stdio_pids.pop(pid, None)
|
||||
# Snapshot pgids for the pids we're about to kill, then drop the
|
||||
# entries so a future spawn can't collide with stale state.
|
||||
pgids: Dict[int, int] = {pid: _stdio_pgids[pid] for pid in pids if pid in _stdio_pgids}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue