diff --git a/src/mcp/shared/direct_dispatcher.py b/src/mcp/shared/direct_dispatcher.py index e17283afa2..871bc17879 100644 --- a/src/mcp/shared/direct_dispatcher.py +++ b/src/mcp/shared/direct_dispatcher.py @@ -250,6 +250,10 @@ async def _dispatch_request( in_flight_key = coerce_request_id(request_id) if in_flight_key in self._in_flight_ids: raise ValueError(f"request id {request_id!r} is already in flight") + # Advance the mint counter past any supplied integer id so + # the monotonic sequence never revisits it after completion. + if isinstance(in_flight_key, int): + self._next_id = max(self._next_id, in_flight_key) else: # Synthesize an id (the DispatchContext contract reserves None # for notifications), minting past any key a supplied id diff --git a/src/mcp/shared/jsonrpc_dispatcher.py b/src/mcp/shared/jsonrpc_dispatcher.py index 87bdf31ceb..ec546fc254 100644 --- a/src/mcp/shared/jsonrpc_dispatcher.py +++ b/src/mcp/shared/jsonrpc_dispatcher.py @@ -346,6 +346,12 @@ async def send_raw_request( pending_key = coerce_request_id(request_id) if pending_key in self._pending: raise ValueError(f"request id {request_id!r} is already in flight") + # Advance the mint counter past any supplied integer id so the + # monotonic sequence can never revisit it after the request completes. + # This satisfies the spec: "The request ID MUST NOT have been + # previously used by the requestor within the same session." + if isinstance(pending_key, int): + self._next_id = max(self._next_id, pending_key) else: # Mint past any key a supplied id occupies: the collision error is # reserved for the caller who actually chose the id. diff --git a/tests/shared/test_dispatcher.py b/tests/shared/test_dispatcher.py index c6ebb401ff..aeaf8a203d 100644 --- a/tests/shared/test_dispatcher.py +++ b/tests/shared/test_dispatcher.py @@ -474,11 +474,12 @@ async def parked() -> None: tg.start_soon(parked) await entered.wait() - # The counter mints 1 and 2, then skips the occupied 3 to 4. + # The counter is advanced to 3 when "3" is supplied, so + # subsequent mints produce 4, 5, 6 — never revisiting 3. for _ in range(3): await client.send_raw_request("plain", None) release.set() - assert [request_id for request_id in seen_ids if request_id != "3"] == [1, 2, 4] + assert [request_id for request_id in seen_ids if request_id != "3"] == [4, 5, 6] @pytest.mark.anyio @@ -512,6 +513,64 @@ async def first() -> None: assert await client.send_raw_request("again", None, {"request_id": "7"}) == {} +@pytest.mark.anyio +async def test_minted_ids_never_reuse_a_completed_caller_supplied_id(pair_factory: PairFactory): + """Regression: after a caller-supplied integer id completes, the mint counter + must have advanced past it so no future minted id collides. This is the bug + from GH-3126: the counter could land on a previously-used supplied id because + the guard only checked `_pending`/`_in_flight_ids` (cleared on completion).""" + seen_ids: list[RequestId | None] = [] + + async def track( + ctx: DispatchContext[TransportContext], method: str, params: Mapping[str, Any] | None + ) -> dict[str, Any]: + seen_ids.append(ctx.request_id) + return {} + + async with running_pair(pair_factory, server_on_request=track) as (client, *_): + with anyio.fail_after(5): + # Send a request with a caller-supplied integer id. + await client.send_raw_request("supplied", None, {"request_id": 5}) + # Now send several auto-minted requests. None should reuse id 5. + for _ in range(6): + await client.send_raw_request("minted", None) + + # The first id is the supplied 5; the rest are minted sequentially starting + # above 5 (i.e. 6, 7, 8, 9, 10, 11). + assert seen_ids[0] == 5 + minted_ids = seen_ids[1:] + assert 5 not in minted_ids + # Verify they are unique and monotonically increasing integers > 5. + assert all(isinstance(i, int) and i > 5 for i in minted_ids) + assert len(minted_ids) == len(set(minted_ids)) + + +@pytest.mark.anyio +async def test_minted_ids_never_reuse_a_completed_numeric_string_id(pair_factory: PairFactory): + """Same as above but with a numeric-string supplied id ("3"), which coerces to + int 3 in the collision domain. Minted ids must skip past 3.""" + seen_ids: list[RequestId | None] = [] + + async def track( + ctx: DispatchContext[TransportContext], method: str, params: Mapping[str, Any] | None + ) -> dict[str, Any]: + seen_ids.append(ctx.request_id) + return {} + + async with running_pair(pair_factory, server_on_request=track) as (client, *_): + with anyio.fail_after(5): + await client.send_raw_request("supplied", None, {"request_id": "3"}) + for _ in range(4): + await client.send_raw_request("minted", None) + + assert seen_ids[0] == "3" + minted_ids = seen_ids[1:] + # 3 should never appear (even though "3" completed and left _pending/_in_flight). + assert 3 not in minted_ids + assert all(isinstance(i, int) and i > 3 for i in minted_ids) + assert len(minted_ids) == len(set(minted_ids)) + + @pytest.mark.anyio async def test_notify_intercept_sees_every_notification_and_consumes_on_true(pair_factory: PairFactory): """The intercept sees every inbound notification; a frame it consumes never reaches `on_notify`, the rest do."""