From cff103a8b7d041f07e5cddb17ec6c9281045fa31 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 9 Sep 2026 04:50:17 -0700 Subject: [PATCH] fix(mcp): classify an SDK-first transport close after dispatch as an ambiguous stdio death The child watcher polls every 250 ms, so in practice the MCP SDK sees the closed pipe first and `call_tool` raises ClosedResourceError / "Connection closed" before the watcher fires. That exception is not a _StdioChildExited, so it fell past the stdio recoverer into _handle_session_expired_and_retry, which reconnects and replays the call -- the exact duplicated-side-effect path the previous two commits close. Live repro against a real stdio child that applies an effect then exits without replying: 2 effects with the contributor's commits alone, 1 effect + outcome_uncertain after this. On a stdio server, a session-expired-class error raised by an RPC that was already dispatched is re-raised as _StdioChildExited(in_flight=True) so the stdio recoverer owns it. HTTP servers are untouched: their session-expired retry is still the right recovery. Tests trimmed to the salvage bar: the contributor's test_precall_respawn_retry_dying_midcall_is_uncertain_without_replay (a variant of the watcher-race case) is replaced by the SDK-first regression the review on #106440 asked for; the existing pre-call retry test still pins that a never-sent call is retried once. --- .../test_mcp_stdio_fastfail_reconnect.py | 38 +++++++++---------- tools/mcp_tool_handlers.py | 14 ++++++- 2 files changed, 32 insertions(+), 20 deletions(-) diff --git a/tests/tools/test_mcp_stdio_fastfail_reconnect.py b/tests/tools/test_mcp_stdio_fastfail_reconnect.py index 930a3f389e..c6357f197f 100644 --- a/tests/tools/test_mcp_stdio_fastfail_reconnect.py +++ b/tests/tools/test_mcp_stdio_fastfail_reconnect.py @@ -182,51 +182,51 @@ def test_midcall_child_exit_reconnects_without_replay(monkeypatch, tmp_path): _cleanup(mcp_tool, "srv-midcall") -def test_precall_respawn_retry_dying_midcall_is_uncertain_without_replay(monkeypatch, tmp_path): - """A safe pre-call retry becomes uncertain if its replacement dies after dispatch.""" +def test_sdk_first_transport_close_midcall_is_uncertain_without_replay(monkeypatch, tmp_path): + """The SDK usually notices the closed pipe before the 250 ms child watcher does and raises a + transport-closure error. On a stdio server that is the same ambiguous mid-call death: it must + surface as uncertain, never reach the session-expired recoverer, which would replay the call.""" monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + anyio = pytest.importorskip("anyio") from tools import mcp_tool from tools.mcp_tool_handlers import _make_tool_handler - alive = {"v": False} effects = {"n": 0} - async def _never_dispatched(*a, **kw): - raise AssertionError("the original dead child must not receive the call") - - async def _effect_then_die(*a, **kw): + async def _effect_then_pipe_closes(*a, **kw): effects["n"] += 1 - alive["v"] = False - await asyncio.sleep(30) + raise anyio.ClosedResourceError + + async def _good_call(*a, **kw): + effects["n"] += 1 + return _success_result() async def _watch_children(): - while alive["v"]: - await asyncio.sleep(0.05) + await asyncio.sleep(30) # the watcher loses the race def _respawn(server): - alive["v"] = True new_session = MagicMock() - new_session.call_tool = _effect_then_die + new_session.call_tool = _good_call server.session = new_session server._ready.set() server = _install_stub_server( - mcp_tool, "srv-retry-midcall", _never_dispatched, - children_dead=lambda: not alive["v"], + mcp_tool, "srv-sdk-first", _effect_then_pipe_closes, + children_dead=lambda: False, on_reconnect=_respawn, ) server._watch_stdio_children = _watch_children + server._is_http = lambda: False _mcp_loop._ensure_mcp_loop() try: - handler = _make_tool_handler("srv-retry-midcall", "tool1", 10.0) + handler = _make_tool_handler("srv-sdk-first", "tool1", 10.0) parsed = json.loads(handler({})) 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 uncertain retry must not be invoked a third time" + assert effects["n"] == 1, "the session-expired recoverer must not replay an in-flight stdio call" finally: - _cleanup(mcp_tool, "srv-retry-midcall") + _cleanup(mcp_tool, "srv-sdk-first") def test_dead_child_never_returning_is_not_reported_as_a_timeout( diff --git a/tools/mcp_tool_handlers.py b/tools/mcp_tool_handlers.py index 600bcd7c30..8b21774229 100644 --- a/tools/mcp_tool_handlers.py +++ b/tools/mcp_tool_handlers.py @@ -334,7 +334,19 @@ async def _call_tool_racing_stdio_death(server, server_name: str, tool_name: str f"MCP stdio subprocess for '{server_name}' exited mid-call", in_flight=True, ) - return await rpc_task + try: + return await rpc_task + except Exception as exc: + # The SDK usually sees the closed pipe before the 250 ms watcher poll does. On a stdio + # server a transport-closure error after dispatch is the same ambiguous mid-call death; + # it must not fall through to the session-expired recoverer, which replays the call. + _is_http = getattr(server, "_is_http", None) + if callable(_is_http) and _is_http() is False and _is_session_expired_error(exc): + raise _StdioChildExited( + f"MCP stdio subprocess for '{server_name}' closed its transport mid-call", + in_flight=True, + ) from exc + raise finally: watch_task.cancel() if not rpc_task.done():