From ba64b78ecff84190e855c293a308fe0058261866 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Fri, 31 Jul 2026 15:18:54 +0900 Subject: [PATCH] fix: nested MCP cleanup error redaction --- src/agents/mcp/server.py | 86 +++++++-------- tests/mcp/test_server_errors.py | 178 +++++++++++++++++++++++++++++++- 2 files changed, 212 insertions(+), 52 deletions(-) diff --git a/src/agents/mcp/server.py b/src/agents/mcp/server.py index 2b5a8a25fa..944d0d1738 100644 --- a/src/agents/mcp/server.py +++ b/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 diff --git a/tests/mcp/test_server_errors.py b/tests/mcp/test_server_errors.py index d33308dfb6..5f10da4ce7 100644 --- a/tests/mcp/test_server_errors.py +++ b/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