Skip to content

Commit 6290242

Browse files
committed
fix(dispatcher): advance mint counter past caller-supplied request ids
The dispatcher's minted-id guard only checked the in-flight table, so once a caller-supplied numeric request id completed, the monotonic counter could reach the same value and put it on the wire again. The spec forbids reusing a request id within a session ("The request ID MUST NOT have been previously used by the requestor within the same session"), and stateful peers may cross-wire responses for duplicate ids (#3060). When a supplied id is accepted, advance the mint counter past its coerced key so minted ids can never revisit it — in both JSONRPCDispatcher (pending keys) and DirectDispatcher (in-flight ids). Minted ids are ints, so only numeric keys ("7" and 7 share the coerced key) can collide; string ids are unaffected. Caller re-supply of its own used id stays allowed, as asserted by existing tests. One existing test's expected mint sequence changes from [1, 2, 4] to [4, 5, 6]: the counter now clears the supplied id at acceptance rather than skipping it only on collision. The never-collide contract it tests still holds. Closes #3126
1 parent d060b36 commit 6290242

3 files changed

Lines changed: 83 additions & 3 deletions

File tree

src/mcp/shared/direct_dispatcher.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,11 @@ async def _dispatch_request(
250250
in_flight_key = coerce_request_id(request_id)
251251
if in_flight_key in self._in_flight_ids:
252252
raise ValueError(f"request id {request_id!r} is already in flight")
253+
# Same mint-past rule as JSONRPCDispatcher: the counter must
254+
# clear a completed supplied id so minted ids never revisit
255+
# it (#3126). Minted ids are ints, so only numeric keys collide.
256+
if isinstance(in_flight_key, int):
257+
self._next_id = max(self._next_id, in_flight_key)
253258
else:
254259
# Synthesize an id (the DispatchContext contract reserves None
255260
# for notifications), minting past any key a supplied id

src/mcp/shared/jsonrpc_dispatcher.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,12 @@ async def send_raw_request(
346346
pending_key = coerce_request_id(request_id)
347347
if pending_key in self._pending:
348348
raise ValueError(f"request id {request_id!r} is already in flight")
349+
# The mint counter must also clear this id once it completes, not
350+
# only while it is pending: the spec forbids reusing a request id
351+
# within a session (#3126). Minted ids are ints, so only numeric
352+
# keys ("7" and 7 share the coerced key) can collide.
353+
if isinstance(pending_key, int):
354+
self._next_id = max(self._next_id, pending_key)
349355
else:
350356
# Mint past any key a supplied id occupies: the collision error is
351357
# reserved for the caller who actually chose the id.

tests/shared/test_dispatcher.py

Lines changed: 72 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,15 @@
2525

2626
from mcp.shared._compat import resync_tracer
2727
from mcp.shared.direct_dispatcher import DirectDispatcher, create_direct_dispatcher_pair
28-
from mcp.shared.dispatcher import DispatchContext, Dispatcher, OnNotify, OnNotifyIntercept, OnRequest, Outbound
28+
from mcp.shared.dispatcher import (
29+
DispatchContext,
30+
Dispatcher,
31+
OnNotify,
32+
OnNotifyIntercept,
33+
OnRequest,
34+
Outbound,
35+
coerce_request_id,
36+
)
2937
from mcp.shared.exceptions import MCPError
3038
from mcp.shared.transport_context import TransportContext
3139

@@ -451,7 +459,9 @@ async def first() -> None:
451459
@pytest.mark.anyio
452460
async def test_minted_ids_skip_a_caller_supplied_id_still_in_flight(pair_factory: PairFactory):
453461
"""The dispatcher mints PAST a key a supplied id occupies — the collision error
454-
is reserved for the caller who chose the id, never an innocent minted request."""
462+
is reserved for the caller who chose the id, never an innocent minted request.
463+
Since the counter is advanced past the supplied key on acceptance (#3126),
464+
minted ids continue from beyond it rather than skipping it only on collision."""
455465
entered = anyio.Event()
456466
release = anyio.Event()
457467
seen_ids: list[RequestId | None] = []
@@ -478,7 +488,10 @@ async def parked() -> None:
478488
for _ in range(3):
479489
await client.send_raw_request("plain", None)
480490
release.set()
481-
assert [request_id for request_id in seen_ids if request_id != "3"] == [1, 2, 4]
491+
# The counter was advanced past 3 when the supplied id was accepted
492+
# (#3126), so minting continues from 4 instead of skipping only on
493+
# collision — the "never collide with a supplied id" contract holds.
494+
assert [request_id for request_id in seen_ids if request_id != "3"] == [4, 5, 6]
482495

483496

484497
@pytest.mark.anyio
@@ -576,3 +589,59 @@ def broken_intercept(method: str, params: Mapping[str, Any] | None) -> bool:
576589
if TYPE_CHECKING:
577590
_d: Dispatcher[TransportContext] = DirectDispatcher(TransportContext(kind="direct", can_send_request=True))
578591
_o: Outbound = _d
592+
593+
594+
@pytest.mark.anyio
595+
async def test_minted_ids_never_reuse_a_completed_caller_supplied_id(pair_factory: PairFactory):
596+
"""The mint counter must skip past a COMPLETED caller-supplied numeric id, not
597+
just in-flight ones. Reusing a completed id on the wire violates the spec's
598+
"request id MUST NOT have been previously used by the requestor within the
599+
same session" and can cross-wire responses on stateful peers (#3126)."""
600+
async with running_pair(pair_factory) as (client, _server, _crec, srec):
601+
with anyio.fail_after(5):
602+
# A caller-supplied numeric id that will COMPLETE before any minting.
603+
await client.send_raw_request("supplied", None, {"request_id": 2})
604+
# Three dispatcher-minted requests after the supplied one is done.
605+
await client.send_raw_request("minted-1", None)
606+
await client.send_raw_request("minted-2", None)
607+
await client.send_raw_request("minted-3", None)
608+
wire_ids = [ctx.request_id for ctx in srec.contexts]
609+
assert wire_ids[0] == 2
610+
minted = wire_ids[1:]
611+
assert len(minted) == 3
612+
# None of the minted ids may revisit the completed supplied id 2 (nor
613+
# collide with each other).
614+
assert len(set(map(coerce_request_id, minted))) == 3
615+
assert all(coerce_request_id(i) != 2 for i in minted), (
616+
f"minted ids {minted} revisited completed supplied id 2 (issue #3126)"
617+
)
618+
619+
620+
@pytest.mark.anyio
621+
async def test_minted_ids_never_reuse_a_completed_numeric_string_supplied_id(pair_factory: PairFactory):
622+
"""`"2"` and `2` are one id in the correlation domain, so a completed
623+
numeric-STRING supplied id must be skipped by the mint counter too (#3126)."""
624+
async with running_pair(pair_factory) as (client, _server, _crec, srec):
625+
with anyio.fail_after(5):
626+
await client.send_raw_request("supplied", None, {"request_id": "2"})
627+
await client.send_raw_request("minted-1", None)
628+
await client.send_raw_request("minted-2", None)
629+
wire_ids = [ctx.request_id for ctx in srec.contexts]
630+
assert wire_ids[0] == "2"
631+
minted = wire_ids[1:]
632+
assert all(coerce_request_id(i) != 2 for i in minted), (
633+
f"minted ids {minted} revisited completed supplied id '2' (issue #3126)"
634+
)
635+
636+
637+
@pytest.mark.anyio
638+
async def test_supplying_a_used_id_yourself_is_still_allowed(pair_factory: PairFactory):
639+
"""Re-supplying an id the CALLER already used is an explicit API contract
640+
(see `test_send_raw_request_with_in_flight_request_id_raises_and_frees_id_on_completion`)
641+
and must keep working when the mint counter is taught to skip used ids (#3126)."""
642+
async with running_pair(pair_factory) as (client, _server, _crec, srec):
643+
with anyio.fail_after(5):
644+
await client.send_raw_request("first", None, {"request_id": 5})
645+
# Same caller re-supplies its own completed id: allowed on purpose.
646+
await client.send_raw_request("second", None, {"request_id": 5})
647+
assert [ctx.request_id for ctx in srec.contexts] == [5, 5]

0 commit comments

Comments
 (0)