mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix(tui): prevent killpg suicide during MCP shutdown
Root cause: gateway spawns LSP servers (jdtls/pyright/yaml-ls) and slash_worker without start_new_session=True, so they inherit the gateway process group (= TUI parent PID). When mcp_tool _snapshot_child_pids() races with these spawns during stdio MCP server startup, non-MCP children leak into _stdio_pgids with the TUI parent PGID. shutdown_mcp_servers() then killpg(tui_parent_pid, SIGTERM), killing the TUI itself. Evidence: tui_gateway_crash.log shows recurring SIGTERM stacks: shutdown_mcp_servers -> _kill_orphaned_mcp_children -> _send_signal -> killpg(pgid, sig) -> SIGTERM received Fix (3 layers): 1. agent/lsp/client.py: add start_new_session=True to LSP server spawn so each LSP server gets its own process group/session. 2. tui_gateway/server.py: same fix for slash_worker spawn, the symmetric root-cause patch so no gateway direct child shares the TUI parent pgid. 3. tools/mcp_tool.py: add _filter_mcp_children() defense-in-depth that drops non-MCP children (slash_worker, jdtls/eclipse LSP) from the PID delta before they can poison _stdio_pgids.
This commit is contained in:
parent
04eed932eb
commit
18a9467fca
4 changed files with 116 additions and 1 deletions
|
|
@ -263,6 +263,13 @@ class LSPClient:
|
|||
cmd = self._win_wrap_cmd(cmd)
|
||||
|
||||
try:
|
||||
# start_new_session=True detaches the LSP server into its own
|
||||
# process group / session. Without this, the LSP server inherits
|
||||
# the gateway's pgid (= TUI parent PID). When mcp_tool's
|
||||
# _kill_orphaned_mcp_children races with LSP spawn and sweeps the
|
||||
# gateway's child set, it captures the LSP PID, records the
|
||||
# inherited pgid, and killpg() then kills the TUI parent itself.
|
||||
# See tui_gateway_crash.log "killpg → SIGTERM received" stacks.
|
||||
self._proc = await asyncio.create_subprocess_exec(
|
||||
cmd[0],
|
||||
*cmd[1:],
|
||||
|
|
@ -271,6 +278,7 @@ class LSPClient:
|
|||
stderr=asyncio.subprocess.PIPE,
|
||||
env=env,
|
||||
cwd=self._cwd,
|
||||
start_new_session=True,
|
||||
)
|
||||
except FileNotFoundError as e:
|
||||
raise LSPProtocolError(
|
||||
|
|
|
|||
|
|
@ -47,6 +47,46 @@ def _make_mock_server(name, session=None, tools=None):
|
|||
return server
|
||||
|
||||
|
||||
class TestFilterMCPChildren:
|
||||
def test_filters_gateway_children_by_argv_marker(self, monkeypatch):
|
||||
"""Non-MCP children start with an interpreter/binary, not the marker."""
|
||||
import sys
|
||||
|
||||
import tools.mcp_tool as mcp_tool
|
||||
|
||||
cmdlines = {
|
||||
101: [
|
||||
"/usr/bin/python3",
|
||||
"-m",
|
||||
"tui_gateway.slash_worker",
|
||||
"--session-key",
|
||||
"abc",
|
||||
],
|
||||
102: [
|
||||
"/usr/bin/java",
|
||||
"-jar",
|
||||
"/opt/jdtls/plugins/org.eclipse.equinox.launcher_1.7.0.jar",
|
||||
],
|
||||
103: ["/usr/bin/node", "server.js"],
|
||||
}
|
||||
|
||||
class FakeProcess:
|
||||
def __init__(self, pid):
|
||||
self.pid = pid
|
||||
|
||||
def cmdline(self):
|
||||
return cmdlines[self.pid]
|
||||
|
||||
fake_psutil = SimpleNamespace(
|
||||
Process=FakeProcess,
|
||||
NoSuchProcess=ProcessLookupError,
|
||||
AccessDenied=PermissionError,
|
||||
)
|
||||
monkeypatch.setitem(sys.modules, "psutil", fake_psutil)
|
||||
|
||||
assert mcp_tool._filter_mcp_children({101, 102, 103}) == {103}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Config loading
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -1867,7 +1867,15 @@ class MCPServerTask:
|
|||
write_stream,
|
||||
):
|
||||
# Capture the newly spawned subprocess PID for force-kill cleanup.
|
||||
new_pids = _snapshot_child_pids() - pids_before
|
||||
# Filter out non-MCP children that race into the snapshot window:
|
||||
# slash_worker and LSP servers (jdtls/pyright/yaml-ls) are spawned
|
||||
# directly by the gateway without start_new_session, so their pgid
|
||||
# equals the TUI parent PID. If they leak into _stdio_pgids, the
|
||||
# shutdown sweep's killpg() kills the TUI parent itself.
|
||||
# See agent/lsp/client.py for the complementary start_new_session fix.
|
||||
new_pids = _filter_mcp_children(
|
||||
_snapshot_child_pids() - pids_before
|
||||
)
|
||||
if new_pids:
|
||||
# Capture pgid while the child is alive — once it exits we
|
||||
# can no longer call ``os.getpgid`` on it, and the cleanup
|
||||
|
|
@ -3005,6 +3013,56 @@ def _snapshot_child_pids() -> set:
|
|||
return set()
|
||||
|
||||
|
||||
# Non-MCP gateway children that can race into the _snapshot_child_pids() delta
|
||||
# during stdio MCP server spawn. LSP servers and slash_worker now use
|
||||
# start_new_session=True too; this remains defense-in-depth for any future
|
||||
# non-MCP child spawn that briefly appears in the MCP snapshot delta. Match
|
||||
# argv markers instead of argv[0] because Python/Java children begin with the
|
||||
# interpreter or binary path.
|
||||
_NON_MCP_CHILD_CMDLINE_MARKERS: tuple[str, ...] = (
|
||||
"tui_gateway.slash_worker",
|
||||
"tui_gateway.entry",
|
||||
"-dorg.eclipse.equinox.launcher", # jdtls (legacy arg style)
|
||||
"eclipse.jdt.ls",
|
||||
"org.eclipse.equinox.launcher_",
|
||||
)
|
||||
|
||||
|
||||
def _filter_mcp_children(pids: set) -> set:
|
||||
"""Remove non-MCP children from a PID snapshot delta.
|
||||
|
||||
_snapshot_child_pids() returns *all* direct children of the gateway. When
|
||||
a stdio MCP server spawns concurrently with a slash_worker or LSP server
|
||||
spawn, the delta ``_snapshot_child_pids() - pids_before`` can include
|
||||
PIDs that are NOT the MCP server. Tracking those PIDs in _stdio_pgids is
|
||||
catastrophic if a future child lacks start_new_session: its pgid can be the
|
||||
TUI parent's PID, so the shutdown sweep's killpg() kills the TUI itself.
|
||||
"""
|
||||
if not pids:
|
||||
return pids
|
||||
try:
|
||||
import psutil
|
||||
except ImportError:
|
||||
# psutil unavailable — keep all PIDs (preserves prior behavior).
|
||||
return pids
|
||||
filtered: set = set()
|
||||
for pid in pids:
|
||||
try:
|
||||
argv = psutil.Process(pid).cmdline()
|
||||
except (psutil.NoSuchProcess, psutil.AccessDenied, OSError):
|
||||
# Process raced away or is a zombie — skip it; it cannot be the
|
||||
# MCP server we just spawned and is not safe to track.
|
||||
continue
|
||||
if any(
|
||||
marker in arg
|
||||
for arg in argv[1:]
|
||||
for marker in _NON_MCP_CHILD_CMDLINE_MARKERS
|
||||
):
|
||||
continue
|
||||
filtered.add(pid)
|
||||
return filtered
|
||||
|
||||
|
||||
def _mcp_loop_exception_handler(loop, context):
|
||||
"""Suppress benign 'Event loop is closed' noise during shutdown.
|
||||
|
||||
|
|
|
|||
|
|
@ -284,6 +284,14 @@ class _SlashWorker:
|
|||
self._closed = False
|
||||
from hermes_cli._subprocess_compat import windows_hide_flags
|
||||
|
||||
# start_new_session=True detaches the slash worker into its own
|
||||
# process group / session. Without this, the worker inherits the
|
||||
# gateway's pgid (= TUI parent PID). When mcp_tool's
|
||||
# _kill_orphaned_mcp_children races with slash_worker spawn and sweeps
|
||||
# the gateway's child set, it captures the worker PID, records the
|
||||
# inherited pgid, and killpg() then kills the TUI parent itself.
|
||||
# See agent/lsp/client.py for the symmetric LSP server fix and
|
||||
# tools/mcp_tool.py _filter_mcp_children for defense-in-depth.
|
||||
self.proc = subprocess.Popen(
|
||||
argv,
|
||||
stdin=subprocess.PIPE,
|
||||
|
|
@ -296,6 +304,7 @@ class _SlashWorker:
|
|||
# Tier-1 secrets (gateway/GitHub/infra) are still stripped (#29157).
|
||||
env=hermes_subprocess_env(inherit_credentials=True),
|
||||
creationflags=windows_hide_flags(),
|
||||
start_new_session=True,
|
||||
)
|
||||
threading.Thread(target=self._drain_stdout, daemon=True).start()
|
||||
threading.Thread(target=self._drain_stderr, daemon=True).start()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue