Skip to content

fix: never mint a request id already used by a completed caller-supplied id#3127

Open
ayaangazali wants to merge 1 commit into
modelcontextprotocol:mainfrom
ayaangazali:fix/minted-request-id-reuse
Open

fix: never mint a request id already used by a completed caller-supplied id#3127
ayaangazali wants to merge 1 commit into
modelcontextprotocol:mainfrom
ayaangazali:fix/minted-request-id-reuse

Conversation

@ayaangazali

Copy link
Copy Markdown

Summary

Fixes #3126.

When a caller supplies its own request id via CallOptions["request_id"], the dispatcher's minted-id sequence could later land on that same id once the supplied request completed, sending two different requests under one id in the same session. The spec says the requestor must never reuse a request id within a session, and the SDK pins the same rule as protocol:request-id:unique in tests/interaction/_requirements.py.

Root cause

Both mint loops only skipped ids still in flight:

request_id = self._allocate_id()
while request_id in self._pending:   # entries are popped on completion
    request_id = self._allocate_id()

The comment right above ("Mint past any key a supplied id occupies") shows the intent, but the guard stopped holding the moment the supplied request completed. Since coerce_request_id folds "2" and 2 into one key, numeric-string ids hit this too. DirectDispatcher had the same pattern with _in_flight_ids (discarded in a finally).

Fix

Accepting a supplied id now advances the mint counter past its coerced integer key:

if isinstance(pending_key, int):
    self._next_id = max(self._next_id, pending_key)

That makes the old skip loops provably unreachable (the counter is always past every supplied integer key and every previously minted id), so they are removed rather than left as dead branches the coverage gate would flag.

Two deliberate scope notes:

  • A caller re-supplying its own completed id stays allowed; that behavior is asserted by test_send_raw_request_with_in_flight_request_id_raises_and_frees_id_on_completion and this PR does not touch it. Only the dispatcher's own minting is constrained.
  • Minted id values shift when a caller supplies integer ids (e.g. the pinned sequence in test_minted_ids_skip_a_caller_supplied_id_still_in_flight went from [1, 2, 4] to [4, 5, 6]). Ids are opaque per spec, so I did not treat this as a breaking change needing a migration entry; happy to add one if you see it differently.

Tests

  • New test_minted_ids_never_reuse_a_completed_supplied_id, parametrized over both dispatchers and both id spellings (2, "2"). Fails on current main with wire ids [2, 1, 2]; passes with the fix ([2, 3, 4]).
  • Updated the pinned sequence in test_minted_ids_skip_a_caller_supplied_id_still_in_flight.
  • CallOptions.request_id docstring extended with the new guarantee.

Ran locally:

./scripts/test                          # 5448 passed, coverage 100.00%, strict-no-cover clean
uv run --frozen ruff format . && uv run --frozen ruff check .
uv run --frozen pyright                 # 0 errors

AI disclosure, per CONTRIBUTING: Claude Code helped me find this and sanity-check the invariant reasoning; I wrote up the repro, ran everything locally, and I can defend each line here in my own words. First PR to this repo and I'm a freshman still learning the ropes, so if the counter-advance approach clashes with something I haven't seen, please say so and I'll rework it.

Copilot AI review requested due to automatic review settings July 18, 2026 11:12

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 4 files

Re-trigger cubic

…ied 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.
@ayaangazali
ayaangazali force-pushed the fix/minted-request-id-reuse branch from 79af94f to 64bfa61 Compare July 18, 2026 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dispatcher mints a request id already used by a completed caller-supplied request (spec: ids MUST NOT be reused in a session)

2 participants