From 144b86ef48b5a3859a238fc45e7596c8aacd4e0e Mon Sep 17 00:00:00 2001 From: Eva Date: Wed, 9 Sep 2026 16:43:59 +0700 Subject: [PATCH] fix(mcp): avoid replay after mid-call stdio exit (cherry picked from commit b29e68d0b020c805d1fccfc55f3a7c6ee6589ba9) --- .../test_mcp_stdio_fastfail_reconnect.py | 33 ++++++++------- tools/mcp_tool_handlers.py | 41 +++++++++++++++---- 2 files changed, 52 insertions(+), 22 deletions(-) diff --git a/tests/tools/test_mcp_stdio_fastfail_reconnect.py b/tests/tools/test_mcp_stdio_fastfail_reconnect.py index 3ac3dc0d20..dca94a58e6 100644 --- a/tests/tools/test_mcp_stdio_fastfail_reconnect.py +++ b/tests/tools/test_mcp_stdio_fastfail_reconnect.py @@ -9,15 +9,17 @@ subprocess (#95626 added the reconnect signal). Signalling alone still lost the call: a gateway restart kills every MCP stdio child, and the first call from a surviving agent session (or a cron run spanning the restart) failed in 0.00s while the subprocess was respawned -seconds later. Both fast-fail sites now respawn AND retry once: +seconds later. Both fast-fail sites request a respawn, but only a call that +has not reached the server is safe to retry: -- pre-call gate (children already dead when the call arrives); -- mid-call watcher race (children die while the RPC is in flight). +- pre-call gate (children already dead when the call arrives): retry once; +- mid-call watcher race (children die while the RPC is in flight): report an + uncertain outcome without replaying a possibly completed side effect. -Both must recover transparently, and both must stop after ONE retry so a -server that keeps dying parks via run()'s rapid-drop budget instead of -hot-cycling respawns forever. The error text must never claim a timeout — -that wording is what misdirected the original investigation. +The pre-call path must stop after ONE retry so a server that keeps dying parks +via run()'s rapid-drop budget instead of hot-cycling respawns forever. The +error text must never claim a timeout — that wording is what misdirected the +original investigation. """ import asyncio @@ -131,19 +133,22 @@ def test_precall_dead_children_respawn_and_retry(monkeypatch, tmp_path): _cleanup(mcp_tool, "srv-dead") -def test_midcall_child_exit_respawn_and_retry(monkeypatch, tmp_path): - """Subprocess dies while the RPC is in flight → respawn and retry once, - so the caller still gets its result.""" +def test_midcall_child_exit_reconnects_without_replay(monkeypatch, tmp_path): + """An in-flight side effect may have completed, so reconnect but do not replay it.""" monkeypatch.setenv("HERMES_HOME", str(tmp_path)) from tools import mcp_tool from tools.mcp_tool_handlers import _make_tool_handler alive = {"v": True} + effects = {"n": 0} async def _hanging_call(*a, **kw): + effects["n"] += 1 + alive["v"] = False await asyncio.sleep(30) async def _good_call(*a, **kw): + effects["n"] += 1 return _success_result() async def _watch_children(): @@ -167,12 +172,12 @@ def test_midcall_child_exit_respawn_and_retry(monkeypatch, tmp_path): _mcp_loop._ensure_mcp_loop() try: handler = _make_tool_handler("srv-midcall", "tool1", 10.0) - # The child dies once the RPC is in flight. - alive["v"] = False parsed = json.loads(handler({})) - assert "error" not in parsed, parsed - assert parsed["result"] == "ok", parsed + assert parsed["outcome_uncertain"] is True, parsed + assert "may have completed" in parsed["error"], parsed + assert "did not replay" in parsed["error"], parsed assert server._reconnect_event.set_calls == 1 + assert effects["n"] == 1, "the replacement session must not repeat an uncertain side effect" finally: _cleanup(mcp_tool, "srv-midcall") diff --git a/tools/mcp_tool_handlers.py b/tools/mcp_tool_handlers.py index 9d9c4a360a..4d101b15eb 100644 --- a/tools/mcp_tool_handlers.py +++ b/tools/mcp_tool_handlers.py @@ -33,6 +33,9 @@ _STDIO_NO_RESPAWN_MSG = ( _STDIO_DIED_AGAIN_MSG = ( "MCP server '{s}' respawned its stdio subprocess and it exited again immediately. The server is not starting " "cleanly — do NOT retry this tool; ask the user to check the server's command and its stderr log.") +_STDIO_OUTCOME_UNCERTAIN_MSG = ( + "MCP server '{s}' lost its stdio subprocess after the tool call began. The operation may have completed, so " + "Hermes did not replay it. Do NOT retry automatically; inspect the external state first.") def _trust_gate_check(server_name: str, tool_name: str) -> Optional[str]: @@ -187,11 +190,19 @@ def _handle_session_expired_and_retry(server_name: str, exc: BaseException, retr class _StdioChildExited(RuntimeError): """Stdio subprocess gone when (or while) a call ran. Deliberately NOT a TimeoutError.""" + def __init__(self, message: str, *, in_flight: bool): + super().__init__(message) + self.in_flight = in_flight + def _handle_stdio_child_exited_and_retry(server_name: str, exc: Exception, retry_call, op_description: str): - """Respawn a dead stdio child and retry once; None if not our error. Never spawns itself: it - sets ``_reconnect_event`` and waits, so spawn frequency stays governed by ``run()``'s - rapid-drop budget. Single-shot: a child that dies again reports and stops. + """Respawn a dead stdio child; retry once only when it was dead before dispatch. + + A mid-call exit is ambiguous: the server may have applied a side effect before its + response pipe disappeared. Reconnect for future calls but never replay that operation. + None means this is not our error. This function never spawns itself: it sets + ``_reconnect_event`` and waits, so spawn frequency stays governed by ``run()``'s rapid-drop + budget. A pre-dispatch retry whose child dies again reports and stops. Why retrying here cannot hot-cycle respawns: this function never spawns anything. It sets ``_reconnect_event`` (one signal, same as before) and waits for the server task to publish a fresh @@ -203,13 +214,20 @@ def _handle_stdio_child_exited_and_retry(server_name: str, exc: Exception, retry reconnected = False srv = _lookup_reconnectable_server(server_name) if srv is not None: - logger.info("MCP server '%s': %s found the stdio subprocess dead (%s); respawning and retrying once.", - server_name, op_description, exc) + action = "reconnecting without replay" if exc.in_flight else "respawning and retrying once" + logger.info("MCP server '%s': %s found the stdio subprocess dead (%s); %s.", + server_name, op_description, exc, action) if _mcp_loop_running(): reconnected = _loop._signal_reconnect_and_wait( server_name, srv, op_description=op_description, timeout=_core._STDIO_RESPAWN_WAIT_SEC) else: # No MCP loop to wait on (non-async adapters, tests): still request the respawn. _loop._signal_reconnect(srv) + if exc.in_flight: + return _strike( + server_name, + _STDIO_OUTCOME_UNCERTAIN_MSG.format(s=server_name), + outcome_uncertain=True, + ) if not reconnected: return _strike(server_name, _STDIO_NO_RESPAWN_MSG.format(s=server_name, t=_core._STDIO_RESPAWN_WAIT_SEC)) try: @@ -283,12 +301,16 @@ async def _call_tool_racing_stdio_death(server, server_name: str, tool_name: str """``session.call_tool`` that fails fast when the stdio child is/gets dead: pre-call (a dead child must not hold the slot for the full timeout) and mid-call (race against ``_watch_stdio_children``). Both raise :class:`_StdioChildExited` for the respawn path, which - owns the reconnect signal. callable()/``is True`` because MagicMock attributes are truthy.""" + owns the reconnect signal; only pre-call failure is safe to replay. callable()/``is True`` + because MagicMock attributes are truthy.""" # Fast-fail (#81995): a stdio subprocess that is already dead must not own this call slot — fail # immediately instead of waiting out the full tool timeout on a transport nobody will ever answer. _stdio_dead = getattr(server, "_stdio_children_dead", None) if callable(_stdio_dead) and _stdio_dead() is True: - raise _StdioChildExited(f"MCP stdio subprocess for '{server_name}' had already exited when the call was dispatched") + raise _StdioChildExited( + f"MCP stdio subprocess for '{server_name}' had already exited when the call was dispatched", + in_flight=False, + ) _call_coro = server.session.call_tool(tool_name, arguments=args) _watch_children = getattr(server, "_watch_stdio_children", None) if not (inspect.iscoroutinefunction(_watch_children) and asyncio.iscoroutine(_call_coro)): @@ -302,7 +324,10 @@ async def _call_tool_racing_stdio_death(server, server_name: str, tool_name: str done, _pending = await asyncio.wait({rpc_task, watch_task}, return_when=asyncio.FIRST_COMPLETED) if watch_task in done and not rpc_task.done(): rpc_task.cancel() - raise _StdioChildExited(f"MCP stdio subprocess for '{server_name}' exited mid-call") + raise _StdioChildExited( + f"MCP stdio subprocess for '{server_name}' exited mid-call", + in_flight=True, + ) return await rpc_task finally: watch_task.cancel()