notifications/cancelled for request 7 cancels a concurrent request "7" (_in_flight keyed by coerce_request_id) #3807

Activity

1. claude

   added

   bugSomething isn't workingSomething isn't working

   v2Affects the v2 line (2.x on main)Affects the v2 line (2.x on main)

   on Nov 3, 2026
2. Shanmukh-l33t commented on Nov 3, 2026

   

   Shanmukh-l33t

   on Nov 3, 2026

   More actions

   Confirming the reproduction independently, in-process against main at af4c6f4 (memory-stream harness modeled on tests/server/test_cancel_handling.py, no stdio): with one blocking tools/call at id 7 (int) and another at id "7" (str) both in flight, notifications/cancelled naming 7 cancelled the "7" handler. Request 7 was then answered; request "7" never got a response. So the repro holds at the JSONRPCDispatcher level, not just over stdio.

   Two details from reading the code that bear on the fix design:

   1. The folding is documented as deliberate — coerce_request_id's docstring says "7" and 7 are one id for correlation purposes, even where the wire carries the verbatim value (matches the TS SDK). So keying _in_flight by verbatim id would change that contract and make notifications/cancelled correlation ambiguous. The invariant that actually breaks is one-entry-per-correlation-id: _in_flight[coerce_request_id(req.id)] (jsonrpc_dispatcher.py:608) lets a second concurrent request silently overwrite the first request's entry, which is why the wrong handler is cancelled and the first becomes un-cancellable.

   2. The outbound side already refuses exactly this collision — send_raw_request raises ValueError(f"request id {request_id!r} is already in flight") when a supplied id collides in _pending, and DirectDispatcher mirrors it in its in-memory domain. The inbound side has no equivalent guard.

   Approach I'd take, for what it's worth: keep the coerced key, but when an inbound request arrives whose coerced key is already in _in_flight, answer the newcomer with a JSON-RPC error rather than sharing/overwriting the slot — mirroring the outbound duplicate-id behavior and keeping the one-entry-per-correlation-id invariant that cancellation relies on. A regression test would assert (a) the second colliding request receives an error response, (b) the first request remains cancellable and completes normally.

   Happy to implement this if a maintainer wants an outside PR for it — the reporter has first call either way.

   Disclosure: written with AI assistance; the reproduction above was executed locally against main before posting.
3. media-ctrl commented on Nov 4, 2026

   

   media-ctrl

   on Nov 4, 2026

   More actions

   Confirming the repro and widening it: the collision domain is bigger than "07" and "+7".

   coerce_request_id (src/mcp/shared/dispatcher.py:56) is int(request_id) for strings, and int() also accepts leading and trailing whitespace, a leading +, and non-ASCII decimal digits. int("7 "), int(" 7"), int("\u0667") (Arabic-Indic seven) and int("\uff17") (fullwidth seven) all return 7, so all of them share a slot with the integer 7 at jsonrpc_dispatcher.py:608. Any of those spellings can arrive on the wire from a peer.

   The strongest argument for exact matching is that the coercion buys nothing on this path. The server answers with dctx._request_id, which is req.id verbatim, so the inbound table never has to correlate a value the peer transformed. It only exists to make a stringified cancel land, and that is the one place a wrong hit is unrecoverable. The outbound _pending table is the opposite case: there the tolerance protects against a peer that stringifies ids on the way back, and it has no cross-request cancellation surface.

   That split suggests per-table keys rather than one shared function:

   • _in_flight: key on the typed id ((type(id), id)), cancel on the exact typed id.
   • _pending: keep the typed key and fall back to the coerced key only when the typed key misses and exactly one pending entry folds to it.

   A caveat on the exact-first fallback: it is only safe when the loser of an ambiguity cannot be a live request, and for cancels it always can be. The way to keep late cancels deterministic instead of tolerant is a bounded set of recently completed typed ids, so a cancel for a request that answered one frame ago hits a tombstone and is dropped per the spec SHOULD on unknown or completed ids, rather than being retried against a numeric twin.

   Same shape for progress, which your repro does not exercise: _dispatch_notification resolves progressToken through coerce_request_id at :651, so concurrent requests using 7 and "7" share one on_progress callback and each one's progress frames reach the other's handler. A typed key fixes both tables with one decision.

   Separately, the same dict has a second defect your repro does not need: identical ids blind-overwrite the slot. The TODO at :606-608 names it ("duplicate ids blind-overwrite (v1/TS parity); revisit rejecting with INVALID_REQUEST"). A typed key alone leaves that path silent, so the change likely wants -32600 for an id already in flight at the same time.

   test_cancelled_correlates_across_string_and_int_request_id_forms should be inverted rather than deleted, plus one new case: a cancel for "7" leaves an in-flight 7 running to completion and answering.
4. 1828703450qqcom-ux commented on Nov 4, 2026

   

   1828703450qqcom-ux

   on Nov 4, 2026

   More actions

   I have a fix ready for this issue in PR #3815 (currently auto-closed pending assignment).

   Root cause: JSONRPCDispatcher._in_flight keys inbound requests by coerce_request_id(req.id), which folds "7" and 7 into one dict slot. Two concurrent requests whose ids differ only in JSON-RPC type blind-overwrite each other, and a notifications/cancelled naming one cancels whichever is stored.

   Fix: store in-flight requests under the verbatim id; cancellation looks up the exact id first and falls back to coerced/stringified forms so the existing cross-type correlation tests (test_cancelled_correlates_across_string_and_int_request_id_forms) pass unchanged.

   Validation: full tests/shared/test_jsonrpc_dispatcher.py 80 passed / 0 failed. New regression test test_peer_cancel_does_not_cross_request_id_types fails on main and passes after.

   Could I be assigned to this issue so the PR can stay open? Thanks!
5. Shanmukh-l33t commented on Nov 4, 2026

   

   Shanmukh-l33t

   on Nov 4, 2026

   More actions

   Confirmed the widening — all five spellings fold to one slot. I ran it directly:

   >>> [int(s) for s in ["7 ", " 7", "\u0667", "\uff17", "+7"]]
   [7, 7, 7, 7, 7]

   and coerce_request_id (src/mcp/shared/dispatcher.py:56) is int(request_id) for any str that parses, so self._in_flight[coerce_request_id(req.id)] (with the duplicate ids blind-overwrite TODO directly above it) keys all of them identically. The notifications/progress lookup resolves progressToken through the same function against self._pending, so that's one decision covering both tables, not two.

   On the split-table shape: the typed key belongs on _in_flight because it's the only table where a wrong hit is unrecoverable — the handler answers with dctx._request_id, set to req.id verbatim, so a coerced cancel can kill a live request whose response id never matches the key it was cancelled under. Keeping tolerance on _pending with exact-key-first and a single-fold-fallback preserves the peer-stringifies-ids case without a cross-request surface.

   The bounded set of recently-completed typed ids is what makes late cancels deterministic rather than merely unlikely to misfire — without it, a fallback can still land on a numeric twin that happens to be live.

   Test shape I'd expect: invert test_cancelled_correlates_across_string_and_int_request_id_forms into "a cancel for "7" leaves an in-flight 7 running and answering", plus the mirrored progress case — concurrent 7 and "7" requests each keep their own on_progress. The blind-overwrite → -32600 question feels orthogonal to key typing and probably wants its own change.

   (Written with AI assistance; the code references and the int() behaviour above were re-verified against main today.)