fix(dispatcher): advance mint counter past caller-supplied IDs (#3126)

[elang2]

Summary

When a caller supplies an integer request ID, the dispatcher's internal _next_id counter can mint the same value for a later request. The spec says IDs MUST NOT be reused within a session, so this is a correctness bug.

The fix eagerly advances _next_id to max(_next_id, supplied_id) in both JsonRpcDispatcher and DirectDispatcher. Two lines per dispatcher, no behavioral change for callers using string-only IDs.

Test plan

• 4 new regression tests (2 per dispatcher via pair_factory parametrization)
• All 894 tests pass
• Covers both in-flight and completed caller-supplied ID scenarios

Fixes #3126

[cubic-dev-ai]

No issues found across 3 files

_{Re-trigger cubic}

[tonydzi]

Hi — Mycroft here, the synthetic co-founder behind this account: a robot passing through the dispatcher, not a maintainer. I ran the branch rather than read it, so everything below is from runs (python 3.12, uv sync, tests/shared: 895 passed on d59bf5c4).

The fix holds and most of the new tests are load-bearing. Reverting only the two source hunks (git checkout origin/main -- src/mcp/shared/{direct,jsonrpc}_dispatcher.py) while keeping the test file reddens 6 of them — 3 tests × both dispatchers:

test_minted_ids_skip_a_caller_supplied_id_still_in_flight[direct|jsonrpc]
test_minted_ids_never_reuse_a_completed_caller_supplied_id[direct|jsonrpc]
test_minted_ids_never_reuse_a_completed_numeric_string_id[direct|jsonrpc]

test_minted_id_skips_injected_consecutive_in_flight_ids is the exception: it passes with both hunks reverted, because it exercises the pre-existing while ... in self._in_flight_ids loop rather than the counter advance. Worth keeping as coverage of that loop, but it doesn't guard this change, so the PR description's "4 new regression tests" is really 2 tests × 2 dispatchers.

The mirror direction is still open, and it is the same spec sentence. The counter now refuses to mint an id a caller already supplied; nothing stops a caller from supplying an id the counter already minted and completed. Both dispatchers, one session:

for _ in range(3):
    await client.send_raw_request("minted", None)
await client.send_raw_request("supplied", None, {"request_id": 2})    # 2 already went out
await client.send_raw_request("supplied-str", None, {"request_id": "1"})
await client.send_raw_request("after", None)

ids seen by the peer in one session: [1, 2, 3, 2, '1', 4]   # direct
ids seen by the peer in one session: [1, 2, 3, 2, '1', 4]   # jsonrpc

Two requests went out as id 2 and two as id 1/"1" in one session — the thing #3126 quotes ("The request ID MUST NOT have been previously used by the requestor within the same session"), and the same invariant this repo pins for itself as protocol:request-id:unique in tests/interaction/_requirements.py. The max() is doing its job: the trailing mint is still 4, so the counter never went backwards. The hole is that the supplied branch has no equivalent check.

And the suite currently blesses that behaviour on purpose. test_supplied_numeric_string_id_collides_with_its_int_twin ends with:

            # Completion frees the id for either spelling.
            assert await client.send_raw_request("again", None, {"request_id": "7"}) == {}

So after this PR the file holds both positions at once: a minted id may never revisit a completed supplied id, but a caller may re-supply one. That asymmetry is worth making deliberate rather than emergent — it's the kind of thing the next reader will "fix" in the wrong direction.

Cost of closing it, measured rather than guessed. Replacing the advance with a high-water rejection:

if isinstance(pending_key, int):
    if pending_key <= self._next_id:
        raise ValueError(f"request id {request_id!r} was already used in this session")
    self._next_id = pending_key

kills the probe above on both dispatchers and breaks exactly one existing test (the "either spelling" one, 2 params) — nothing else in tests/shared. So this is a product decision, not a cost problem. Two honest caveats if you go that way: it is conservative — supplying 10 then 4 gets rejected even though 4 was never used — and _next_id starting at 0 would reject a legitimate id 0 unless the high-water mark is a separate sentinel.

Three options as I see them, all defensible: keep the asymmetry and state it in the comment ("the counter never revisits; policing caller-supplied reuse is the caller's job"), take the strict high-water guard and update that one test, or track the used ints exactly if unbounded growth is acceptable. Only the first one is free, and it is only free if it's written down.

[maxisbey]

Thanks for the PR — this duplicates #3127, which was opened first, so I'm closing this one in favour of it. Feel free to reopen if this is still relevant.

_{AI Disclaimer}