diff --git a/tests/tools/test_mcp_stability.py b/tests/tools/test_mcp_stability.py index c266dc98bc2..b9a1b92c9cd 100644 --- a/tests/tools/test_mcp_stability.py +++ b/tests/tools/test_mcp_stability.py @@ -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() diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index a57bdf98aff..0877807915c 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -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}