diff --git a/src/adcp/protocols/mcp.py b/src/adcp/protocols/mcp.py index fe8516cc..fc023307 100644 --- a/src/adcp/protocols/mcp.py +++ b/src/adcp/protocols/mcp.py @@ -9,6 +9,14 @@ from typing import TYPE_CHECKING, Any from urllib.parse import urlparse +# ExceptionGroup is available in Python 3.11+ +# In 3.11+, it's a built-in type. For 3.10, we need to handle its absence. +try: + _ExceptionGroup: type[BaseException] | None = ExceptionGroup # type: ignore[name-defined] +except NameError: + # Python 3.10 - ExceptionGroup doesn't exist + _ExceptionGroup = None + logger = logging.getLogger(__name__) if TYPE_CHECKING: @@ -23,6 +31,14 @@ except ImportError: MCP_AVAILABLE = False +try: + from httpx import HTTPStatusError + + HTTPX_AVAILABLE = True +except ImportError: + HTTPX_AVAILABLE = False + HTTPStatusError = None # type: ignore[assignment, misc] + from adcp.exceptions import ADCPConnectionError, ADCPTimeoutError from adcp.protocols.base import ProtocolAdapter from adcp.types.core import DebugInfo, TaskResult, TaskStatus @@ -56,22 +72,45 @@ async def _cleanup_failed_connection(self, context: str) -> None: self._session = None try: await old_stack.aclose() - except asyncio.CancelledError: - logger.debug(f"MCP session cleanup cancelled {context}") - except RuntimeError as cleanup_error: - # Known anyio task group cleanup issue - error_msg = str(cleanup_error).lower() - if "cancel scope" in error_msg or "async context" in error_msg: - logger.debug(f"Ignoring anyio cleanup error {context}: {cleanup_error}") + except BaseException as cleanup_error: + # Handle all cleanup errors including ExceptionGroup + # Re-raise KeyboardInterrupt and SystemExit immediately + if isinstance(cleanup_error, (KeyboardInterrupt, SystemExit)): + raise + + if isinstance(cleanup_error, asyncio.CancelledError): + logger.debug(f"MCP session cleanup cancelled {context}") + return + + # Handle ExceptionGroup from task group failures (Python 3.11+) + if _ExceptionGroup is not None and isinstance( + cleanup_error, _ExceptionGroup + ): + for exc in cleanup_error.exceptions: # type: ignore[attr-defined] + self._log_cleanup_error(exc, context) else: - logger.warning( - f"Unexpected RuntimeError during cleanup {context}: {cleanup_error}" - ) - except Exception as cleanup_error: - # Log unexpected cleanup errors but don't raise to preserve original error - logger.warning( - f"Unexpected error during cleanup {context}: {cleanup_error}", exc_info=True - ) + self._log_cleanup_error(cleanup_error, context) + + def _log_cleanup_error(self, exc: BaseException, context: str) -> None: + """Log a cleanup error without raising.""" + # Check for known cleanup error patterns from httpx/anyio + exc_str = str(exc).lower() + + # Common cleanup errors that are expected when connection fails + is_known_cleanup_error = ( + isinstance(exc, RuntimeError) + and ("cancel scope" in exc_str or "async context" in exc_str) + ) or ( + # HTTP errors during cleanup (if httpx is available) + HTTPX_AVAILABLE and HTTPStatusError is not None and isinstance(exc, HTTPStatusError) + ) + + if is_known_cleanup_error: + # Expected cleanup errors - log at debug level without stack trace + logger.debug(f"Ignoring expected cleanup error {context}: {exc}") + else: + # Truly unexpected cleanup errors - log at warning with full context + logger.warning(f"Unexpected error during cleanup {context}: {exc}", exc_info=True) async def _get_session(self) -> ClientSession: """ @@ -146,7 +185,11 @@ async def _get_session(self) -> ClientSession: ) return self._session # type: ignore[no-any-return] - except Exception as e: + except BaseException as e: + # Catch BaseException to handle CancelledError from failed initialization + # Re-raise KeyboardInterrupt and SystemExit immediately + if isinstance(e, (KeyboardInterrupt, SystemExit)): + raise last_error = e # Clean up the exit stack on failure to avoid resource leaks await self._cleanup_failed_connection("during connection attempt") diff --git a/tests/test_protocols.py b/tests/test_protocols.py index eaf12f71..2edfa2e2 100644 --- a/tests/test_protocols.py +++ b/tests/test_protocols.py @@ -1,5 +1,6 @@ """Tests for protocol adapters.""" +import sys from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -844,3 +845,37 @@ def create_mock_exit_stack(): # Verify adapter state is clean after all failed attempts assert adapter._exit_stack is None assert adapter._session is None + + @pytest.mark.asyncio + @pytest.mark.skipif( + sys.version_info < (3, 11), + reason="ExceptionGroup is only available in Python 3.11+", + ) + async def test_cleanup_handles_exception_group(self, mcp_config): + """Test that cleanup handles ExceptionGroup from task group failures.""" + from contextlib import AsyncExitStack + + import httpx + + adapter = MCPAdapter(mcp_config) + + # Create an ExceptionGroup like what anyio task groups raise + http_error = httpx.HTTPStatusError( + "Client error '405 Method Not Allowed' for url 'https://test.example.com'", + request=MagicMock(), + response=MagicMock(status_code=405), + ) + exception_group = ExceptionGroup("unhandled errors in a TaskGroup", [http_error]) + + # Mock exit stack that raises ExceptionGroup on cleanup + mock_exit_stack = AsyncMock(spec=AsyncExitStack) + mock_exit_stack.aclose = AsyncMock(side_effect=exception_group) + adapter._exit_stack = mock_exit_stack + + # cleanup should not raise despite the ExceptionGroup + await adapter._cleanup_failed_connection("during test") + + # Verify cleanup was attempted and state is clean + mock_exit_stack.aclose.assert_called_once() + assert adapter._exit_stack is None + assert adapter._session is None