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.
This commit is contained in:
@@ -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(
|
||||
|
||||
@@ -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():
|
||||
|
||||
Reference in New Issue
Block a user