Repository navigation
mcp: bind peer cancellation before request ID reuse - #1390
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. |
| // 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 | ||
|
|
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?
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.
|
@skosovsky thank you for the contribution! |
2 participants
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.