fix: auto-send notifications/cancelled on anyio task cancellation#2479
Open
cubaseuser123 wants to merge 1 commit intomodelcontextprotocol:mainfrom
Open
fix: auto-send notifications/cancelled on anyio task cancellation#2479cubaseuser123 wants to merge 1 commit intomodelcontextprotocol:mainfrom
cubaseuser123 wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
Closes modelcontextprotocol#1410. Users of high-level APIs like client.call_tool() have no access to the internal equest_id, making it impossible to manually send a cancel notification when a task is aborted. The server would keep processing a request that nobody is waiting for. This fix intercepts �nyio task/scope cancellation inside send_request and automatically dispatches otifications/cancelled to the peer before re-raising. The send is shielded from the cancellation scope and guarded by a 2-second timeout to prevent deadlock if the transport buffer is full. A equest_sent flag ensures we only notify the server if the request actually hit the wire — avoiding spurious cancels for requests that were never received. This mirrors the pattern used by the TypeScript SDK with AbortSignal and applies to both client→server and server→client request directions since the change lives in BaseSession.
8f4ea8c to
ed89a6a
Compare
|
The |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1410.
Motivation and Context
The friction here is pretty specific: when you use high-level request wrappers like
client.call_tool(), the generatedrequest_idstays hidden internally insidesend_request. So if you need to abort that task from the outside using standard Python async patterns (likeanyio.fail_afterorCancelScope.cancel()), the client aborts locally, but it has no way to tell the server. The server continues executing an orphaned request that the client has already stopped waiting for.So I moved the cancellation hook into
BaseSession.send_request. The idea is simple: intercept theanyiocancellation before it bubbles up, read therequest_idwe just generated, and automatically dispatch anotifications/cancelledstraight to the peer.How Has This Been Tested?
I made sure it runs cleanly end-to-end. Added
test_anyio_cancel_scope_sends_cancelled_notificationtotests/server/test_cancel_handling.pywhich:call_toolCancelScope.cancel()(Side note: Verified locally against the full regression suite using
anyio4.10.0 on Windows.)Breaking Changes
None.
Types of changes
Checklist
Additional context
A few quick implementation details:
request_sentflag: Cancels are only dispatched if the request actually hit the wire (so we don't spam the server for requests that failed on_write_stream.send()).with anyio.CancelScope(shield=True)so it survives the very cancellation it's reacting to.with anyio.move_on_after(2.0)inside the shield. If the transport buffer is completely full, we don't want to hang the user's cancellation handler forever.BaseSession, it automatically works for both client→server and server→client directions.Happy to tweak the timeout value or the notification reason string if there's a preference. Let me know if I should squash anything.
Just a heads-up for reviewers — the CI failures here are pre-existing and not related to the changes in this PR.
The
pyrighthook in.pre-commit-config.yamlruns against the whole repo. Currently, the upstreammainbranch (commit3d7b311) has 21 Pyright errors located entirely insrc/mcp/os/win32/utilities.py.Because pre-commit is configured with
fail_fast: true, it aborts early and cascades all 20 test matrix jobs (checks / test) to fail with exit code 2 before Pytest even gets a chance to run. (I noticed this is happening on other recent PRs like #2475 as well).I have verified the test suite locally using parallel workers (
pytest -n auto):uv run pyright src/mcp/shared/session.pyreturns 0 errors.Happy to rebase to trigger a clean run once the upstream Pyright issue on
mainis resolved, or if a maintainer wants to merge this as-is!