From db77e60609e6ab5598d1442f0ff0228251d5da89 Mon Sep 17 00:00:00 2001 From: Aishwarya Date: Thu, 22 May 2025 19:27:29 +0545 Subject: [PATCH 1/9] Handling TODO mentioned in the lowlevel/server.py. Initital commit, need feedback. --- src/mcp/server/lowlevel/server.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/mcp/server/lowlevel/server.py b/src/mcp/server/lowlevel/server.py index 8c459383c8..09e9440cee 100644 --- a/src/mcp/server/lowlevel/server.py +++ b/src/mcp/server/lowlevel/server.py @@ -609,6 +609,19 @@ async def _handle_message( await self._handle_request(message, req, session, lifespan_context, raise_exceptions) case types.ClientNotification(root=notify): await self._handle_notification(notify) + case Exception(): + logger.error(f"Received error message: {message}") + if raise_exceptions: + raise message + # Send the error as a notification since we don't have a request context + await session.send_log_message( + level="error", + data=types.ErrorData( + code=types.INTERNAL_ERROR, + message=str(message), + data=None + ) + ) for warning in w: logger.info("Warning: %s: %s", warning.category.__name__, warning.message) From ce289690405bd52eaa485b848953c913fee6b215 Mon Sep 17 00:00:00 2001 From: Aishwarya Date: Thu, 22 May 2025 19:47:26 +0545 Subject: [PATCH 2/9] lint fix --- src/mcp/server/lowlevel/server.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/mcp/server/lowlevel/server.py b/src/mcp/server/lowlevel/server.py index 09e9440cee..4b859784e9 100644 --- a/src/mcp/server/lowlevel/server.py +++ b/src/mcp/server/lowlevel/server.py @@ -613,14 +613,13 @@ async def _handle_message( logger.error(f"Received error message: {message}") if raise_exceptions: raise message - # Send the error as a notification since we don't have a request context + # Send the error as a notification + # as we don't have a request context await session.send_log_message( level="error", data=types.ErrorData( - code=types.INTERNAL_ERROR, - message=str(message), - data=None - ) + code=types.INTERNAL_ERROR, message=str(message), data=None + ), ) for warning in w: From 39220b01187ef2f62b26b74fb43f06ccb333da41 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 15 Jul 2025 13:51:19 +0100 Subject: [PATCH 3/9] fix: Improve exception handling in lowlevel server - Handle exceptions in match statement as suggested by TODO - Use proper dict structure for error logging instead of ErrorData - Add comprehensive tests with parameterization - Include exception type, module, and args in log data Reported-by: AishwaryaKalloli Github-Issue: #786 --- src/mcp/server/lowlevel/server.py | 17 +++-- .../test_lowlevel_exception_handling.py | 76 +++++++++++++++++++ 2 files changed, 85 insertions(+), 8 deletions(-) create mode 100644 tests/server/test_lowlevel_exception_handling.py diff --git a/src/mcp/server/lowlevel/server.py b/src/mcp/server/lowlevel/server.py index 4b859784e9..761cafaec4 100644 --- a/src/mcp/server/lowlevel/server.py +++ b/src/mcp/server/lowlevel/server.py @@ -602,24 +602,25 @@ async def _handle_message( raise_exceptions: bool = False, ): with warnings.catch_warnings(record=True) as w: - # TODO(Marcelo): We should be checking if message is Exception here. - match message: # type: ignore[reportMatchNotExhaustive] + match message: case RequestResponder(request=types.ClientRequest(root=req)) as responder: with responder: await self._handle_request(message, req, session, lifespan_context, raise_exceptions) case types.ClientNotification(root=notify): await self._handle_notification(notify) case Exception(): - logger.error(f"Received error message: {message}") + logger.error(f"Received exception from stream: {message}") if raise_exceptions: raise message - # Send the error as a notification - # as we don't have a request context await session.send_log_message( level="error", - data=types.ErrorData( - code=types.INTERNAL_ERROR, message=str(message), data=None - ), + data={ + "message": str(message), + "type": type(message).__name__, + "module": type(message).__module__, + "args": getattr(message, "args", None), + }, + logger="mcp.server.exception_handler", ) for warning in w: diff --git a/tests/server/test_lowlevel_exception_handling.py b/tests/server/test_lowlevel_exception_handling.py new file mode 100644 index 0000000000..35e7b1e1dc --- /dev/null +++ b/tests/server/test_lowlevel_exception_handling.py @@ -0,0 +1,76 @@ +from unittest.mock import AsyncMock, Mock + +import pytest + +import mcp.types as types +from mcp.server.lowlevel.server import Server +from mcp.server.session import ServerSession +from mcp.shared.session import RequestResponder + + +@pytest.mark.anyio +async def test_exception_handling_with_raise_exceptions_true(): + """Test that exceptions are re-raised when raise_exceptions=True""" + server = Server("test-server") + session = Mock(spec=ServerSession) + session.send_log_message = AsyncMock() + + test_exception = RuntimeError("Test error") + + with pytest.raises(RuntimeError, match="Test error"): + await server._handle_message(test_exception, session, {}, raise_exceptions=True) + + # Should not send log message when re-raising + session.send_log_message.assert_not_called() + + +@pytest.mark.anyio +@pytest.mark.parametrize( + "exception_class,message", + [ + (ValueError, "Test validation error"), + (RuntimeError, "Test runtime error"), + (KeyError, "Test key error"), + (Exception, "Basic error"), + ], +) +async def test_exception_handling_with_raise_exceptions_false(exception_class, message): + """Test that exceptions are logged when raise_exceptions=False""" + server = Server("test-server") + session = Mock(spec=ServerSession) + session.send_log_message = AsyncMock() + + test_exception = exception_class(message) + + await server._handle_message(test_exception, session, {}, raise_exceptions=False) + + # Should send log message + session.send_log_message.assert_called_once() + call_args = session.send_log_message.call_args + + assert call_args.kwargs["level"] == "error" + assert call_args.kwargs["data"]["message"] == str(test_exception) + assert call_args.kwargs["data"]["type"] == exception_class.__name__ + assert call_args.kwargs["logger"] == "mcp.server.exception_handler" + + +@pytest.mark.anyio +async def test_normal_message_handling_not_affected(): + """Test that normal messages still work correctly""" + server = Server("test-server") + session = Mock(spec=ServerSession) + + # Create a mock RequestResponder + responder = Mock(spec=RequestResponder) + responder.request = types.ClientRequest(root=types.PingRequest(method="ping")) + responder.__enter__ = Mock(return_value=responder) + responder.__exit__ = Mock(return_value=None) + + # Mock the _handle_request method to avoid complex setup + server._handle_request = AsyncMock() + + # Should handle normally without any exception handling + await server._handle_message(responder, session, {}, raise_exceptions=False) + + # Verify _handle_request was called + server._handle_request.assert_called_once() From 1934a586027f74f88a2886be71f4625318be4d90 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Thu, 21 Aug 2025 15:44:01 +0100 Subject: [PATCH 4/9] Address kludex review comments - use __exit__ format for exception data Changed exception data format to match Python's __exit__ format: - exception_type: The exception class name - exception_value: The exception message/value - exception_traceback: The traceback (set to None for now) Updated tests to verify the new data format. --- src/mcp/server/lowlevel/server.py | 7 +++---- tests/server/test_lowlevel_exception_handling.py | 5 +++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/mcp/server/lowlevel/server.py b/src/mcp/server/lowlevel/server.py index 761cafaec4..b0238eb468 100644 --- a/src/mcp/server/lowlevel/server.py +++ b/src/mcp/server/lowlevel/server.py @@ -615,10 +615,9 @@ async def _handle_message( await session.send_log_message( level="error", data={ - "message": str(message), - "type": type(message).__name__, - "module": type(message).__module__, - "args": getattr(message, "args", None), + "exception_type": type(message).__name__, + "exception_value": str(message), + "exception_traceback": None, }, logger="mcp.server.exception_handler", ) diff --git a/tests/server/test_lowlevel_exception_handling.py b/tests/server/test_lowlevel_exception_handling.py index 35e7b1e1dc..7208e07ddc 100644 --- a/tests/server/test_lowlevel_exception_handling.py +++ b/tests/server/test_lowlevel_exception_handling.py @@ -49,8 +49,9 @@ async def test_exception_handling_with_raise_exceptions_false(exception_class, m call_args = session.send_log_message.call_args assert call_args.kwargs["level"] == "error" - assert call_args.kwargs["data"]["message"] == str(test_exception) - assert call_args.kwargs["data"]["type"] == exception_class.__name__ + assert call_args.kwargs["data"]["exception_type"] == exception_class.__name__ + assert call_args.kwargs["data"]["exception_value"] == str(test_exception) + assert call_args.kwargs["data"]["exception_traceback"] is None assert call_args.kwargs["logger"] == "mcp.server.exception_handler" From df6e820890accc1aaa618bfebd61827e58a14eeb Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Thu, 21 Aug 2025 15:50:15 +0100 Subject: [PATCH 5/9] Fix type annotations in test_lowlevel_exception_handling.py Added proper type hints for parametrized test function parameters to resolve pyright errors. --- tests/server/test_lowlevel_exception_handling.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/server/test_lowlevel_exception_handling.py b/tests/server/test_lowlevel_exception_handling.py index 7208e07ddc..6d367421c2 100644 --- a/tests/server/test_lowlevel_exception_handling.py +++ b/tests/server/test_lowlevel_exception_handling.py @@ -1,3 +1,4 @@ +from typing import Type from unittest.mock import AsyncMock, Mock import pytest @@ -34,7 +35,7 @@ async def test_exception_handling_with_raise_exceptions_true(): (Exception, "Basic error"), ], ) -async def test_exception_handling_with_raise_exceptions_false(exception_class, message): +async def test_exception_handling_with_raise_exceptions_false(exception_class: type[Exception], message: str): """Test that exceptions are logged when raise_exceptions=False""" server = Server("test-server") session = Mock(spec=ServerSession) From 904294e6c51e775fc58c8b13340f27359a109e2f Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Thu, 21 Aug 2025 15:52:20 +0100 Subject: [PATCH 6/9] Remove unused Type import Use modern type[Exception] annotation instead of deprecated typing.Type. --- tests/server/test_lowlevel_exception_handling.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/server/test_lowlevel_exception_handling.py b/tests/server/test_lowlevel_exception_handling.py index 6d367421c2..59fecc7825 100644 --- a/tests/server/test_lowlevel_exception_handling.py +++ b/tests/server/test_lowlevel_exception_handling.py @@ -1,4 +1,3 @@ -from typing import Type from unittest.mock import AsyncMock, Mock import pytest From 81335fbf4af12ad0977a2ed33a46d6a44611550a Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Mon, 8 Sep 2025 11:00:08 -0700 Subject: [PATCH 7/9] fix: avoid exposing server exception details to clients Send generic "Internal Server Error" message to clients instead of exposing exception types, values, and tracebacks. This prevents leaking sensitive implementation details while still logging full exception information server-side for debugging. --- src/mcp/server/lowlevel/server.py | 6 +----- tests/server/test_lowlevel_exception_handling.py | 4 +--- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/mcp/server/lowlevel/server.py b/src/mcp/server/lowlevel/server.py index 7625cd4cd1..7cd2562ba8 100644 --- a/src/mcp/server/lowlevel/server.py +++ b/src/mcp/server/lowlevel/server.py @@ -613,11 +613,7 @@ async def _handle_message( raise message await session.send_log_message( level="error", - data={ - "exception_type": type(message).__name__, - "exception_value": str(message), - "exception_traceback": None, - }, + data="Internal Server Error", logger="mcp.server.exception_handler", ) diff --git a/tests/server/test_lowlevel_exception_handling.py b/tests/server/test_lowlevel_exception_handling.py index 59fecc7825..29df0452dd 100644 --- a/tests/server/test_lowlevel_exception_handling.py +++ b/tests/server/test_lowlevel_exception_handling.py @@ -49,9 +49,7 @@ async def test_exception_handling_with_raise_exceptions_false(exception_class: t call_args = session.send_log_message.call_args assert call_args.kwargs["level"] == "error" - assert call_args.kwargs["data"]["exception_type"] == exception_class.__name__ - assert call_args.kwargs["data"]["exception_value"] == str(test_exception) - assert call_args.kwargs["data"]["exception_traceback"] is None + assert call_args.kwargs["data"] == "Internal Server Error" assert call_args.kwargs["logger"] == "mcp.server.exception_handler" From d6fba2b91ab4d1449df9f2412f9175eb1e4fb6dc Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 30 Sep 2025 14:08:14 +0100 Subject: [PATCH 8/9] fix: amend ordering to always send log message --- src/mcp/server/lowlevel/server.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mcp/server/lowlevel/server.py b/src/mcp/server/lowlevel/server.py index 4c89b512df..1a69cbe485 100644 --- a/src/mcp/server/lowlevel/server.py +++ b/src/mcp/server/lowlevel/server.py @@ -650,13 +650,13 @@ async def _handle_message( await self._handle_notification(notify) case Exception(): logger.error(f"Received exception from stream: {message}") - if raise_exceptions: - raise message await session.send_log_message( level="error", data="Internal Server Error", logger="mcp.server.exception_handler", ) + if raise_exceptions: + raise message for warning in w: logger.info("Warning: %s: %s", warning.category.__name__, warning.message) From 2fa2f6779b707bf5a7f86ca2d26e5efda4869c7a Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 30 Sep 2025 14:13:15 +0100 Subject: [PATCH 9/9] fix: update tests to reflect logging --- tests/server/test_lowlevel_exception_handling.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/server/test_lowlevel_exception_handling.py b/tests/server/test_lowlevel_exception_handling.py index 29df0452dd..5d4c3347f6 100644 --- a/tests/server/test_lowlevel_exception_handling.py +++ b/tests/server/test_lowlevel_exception_handling.py @@ -20,8 +20,7 @@ async def test_exception_handling_with_raise_exceptions_true(): with pytest.raises(RuntimeError, match="Test error"): await server._handle_message(test_exception, session, {}, raise_exceptions=True) - # Should not send log message when re-raising - session.send_log_message.assert_not_called() + session.send_log_message.assert_called_once() @pytest.mark.anyio