From 2cb7220d453ac909e7db4ec731b8b1d4222270c6 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Wed, 26 Nov 2025 05:37:02 -0500 Subject: [PATCH 1/4] fix: handle ExceptionGroup and CancelledError in MCP error flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an MCP connection fails, two issues were masking the real error: 1. Cleanup raises ExceptionGroup containing the HTTP error (Python 3.11+) 2. Session initialization raises CancelledError instead of the real error This fix addresses both: **ExceptionGroup handling:** - Catch BaseException in cleanup to handle ExceptionGroup - Extract and log individual exceptions from the group - Gracefully handle Python 3.10 (no ExceptionGroup) and 3.11+ - Log HTTPStatusError at debug level (expected during cleanup) **CancelledError handling:** - Catch BaseException in connection loop (not just Exception) - Properly convert CancelledError to ADCPConnectionError - Preserve KeyboardInterrupt and SystemExit behavior Now users see the actual connection error (e.g., "405 Method Not Allowed") instead of confusing cleanup error messages. Added test to verify ExceptionGroup handling works correctly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/adcp/protocols/mcp.py | 64 +++++++++++++++++++++++++++++---------- tests/test_protocols.py | 30 ++++++++++++++++++ 2 files changed, 78 insertions(+), 16 deletions(-) diff --git a/src/adcp/protocols/mcp.py b/src/adcp/protocols/mcp.py index fe8516cc..6688f06d 100644 --- a/src/adcp/protocols/mcp.py +++ b/src/adcp/protocols/mcp.py @@ -4,11 +4,20 @@ import asyncio import logging +import sys import time from contextlib import AsyncExitStack from typing import TYPE_CHECKING, Any from urllib.parse import urlparse +# ExceptionGroup is available in Python 3.11+ +if sys.version_info >= (3, 11): + from builtins import BaseExceptionGroup +else: + # For Python 3.10, BaseExceptionGroup doesn't exist + # We handle this gracefully by checking if it's None before using isinstance + BaseExceptionGroup = None + logger = logging.getLogger(__name__) if TYPE_CHECKING: @@ -56,22 +65,41 @@ 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 + 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 BaseExceptionGroup is not None and isinstance( + cleanup_error, BaseExceptionGroup + ): + for exc in cleanup_error.exceptions: + 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_type_name = type(exc).__name__ + 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 ( + exc_type_name == "HTTPStatusError" # HTTP errors during cleanup + ) + + 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 +174,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..a5fce1de 100644 --- a/tests/test_protocols.py +++ b/tests/test_protocols.py @@ -844,3 +844,33 @@ 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 + 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 From 7e1a6ae21d438d5be765574eef8c6b93a30b37c9 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Wed, 26 Nov 2025 06:45:57 -0500 Subject: [PATCH 2/4] fix: improve ExceptionGroup and error handling robustness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses code review feedback to make error handling more robust: 1. Fix ExceptionGroup import - use built-in type directly, not from builtins 2. Add KeyboardInterrupt/SystemExit re-raise in cleanup path 3. Replace string-based HTTPStatusError check with isinstance 4. Update all references from BaseExceptionGroup to ExceptionGroup These changes improve type safety, handle critical interrupts correctly, and make the code more maintainable. All tests pass and mypy type checking succeeds. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/adcp/protocols/mcp.py | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/adcp/protocols/mcp.py b/src/adcp/protocols/mcp.py index 6688f06d..e418560b 100644 --- a/src/adcp/protocols/mcp.py +++ b/src/adcp/protocols/mcp.py @@ -12,11 +12,11 @@ # ExceptionGroup is available in Python 3.11+ if sys.version_info >= (3, 11): - from builtins import BaseExceptionGroup + ExceptionGroup = ExceptionGroup # Built-in type in 3.11+ else: - # For Python 3.10, BaseExceptionGroup doesn't exist + # For Python 3.10, ExceptionGroup doesn't exist # We handle this gracefully by checking if it's None before using isinstance - BaseExceptionGroup = None + ExceptionGroup = None logger = logging.getLogger(__name__) @@ -32,6 +32,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 @@ -67,13 +75,17 @@ async def _cleanup_failed_connection(self, context: str) -> None: await old_stack.aclose() 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 BaseExceptionGroup is not None and isinstance( - cleanup_error, BaseExceptionGroup + if ExceptionGroup is not None and isinstance( + cleanup_error, ExceptionGroup ): for exc in cleanup_error.exceptions: self._log_cleanup_error(exc, context) @@ -83,15 +95,15 @@ async def _cleanup_failed_connection(self, context: str) -> None: 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_type_name = type(exc).__name__ 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) + isinstance(exc, RuntimeError) + and ("cancel scope" in exc_str or "async context" in exc_str) ) or ( - exc_type_name == "HTTPStatusError" # HTTP errors during cleanup + # HTTP errors during cleanup (if httpx is available) + HTTPX_AVAILABLE and HTTPStatusError is not None and isinstance(exc, HTTPStatusError) ) if is_known_cleanup_error: From 9213d4f947d9a12309dcc33c60ee826fb8462bc3 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Wed, 26 Nov 2025 06:50:31 -0500 Subject: [PATCH 3/4] fix: use try/except for ExceptionGroup to satisfy ruff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ruff doesn't recognize sys.version_info guards for undefined names. Changed to try/except pattern which both ruff and mypy accept. Also removed unused sys import since we no longer need version checking. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/adcp/protocols/mcp.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/adcp/protocols/mcp.py b/src/adcp/protocols/mcp.py index e418560b..fc023307 100644 --- a/src/adcp/protocols/mcp.py +++ b/src/adcp/protocols/mcp.py @@ -4,19 +4,18 @@ import asyncio import logging -import sys import time from contextlib import AsyncExitStack from typing import TYPE_CHECKING, Any from urllib.parse import urlparse # ExceptionGroup is available in Python 3.11+ -if sys.version_info >= (3, 11): - ExceptionGroup = ExceptionGroup # Built-in type in 3.11+ -else: - # For Python 3.10, ExceptionGroup doesn't exist - # We handle this gracefully by checking if it's None before using isinstance - ExceptionGroup = None +# 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__) @@ -84,10 +83,10 @@ async def _cleanup_failed_connection(self, context: str) -> None: return # Handle ExceptionGroup from task group failures (Python 3.11+) - if ExceptionGroup is not None and isinstance( - cleanup_error, ExceptionGroup + if _ExceptionGroup is not None and isinstance( + cleanup_error, _ExceptionGroup ): - for exc in cleanup_error.exceptions: + for exc in cleanup_error.exceptions: # type: ignore[attr-defined] self._log_cleanup_error(exc, context) else: self._log_cleanup_error(cleanup_error, context) From 779c6f18a9a51e7c9bfa203fb2b68689d65ad30c Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Wed, 26 Nov 2025 07:08:54 -0500 Subject: [PATCH 4/4] fix: skip ExceptionGroup test on Python 3.10 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test_cleanup_handles_exception_group test uses ExceptionGroup which doesn't exist in Python 3.10. Added skipif marker to skip this test on Python < 3.11. The functionality still works correctly in 3.10 (it gracefully handles the absence of ExceptionGroup), we just can't test it directly since we can't create an ExceptionGroup in the test. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- tests/test_protocols.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_protocols.py b/tests/test_protocols.py index a5fce1de..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 @@ -846,6 +847,10 @@ def create_mock_exit_stack(): 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