Skip to content

Commit 47f7a39

Browse files
committed
fix(server): type-preserve streamable HTTP request stream keys
Route per-request streams with i:/s:-prefixed keys so integer and string JSON-RPC ids stay distinct, and so a string id cannot share the standalone GET slot. Mention notifications/cancelled in the 409 conflict message. Cover both cases with regression tests.
1 parent 5fd834e commit 47f7a39

2 files changed

Lines changed: 191 additions & 18 deletions

File tree

src/mcp/server/streamable_http.py

Lines changed: 40 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,32 @@
5757
CONTENT_TYPE_JSON = "application/json"
5858
CONTENT_TYPE_SSE = "text/event-stream"
5959

60-
# Special key for the standalone GET stream
60+
# Type aliases
61+
StreamId = str
62+
EventId = str
63+
# An SSE event-dict as accepted by sse-starlette (`event`, `data`, `id`, `retry`).
64+
SSEEvent = dict[str, Any]
65+
66+
# Special key for the standalone GET stream. Request routing keys never equal
67+
# this value (see `_request_stream_key`), so a JSON-RPC string id of
68+
# `"_GET_stream"` cannot lock out or share the standalone GET slot.
6169
GET_STREAM_KEY = "_GET_stream"
6270

71+
72+
def _request_stream_key(request_id: RequestId) -> StreamId:
73+
"""Type-preserving routing key for per-request streams and the event store.
74+
75+
JSON-RPC treats integer ``1`` and string ``\"1\"`` as distinct ids. Using
76+
``str(id)`` collapses them (so an in-flight numeric id falsely 409s a
77+
string id, or cross-wires responses) and also lets a string id equal to
78+
``GET_STREAM_KEY`` share the standalone GET slot.
79+
"""
80+
# RequestId is int | str; exclude bool (a subclass of int) defensively.
81+
if type(request_id) is int:
82+
return f"i:{request_id}"
83+
return f"s:{request_id}"
84+
85+
6386
# Buffer for the per-request `_request_streams` so the serial `message_router`
6487
# can deposit a response and move on instead of head-of-line blocking the
6588
# whole session on a lazily-started `sse_writer`. See #1764.
@@ -76,12 +99,6 @@
7699
# Pattern ensures entire string contains only valid characters by using ^ and $ anchors
77100
SESSION_ID_PATTERN = re.compile(r"^[\x21-\x7E]+$")
78101

79-
# Type aliases
80-
StreamId = str
81-
EventId = str
82-
# An SSE event-dict as accepted by sse-starlette (`event`, `data`, `id`, `retry`).
83-
SSEEvent = dict[str, Any]
84-
85102

86103
def check_accept_headers(request: Request) -> tuple[bool, bool]:
87104
"""Return (has_json, has_sse) for the request's Accept header, with RFC 7231 wildcard handling.
@@ -598,16 +615,22 @@ async def _handle_post_request(self, scope: Scope, request: Request, receive: Re
598615
else request.headers.get(MCP_PROTOCOL_VERSION_HEADER, DEFAULT_NEGOTIATED_VERSION)
599616
)
600617

601-
request_id = str(message.id)
602-
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.
618+
# Type-preserving routing key so int 1 and str "1" stay distinct, and so
619+
# a string id never collides with GET_STREAM_KEY. The membership test and
620+
# the assignment below must not have an await between them: with
621+
# resumability on, EventStore.store_event is user code and any await
622+
# re-opens a check-then-act window (#3137 / #3163).
623+
request_id = _request_stream_key(message.id)
624+
625+
# A second concurrent POST reusing an in-flight id would overwrite that
626+
# slot and cross-wire responses (one caller receives the other's payload;
627+
# the other hangs). Reject with 409 rather than silently re-routing. Ids
628+
# may still be reused after the earlier request has completed; clients
629+
# that retry after a timeout should send notifications/cancelled first.
608630
if request_id in self._request_streams:
609631
response = self._create_error_response(
610-
f"Conflict: Request id {request_id} is already in flight for this session",
632+
f"Conflict: Request id {message.id!r} is already in flight for this "
633+
f"session; send notifications/cancelled before reusing the id",
611634
HTTPStatus.CONFLICT,
612635
)
613636
await response(scope, receive, send)
@@ -1039,7 +1062,7 @@ async def message_router():
10391062
# Null-id errors (e.g., parse errors) fall through to
10401063
# the GET stream since they can't be correlated.
10411064
if isinstance(message, JSONRPCResponse | JSONRPCError) and message.id is not None:
1042-
target_request_id = str(message.id)
1065+
target_request_id = _request_stream_key(message.id)
10431066
# Extract related_request_id from meta if it exists
10441067
elif (
10451068
session_message.metadata is not None
@@ -1056,7 +1079,7 @@ async def message_router():
10561079
# storing or queueing rather than park it (#1764).
10571080
logger.debug(f"Dropped message related to request {related_request_id} in JSON mode")
10581081
continue
1059-
target_request_id = str(related_request_id)
1082+
target_request_id = _request_stream_key(related_request_id)
10601083

10611084
request_stream_id = target_request_id if target_request_id is not None else GET_STREAM_KEY
10621085

tests/shared/test_streamable_http.py

Lines changed: 151 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1011,6 +1011,155 @@ async def test_request_id_reuse_after_completion_allowed(basic_app: Starlette) -
10111011
assert body["result"]["content"][0]["text"] == "Called test_tool"
10121012

10131013

1014+
@pytest.mark.anyio
1015+
async def test_numeric_and_string_request_ids_do_not_collide_while_in_flight(basic_app: Starlette) -> None:
1016+
"""Integer id 1 and string id "1" are distinct JSON-RPC ids and may run concurrently.
1017+
1018+
Routing used to key streams with str(id), so an in-flight numeric 1 falsely
1019+
409'd a string "1" (or cross-wired responses). Raw HTTP is required to control
1020+
the JSON-RPC id types on the wire.
1021+
"""
1022+
async with make_client(basic_app) as client:
1023+
init_response = await client.post(
1024+
"/mcp",
1025+
headers={
1026+
"Accept": "application/json, text/event-stream",
1027+
"Content-Type": "application/json",
1028+
},
1029+
json=INIT_REQUEST,
1030+
)
1031+
assert init_response.status_code == 200
1032+
headers = {
1033+
"Accept": "application/json, text/event-stream",
1034+
"Content-Type": "application/json",
1035+
MCP_SESSION_ID_HEADER: init_response.headers[MCP_SESSION_ID_HEADER],
1036+
MCP_PROTOCOL_VERSION_HEADER: extract_protocol_version_from_sse(init_response),
1037+
}
1038+
1039+
async with client.stream(
1040+
"POST",
1041+
"/mcp",
1042+
headers=headers,
1043+
json={
1044+
"jsonrpc": "2.0",
1045+
"method": "tools/call",
1046+
"params": {"name": "wait_for_lock_with_notification", "arguments": {}},
1047+
"id": 1,
1048+
},
1049+
) as first_response:
1050+
assert first_response.status_code == 200
1051+
lines = first_response.aiter_lines()
1052+
with anyio.fail_after(5):
1053+
notification = await next_sse_data(lines)
1054+
assert notification["params"]["data"] == "First notification before lock"
1055+
1056+
string_id_response = await client.post(
1057+
"/mcp",
1058+
headers=headers,
1059+
json={
1060+
"jsonrpc": "2.0",
1061+
"method": "tools/call",
1062+
"params": {"name": "test_tool", "arguments": {}},
1063+
"id": "1",
1064+
},
1065+
)
1066+
assert string_id_response.status_code == 200
1067+
string_body = first_sse_data(string_id_response)
1068+
assert string_body["id"] == "1"
1069+
assert string_body["result"]["content"][0]["text"] == "Called test_tool"
1070+
1071+
release_response = await client.post(
1072+
"/mcp",
1073+
headers=headers,
1074+
json={
1075+
"jsonrpc": "2.0",
1076+
"method": "tools/call",
1077+
"params": {"name": "release_lock", "arguments": {}},
1078+
"id": "release-lock-type-key",
1079+
},
1080+
)
1081+
assert release_response.status_code == 200
1082+
1083+
with anyio.fail_after(5):
1084+
second_notification = await next_sse_data(lines)
1085+
final = await next_sse_data(lines)
1086+
assert second_notification["params"]["data"] == "Second notification after lock"
1087+
assert final["id"] == 1
1088+
assert final["result"]["content"][0]["text"] == "Completed"
1089+
1090+
1091+
@pytest.mark.anyio
1092+
async def test_request_id_matching_get_stream_key_does_not_block_standalone_get(basic_app: Starlette) -> None:
1093+
"""A POST with string id \"_GET_stream\" must not occupy the standalone GET slot.
1094+
1095+
Pre-fix, both used the bare string as the routing key, so a client could lock
1096+
itself out of GET (or have GET overwrite its POST slot).
1097+
"""
1098+
async with make_client(basic_app) as client:
1099+
init_response = await client.post(
1100+
"/mcp",
1101+
headers={
1102+
"Accept": "application/json, text/event-stream",
1103+
"Content-Type": "application/json",
1104+
},
1105+
json=INIT_REQUEST,
1106+
)
1107+
assert init_response.status_code == 200
1108+
session_id = init_response.headers[MCP_SESSION_ID_HEADER]
1109+
negotiated_version = extract_protocol_version_from_sse(init_response)
1110+
headers = {
1111+
"Accept": "application/json, text/event-stream",
1112+
"Content-Type": "application/json",
1113+
MCP_SESSION_ID_HEADER: session_id,
1114+
MCP_PROTOCOL_VERSION_HEADER: negotiated_version,
1115+
}
1116+
1117+
async with client.stream(
1118+
"POST",
1119+
"/mcp",
1120+
headers=headers,
1121+
json={
1122+
"jsonrpc": "2.0",
1123+
"method": "tools/call",
1124+
"params": {"name": "wait_for_lock_with_notification", "arguments": {}},
1125+
"id": "_GET_stream",
1126+
},
1127+
) as post_response:
1128+
assert post_response.status_code == 200
1129+
lines = post_response.aiter_lines()
1130+
with anyio.fail_after(5):
1131+
notification = await next_sse_data(lines)
1132+
assert notification["params"]["data"] == "First notification before lock"
1133+
1134+
get_headers = {
1135+
"Accept": "text/event-stream",
1136+
MCP_SESSION_ID_HEADER: session_id,
1137+
MCP_PROTOCOL_VERSION_HEADER: negotiated_version,
1138+
}
1139+
async with client.stream("GET", "/mcp", headers=get_headers) as get_response:
1140+
assert get_response.status_code == 200
1141+
assert get_response.headers.get("Content-Type") == "text/event-stream"
1142+
1143+
release_response = await client.post(
1144+
"/mcp",
1145+
headers=headers,
1146+
json={
1147+
"jsonrpc": "2.0",
1148+
"method": "tools/call",
1149+
"params": {"name": "release_lock", "arguments": {}},
1150+
"id": "release-after-get-key",
1151+
},
1152+
)
1153+
assert release_response.status_code == 200
1154+
1155+
with anyio.fail_after(5):
1156+
second_notification = await next_sse_data(lines)
1157+
final = await next_sse_data(lines)
1158+
assert second_notification["params"]["data"] == "Second notification after lock"
1159+
assert final["id"] == "_GET_stream"
1160+
assert final["result"]["content"][0]["text"] == "Completed"
1161+
1162+
10141163
@pytest.mark.anyio
10151164
async def test_duplicate_request_id_rejected_during_priming_event_store() -> None:
10161165
"""The duplicate-id guard holds while the event store persists the priming event.
@@ -1029,7 +1178,8 @@ def __init__(self) -> None:
10291178
self.release = anyio.Event()
10301179

10311180
async def store_event(self, stream_id: StreamId, message: types.JSONRPCMessage | None) -> EventId:
1032-
if stream_id == "gated-1" and message is None:
1181+
# Routing keys are type-tagged (`s:` for string JSON-RPC ids).
1182+
if stream_id == "s:gated-1" and message is None:
10331183
self.entered.set()
10341184
await self.release.wait()
10351185
return await super().store_event(stream_id, message)

0 commit comments

Comments
 (0)