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

Conversation

lukacf
Copy link

@lukacf lukacf commented Jul 16, 2025

When clients cancel long-running tool calls, the server crashes with an assertion error and becomes unresponsive. This PR fixes the issue by preventing duplicate responses.

Problem

  • RequestResponder.cancel() sends an error response when cancelled
  • _handle_request tries to send another error response
  • This causes "Request already responded to" assertion
  • Server becomes completely unresponsive

Solution

Catch CancelledError in _handle_request and return early without sending a second response.

Testing

Added test that cancels a tool call and verifies the server remains responsive.

Fixes #1152
Related: jlowin/fastmcp#508, jlowin/fastmcp#823

lukacf added 6 commits July 16, 2025 12:31
When a tool call is cancelled, RequestResponder.cancel() sends an error
response. Previously, _handle_request would try to send another error
response, causing "Request already responded to" assertion.

This fix catches CancelledError and returns early, preventing the
duplicate response.

Fixes modelcontextprotocol#1152
@lukacf lukacf requested a review from a team as a code owner July 18, 2025 18:58
@lukacf lukacf requested a review from felixweinberger July 18, 2025 18:58
Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

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

Thanks for woking on the fix! Left some comments

@@ -0,0 +1,138 @@
"""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!

- Use anyio.get_cancelled_exc_class() instead of asyncio.CancelledError
  for proper backend compatibility (supports both asyncio and trio)
- Rewrote cancel handling tests to use integration testing pattern with
  create_connected_server_and_client_session instead of mocking
- Tests now properly send CancelledNotification messages through real
  client-server communication
- Added proper type annotations and fixed type issues in tests

Addresses review feedback about using anyio idioms and integration testing.

Reported-by: @ihrpr
@lukacf lukacf force-pushed the fix-cancelled-error-response branch from 89075b0 to a5a112a Compare July 21, 2025 19:37
@lukacf lukacf requested a review from ihrpr July 21, 2025 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CancelledError causes duplicate response and server crash
2 participants