diff --git a/src/agents/mcp/manager.py b/src/agents/mcp/manager.py index b100af8bb8..235816819d 100644 --- a/src/agents/mcp/manager.py +++ b/src/agents/mcp/manager.py @@ -283,9 +283,12 @@ async def _attempt_connect( self._remove_failed_server(server) self.errors.pop(server, None) except asyncio.CancelledError as exc: + # Always record so connect_all()'s failure cleanup includes this server. + # Re-raising without recording left partially-opened servers uncleaned + # (especially under `async with`, where __aexit__ never runs). + self._record_failure(server, exc, phase="connect") if not self.suppress_cancelled_error: raise - self._record_failure(server, exc, phase="connect") except Exception as exc: self._record_failure(server, exc, phase="connect") if raise_on_error: diff --git a/tests/mcp/test_mcp_server_manager.py b/tests/mcp/test_mcp_server_manager.py index 3ed2f35a86..ccf026e7ee 100644 --- a/tests/mcp/test_mcp_server_manager.py +++ b/tests/mcp/test_mcp_server_manager.py @@ -173,15 +173,23 @@ async def read_resource(self, uri: str) -> ReadResourceResult: class CancelledServer(MCPServer): + def __init__(self) -> None: + super().__init__() + self.resource_open = False + self.cleanup_calls = 0 + @property def name(self) -> str: return "cancelled" async def connect(self) -> None: + # Simulate a transport that opened resources before cancellation. + self.resource_open = True raise asyncio.CancelledError() async def cleanup(self) -> None: - return None + self.cleanup_calls += 1 + self.resource_open = False async def list_tools( self, run_context: RunContextWrapper[Any] | None = None, agent: Any | None = None @@ -548,5 +556,28 @@ async def test_manager_cleanup_runs_on_cancelled_error_during_connect() -> None: with pytest.raises(asyncio.CancelledError): await manager.connect_all() assert server.cleanup_calls == 1 + # The cancelled server must be recorded and cleaned by connect_all()'s + # failure path — callers cannot rely on a later cleanup_all() because + # `async with` never reaches __aexit__ when __aenter__ raises. + assert cancelled_server in manager.failed_servers + assert cancelled_server.cleanup_calls == 1 + assert cancelled_server.resource_open is False finally: await manager.cleanup_all() + + +@pytest.mark.asyncio +async def test_manager_async_with_cleans_cancelled_server_when_unsuppressed() -> None: + server = CleanupAwareServer() + cancelled_server = CancelledServer() + + with pytest.raises(asyncio.CancelledError): + async with MCPServerManager( + [server, cancelled_server], + suppress_cancelled_error=False, + ): + raise AssertionError("context body should not run when connect raises") + + assert server.cleanup_calls == 1 + assert cancelled_server.cleanup_calls == 1 + assert cancelled_server.resource_open is False