Repository navigation
mcp: bind peer cancellation before request ID reuse - #1390
Merged
guglielmo-san merged 3 commits intoOct 13, 2026
Merged
guglielmo-san merged 3 commits into
guglielmo-san merged 3 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Comment on lines
+1
to
+6
| // Copyright 2026 The Go MCP SDK Authors. All rights reserved. | ||
| // Use of this source code is governed by the license | ||
| // that can be found in the LICENSE file. | ||
|
|
||
| package mcp | ||
|
|
Contributor
There was a problem hiding this comment.
considering that this error is very niche, can you please discard this test and just leave the code change?
Contributor
Author
There was a problem hiding this comment.
Done — removed cancellation_reuse_test.go from this PR and kept only the production change in mcp/transport.go. The additional regression harness remains in our downstream integration branch. Existing cancellation tests pass with -race after the removal.
guglielmo-san
approved these changes
Oct 13, 2026
Contributor
|
@skosovsky thank you for the contribution! |
guglielmo-san
added a commit
that referenced
this pull request
Oct 20, 2026
…sions (#1267) The cancellation utility of 2026-07-28 says that a server receiving `notifications/cancelled` SHOULD "Not send a response for the cancelled request". On that revision stdio is the transport that carries the notification: over streamable HTTP, closing the response stream is the cancellation and no notification is expected (see `StreamableHTTPOptions.PropagateRequestCancellation`). This SDK answered the cancelled request anyway, because `processResult` in `internal/jsonrpc2/conn.go` writes the response with `notDone{req.ctx}` and no application code runs in between. This change drops that response on the stdio-style transports (`StdioTransport`, `CommandTransport`, `IOTransport` and `InMemoryTransport`) for sessions on 2026-07-28 or later. Sessions on earlier versions, the SSE transport and the streamable transport keep answering a cancelled request, which is why this updates #1259 rather than fixing it. - `internal/jsonrpc2`: `Connection.CancelFromPeer` is `CancelCause` for a cancellation the peer asked for. Before it cancels the call's context, it tells a writer that implements the new optional `ResponseDropper` interface which call's response to drop. It does so under the connection's lock, in the critical section that finds the request, and `processResult` removes the request under the same lock before it writes the response, so the writer is told either before it is handed the response or not at all; `ResponseDropper` documents that `DropResponse` runs with the lock held. The connection still writes the response once the handler returns, and dropping it is the writer's decision. `Cancel` and `CancelCause` keep their meaning, so the listens `ServerSession.Close` cancels still get their results. A `subscriptions/listen` that the client cancels with `notifications/cancelled` is treated like any other cancelled request: on these sessions it no longer gets its `SubscriptionsListenResult`. - `mcp`: `canceller.Preempt` calls `CancelFromPeer` where #1390 calls `CancelCause`, directly rather than in a goroutine. `ioConn` implements `DropResponse` and discards the response to a dropped call when the negotiated version is 2026-07-28 or later. `loggingConn` forwards `DropResponse` and `sessionUpdated` to its delegate. The jsonrpc2 connection asks its writer for `ResponseDropper`, and `ServerSession` asks its connection for `sessionUpdated`, which is how `ioConn` learns the version `DropResponse` checks. Without the forwarders, a `LoggingTransport` would hide both from `ioConn`. That also gives a server-side `LoggingTransport` the session updates #1328 reports missing; the client side of #1328 is not covered. - `docs/protocol.md` says which transports and versions send no response. Tests: `TestCancelFromPeer` in `internal/jsonrpc2`, for a cancellation from the peer and a local `Cancel`; `TestIOConnDropResponse`, `TestIOConnCancelledCallGetsNoResponse` (end to end over an in-memory transport wrapped in `LoggingTransport`) and `TestLoggingConnDropResponse` in `mcp`. Updates #1259. --------- Co-authored-by: Guglielmo Colombo <guglielmoc@google.com>
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 peer cancellation is currently scheduled in a goroutine before looking up its request. If the old invocation finishes and the read loop admits a replacement with the same ID first, that lookup can cancel the replacement.
Call
CancelCausedirectly fromcanceller.Preempt, binding cancellation to the active invocation before the read loop advances.CancelCausecaptures the request under the existing connection lock and cancels its context; it does not wait for handler completion or response delivery. The peer's cancellation reason is preserved. No public API, numeric-ID behavior, or response-suppression behavior changes.This PR changes only
mcp/transport.go. The dedicated cancellation-reuse test harness is retained downstream.Validation on Go 1.27.1, darwin/arm64:
go test -race ./mcp -run TestCancellation -count=1 -timeout=180sandgit diff --checkpassed.go test ./... -timeout=10m, cancellation race tests,go vet ./..., andgo build ./....Related: #1267 changes this same call to
CancelFromPeer; if that lands first, the invocation lookup should still happen before returning from preemption. This fix is independent of #1375 and contains no numeric decoding or transport hardening changes.Fixes #1389.