Skip to content

Commit e9ebe4f

Browse files
author
dragogargo
committed
fix: send notifications/cancelled on request timeout and caller cancellation
ClientSession.send_request() never emitted a notifications/cancelled message when a request was interrupted, either by the SDK's own timeout or by the caller's cancellation. This violated the MCP spec and caused server-side tool coroutines to leak — they remained suspended holding resources (DB connections, locks, file handles) until the session ended. Handle both cancellation paths: - TimeoutError from anyio.fail_after: send notification before raising - External CancelledError from caller: catch, send notification, re-raise The notification is sent inside a shielded cancel scope to guarantee delivery even when called from a cancelled task. The server's existing CancelledNotification handler already cancels in-flight requests on receipt, so no server-side changes are needed. Fixes #2507
1 parent 3d7b311 commit e9ebe4f

2 files changed

Lines changed: 119 additions & 0 deletions

File tree

src/mcp/shared/session.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
INVALID_PARAMS,
2424
REQUEST_TIMEOUT,
2525
CancelledNotification,
26+
CancelledNotificationParams,
2627
ClientNotification,
2728
ClientRequest,
2829
ClientResult,
@@ -292,9 +293,13 @@ async def send_request(
292293
with anyio.fail_after(timeout):
293294
response_or_error = await response_stream_reader.receive()
294295
except TimeoutError:
296+
await self._send_cancelled_notification(request_id, "request timed out")
295297
class_name = request.__class__.__name__
296298
message = f"Timed out while waiting for response to {class_name}. Waited {timeout} seconds."
297299
raise MCPError(code=REQUEST_TIMEOUT, message=message)
300+
except anyio.get_cancelled_exc_class():
301+
await self._send_cancelled_notification(request_id, "request cancelled by caller")
302+
raise
298303

299304
if isinstance(response_or_error, JSONRPCError):
300305
raise MCPError.from_jsonrpc_error(response_or_error)
@@ -540,6 +545,26 @@ async def send_progress_notification(
540545
) -> None:
541546
"""Sends a progress notification for a request that is currently being processed."""
542547

548+
async def _send_cancelled_notification(self, request_id: RequestId, reason: str) -> None:
549+
"""Send a cancellation notification to the remote side (best-effort).
550+
551+
Uses a shielded cancel scope so the notification is delivered even
552+
when called from inside a cancelled task.
553+
"""
554+
try:
555+
with anyio.CancelScope(shield=True):
556+
notification = CancelledNotification(
557+
method="notifications/cancelled",
558+
params=CancelledNotificationParams(request_id=request_id, reason=reason),
559+
)
560+
jsonrpc_notification = JSONRPCNotification(
561+
jsonrpc="2.0",
562+
**notification.model_dump(by_alias=True, mode="json", exclude_none=True),
563+
)
564+
await self._write_stream.send(SessionMessage(message=jsonrpc_notification))
565+
except Exception:
566+
logging.warning("Failed to send cancellation notification for request %s", request_id)
567+
543568
async def _handle_incoming(
544569
self, req: RequestResponder[ReceiveRequestT, SendResultT] | ReceiveNotificationT | Exception
545570
) -> None:

tests/shared/test_session.py

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,3 +416,97 @@ async def make_request(client_session: ClientSession):
416416
# Pending request completed successfully
417417
assert len(result_holder) == 1
418418
assert isinstance(result_holder[0], EmptyResult)
419+
420+
421+
@pytest.mark.anyio
422+
async def test_send_request_sends_cancelled_notification_on_timeout():
423+
"""Client must send notifications/cancelled when a request times out.
424+
425+
Regression test for https://github.com/modelcontextprotocol/python-sdk/issues/2507.
426+
"""
427+
cancelled_notifications: list[SessionMessage] = []
428+
429+
async with create_client_server_memory_streams() as (client_streams, server_streams):
430+
client_read, client_write = client_streams
431+
server_read, server_write = server_streams
432+
433+
async def mock_server():
434+
async for message in server_read:
435+
assert isinstance(message, SessionMessage)
436+
if isinstance(message.message, JSONRPCRequest):
437+
pass
438+
elif isinstance(message.message, types.JSONRPCNotification):
439+
cancelled_notifications.append(message)
440+
441+
async with (
442+
anyio.create_task_group() as tg,
443+
ClientSession(
444+
read_stream=client_read,
445+
write_stream=client_write,
446+
read_timeout_seconds=0.1,
447+
) as client_session,
448+
):
449+
tg.start_soon(mock_server)
450+
451+
with pytest.raises(MCPError, match="Timed out"):
452+
await client_session.send_ping()
453+
454+
await anyio.sleep(0.05)
455+
tg.cancel_scope.cancel()
456+
457+
assert len(cancelled_notifications) == 1
458+
notif = cancelled_notifications[0].message
459+
assert isinstance(notif, types.JSONRPCNotification)
460+
assert notif.method == "notifications/cancelled"
461+
assert notif.params is not None
462+
assert notif.params.get("reason") == "request timed out"
463+
464+
465+
@pytest.mark.anyio
466+
async def test_send_request_sends_cancelled_notification_on_caller_cancel():
467+
"""Client must send notifications/cancelled when caller cancels the request.
468+
469+
Regression test for https://github.com/modelcontextprotocol/python-sdk/issues/2507.
470+
"""
471+
cancelled_notifications: list[SessionMessage] = []
472+
ev_request_sent = anyio.Event()
473+
474+
async with create_client_server_memory_streams() as (client_streams, server_streams):
475+
client_read, client_write = client_streams
476+
server_read, server_write = server_streams
477+
478+
async def mock_server():
479+
async for message in server_read:
480+
assert isinstance(message, SessionMessage)
481+
if isinstance(message.message, JSONRPCRequest):
482+
ev_request_sent.set()
483+
elif isinstance(message.message, types.JSONRPCNotification):
484+
cancelled_notifications.append(message)
485+
486+
async def make_request(client_session: ClientSession):
487+
await client_session.send_ping()
488+
489+
async with (
490+
anyio.create_task_group() as tg,
491+
ClientSession(
492+
read_stream=client_read,
493+
write_stream=client_write,
494+
) as client_session,
495+
):
496+
tg.start_soon(mock_server)
497+
498+
async with anyio.create_task_group() as request_tg:
499+
request_tg.start_soon(make_request, client_session)
500+
with anyio.fail_after(1):
501+
await ev_request_sent.wait()
502+
request_tg.cancel_scope.cancel()
503+
504+
await anyio.sleep(0.05)
505+
tg.cancel_scope.cancel()
506+
507+
assert len(cancelled_notifications) == 1
508+
notif = cancelled_notifications[0].message
509+
assert isinstance(notif, types.JSONRPCNotification)
510+
assert notif.method == "notifications/cancelled"
511+
assert notif.params is not None
512+
assert notif.params.get("reason") == "request cancelled by caller"

0 commit comments

Comments
 (0)