Repository navigation
Fix dropped notifications/cancelled when a request is canceled - #1917
Open
zerotohero3 wants to merge 1 commit into
Open
zerotohero3 wants to merge 1 commit into
zerotohero3 wants to merge 1 commit into
Conversation
Canceling the CancellationToken passed to a request never sent notifications/cancelled to the peer, so remote handlers kept running. SendRequestAsync registered the callback that sends the notification and then awaited tcs.Task.WaitAsync(cancellationToken) inside a using for that registration. Cancellation callbacks run in reverse order of registration, so WaitAsync's callback ran first, its continuation ran synchronously as part of Cancel(), and the using disposed the notification registration before its callback had run. Start the wait before registering so the notification callback runs first. Separately, ProcessMessagesCoreAsync tracked every incoming message with an ID in _handlingRequests, including responses to our own requests, and removed entries by key only. Since each side allocates request IDs independently, a response could replace and then remove the entry of an in-flight request with the same ID, causing a later notifications/cancelled for that request to be ignored. Track only incoming requests, and only remove the entry a message registered. Refs modelcontextprotocol#1365
This branch has not been deployed
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.
Refs #1365. This fixes the stdio, stream and legacy SSE part. The Streamable HTTP part is not addressed here (see Scope).
Motivation and Context
Canceling the
CancellationTokenpassed to a request (for exampleCallToolAsync) is supposed to sendnotifications/cancelledto the peer so the remote handler gets canceled. Today that notification is never sent, and a second bug can make the receiver ignore it even when it does arrive.1. The cancellation callback is unregistered before it runs
SendRequestAsyncregisters the callback that sendsnotifications/cancelled, then awaitstcs.Task.WaitAsync(cancellationToken)inside ausingfor that registration.WaitAsyncregisters on the same token afterwards, so when the token is canceled:awaitcontinuation runs synchronously as part ofCancel().usingblock and disposes the notification registration before its callback has run.The fix starts the wait before registering the notification callback, so that callback runs first.
2. Incoming responses clobber
_handlingRequestsentriesProcessMessagesCoreAsyncputs every incomingJsonRpcMessageWithIdinto_handlingRequests, including responses to our own requests, and removes entries by key only. Request ids are allocated independently by each side, both starting at 1, so collisions are expected. For example, on a pre-MRTR protocol version a tool that callsElicitAsyncgets a response whose id can match the in-flighttools/call. That response replaces thetools/call's entry and then removes it, so a laternotifications/cancelledfor thetools/callis ignored.The fix:
The compare-and-remove uses the
ICollection<KeyValuePair<,>>.Removeidiom already used inMcpServerPrimitiveCollection.How Has This Been Tested?
I added these tests to
CancellationTests:CancellationPropagation_CancelingRequest_CancelsServerHandlerruns on the default and2025-11-25protocol versions. It asserts that the server-side tool's token is canceled after the client cancels.CancellationPropagation_ResponseWithSameIdAsInFlightRequest_DoesNotPreventCancellationcovers a tool that elicits on2025-11-25and is then canceled, with a colliding request id.Before and after:
main; the tool is never canceled.The existing tests didn't catch this because they only assert that the client observes
OperationCanceledException, whichWaitAsyncproduces either way.I ran the full suites on net10.0 against both
mainand this branch, and they show the same set of failures:ModelContextProtocol.Tests: 2,368 passed. The 26 failures are identical on both, all inClientIntegrationTests, which needsnpx; my environment doesn't have it.ModelContextProtocol.AspNetCore.Tests(excluding theOAuthnamespace): 467 passed, 0 failed. TheOAuthtests hang in my local container onmainand on this branch alike.ModelContextProtocol.Corebuilds with no warnings for netstandard2.0, net8.0, net9.0 and net10.0. The new tests passed 10 consecutive runs.Scope
For Streamable HTTP, the transport's
SendMessageAsyncdoesn't return until the POST's response is read, soRegisterCancellationis never reached while a request is in flight. Canceling the token aborts the POST instead:Fixing that needs a design decision (see #1377), so it is left out of this PR. #1377 also adds an
await Task.Delay(500)before the registration is disposed to give the notification time to go out. This PR fixes the underlying ordering issue, so that delay isn't needed.An alternative to the reordering in fix 1 would avoid relying on callback order. The registration's callback would complete
tcsitself, which is safe because it usesRunContinuationsAsynchronously, andWaitAsyncwould be dropped. I went with the smaller change, but I'm happy to switch.Breaking Changes
None. The behavior change is that remote handlers now actually get canceled when the caller cancels, which is what the API already implies.
Types of changes
Checklist