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. |
|
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. |
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>
|
@skosovsky thank you for the contribution, I decided to fix this with an easier approach in #1401 |
|
@guglielmo-san Thanks for the simpler approach in #1401. Following up on your comment above, I opened #1405 for the remaining precision loss in integer-valued decimal/exponent IDs, such as |
2 participants
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.