Repository navigation
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. |
Contributor
Author
|
The Test, Conformance Tests, and CodeQL workflows currently report The local full Go test suite and targeted regression tests with the race detector passed, as noted in the PR description. Upstream CI, including the conformance suite, still needs to run. |
This was referenced Oct 12, 2026
guglielmo-san
added a commit
that referenced
this pull request
Oct 13, 2026
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 `CancelCause` directly from `canceller.Preempt`, binding cancellation to the active invocation before the read loop advances. `CancelCause` captures 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: - After removing the dedicated test: `go test -race ./mcp -run TestCancellation -count=1 -timeout=180s` and `git diff --check` passed. - The unchanged production fix previously passed `go test ./... -timeout=10m`, cancellation race tests, `go vet ./...`, and `go build ./...`. - Separate protocol conformance scripts were not run. 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. --------- Co-authored-by: Guglielmo Colombo <guglielmoc@google.com>
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
1 participant
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.
JSON-RPC numeric ID
9007199254740993is decoded as9007199254740992. A successful server response therefore echoes a different ID, and two distinct incoming IDs can become the same internal map key. This reproduces on v1.8.0 and on main at 944ac19.This change decodes envelope IDs through
json.Numberand converts them exactly to the existing int64 representation. Integral decimal/exponent forms remain supported using an exact rational conversion, with a magnitude check before allocating a rational. Fractional and out-of-int64-range wire IDs return an error instead of being truncated or overflowing.Cancellation request IDs also decode as
json.Number, so cancellation matches the preserved request ID. Metadata decoding remains unchanged. The exportedMakeIDfunction acceptsjson.Number; its existing float64 path is unchanged and cannot recover precision already lost by callers. NumericCancelledParams.RequestIDvalues decoded from JSON now have dynamic typejson.Number, which is documented.Tests cover request/response round trips above 2^53, signed int64 boundaries, integral decimal/exponent notation, invalid numeric IDs, and cancellation parameter decoding.
Validation:
go test ./...passed.-racepassed.go build ./...andgit diff --checkpassed.9007199254740993, and confirmed adjacent IDs remain distinct. That external harness is not included in this diff.No module dependencies are added. The numeric ID representation remains limited to int64; arbitrary-precision IDs are outside this patch's scope.
Fixes #1376.