Skip to content

Commit c3f28ea

Browse files
committed
fix: log notification errors instead of forwarding raw exceptions
For notification connection errors, log a warning and continue rather than forwarding a raw Exception to the read stream (where it would be silently ignored by _handle_incoming anyway). This keeps the post_writer loop alive and achieves 100% coverage. Also move contextlib.suppress inside the request branch — we only need to suppress errors from the JSONRPCError construction/send, not from the isinstance check itself.
1 parent 78525ff commit c3f28ea

File tree

2 files changed

+58
-4
lines changed

2 files changed

+58
-4
lines changed

src/mcp/client/streamable_http.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -475,8 +475,8 @@ async def handle_request_async() -> None:
475475
except anyio.get_cancelled_exc_class():
476476
raise
477477
except Exception as exc:
478-
with contextlib.suppress(Exception):
479-
if isinstance(message, JSONRPCRequest):
478+
if isinstance(message, JSONRPCRequest):
479+
with contextlib.suppress(Exception):
480480
error_data = ErrorData(
481481
code=INTERNAL_ERROR,
482482
message=str(exc) or "Connection error",
@@ -485,8 +485,8 @@ async def handle_request_async() -> None:
485485
JSONRPCError(jsonrpc="2.0", id=message.id, error=error_data)
486486
)
487487
await read_stream_writer.send(error_msg)
488-
else:
489-
await read_stream_writer.send(exc)
488+
else:
489+
logger.warning(f"Failed to send notification: {exc}")
490490

491491
# If this is a request, start a new task to handle it
492492
if isinstance(message, JSONRPCRequest):

tests/shared/test_streamable_http.py

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@
5757
CallToolResult,
5858
InitializeResult,
5959
JSONRPCError,
60+
JSONRPCNotification,
6061
JSONRPCRequest,
6162
ListToolsResult,
6263
PaginatedRequestParams,
@@ -2361,3 +2362,56 @@ async def test_connection_error_forwarded_to_read_stream():
23612362
assert isinstance(result, SessionMessage)
23622363
assert isinstance(result.message, JSONRPCError)
23632364
assert result.message.id == "init-err"
2365+
2366+
2367+
@pytest.mark.anyio
2368+
async def test_notification_connection_error_logged_not_forwarded():
2369+
"""Test that notification connection errors are logged and do not crash
2370+
the post_writer loop.
2371+
2372+
Unlike request errors (which must be forwarded as JSONRPCError to unblock
2373+
send_request), notification errors are simply logged since nothing waits
2374+
for a notification response.
2375+
"""
2376+
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
2377+
s.bind(("127.0.0.1", 0))
2378+
_, port = s.getsockname()
2379+
2380+
async with streamable_http_client(f"http://127.0.0.1:{port}/mcp") as (
2381+
read_stream,
2382+
write_stream,
2383+
):
2384+
async with read_stream, write_stream:
2385+
# Send a notification — this will fail to connect but should
2386+
# not crash the post_writer
2387+
notification = SessionMessage(
2388+
JSONRPCNotification(
2389+
jsonrpc="2.0",
2390+
method="notifications/initialized",
2391+
)
2392+
)
2393+
await write_stream.send(notification)
2394+
2395+
# Send a request after the notification to verify post_writer
2396+
# is still alive
2397+
request = SessionMessage(
2398+
JSONRPCRequest(
2399+
jsonrpc="2.0",
2400+
id="after-notif",
2401+
method="initialize",
2402+
params={
2403+
"protocolVersion": types.LATEST_PROTOCOL_VERSION,
2404+
"capabilities": {},
2405+
"clientInfo": {"name": "test", "version": "1.0"},
2406+
},
2407+
)
2408+
)
2409+
await write_stream.send(request)
2410+
2411+
# The request error should arrive (proving post_writer survived
2412+
# the notification error)
2413+
with anyio.fail_after(5):
2414+
result = await read_stream.receive()
2415+
assert isinstance(result, SessionMessage)
2416+
assert isinstance(result.message, JSONRPCError)
2417+
assert result.message.id == "after-notif"

0 commit comments

Comments
 (0)