Skip to content

Commit 79af94f

Browse files
committed
fix: never mint a request id already used by a completed caller-supplied id
The mint loops in JSONRPCDispatcher.send_raw_request and DirectDispatcher._dispatch_request only skipped ids still in flight, so once a caller-supplied integer (or numeric-string) id completed, the monotonic counter could reach the same value and reuse it on the wire. The spec forbids reusing a request id within a session. Accepting a supplied id now advances the mint counter past its coerced key, which makes the skip loops provably unreachable, so they are removed.
1 parent 3a6f299 commit 79af94f

4 files changed

Lines changed: 47 additions & 13 deletions

File tree

src/mcp/shared/direct_dispatcher.py

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -250,14 +250,18 @@ 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+
# Advance the mint counter past the supplied key: ids are
254+
# single-use within a session, so a minted id may not
255+
# revisit this key even after the request completes.
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
255-
# for notifications), minting past any key a supplied id
256-
# occupies: the collision error is reserved for the caller
257-
# who actually chose the id.
260+
# for notifications). Cannot collide: the counter is past
261+
# every supplied integer key (advanced above) and every
262+
# previously minted id; the collision error is reserved for
263+
# the caller who actually chose the id.
258264
self._next_id += 1
259-
while self._next_id in self._in_flight_ids:
260-
self._next_id += 1
261265
request_id = self._next_id
262266
in_flight_key = request_id
263267
self._in_flight_ids.add(in_flight_key)

src/mcp/shared/dispatcher.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,10 @@ class CallOptions(TypedDict, total=False):
8787
Callers that need to know a request's id before its result arrives (a
8888
`subscriptions/listen` stream is demultiplexed by it) mint their own ids
8989
here; string ids that don't parse as integers can never collide with the
90-
dispatcher's minted sequence. Per the class contract, dispatchers that
91-
predate this key ignore it and mint as usual.
90+
dispatcher's minted sequence, and a supplied integer (or numeric-string)
91+
key advances that sequence past itself, so a minted id never reuses a
92+
supplied one. Per the class contract, dispatchers that predate this key
93+
ignore it and mint as usual.
9294
"""
9395

9496
timeout: float

src/mcp/shared/jsonrpc_dispatcher.py

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -335,12 +335,16 @@ async def send_raw_request(
335335
pending_key = coerce_request_id(request_id)
336336
if pending_key in self._pending:
337337
raise ValueError(f"request id {request_id!r} is already in flight")
338+
# Advance the mint counter past the supplied key: ids are single-use
339+
# within a session, so a minted id may not revisit this key even
340+
# after the request completes.
341+
if isinstance(pending_key, int):
342+
self._next_id = max(self._next_id, pending_key)
338343
else:
339-
# Mint past any key a supplied id occupies: the collision error is
340-
# reserved for the caller who actually chose the id.
344+
# Cannot collide: the counter is past every supplied integer key
345+
# (advanced above) and every previously minted id. The collision
346+
# error is reserved for the caller who actually chose the id.
341347
request_id = self._allocate_id()
342-
while request_id in self._pending:
343-
request_id = self._allocate_id()
344348
pending_key = request_id
345349
out_params = dict(params) if params is not None else {}
346350
out_meta = dict(out_params.get("_meta") or {})

tests/shared/test_dispatcher.py

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -474,11 +474,35 @@ async def parked() -> None:
474474

475475
tg.start_soon(parked)
476476
await entered.wait()
477-
# The counter mints 1 and 2, then skips the occupied 3 to 4.
477+
# Accepting "3" advanced the counter past its coerced key, so
478+
# the mints start at 4.
478479
for _ in range(3):
479480
await client.send_raw_request("plain", None)
480481
release.set()
481-
assert [request_id for request_id in seen_ids if request_id != "3"] == [1, 2, 4]
482+
assert [request_id for request_id in seen_ids if request_id != "3"] == [4, 5, 6]
483+
484+
485+
@pytest.mark.anyio
486+
@pytest.mark.parametrize("supplied_id", [2, "2"], ids=["int", "numeric-string"])
487+
async def test_minted_ids_never_reuse_a_completed_supplied_id(pair_factory: PairFactory, supplied_id: RequestId):
488+
"""Ids are single-use within a session (spec: "The request ID MUST NOT have been
489+
previously used by the requestor within the same session"), so the mint counter
490+
skips a supplied integer key even after that request completes — a peer that
491+
tracks ids per session would see two different requests under one id."""
492+
seen_ids: list[RequestId | None] = []
493+
494+
async def record(
495+
ctx: DispatchContext[TransportContext], method: str, params: Mapping[str, Any] | None
496+
) -> dict[str, Any]:
497+
seen_ids.append(ctx.request_id)
498+
return {}
499+
500+
async with running_pair(pair_factory, server_on_request=record) as (client, *_):
501+
with anyio.fail_after(5):
502+
await client.send_raw_request("supplied", None, {"request_id": supplied_id})
503+
await client.send_raw_request("plain", None)
504+
await client.send_raw_request("plain", None)
505+
assert seen_ids == [supplied_id, 3, 4]
482506

483507

484508
@pytest.mark.anyio

0 commit comments

Comments
 (0)