fix: nested MCP cleanup error redaction

src/agents/mcp/server.py

@@ -1144,62 +1144,48 @@ async def cleanup(self):
                 )
                 raise
             except BaseExceptionGroup as eg:
-                # Extract HTTP errors from ExceptionGroup raised during cleanup
-                # This happens when background tasks fail (e.g., HTTP errors)
-                http_error = None
-                connect_error = None
-                timeout_error = None
-
-                for exc in eg.exceptions:
-                    if isinstance(exc, httpx.HTTPStatusError):
-                        http_error = exc
-                    elif isinstance(exc, httpx.ConnectError):
-                        connect_error = exc
-                    elif isinstance(exc, httpx.TimeoutException):
-                        timeout_error = exc
-                del exc
-
-                # Only raise HTTP errors if we're cleaning up after a failed connection.
-                # During normal teardown, log them instead.
-                if http_error:
-                    if is_failed_connection_cleanup:
-                        cleanup_error = self._user_error_for_http_error(http_error)
-                        cleanup_cause = _safe_transport_cause(http_error)
-                        if cleanup_cause is None:
-                            http_error = None
-                    else:
-                        # Normal teardown - log but don't raise
-                        _log_transport_warning(
-                            get_mcp_server_log_message(
-                                "HTTP error during cleanup of MCP server", self
-                            ),
-                            http_error,
-                        )
-                elif connect_error:
-                    if is_failed_connection_cleanup:
-                        cleanup_error = self._user_error_for_http_error(connect_error)
-                        cleanup_cause = _safe_transport_cause(connect_error)
-                        if cleanup_cause is None:
-                            connect_error = None
-                    else:
-                        _log_transport_warning(
-                            get_mcp_server_log_message(
-                                "Connection error during cleanup of MCP server", self
+                http_errors = self._extract_http_errors_from_exception(eg)
+                unsafe_http_error = _first_unsafe_transport_error(http_errors)
+                selected_http_error = unsafe_http_error
+
+                if selected_http_error is None:
+                    # Preserve legacy group diagnostics when HTTP errors are nested but safe.
+                    for error_type in (
+                        httpx.HTTPStatusError,
+                        httpx.ConnectError,
+                        httpx.TimeoutException,
+                    ):
+                        selected_http_error = next(
+                            (
+                                error
+                                for error in reversed(eg.exceptions)
+                                if isinstance(error, Exception) and isinstance(error, error_type)
                             ),
-                            connect_error,
+                            None,
                         )
-                elif timeout_error:
+                        if selected_http_error is not None:
+                            break
+
+                if selected_http_error is not None:
                     if is_failed_connection_cleanup:
-                        cleanup_error = self._user_error_for_http_error(timeout_error)
-                        cleanup_cause = _safe_transport_cause(timeout_error)
+                        cleanup_error = self._user_error_for_http_error(selected_http_error)
+                        cleanup_cause = _safe_transport_cause(selected_http_error)
                         if cleanup_cause is None:
-                            timeout_error = None
+                            http_errors.clear()
+                            del selected_http_error
+                            del unsafe_http_error
                     else:
+                        if isinstance(selected_http_error, httpx.HTTPStatusError):
+                            cleanup_message = "HTTP error during cleanup of MCP server"
+                        elif isinstance(selected_http_error, httpx.ConnectError):
+                            cleanup_message = "Connection error during cleanup of MCP server"
+                        elif isinstance(selected_http_error, httpx.TimeoutException):
+                            cleanup_message = "Timeout error during cleanup of MCP server"
+                        else:
+                            cleanup_message = "Request error during cleanup of MCP server"
                         _log_transport_warning(
-                            get_mcp_server_log_message(
-                                "Timeout error during cleanup of MCP server", self
-                            ),
-                            timeout_error,
+                            get_mcp_server_log_message(cleanup_message, self),
+                            selected_http_error,
                         )
                 else:
                     # No HTTP error found, suppress RuntimeError about cancel scopes

tests/mcp/test_server_errors.py

@@ -1,3 +1,4 @@
+import asyncio
 import builtins
 import logging
 import sys
@@ -63,6 +64,27 @@ def _assert_url_credentials_hidden_from_log_record(record: logging.LogRecord) ->
         assert secret not in attached_values
 
 
+def _assert_not_retained_in_log_record(
+    record: logging.LogRecord,
+    sensitive_value: object,
+) -> None:
+    pending: list[object] = [record.__dict__]
+    seen: set[int] = set()
+
+    while pending:
+        value = pending.pop()
+        assert value is not sensitive_value
+        if id(value) in seen:
+            continue
+        seen.add(id(value))
+
+        if isinstance(value, dict):
+            pending.extend(value.keys())
+            pending.extend(value.values())
+        elif isinstance(value, list | tuple | set | frozenset):
+            pending.extend(value)
+
+
 class CrashingClientSessionServer(_MCPServerWithClientSession):
     def __init__(self):
         super().__init__(cache_tools_list=False, client_session_timeout_seconds=5)
@@ -500,8 +522,26 @@ async def test_failed_connection_cleanup_hides_url_credentials_from_exception_gr
     _assert_not_retained_in_traceback_locals(exc_info.value, http_error)
 
 
+@pytest.mark.asyncio
+async def test_failed_connection_cleanup_checks_every_nested_transport_error():
+    server = MCPServerSse(params={"url": _SAFE_URL})
+    cleanup_group, _, unsafe_error = _mixed_request_error_group(_CREDENTIALED_URL)
+
+    with patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_group)):
+        with pytest.raises(UserError) as exc_info:
+            await server.cleanup()
+
+    assert "Could not reach the server" in str(exc_info.value)
+    _assert_url_credentials_hidden(exc_info.value)
+    _assert_not_retained_in_traceback_locals(exc_info.value, cleanup_group)
+    _assert_not_retained_in_traceback_locals(exc_info.value, unsafe_error)
+    assert server.session is None
+    assert server._get_session_id is None
+
+
 @pytest.mark.asyncio
 @pytest.mark.parametrize("redacted", [True, False])
+@pytest.mark.parametrize("nested", [False, True])
 @pytest.mark.parametrize(
     ("url", "safe_to_attach"),
     [
@@ -513,6 +553,7 @@ async def test_normal_cleanup_only_logs_safe_transport_exceptions(
     monkeypatch,
     caplog,
     redacted: bool,
+    nested: bool,
     url: str,
     safe_to_attach: bool,
 ):
@@ -523,7 +564,10 @@ async def test_normal_cleanup_only_logs_safe_transport_exceptions(
         "timed out",
         request=httpx.Request("GET", url),
     )
-    cleanup_group = BaseExceptionGroup("cleanup failed", [timeout_error])
+    inner_error: BaseException = timeout_error
+    if nested:
+        inner_error = BaseExceptionGroup("nested cleanup failed", [inner_error])
+    cleanup_group = BaseExceptionGroup("cleanup failed", [inner_error])
 
     with (
         patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_group)),
@@ -534,9 +578,139 @@ async def test_normal_cleanup_only_logs_safe_transport_exceptions(
     record = caplog.records[-1]
     if not redacted and safe_to_attach:
         assert record.exc_info is not None
-        assert record.exc_info[1] is timeout_error
+        if nested:
+            assert record.levelno == logging.ERROR
+            assert record.exc_info[1] is cleanup_group
+        else:
+            assert record.levelno == logging.WARNING
+            assert record.exc_info[1] is timeout_error
     else:
         assert record.exc_info is None
+        assert record.exc_text is None
+        _assert_not_retained_in_log_record(record, cleanup_group)
+        _assert_not_retained_in_log_record(record, timeout_error)
 
     if not safe_to_attach:
         _assert_url_credentials_hidden_from_log_record(record)
+
+    assert server.session is None
+    assert server._get_session_id is None
+
+
+@pytest.mark.asyncio
+async def test_normal_cleanup_preserves_safe_nested_group_diagnostics(monkeypatch, caplog):
+    monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False)
+    server = MCPServerSse(params={"url": _SAFE_URL})
+    server.session = MagicMock()
+    timeout_error = httpx.ReadTimeout(
+        "timed out",
+        request=httpx.Request("GET", _SAFE_URL),
+    )
+    cleanup_group = BaseExceptionGroup(
+        "cleanup failed",
+        [
+            ValueError("ordinary sibling failure"),
+            BaseExceptionGroup("nested cleanup failed", [timeout_error]),
+        ],
+    )
+
+    with (
+        patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_group)),
+        caplog.at_level(logging.ERROR, logger="openai.agents"),
+    ):
+        await server.cleanup()
+
+    record = caplog.records[-1]
+    assert record.levelno == logging.ERROR
+    assert record.exc_info is not None
+    assert record.exc_info[1] is cleanup_group
+    assert "ordinary sibling failure" in logging.Formatter().format(record)
+    assert server.session is None
+    assert server._get_session_id is None
+
+
+@pytest.mark.asyncio
+async def test_normal_cleanup_checks_every_nested_transport_error_before_logging(
+    monkeypatch,
+    caplog,
+):
+    monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False)
+    server = MCPServerSse(params={"url": _SAFE_URL})
+    server.session = MagicMock()
+    cleanup_group, _, unsafe_error = _mixed_request_error_group(_CREDENTIALED_URL)
+
+    with (
+        patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_group)),
+        caplog.at_level(logging.WARNING, logger="openai.agents"),
+    ):
+        await server.cleanup()
+
+    record = caplog.records[-1]
+    assert record.exc_info is None
+    assert record.exc_text is None
+    _assert_not_retained_in_log_record(record, cleanup_group)
+    _assert_not_retained_in_log_record(record, unsafe_error)
+    _assert_url_credentials_hidden_from_log_record(record)
+    assert server.session is None
+    assert server._get_session_id is None
+
+
+@pytest.mark.asyncio
+async def test_normal_cleanup_preserves_non_http_exception_group_logging(monkeypatch, caplog):
+    monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False)
+    server = MCPServerSse(params={"url": _SAFE_URL})
+    server.session = MagicMock()
+    cleanup_group = BaseExceptionGroup("cleanup failed", [ValueError("ordinary failure")])
+
+    with (
+        patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_group)),
+        caplog.at_level(logging.ERROR, logger="openai.agents"),
+    ):
+        await server.cleanup()
+
+    record = caplog.records[-1]
+    assert record.exc_info is not None
+    assert record.exc_info[1] is cleanup_group
+    assert server.session is None
+    assert server._get_session_id is None
+
+
+@pytest.mark.asyncio
+async def test_normal_cleanup_preserves_cancel_scope_suppression(monkeypatch, caplog):
+    monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False)
+    server = MCPServerSse(params={"url": _SAFE_URL})
+    server.session = MagicMock()
+    cleanup_group = BaseExceptionGroup(
+        "cleanup failed",
+        [RuntimeError("Attempted to exit cancel scope in a different task")],
+    )
+
+    with (
+        patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_group)),
+        caplog.at_level(logging.DEBUG, logger="openai.agents"),
+    ):
+        await server.cleanup()
+
+    record = caplog.records[-1]
+    assert record.levelno == logging.DEBUG
+    assert record.exc_info is not None
+    assert record.exc_info[1] is cleanup_group
+    assert server.session is None
+    assert server._get_session_id is None
+
+
+@pytest.mark.asyncio
+async def test_cleanup_propagates_cancellation_and_clears_session_state():
+    server = MCPServerSse(params={"url": _SAFE_URL})
+    server.session = MagicMock()
+
+    with patch.object(
+        server.exit_stack,
+        "aclose",
+        AsyncMock(side_effect=asyncio.CancelledError()),
+    ):
+        with pytest.raises(asyncio.CancelledError):
+            await server.cleanup()
+
+    assert server.session is None
+    assert server._get_session_id is None