Skip to content

Commit 5fd834e

Browse files
committed
fix(server): reject concurrent duplicate JSON-RPC request ids
Stateful streamable HTTP keyed per-request streams by bare request id and overwrote in-flight entries, cross-wiring concurrent POSTs that reused an id. Reject the collision with 409 and reserve the slot before the priming await so resumability cannot reopen the race. Fixes #3137
1 parent a4f4ccd commit 5fd834e

2 files changed

Lines changed: 322 additions & 8 deletions

File tree

src/mcp/server/streamable_http.py

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -600,6 +600,19 @@ async def _handle_post_request(self, scope: Scope, request: Request, receive: Re
600600

601601
request_id = str(message.id)
602602

603+
# `_request_streams` is keyed by the bare JSON-RPC id. A second concurrent
604+
# POST reusing an in-flight id would overwrite that slot and cross-wire
605+
# responses (one caller receives the other's payload; the other hangs).
606+
# Reject the collision with 409 rather than silently re-routing. Ids may
607+
# still be reused after the earlier request has completed.
608+
if request_id in self._request_streams:
609+
response = self._create_error_response(
610+
f"Conflict: Request id {request_id} is already in flight for this session",
611+
HTTPStatus.CONFLICT,
612+
)
613+
await response(scope, receive, send)
614+
return
615+
603616
if self.is_json_response_enabled:
604617
self._request_streams[request_id] = anyio.create_memory_object_stream[EventMessage](
605618
REQUEST_STREAM_BUFFER_SIZE
@@ -631,19 +644,25 @@ async def _handle_post_request(self, scope: Scope, request: Request, receive: Re
631644
await self._clean_up_memory_streams(request_id)
632645
await response(scope, receive, send)
633646
else:
634-
# Mint the priming event before any per-request state exists:
635-
# `EventStore.store_event` is user code and may raise, in which
636-
# case the outer handler returns a 500 with nothing to clean up.
637-
# Still strictly precedes dispatch, so storage order == wire order.
638-
priming_event = await self._mint_priming_event(request_id, protocol_version)
639-
640-
sse_stream_writer, sse_stream_reader = anyio.create_memory_object_stream[SSEEvent](0)
641-
self._sse_stream_writers[request_id] = sse_stream_writer
647+
# Reserve the routing slot before any await so a concurrent POST
648+
# reusing this id cannot pass the guard above while we mint the
649+
# priming event (EventStore.store_event is user code and may
650+
# await). Release the reservation if priming raises.
642651
self._request_streams[request_id] = anyio.create_memory_object_stream[EventMessage](
643652
REQUEST_STREAM_BUFFER_SIZE
644653
)
645654
request_stream_reader = self._request_streams[request_id][1]
646655

656+
try:
657+
# Still strictly precedes dispatch, so storage order == wire order.
658+
priming_event = await self._mint_priming_event(request_id, protocol_version)
659+
except BaseException:
660+
await self._clean_up_memory_streams(request_id)
661+
raise
662+
663+
sse_stream_writer, sse_stream_reader = anyio.create_memory_object_stream[SSEEvent](0)
664+
self._sse_stream_writers[request_id] = sse_stream_writer
665+
647666
headers = {
648667
"Cache-Control": "no-cache, no-transform",
649668
"Connection": "keep-alive",

tests/shared/test_streamable_http.py

Lines changed: 295 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,14 @@ def first_sse_data(response: httpx2.Response) -> dict[str, Any]:
9393
raise ValueError("No data event in SSE response") # pragma: no cover
9494

9595

96+
async def next_sse_data(lines: AsyncIterator[str]) -> dict[str, Any]:
97+
"""Return the next SSE `data:` payload from a live line iterator, parsed as JSON."""
98+
while True:
99+
line = await anext(lines)
100+
if line.startswith("data: "):
101+
return json.loads(line.removeprefix("data: "))
102+
103+
96104
def extract_protocol_version_from_sse(response: httpx2.Response) -> str:
97105
"""Extract the negotiated protocol version from an SSE initialization response."""
98106
return first_sse_data(response)["result"]["protocolVersion"]
@@ -808,6 +816,293 @@ async def test_get_sse_stream(basic_app: Starlette) -> None:
808816
assert second_get.status_code == 409
809817

810818

819+
@pytest.mark.anyio
820+
async def test_post_duplicate_request_id_rejected_while_first_still_in_flight(basic_app: Starlette) -> None:
821+
"""A second POST reusing an in-flight request's JSON-RPC id is rejected with 409; the first still completes.
822+
823+
Per-request routing is keyed by request id, so overwriting an in-flight slot would
824+
cross-wire responses (see #3137). Raw HTTP is required to assert the 409 status code.
825+
"""
826+
async with make_client(basic_app) as client:
827+
init_response = await client.post(
828+
"/mcp",
829+
headers={
830+
"Accept": "application/json, text/event-stream",
831+
"Content-Type": "application/json",
832+
},
833+
json=INIT_REQUEST,
834+
)
835+
assert init_response.status_code == 200
836+
headers = {
837+
"Accept": "application/json, text/event-stream",
838+
"Content-Type": "application/json",
839+
MCP_SESSION_ID_HEADER: init_response.headers[MCP_SESSION_ID_HEADER],
840+
MCP_PROTOCOL_VERSION_HEADER: extract_protocol_version_from_sse(init_response),
841+
}
842+
shared_id = "shared-request-id"
843+
844+
async with client.stream(
845+
"POST",
846+
"/mcp",
847+
headers=headers,
848+
json={
849+
"jsonrpc": "2.0",
850+
"method": "tools/call",
851+
"params": {"name": "wait_for_lock_with_notification", "arguments": {}},
852+
"id": shared_id,
853+
},
854+
) as first_response:
855+
assert first_response.status_code == 200
856+
lines = first_response.aiter_lines()
857+
# First notification confirms the request is registered and blocked on the lock.
858+
with anyio.fail_after(5):
859+
notification = await next_sse_data(lines)
860+
assert notification["params"]["data"] == "First notification before lock"
861+
862+
duplicate_response = await client.post(
863+
"/mcp",
864+
headers=headers,
865+
json={
866+
"jsonrpc": "2.0",
867+
"method": "tools/call",
868+
"params": {"name": "test_tool", "arguments": {}},
869+
"id": shared_id,
870+
},
871+
)
872+
assert duplicate_response.status_code == 409
873+
error = duplicate_response.json()["error"]
874+
assert error["code"] == INVALID_REQUEST
875+
assert "already in flight" in error["message"]
876+
877+
release_response = await client.post(
878+
"/mcp",
879+
headers=headers,
880+
json={
881+
"jsonrpc": "2.0",
882+
"method": "tools/call",
883+
"params": {"name": "release_lock", "arguments": {}},
884+
"id": "release-lock-1",
885+
},
886+
)
887+
assert release_response.status_code == 200
888+
889+
with anyio.fail_after(5):
890+
second_notification = await next_sse_data(lines)
891+
final = await next_sse_data(lines)
892+
assert second_notification["params"]["data"] == "Second notification after lock"
893+
assert final["id"] == shared_id
894+
assert final["result"]["content"][0]["text"] == "Completed"
895+
896+
897+
@pytest.mark.anyio
898+
async def test_json_response_duplicate_request_id_rejected_while_first_still_in_flight(json_app: Starlette) -> None:
899+
"""In JSON response mode, a second POST reusing an in-flight id is rejected with 409; the first still completes.
900+
901+
Same collision as the SSE case, exercised on the JSON branch of `_handle_post_request` (#3137).
902+
"""
903+
async with make_client(json_app) as client:
904+
init_response = await client.post(
905+
"/mcp",
906+
headers={
907+
"Accept": "application/json, text/event-stream",
908+
"Content-Type": "application/json",
909+
},
910+
json=INIT_REQUEST,
911+
)
912+
assert init_response.status_code == 200
913+
headers = {
914+
"Accept": "application/json, text/event-stream",
915+
"Content-Type": "application/json",
916+
MCP_SESSION_ID_HEADER: init_response.headers[MCP_SESSION_ID_HEADER],
917+
MCP_PROTOCOL_VERSION_HEADER: init_response.json()["result"]["protocolVersion"],
918+
}
919+
shared_id = "shared-json-request-id"
920+
first_result: dict[str, Any] = {}
921+
922+
async def run_first_request() -> None:
923+
response = await client.post(
924+
"/mcp",
925+
headers=headers,
926+
json={
927+
"jsonrpc": "2.0",
928+
"method": "tools/call",
929+
"params": {"name": "wait_for_lock_with_notification", "arguments": {}},
930+
"id": shared_id,
931+
},
932+
)
933+
assert response.status_code == 200
934+
first_result.update(response.json())
935+
936+
async with anyio.create_task_group() as tg:
937+
tg.start_soon(run_first_request)
938+
# First request registers its stream then blocks server-side on the lock;
939+
# nothing else can run until that await yields.
940+
await anyio.wait_all_tasks_blocked()
941+
942+
duplicate_response = await client.post(
943+
"/mcp",
944+
headers=headers,
945+
json={
946+
"jsonrpc": "2.0",
947+
"method": "tools/call",
948+
"params": {"name": "test_tool", "arguments": {}},
949+
"id": shared_id,
950+
},
951+
)
952+
assert duplicate_response.status_code == 409
953+
error = duplicate_response.json()["error"]
954+
assert error["code"] == INVALID_REQUEST
955+
assert "already in flight" in error["message"]
956+
957+
release_response = await client.post(
958+
"/mcp",
959+
headers=headers,
960+
json={
961+
"jsonrpc": "2.0",
962+
"method": "tools/call",
963+
"params": {"name": "release_lock", "arguments": {}},
964+
"id": "release-lock-json-1",
965+
},
966+
)
967+
assert release_response.status_code == 200
968+
969+
assert first_result["id"] == shared_id
970+
assert first_result["result"]["content"][0]["text"] == "Completed"
971+
972+
973+
@pytest.mark.anyio
974+
async def test_request_id_reuse_after_completion_allowed(basic_app: Starlette) -> None:
975+
"""A request id may be reused once the earlier request with that id has completed.
976+
977+
Only concurrent reuse is ambiguous to route; sequential reuse is allowed (some
978+
clients send a constant id for every request).
979+
"""
980+
async with make_client(basic_app) as client:
981+
init_response = await client.post(
982+
"/mcp",
983+
headers={
984+
"Accept": "application/json, text/event-stream",
985+
"Content-Type": "application/json",
986+
},
987+
json=INIT_REQUEST,
988+
)
989+
assert init_response.status_code == 200
990+
headers = {
991+
"Accept": "application/json, text/event-stream",
992+
"Content-Type": "application/json",
993+
MCP_SESSION_ID_HEADER: init_response.headers[MCP_SESSION_ID_HEADER],
994+
MCP_PROTOCOL_VERSION_HEADER: extract_protocol_version_from_sse(init_response),
995+
}
996+
997+
for _ in range(2):
998+
response = await client.post(
999+
"/mcp",
1000+
headers=headers,
1001+
json={
1002+
"jsonrpc": "2.0",
1003+
"id": 1,
1004+
"method": "tools/call",
1005+
"params": {"name": "test_tool", "arguments": {}},
1006+
},
1007+
)
1008+
assert response.status_code == 200
1009+
body = first_sse_data(response)
1010+
assert body["id"] == 1
1011+
assert body["result"]["content"][0]["text"] == "Called test_tool"
1012+
1013+
1014+
@pytest.mark.anyio
1015+
async def test_duplicate_request_id_rejected_during_priming_event_store() -> None:
1016+
"""The duplicate-id guard holds while the event store persists the priming event.
1017+
1018+
With an event store, the SSE branch awaits `EventStore.store_event` before the
1019+
response starts. The routing slot is reserved before that await so a concurrent
1020+
POST reusing the id cannot slip past the guard and overwrite the slot (#3137).
1021+
"""
1022+
1023+
class GatedEventStore(SimpleEventStore):
1024+
"""Blocks the priming write for the gated stream until released."""
1025+
1026+
def __init__(self) -> None:
1027+
super().__init__()
1028+
self.entered = anyio.Event()
1029+
self.release = anyio.Event()
1030+
1031+
async def store_event(self, stream_id: StreamId, message: types.JSONRPCMessage | None) -> EventId:
1032+
if stream_id == "gated-1" and message is None:
1033+
self.entered.set()
1034+
await self.release.wait()
1035+
return await super().store_event(stream_id, message)
1036+
1037+
def first_message_sse_data(response: httpx2.Response) -> dict[str, Any]:
1038+
"""First non-empty SSE data payload; skips the empty-data priming event."""
1039+
for line in response.text.splitlines():
1040+
if line.startswith("data: ") and line.removeprefix("data: ").strip():
1041+
return json.loads(line.removeprefix("data: "))
1042+
raise ValueError("No message data event in SSE response") # pragma: no cover
1043+
1044+
store = GatedEventStore()
1045+
async with running_app(event_store=store) as app:
1046+
async with make_client(app) as client:
1047+
# Priming events require protocol >= 2025-11-25.
1048+
init_request = {
1049+
"jsonrpc": "2.0",
1050+
"method": "initialize",
1051+
"params": {
1052+
"clientInfo": {"name": "test-client", "version": "1.0"},
1053+
"protocolVersion": types.LATEST_PROTOCOL_VERSION,
1054+
"capabilities": {},
1055+
},
1056+
"id": "init-1",
1057+
}
1058+
init_response = await client.post(
1059+
"/mcp",
1060+
headers={
1061+
"Accept": "application/json, text/event-stream",
1062+
"Content-Type": "application/json",
1063+
},
1064+
json=init_request,
1065+
)
1066+
assert init_response.status_code == 200
1067+
headers = {
1068+
"Accept": "application/json, text/event-stream",
1069+
"Content-Type": "application/json",
1070+
MCP_SESSION_ID_HEADER: init_response.headers[MCP_SESSION_ID_HEADER],
1071+
MCP_PROTOCOL_VERSION_HEADER: first_message_sse_data(init_response)["result"]["protocolVersion"],
1072+
}
1073+
call: dict[str, Any] = {
1074+
"jsonrpc": "2.0",
1075+
"id": "gated-1",
1076+
"method": "tools/call",
1077+
"params": {"name": "test_tool", "arguments": {}},
1078+
}
1079+
results: dict[str, httpx2.Response] = {}
1080+
1081+
async def post_first() -> None:
1082+
results["first"] = await client.post("/mcp", headers=headers, json=call)
1083+
1084+
async with anyio.create_task_group() as tg:
1085+
tg.start_soon(post_first)
1086+
with anyio.fail_after(5):
1087+
await store.entered.wait()
1088+
1089+
# Without early slot reservation this POST would also suspend in the
1090+
# gated store and hang; fail_after turns that regression into a fast fail.
1091+
with anyio.fail_after(5):
1092+
duplicate = await client.post("/mcp", headers=headers, json=call)
1093+
assert duplicate.status_code == 409
1094+
error = duplicate.json()["error"]
1095+
assert error["code"] == INVALID_REQUEST
1096+
assert "already in flight" in error["message"]
1097+
1098+
store.release.set()
1099+
1100+
assert results["first"].status_code == 200
1101+
body = first_message_sse_data(results["first"])
1102+
assert body["id"] == "gated-1"
1103+
assert body["result"]["content"][0]["text"] == "Called test_tool"
1104+
1105+
8111106
@pytest.mark.anyio
8121107
async def test_get_validation(basic_app: Starlette) -> None:
8131108
"""A GET without an Accept header covering text/event-stream is rejected with 406."""

0 commit comments

Comments
 (0)