fix(dispatcher): reject duplicate in-flight request ids#3064
Open
akminx wants to merge 1 commit into
Open
Conversation
The wire dispatcher registered each inbound request in `_in_flight` with a blind overwrite keyed by request id (the `TODO(maxisbey)` from modelcontextprotocol#3046). Two concurrent requests sharing a JSON-RPC id on one session would silently displace each other: the older entry was evicted at registration, so a `notifications/cancelled` for that id always targeted the newer request and the older one became uncancellable. Reject a duplicate id that is still in flight with INVALID_REQUEST instead of overwriting, matching the guard `direct_dispatcher` already applies to caller-supplied ids. Ids remain reusable once the earlier request completes, which deployed clients that send a constant id rely on. With duplicate registration ruled out, the completion path no longer needs the identity guard on its `_in_flight` pop. Replaces the two overwrite-semantics tests with three covering rejection, cancellation-targeting of the original request, and sequential id reuse. Fixes the dispatcher-layer half of modelcontextprotocol#3060; complementary to the transport-level guard in modelcontextprotocol#3063.
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
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.
Motivation and Context
Fixes the dispatcher-layer half of #3060, complementary to the transport-level guard in #3063.
JSONRPCDispatcher._dispatch_requestregistered every inbound request in_in_flightwith a blind overwrite keyed by request id — theTODO(maxisbey): duplicate ids blind-overwrite (v1/TS parity); revisit rejecting with INVALID_REQUESTadded in #3046. On currentmain, two concurrent requests carrying the same JSON-RPC id on one session silently displace each other:_in_flightentry is evicted at registration time, so anotifications/cancelledfor that id (which correlates against_in_flight) always targets the newest request — the older one becomes uncancellable the moment the duplicate arrives.entry.dctx is dctx) specifically to stop the older request's cleanup from evicting the newer entry.@Sammy-Dabbas's #3063 fixes the streamable-HTTP transport, where the slot overwrite happens at POST-handling time before the dispatcher is reached. This change fixes the dispatcher itself, which covers the other transports plus the cancellation-targeting case, as discussed on the issue.
The
direct_dispatcheralready rejects duplicate in-flight ids for caller-supplied ids (request id ... is already in flight); this makes the wire dispatcher consistent with it.How Has This Been Tested?
Rejection is checked before any
awaitin_dispatch_requestand the read loop dispatches messages sequentially, so a duplicate can never register between the guard and the write — no new race is introduced. Added three tests intests/shared/test_jsonrpc_dispatcher.py:test_duplicate_in_flight_request_id_is_rejected_with_invalid_request— a duplicate id while the first is outstanding getsINVALID_REQUEST; its handler never runs; the original still completes.test_duplicate_id_rejection_leaves_original_request_cancellable— after the duplicate is rejected,notifications/cancelledstill targets the original, still-running request (the cancellation-targeting regression).test_request_id_is_reusable_after_the_earlier_request_completes— sequential reuse of an id after completion is still accepted.Full suite passes (5288 passed) at 100.00% branch coverage;
pyrightandruff check/ruff formatare clean on the touched files.Implementation notes
entry.dctx is dctx) is now unreachable dead code, so it is simplified to a plain_in_flight.pop(key, None). The two tests that exercised the old overwrite/identity-guard behavior are replaced by the rejection tests above.TODO(maxisbey)reserved for "v1/TS parity." That parity now points the other way: fix: catch PydanticUserError when generating output schema (pydantic 2.13 compat) #2434 flips the TypeScript server to reject as well, so rejecting here keeps the two SDKs aligned. Happy to adjust the choice (or hold this) if you'd rather fold it into the cancellation work — the tests stand on their own either way.Breaking Changes
Behavior change only for a protocol violation: a second request reusing an id that is still in flight now receives an
INVALID_REQUESTerror instead of silently overwriting the first request's routing. Sequential id reuse after completion is unchanged.Types of changes
Checklist