Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tests/tools/test_mcp_parked_self_probe.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ async def _run_stdio(self, config):
# First connect succeeds (sets _ready), then dies.
self.session = object()
self._ready.set()
self._ever_connected = True
self.session = None
raise RuntimeError("backend outage begins")
if not state["backend_up"]:
Expand Down
1 change: 1 addition & 0 deletions tests/tools/test_mcp_reconnect_log_hygiene.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ async def _run_stdio(self, config):
if state["transport_calls"] == 1:
self.session = object()
self._ready.set()
self._ever_connected = True
self.session = None
raise ConnectionError("backend down")

Expand Down
1 change: 1 addition & 0 deletions tests/tools/test_mcp_reconnect_retry_reset.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ async def _run_stdio(self, config):
# First connect: succeed (sets _ready), then fail.
self.session = object()
self._ready.set()
self._ever_connected = True
self.session = None
raise RuntimeError("blip 1")

Expand Down
63 changes: 63 additions & 0 deletions tests/tools/test_mcp_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -1421,6 +1421,69 @@ async def _test():

asyncio.run(_test())

def test_reconnect_failures_after_initial_success_use_reconnect_budget(self):
"""A server that already registered tools must not be re-classified
as "never connected" just because a later reconnect attempt fails
while ``_ready`` is momentarily clear (#94654).

``_MAX_INITIAL_CONNECT_RETRIES`` is 3 and ``_MAX_RECONNECT_RETRIES``
is 5. Four consecutive post-success reconnect failures exceed the
(buggy) initial-connect ladder but not the reconnect budget, so this
distinguishes the two code paths.
"""
from tools.mcp_tool import MCPServerTask

run_count = 0
target_server = None

async def patched_run_stdio(self_srv, config):
nonlocal run_count, target_server
run_count += 1
if target_server is not self_srv:
return None
if run_count == 1:
# Initial connection succeeds and registers tools. Setting
# ``_ever_connected`` mirrors what the real ``_run_stdio``
# does right after a successful ``_discover_tools()`` call.
self_srv.session = MagicMock()
self_srv._tools = []
self_srv._ready.set()
self_srv._ever_connected = True
return "reconnect"
if run_count <= 5:
# Four consecutive failures on later reconnect attempts --
# a flapping transport, not a server that never connected.
raise ConnectionError("reconnect attempt failed")
self_srv._shutdown_event.set()
await self_srv._shutdown_event.wait()

async def patched_wait_parked(self_srv, timeout=None):
# If the code under test parks early, unblock immediately
# instead of waiting out the real park interval.
self_srv._shutdown_event.set()
return "shutdown"

async def _test():
nonlocal target_server
server = MCPServerTask("test_srv")
target_server = server

with patch.object(MCPServerTask, "_run_stdio", patched_run_stdio), \
patch.object(
MCPServerTask, "_wait_for_reconnect_or_shutdown",
patched_wait_parked,
), \
patch("asyncio.sleep", new_callable=AsyncMock):
await server.run({"command": "test"})

assert server._was_parked is False, (
"server parked after only 4 reconnect failures following a "
"successful initial connection -- it was misclassified as "
"never having connected and re-entered the initial-connect "
"ladder instead of the (larger) reconnect budget"
)

asyncio.run(_test())

def test_preflight_probe_runs_on_initial_http_connect(self):
"""The content-type preflight probe fires on the first HTTP connect."""
Expand Down
21 changes: 19 additions & 2 deletions tools/mcp_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -2396,6 +2396,7 @@ class MCPServerTask:
"_reconnect_retries", "_session_proven", "_was_parked",
"_inflight_tasks", "_reconnecting", "_suspect_reason",
"_teardown_race", "_permanent_grace_used", "_stdio_child_pids",
"_ever_connected",
)

def __init__(self, name: str):
Expand Down Expand Up @@ -2426,6 +2427,12 @@ def __init__(self, name: str):
# keeps getting charged and still reaches the park instead of
# hot-cycling respawns forever.
self._session_proven: bool = False
# Set once tools have ever been registered and never cleared again,
# unlike ``_ready`` (which is cleared on every reconnect cycle). Used
# to tell a genuine first-connection failure from a later reconnect
# failure that merely happens to occur while ``_ready`` is
# momentarily clear — see the ``initial_retries`` ladder in run().
self._ever_connected: bool = False
# True while parked (reconnect budget exhausted) or after a park,
# until the session proves healthy again — used to log the
# parked→revived transition exactly once.
Expand Down Expand Up @@ -3335,6 +3342,7 @@ async def _run_stdio(self, config: dict):
self._mark_lifecycle_started()
await self._discover_tools()
self._ready.set()
self._ever_connected = True
# Session is live again: clear any breaker state from a
# prior outage so the first call after recovery isn't
# gated on a stale consecutive-failure count (#16788).
Expand Down Expand Up @@ -3706,6 +3714,7 @@ def _mcp_http_client_factory(
self.session = session
await self._discover_tools()
self._ready.set()
self._ever_connected = True
# Session is live again: clear any breaker state from a
# prior outage so the first call after recovery isn't
# gated on a stale consecutive-failure count (#16788).
Expand Down Expand Up @@ -3771,6 +3780,7 @@ def _mcp_http_client_factory(
self.session = session
await self._discover_tools()
self._ready.set()
self._ever_connected = True
# Session is live again: clear any breaker state from
# a prior outage so the first call after recovery
# isn't gated on a stale failure count (#16788).
Expand Down Expand Up @@ -3818,6 +3828,7 @@ def _mcp_http_client_factory(
self.session = session
await self._discover_tools()
self._ready.set()
self._ever_connected = True
# Session is live again: clear any breaker state from a
# prior outage so the first call after recovery isn't
# gated on a stale consecutive-failure count (#16788).
Expand Down Expand Up @@ -4116,9 +4127,15 @@ async def run(self, config: dict):

# If this is the first connection attempt, retry with backoff
# before giving up. A transient DNS/network blip at startup
# should not permanently kill the server.
# should not permanently kill the server. Gated on
# ``_ever_connected`` rather than ``_ready`` — ``_ready`` is
# cleared on every reconnect cycle (see below), so a server
# that already registered tools once and then dropped would
# otherwise be misclassified as never having connected and
# re-enter this initial-connect ladder (#94654).
# ``_ever_connected`` itself is set once and never cleared.
# (Ported from Kilo Code's MCP resilience fix.)
if not self._ready.is_set():
if not self._ever_connected:
if failure_class == "permanent":
# Deterministic failure (bad command, non-MCP URL,
# 401/403): every retry hits the same wall. Park
Expand Down
Loading