From 18a9467fca1a750e83c7579ee0cd422c082fd77d Mon Sep 17 00:00:00 2001 From: JabberELF Date: Thu, 18 Jun 2026 21:21:00 +0800 Subject: [PATCH] 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. --- agent/lsp/client.py | 8 +++++ tests/tools/test_mcp_tool.py | 40 ++++++++++++++++++++++++ tools/mcp_tool.py | 60 +++++++++++++++++++++++++++++++++++- tui_gateway/server.py | 9 ++++++ 4 files changed, 116 insertions(+), 1 deletion(-) diff --git a/agent/lsp/client.py b/agent/lsp/client.py index c135e554c5d..2aab98c2b76 100644 --- a/agent/lsp/client.py +++ b/agent/lsp/client.py @@ -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( diff --git a/tests/tools/test_mcp_tool.py b/tests/tools/test_mcp_tool.py index 3fafaf67101..9b130268181 100644 --- a/tests/tools/test_mcp_tool.py +++ b/tests/tools/test_mcp_tool.py @@ -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 # --------------------------------------------------------------------------- diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 211d5ea65a7..5622d68be96 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -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. diff --git a/tui_gateway/server.py b/tui_gateway/server.py index c78d2895514..6bd1ed13ace 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -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()