From 3c9026d823bfb16ecb44561c2b88f5da7870d952 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Tue, 25 Aug 2026 10:49:02 +0000 Subject: [PATCH 1/3] fix(tools): stop treating a post-registration reconnect drop as an initial-connect failure MCPServerTask.run() used `_ready.is_set()` to tell a genuine first connection attempt from a later reconnect. `_ready` is cleared on every reconnect cycle, so once a server has already registered its tools and then drops (keepalive failure, transient TaskGroup exit, etc.), the next failed reconnect attempt is misclassified as "never connected" and burns the 3-attempt initial-connect ladder instead of the 5-attempt reconnect budget, parking the server much sooner and logging "failed initial connection after 3 attempts" even though tools were already registered. Add a sticky `_ever_connected` flag, set once alongside `_ready.set()` right after a successful `_discover_tools()` call and never cleared, and gate the initial-vs-reconnect branch on it instead. Fixes #94654 --- tests/tools/test_mcp_tool.py | 67 ++++++++++++++++++++++++++++++++++++ tools/mcp_tool.py | 20 +++++++++-- 2 files changed, 85 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_mcp_tool.py b/tests/tools/test_mcp_tool.py index bca9dfb4a346..dfe353473696 100644 --- a/tests/tools/test_mcp_tool.py +++ b/tests/tools/test_mcp_tool.py @@ -1421,6 +1421,73 @@ 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; + # guarded because the pre-fix class has no such slot. + self_srv.session = MagicMock() + self_srv._tools = [] + self_srv._ready.set() + try: + self_srv._ever_connected = True + except AttributeError: + pass + 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.""" diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 8393538247d7..bd05de29fd0a 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -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): @@ -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. @@ -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). @@ -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). @@ -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). @@ -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). @@ -4116,9 +4127,14 @@ 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). # (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 From 14dd0ef5793f2e8fc5f9ae4d43d2b08ca0b0593d Mon Sep 17 00:00:00 2001 From: chelsealong Date: Tue, 25 Aug 2026 11:40:43 +0000 Subject: [PATCH 2/3] fix(tests): set _ever_connected in reconnect-scenario test mocks These pre-existing tests fake a successful first connect by calling only _ready.set(), which is what the real code did before this PR. Now that run() gates the initial-vs-reconnect branch on the new sticky _ever_connected flag instead, their later simulated reconnect failures were misclassified as never-connected and hit the 3-attempt ladder, failing test_reconnect_counter_resets_after_successful_session, test_parked_server_self_probes_and_revives, and test_retry_attempts_log_debug_transitions_warn in CI. Set the flag alongside _ready.set() to mirror the real success sites, same as the new test added in tools/mcp_tool.py's own PR. --- tests/tools/test_mcp_parked_self_probe.py | 1 + tests/tools/test_mcp_reconnect_log_hygiene.py | 1 + tests/tools/test_mcp_reconnect_retry_reset.py | 1 + 3 files changed, 3 insertions(+) diff --git a/tests/tools/test_mcp_parked_self_probe.py b/tests/tools/test_mcp_parked_self_probe.py index e67177c2cd7c..61a1a6578e34 100644 --- a/tests/tools/test_mcp_parked_self_probe.py +++ b/tests/tools/test_mcp_parked_self_probe.py @@ -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"]: diff --git a/tests/tools/test_mcp_reconnect_log_hygiene.py b/tests/tools/test_mcp_reconnect_log_hygiene.py index cd6a842cbf30..6c180709b492 100644 --- a/tests/tools/test_mcp_reconnect_log_hygiene.py +++ b/tests/tools/test_mcp_reconnect_log_hygiene.py @@ -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") diff --git a/tests/tools/test_mcp_reconnect_retry_reset.py b/tests/tools/test_mcp_reconnect_retry_reset.py index 0bef5ca7f5c6..60f26f0266f9 100644 --- a/tests/tools/test_mcp_reconnect_retry_reset.py +++ b/tests/tools/test_mcp_reconnect_retry_reset.py @@ -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") From d6172e92c4b7df020aaa47e761196726a9b6aacb Mon Sep 17 00:00:00 2001 From: chelsealong Date: Tue, 25 Aug 2026 13:35:20 +0000 Subject: [PATCH 3/3] chore: address AI-review nits on _ever_connected fix Drop the try/except AttributeError guard in the new regression test now that the slot is always defined, and note in the run() comment that _ever_connected is set once and never cleared. --- tests/tools/test_mcp_tool.py | 8 ++------ tools/mcp_tool.py | 1 + 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/tests/tools/test_mcp_tool.py b/tests/tools/test_mcp_tool.py index dfe353473696..a2b8e0ee5c29 100644 --- a/tests/tools/test_mcp_tool.py +++ b/tests/tools/test_mcp_tool.py @@ -1444,15 +1444,11 @@ async def patched_run_stdio(self_srv, config): 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; - # guarded because the pre-fix class has no such slot. + # does right after a successful ``_discover_tools()`` call. self_srv.session = MagicMock() self_srv._tools = [] self_srv._ready.set() - try: - self_srv._ever_connected = True - except AttributeError: - pass + self_srv._ever_connected = True return "reconnect" if run_count <= 5: # Four consecutive failures on later reconnect attempts -- diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index bd05de29fd0a..69d524397f87 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -4133,6 +4133,7 @@ async def run(self, config: dict): # 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._ever_connected: if failure_class == "permanent":