fix(server): reject duplicate in-flight request ids, v1.x backport of #2434#2474
Draft
Sammy-Dabbas wants to merge 1 commit into
Draft
fix(server): reject duplicate in-flight request ids, v1.x backport of #2434#2474Sammy-Dabbas wants to merge 1 commit into
Sammy-Dabbas wants to merge 1 commit into
Conversation
Backport of modelcontextprotocol#2434 to the v1.x web-standard transport. The transport registered every request via _requestToStreamMapping.set(message.id, streamId) with no duplicate check, so a concurrent POST reusing an in-flight id overwrote the entry, cross-wired the first request's response onto the second POST's stream, and left the first POST hanging. Reject a POST containing a request whose id is already in flight, or duplicated within the same batch, with HTTP 400 and JSON-RPC -32600. Sequential reuse after completion stays allowed since deployed clients send a constant id for every request. Cancelled requests never produce a response, so notifications/cancelled now retires the transport bookkeeping for its target id; without that, a cancelled id would stay in flight forever and lock out cancel-then-reuse clients. Adds the missing isCancelledNotification guard to types. Fixes modelcontextprotocol#2433.
🦋 Changeset detectedLatest commit: 21cd552 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
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.
Backport of #2434 to the v1.x web-standard Streamable HTTP transport, fixing #2433 on the 1.x line.
Motivation and Context
WebStandardStreamableHTTPServerTransport registered every request id in _requestToStreamMapping with no duplicate check, so a second POST reusing an in flight id overwrote the entry, cross-wired the first request's response onto the second POST's stream, and left the first POST hanging. Deployed clients that send a constant id per request hit this under concurrency.
How Has This Been Tested?
Added regression tests for concurrent id collision, duplicate ids within one batch, sequential reuse after completion, cancel-then-reuse, and JSON response mode. The unfixed transport returns 200 with the cross-wired body while the first POST hangs; with the guard the duplicate is rejected with 400 and the first POST completes with its own result. The full transport test file and typecheck passes.
Breaking Changes
None. A duplicate in flight id now returns 400 with JSON-RPC -32600 instead of silently mis-delivering a response. Sequential reuse after completion and all valid traffic are unchanged.
Types of changes
Checklist
Additional context
Same approach as #2434, adapted to the v1.x layout in src/server/webStandardStreamableHttp.ts, with an isCancelledNotification guard added to src/types.ts so cancelled ids stay reusable.