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.)
6. ashirimi1019 commented on Nov 4, 2026

   

   ashirimi1019

   on Nov 4, 2026 – with ChatGPT Codex Connector

   Author

   More actions

   Thanks for the independent reproductions and for proposing #3815. There is a correctness gap in the exact-first, cross-type fallback approach that the single concurrent-twin regression does not exercise.

   Consider two requests with distinct JSON-RPC IDs, "7" and 7. The string-ID request completes, but its cancellation notification arrives late while integer 7 is still running. The exact lookup for "7" misses; coercing it to 7 then cancels the unrelated live request. From the receiver's perspective, this is indistinguishable from a peer incorrectly stringifying the ID of 7. Limiting fallback to a single matching in-flight request does not fix the ambiguity. MCP explicitly requires graceful handling of cancellation/completion races and recommends ignoring unknown or already-completed cancellations.

   We tested this with MCP Faultline against python-sdk main at d7845c0 in isolated Linux builds:

   Variant	Fixed FL-001 scenarios	Randomized scenarios (50 seeds × 3 transports)
   Exact-first + coercion fallback	72/72	138/150 (12 wrong-request cancellations, rule R6)
   Exact typed-ID cancellation only	72/72	150/150

   The regression test_late_cancel_for_a_completed_request_does_not_cancel_its_in_flight_twin fails with the fallback and passes with exact matching. The exact-only patch also passed the full upstream suite (6,236 tests, 100% coverage) and Ruff/Pyright in the pinned Linux validation.

   Evidence: compatibility analysis · Linux CI validation · patch and regression tests.

   My recommendation is to key inbound _in_flight and match cancellations by the exact typed request ID, without coercion fallback. Outbound _pending response compatibility can be considered separately. This does change the existing cross-type cancellation tolerance, so I'd appreciate a maintainer decision on that tradeoff before proposing an upstream PR.

   Disclosure: the test harness and this analysis were developed with AI assistance; linked results come from reproducible CI experiments.
7. Shanmukh-l33t commented on Nov 4, 2026

   

   Shanmukh-l33t

   on Nov 4, 2026

   More actions

   Late to this one, but the code path settles most of it: notifications/cancelled resolves through self._in_flight.get(coerce_request_id(rid)) (jsonrpc_dispatcher.py), so a late cancel naming "7" folds to 7 and cancels the live int-7 entry — and since handlers answer with dctx._request_id = req.id verbatim, the receiver's own echo gives no way to distinguish that from a peer stringifying 7's id. Your scenario is real.

   Your table also settles the design question more cleanly than the fallback-plus-tombstones shape I sketched: exact typed-ID cancellation only for the inbound surface (which is the _in_flight half of what I proposed) — and the tombstone idea, meant to make late cancels deterministic if coercion survives anywhere on that surface, is the weaker option per your numbers. 138/150 → 150/150 by deleting the fallback rather than fencing it is about as clear as evidence gets. I'd drop cross-type coercion for _in_flight entirely, as you recommend.

   Two loose ends I'd keep separate from that decision:

   • _pending tolerance is a different surface. The coercion's original justification is response correlation against a peer that stringifies ids ("matches the TS SDK"), and no cancellation hangs off it — so keeping coerce_request_id there is a lower-stakes, independent call from the inbound one.
   • The duplicate ids blind-overwrite TODO directly above the _in_flight assignment (reject with -32600) is orthogonal to key typing and probably wants its own change.

   On the tradeoff you flagged: dropping cross-type cancellation tolerance is exactly the behavior change maintainers should sign off on explicitly, and the faultline runs are the right artifact to put in front of them. One practical note: #3815 was auto-closed within seconds of opening (16:11:30Z → 16:11:45Z) by the require-linked-issue bot, so there is no open PR behind this — the decision thread here is the vehicle.

   (Written with AI assistance; the code paths above were re-verified against main today. Your harness numbers are quoted from your comment — I haven't rerun MCP Faultline myself.)