Title: fix(dispatcher): reject duplicate in-flight request ids by akminx · Pull Request #3064 · modelcontextprotocol/python-sdk · GitHub
Open Graph Title: fix(dispatcher): reject duplicate in-flight request ids by akminx · Pull Request #3064 · modelcontextprotocol/python-sdk
X Title: fix(dispatcher): reject duplicate in-flight request ids by akminx · Pull Request #3064 · modelcontextprotocol/python-sdk
Description: Opening this as the concrete artifact of the offer I made on #3060. It's ready and green, but I'm putting it up for your convenience, not to jump the queue — happy to close it immediately if you'd rather own this or fold it into the cancellation work. No hard feelings either way. Disclosure: this change was developed with AI assistance. I reviewed the code and the test results, understand the change, and can speak to it. Motivation and Context Fixes the dispatcher-layer half of #3060, complementary to the transport-level guard in #3063. JSONRPCDispatcher._dispatch_request registered every inbound request in _in_flight with a blind overwrite keyed by request id — the TODO(maxisbey): duplicate ids blind-overwrite (v1/TS parity); revisit rejecting with INVALID_REQUEST added in #3046. On current main, two concurrent requests carrying the same JSON-RPC id on one session silently displace each other: The older request's _in_flight entry is evicted at registration time, so a notifications/cancelled for that id (which correlates against _in_flight) always targets the newest request — the older one becomes uncancellable the moment the duplicate arrives. The completion path carried an identity guard (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_dispatcher already 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 await in _dispatch_request and 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 in tests/shared/test_jsonrpc_dispatcher.py: test_duplicate_in_flight_request_id_is_rejected_with_invalid_request — a duplicate id while the first is outstanding gets INVALID_REQUEST; its handler never runs; the original still completes. test_duplicate_id_rejection_leaves_original_request_cancellable — after the duplicate is rejected, notifications/cancelled still 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; pyright and ruff check/ruff format are clean on the touched files. Implementation notes The reject guard reuses the coerced key for the later registration, so a stringified cancellation id still correlates. With duplicate registration ruled out, the completion path's identity-guarded pop (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. This flips the behavior the TODO(maxisbey) reserved for "v1/TS parity." That parity now points the other way: #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_REQUEST error instead of silently overwriting the first request's routing. Sequential id reuse after completion is unchanged. Types of changes Bug fix (non-breaking change which fixes an issue) Checklist I have read the MCP Documentation My code follows the repository's style guidelines New and existing tests pass locally I have added appropriate error handling I have added or updated documentation as needed
Open Graph Description: Opening this as the concrete artifact of the offer I made on #3060. It's ready and green, but I'm putting it up for your convenience, not to jump the queue — happy to close it immediately i...
X Description: Opening this as the concrete artifact of the offer I made on #3060. It's ready and green, but I'm putting it up for your convenience, not to jump the queue — happy to close it immed...
Opengraph URL: https://github.com/modelcontextprotocol/python-sdk/pull/3064
X: @github
Domain: github.com
| route-pattern | /:user_id/:repository/pull/:id/files(.:format) |
| route-controller | pull_requests |
| route-action | files |
| fetch-nonce | v2:f6922c6d-1fbb-cbd2-9a3a-f0293daaff10 |
| current-catalog-service-hash | ae870bc5e265a340912cde392f23dad3671a0a881730ffdadd82f2f57d81641b |
| request-id | A250:26FD39:78808A:A3159F:6A5CD547 |
| html-safe-nonce | 260faa577c88e2da234d61c212e1e2383613e4bac8e313a5145ca2f4f1d1dcd2 |
| visitor-payload | eyJyZWZlcnJlciI6IiIsInJlcXVlc3RfaWQiOiJBMjUwOjI2RkQzOTo3ODgwOEE6QTMxNTlGOjZBNUNENTQ3IiwidmlzaXRvcl9pZCI6IjE5MDAzNzU1NzExNTIyOTUyMzkiLCJyZWdpb25fZWRnZSI6ImlhZCIsInJlZ2lvbl9yZW5kZXIiOiJpYWQifQ== |
| visitor-hmac | 85c02f5ffe0f1a9b3315c35ba71647c467c7908f6f80a708bd50ed3e2316a95c |
| hovercard-subject-tag | pull_request:3992529594 |
| github-keyboard-shortcuts | repository,pull-request-list,pull-request-conversation,pull-request-files-changed,copilot |
| google-site-verification | Apib7-x98H0j5cPqHWwSMm6dNU4GmODRoqxLiDzdx9I |
| octolytics-url | https://collector.github.com/github/collect |
| analytics-location | / |
| fb:app_id | 1401488693436528 |
| apple-itunes-app | app-id=1477376905, app-argument=https://github.com/modelcontextprotocol/python-sdk/pull/3064/files |
| twitter:image | https://avatars.githubusercontent.com/u/272697015?s=400&v=4 |
| twitter:card | summary_large_image |
| og:image | https://avatars.githubusercontent.com/u/272697015?s=400&v=4 |
| og:image:alt | Opening this as the concrete artifact of the offer I made on #3060. It's ready and green, but I'm putting it up for your convenience, not to jump the queue — happy to close it immediately i... |
| og:site_name | GitHub |
| og:type | object |
| hostname | github.com |
| expected-hostname | github.com |
| None | 5290d7e14309ad1e76106a9c4237bd1041517e83ea182c8ab756752cb0c6940b |
| turbo-cache-control | no-preview |
| diff-view | unified |
| go-import | github.com/modelcontextprotocol/python-sdk git https://github.com/modelcontextprotocol/python-sdk.git |
| octolytics-dimension-user_id | 182288589 |
| octolytics-dimension-user_login | modelcontextprotocol |
| octolytics-dimension-repository_id | 862584018 |
| octolytics-dimension-repository_nwo | modelcontextprotocol/python-sdk |
| octolytics-dimension-repository_public | true |
| octolytics-dimension-repository_is_fork | false |
| octolytics-dimension-repository_network_root_id | 862584018 |
| octolytics-dimension-repository_network_root_nwo | modelcontextprotocol/python-sdk |
| turbo-body-classes | logged-out env-production page-responsive full-width |
| disable-turbo | true |
| browser-stats-url | https://api.github.com/_private/browser/stats |
| browser-errors-url | https://api.github.com/_private/browser/errors |
| release | 9c975978430e9ad293956f2bbdaf153b1bd84a99 |
| ui-target | full |
| theme-color | #1e2327 |
| color-scheme | light dark |
Links:
Viewport: width=device-width