Skip to content

server: skip duplicate response on CancelledError #1153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/mcp/server/lowlevel/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,12 @@ async def _handle_request(
response = await handler(req)
except McpError as err:
response = err.error
except anyio.get_cancelled_exc_class():
logger.info(
"Request %s cancelled - duplicate response suppressed",
message.request_id,
)
return
except Exception as err:
if raise_exceptions:
raise err
Expand Down
186 changes: 186 additions & 0 deletions tests/server/test_cancel_handling.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
"""Test that cancelled requests don't cause double responses."""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of mocking happening. I'd suggest to use test_integration or leverage create_connected_server_and_client_session and test with CancelledNotification

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks for the directions!

import anyio
import pytest

import mcp.types as types
from mcp.server.lowlevel.server import Server
from mcp.shared.exceptions import McpError
from mcp.shared.memory import create_connected_server_and_client_session
from mcp.types import (
CallToolRequest,
CallToolRequestParams,
CallToolResult,
CancelledNotification,
CancelledNotificationParams,
ClientNotification,
ClientRequest,
Tool,
)


@pytest.mark.anyio
async def test_cancelled_request_no_double_response():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this test? it doesn't look like it's testing much and test_server_remains_functional_after_cancel looks complete to cover the fix?

"""Verify server handles cancelled requests without double response."""

# Create server with a slow tool
server = Server("test-server")

# Track when tool is called
ev_tool_called = anyio.Event()
request_id = None

@server.list_tools()
async def handle_list_tools() -> list[Tool]:
return [
Tool(
name="slow_tool",
description="A slow tool for testing cancellation",
inputSchema={},
)
]

@server.call_tool()
async def handle_call_tool(name: str, arguments: dict | None) -> list:
nonlocal request_id
if name == "slow_tool":
request_id = server.request_context.request_id
ev_tool_called.set()
await anyio.sleep(10) # Long running operation
return [types.TextContent(type="text", text="Tool called")]
raise ValueError(f"Unknown tool: {name}")

# Connect client to server
async with create_connected_server_and_client_session(server) as client:
# Start the slow tool call in a separate task
async def make_request():
try:
await client.send_request(
ClientRequest(
CallToolRequest(
method="tools/call",
params=CallToolRequestParams(name="slow_tool", arguments={}),
)
),
CallToolResult,
)
pytest.fail("Request should have been cancelled")
except McpError as e:
# Expected - request was cancelled
assert e.error.code == 0 # Request cancelled error code

# Start the request
request_task = anyio.create_task_group()
async with request_task:
request_task.start_soon(make_request)

# Wait for tool to start executing
await ev_tool_called.wait()

# Send cancellation notification
assert request_id is not None
await client.send_notification(
ClientNotification(
CancelledNotification(
method="notifications/cancelled",
params=CancelledNotificationParams(
requestId=request_id,
reason="Test cancellation",
),
)
)
)

# The request should be cancelled and raise McpError


@pytest.mark.anyio
async def test_server_remains_functional_after_cancel():
"""Verify server can handle new requests after a cancellation."""

server = Server("test-server")

# Track tool calls
call_count = 0
ev_first_call = anyio.Event()
first_request_id = None

@server.list_tools()
async def handle_list_tools() -> list[Tool]:
return [
Tool(
name="test_tool",
description="Tool for testing",
inputSchema={},
)
]

@server.call_tool()
async def handle_call_tool(name: str, arguments: dict | None) -> list:
nonlocal call_count, first_request_id
if name == "test_tool":
call_count += 1
if call_count == 1:
first_request_id = server.request_context.request_id
ev_first_call.set()
await anyio.sleep(5) # First call is slow
return [types.TextContent(type="text", text=f"Call number: {call_count}")]
raise ValueError(f"Unknown tool: {name}")

async with create_connected_server_and_client_session(server) as client:
# First request (will be cancelled)
async def first_request():
try:
await client.send_request(
ClientRequest(
CallToolRequest(
method="tools/call",
params=CallToolRequestParams(name="test_tool", arguments={}),
)
),
CallToolResult,
)
pytest.fail("First request should have been cancelled")
except McpError:
pass # Expected

# Start first request
async with anyio.create_task_group() as tg:
tg.start_soon(first_request)

# Wait for it to start
await ev_first_call.wait()

# Cancel it
assert first_request_id is not None
await client.send_notification(
ClientNotification(
CancelledNotification(
method="notifications/cancelled",
params=CancelledNotificationParams(
requestId=first_request_id,
reason="Testing server recovery",
),
)
)
)

# Second request (should work normally)
result = await client.send_request(
ClientRequest(
CallToolRequest(
method="tools/call",
params=CallToolRequestParams(name="test_tool", arguments={}),
)
),
CallToolResult,
)

# Verify second request completed successfully
assert len(result.content) == 1
# Type narrowing for pyright
content = result.content[0]
assert content.type == "text"
assert isinstance(content, types.TextContent)
assert content.text == "Call number: 2"
assert call_count == 2
Loading