Repository navigation
mcp: keep a delivered response when the caller's context is cancelled - #1370
Merged
guglielmo-san merged 3 commits intoOct 13, 2026
Conversation
Comment on lines
+304
to
+309
| case err == nil: | ||
| // The response arrived and decoded. It is returned even if ctx has | ||
| // been cancelled in the meantime: the request is complete, so there | ||
| // is nothing to cancel, and the spec permits notifications/cancelled | ||
| // only for requests believed to still be in progress. | ||
| return nil |
Contributor
There was a problem hiding this comment.
this is the only change i would keep. This PR is aiming to fix a very niche issue and I would prefer to keep the code less convoluted
Contributor
Author
There was a problem hiding this comment.
Thanks, done. The PR now only adds the case err == nil branch in call, plus the regression test. The Await change and its tests are gone. The test still fails on main and passes with the branch, so the one case covers it.
vbcherepanov
force-pushed
the
fix/call-cancel-delivered-response
branch
2 times, most recently
from
October 12, 2026 06:19
1e49108 to
9fc4206
Compare
call checked ctx.Err() before it checked whether Await had succeeded, so a context cancelled while the result was being decoded discarded the result and sent notifications/cancelled for a completed request. Return the result when Await succeeds; a request still in flight is retired and notified as before. Fixes modelcontextprotocol#1368
vbcherepanov
force-pushed
the
fix/call-cancel-delivered-response
branch
from
October 12, 2026 12:02
9fc4206 to
ba70c87
Compare
Comment on lines
+370
to
+377
| // cancelAfterResponseTransport wraps a Transport so that a test can cancel a | ||
| // context at a point the client cannot observe from the outside: the first | ||
| // Read after the response to the given method has been delivered. The | ||
| // jsonrpc2 read loop dispatches a message before it reads the next one, so | ||
| // by then the call has been retired with its result while the caller may | ||
| // still be decoding it. The transport also records the methods written to | ||
| // it, so a test can check that no cancelled notification followed. | ||
| type cancelAfterResponseTransport struct { |
Contributor
There was a problem hiding this comment.
i think we can also drop entirely this test and leave the code change
Contributor
Author
There was a problem hiding this comment.
Done, the test is removed. The PR is now only the case err == nil branch in call.
guglielmo-san
approved these changes
Oct 13, 2026
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
2 participants
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.
A client call could return
context.Canceledfor a request whose response had already arrived.callinmcp/transport.gocheckedctx.Err()before it checked whetherAwaithad succeeded, so a context that was cancelled while the result was being decoded discarded the result, retired the call and sentnotifications/cancelledfor a request that was already complete.This change adds a
case err == nilat the top of the switch incall, so a successfulAwaitreturns the result. Everything else is unchanged: a request that is genuinely in flight is retired eagerly and the notification is sent off the caller's return path, as introduced for #1150.The specification permits cancellation notifications only for requests "believed to still be in-progress" (2025-11-25 and 2026-07-28, Cancellation, Behavior Requirements).
Fixes #1368
Tested with
go test -race ./...,go vetand staticcheck v0.6.1. Following review, the PR has no test of its own: the race is hard to reproduce without a custom transport.