Skip to content

Commit cfe2fe7

Browse files
committed
Reject duplicate in-flight JSON-RPC request IDs
## Motivation and Context A request id is the only key that routes request-scoped messages back to the request that caused them: progress and log notifications, server-to-client requests such as sampling and elicitation, and `notifications/cancelled` all resolve through it. Nothing checked that the id was free, so a second request arriving under an id already in flight took the routing entry over. From that point the first request's notifications were written to the second request's SSE stream, and whichever finished first removed the shared entry, leaving the survivor's messages nowhere to go. The spec puts the uniqueness obligation on the sender, and 2026-07-28 draws it at exactly the window this change enforces: "The request ID MUST NOT match the ID of any other request the sender has issued and not yet received a response for". Earlier revisions worded it as never reusing an id within the session. Neither says what a receiver does with a duplicate, but the Streamable HTTP rules require that messages the server sends before the response "SHOULD relate to the originating client request", and two live requests sharing one id make that impossible to honor for either of them. Answering the second one is the only option that stays correct, so it is refused the same way a duplicate `initialize` is, and for the same reason: a repeated id must not silently displace state the first one established. The reference SDKs currently accept the duplicate and let the newer request take the entry. That is not a settled design: the Python SDK carries a TODO naming rejection with `INVALID_REQUEST` as the revisit, so this moves toward where they expect to end up rather than away from them. `ServerSession#register_in_flight` now claims the id atomically and reports a collision instead of overwriting, which `Server` turns into an Invalid Request for every transport. `StreamableHTTPTransport` refuses the colliding POST with 409 before it registers a stream, since the stream is registered ahead of dispatch and would otherwise take over the routing entry for as long as the rejection takes. Removal is now guarded by identity everywhere the registry is keyed by request id, so a request that loses a race cannot retire a registration it does not own. `initialize` never registers, so its `ensure` no longer unregisters: a refused duplicate `initialize` reusing an in-flight id used to evict that registration on the way out, silently disabling cancellation for the request that owned it (Streamable HTTP refuses such a POST before dispatch; stdio reached this path). `drop_broken_stream` had a sharper form of the same bug: it removed whichever stream held the id while closing the one passed in, which are not necessarily the same stream. Only ids that are in flight are refused, which is the scope the current spec draws. Enforcing the older "never within the session" wording would instead mean remembering every id a session ever used, and the in-flight window is the part routing depends on either way. Sequential reuse keeps working, and neither this SDK's client (`SecureRandom.uuid`) nor a client built on the TypeScript SDK (a per-connection counter) can produce a collision. ## How Has This Been Tested? New tests in `test/mcp/server/transports/streamable_http_transport_test.rb` reproduce the collision: a tool parked mid-request, a second POST reusing its id, and assertions that the second POST is refused and that the first request still receives the progress frame it emits afterwards. Others cover the identity-guarded removal and confirm that an id can be reused once the earlier request has finished. New tests in `test/mcp/server_cancellation_test.rb` cover the registry directly, including the transport-independent Invalid Request and the survival of an in-flight registration across a refused duplicate `initialize` under the same id. All of them except the sequential-reuse guard fail without this change. `bundle exec rake` (tests, RuboCop, and conformance baseline) passes. ## Breaking Changes A request whose id is already in flight on the same session is now answered with Invalid Request (HTTP 409 on Streamable HTTP) instead of being processed. A client that reuses ids concurrently was already violating the specification and was already having its notifications misrouted.
1 parent ad73154 commit cfe2fe7

5 files changed

Lines changed: 286 additions & 12 deletions

File tree

lib/mcp/server.rb

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -619,8 +619,24 @@ def handle_request(request, method, session: nil, related_request_id: nil)
619619

620620
# `initialize` MUST NOT be cancelled (MCP spec 2025-11-25, cancellation item 2),
621621
# so do not track it in the in-flight registry.
622-
cancellation = if related_request_id && method != Methods::INITIALIZE
623-
session&.register_in_flight(related_request_id)
622+
cancellation = nil
623+
if related_request_id && method != Methods::INITIALIZE && session
624+
cancellation = session.register_in_flight(related_request_id)
625+
626+
# The spec puts the uniqueness obligation on the sender - "The request ID MUST NOT have been previously used by
627+
# the requestor within the same session" - and says nothing about what a receiver does with a duplicate.
628+
# Answering one is the only option that stays correct: the id routes request-scoped messages back to
629+
# the request that caused them, and the transport's rule is that those messages "SHOULD relate to
630+
# the originating client request", which a second live request under the same id makes impossible to honor
631+
# for either of them. Refused the same way a duplicate `initialize` is, and for the same reason:
632+
# so that a repeated id cannot silently displace state negotiated by the first one.
633+
if cancellation.nil?
634+
raise RequestHandlerError.new(
635+
"Invalid Request: request id #{related_request_id.inspect} is already in flight",
636+
request,
637+
error_type: :invalid_request,
638+
)
639+
end
624640
end
625641

626642
->(params) {
@@ -727,7 +743,10 @@ def handle_request(request, method, session: nil, related_request_id: nil)
727743
reported_exception = wrapped
728744
raise wrapped
729745
ensure
730-
session&.unregister_in_flight(related_request_id) if related_request_id
746+
# `cancellation` is non-nil exactly when this request claimed the id above, so this also keeps `initialize`
747+
# (which never registers) from evicting an in-flight registration under a reused id when the duplicate-`initialize`
748+
# refusal raises out of the handler.
749+
session&.unregister_in_flight(related_request_id, cancellation: cancellation) if related_request_id && cancellation
731750
end
732751
}
733752
end

lib/mcp/server/transports/streamable_http_transport.rb

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -450,8 +450,12 @@ def drop_broken_stream(session_id, stream, related_request_id)
450450

451451
@mutex.synchronize do
452452
session = @sessions[session_id]
453-
if related_request_id && session&.dig(:post_request_streams, related_request_id)
454-
session[:post_request_streams].delete(related_request_id)
453+
if related_request_id
454+
# Unregister only our own stream: removing on the id alone would drop whichever stream currently holds it,
455+
# which is not necessarily the one that failed. The failed stream is closed either way, and a request-scoped
456+
# failure never reaches the session teardown below.
457+
registered = session&.dig(:post_request_streams, related_request_id)
458+
session[:post_request_streams].delete(related_request_id) if registered.equal?(stream)
455459
streams_to_close << stream
456460
else
457461
cleanup_and_collect_stream(session_id, streams_to_close)
@@ -1595,6 +1599,14 @@ def handle_regular_request(body_string, session_id, related_request_id: nil)
15951599
end
15961600
end
15971601

1602+
# `Server` refuses a duplicate id as well, but only once the request reaches it. The SSE branch below
1603+
# registers this request's stream under that id first, so without this check the colliding request
1604+
# would take over the routing entry for the moment it takes to be rejected, and its `ensure` would then
1605+
# clear the entry the original request still needs.
1606+
if related_request_id && server_session&.in_flight?(related_request_id)
1607+
return request_id_conflict_response
1608+
end
1609+
15981610
if session_id && !@stateless && !@enable_json_response
15991611
handle_request_with_sse_response(body_string, session_id, server_session, related_request_id: related_request_id)
16001612
else
@@ -1618,7 +1630,11 @@ def handle_request_with_sse_response(body_string, session_id, server_session, re
16181630
session = @sessions[session_id]
16191631
if session && related_request_id
16201632
session[:post_request_streams] ||= {}
1621-
session[:post_request_streams][related_request_id] = stream
1633+
1634+
# Claim the id only while it is free. `handle_regular_request` already refused the colliding request,
1635+
# so reaching an occupied slot means a race got past that check; leaving the first stream in place keeps
1636+
# its messages going where they belong.
1637+
session[:post_request_streams][related_request_id] ||= stream
16221638
end
16231639
end
16241640

@@ -1630,7 +1646,11 @@ def handle_request_with_sse_response(body_string, session_id, server_session, re
16301646
if related_request_id
16311647
@mutex.synchronize do
16321648
session = @sessions[session_id]
1633-
session[:post_request_streams]&.delete(related_request_id) if session
1649+
# Only retire our own registration: a request that never claimed the id, or one whose claim has
1650+
# already been replaced, must not unregister the stream that owns it.
1651+
registered = session&.dig(:post_request_streams, related_request_id)
1652+
1653+
session[:post_request_streams].delete(related_request_id) if registered.equal?(stream)
16341654
end
16351655
end
16361656

@@ -1849,6 +1869,16 @@ def session_already_connected_response
18491869
)
18501870
end
18511871

1872+
# The POST counterpart of the GET conflict above. A request id already in flight cannot be given
1873+
# a stream of its own, because the id is what routes request-scoped messages back.
1874+
def request_id_conflict_response
1875+
json_rpc_error_response(
1876+
status: 409,
1877+
code: JsonRpcHandler::ErrorCode::INVALID_REQUEST,
1878+
message: "Conflict: Request id is already in flight for this session",
1879+
)
1880+
end
1881+
18521882
def setup_sse_stream(session_id)
18531883
body = create_sse_body(session_id)
18541884

lib/mcp/server_session.rb

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,19 +58,41 @@ def lock_era!(era)
5858
@era = era
5959
end
6060

61-
# Registers a `Cancellation` token for an in-flight request.
61+
# Registers a `Cancellation` token for an in-flight request, or returns `nil` when `request_id` is already in flight.
62+
# The request id is the only key that routes request-scoped notifications, server-to-client requests,
63+
# and `notifications/cancelled` back to the request that caused them, so a second live request under the same id
64+
# has no destination of its own. Rather than let the newcomer displace the registration, report the collision
65+
# and leave the first request intact; the caller turns that into an Invalid Request.
6266
def register_in_flight(request_id)
6367
return if request_id.nil?
6468

6569
cancellation = Cancellation.new(request_id: request_id)
66-
@in_flight_mutex.synchronize { @in_flight[request_id] = cancellation }
67-
cancellation
70+
registered = @in_flight_mutex.synchronize do
71+
next false if @in_flight.key?(request_id)
72+
73+
@in_flight[request_id] = cancellation
74+
true
75+
end
76+
77+
registered ? cancellation : nil
6878
end
6979

70-
def unregister_in_flight(request_id)
80+
# Removes an in-flight registration. Passing the `Cancellation` that `register_in_flight` returned removes
81+
# the entry only while it is still that one, so a request can never evict a registration it does not own.
82+
def unregister_in_flight(request_id, cancellation: nil)
7183
return if request_id.nil?
7284

73-
@in_flight_mutex.synchronize { @in_flight.delete(request_id) }
85+
@in_flight_mutex.synchronize do
86+
next if cancellation && !@in_flight[request_id].equal?(cancellation)
87+
88+
@in_flight.delete(request_id)
89+
end
90+
end
91+
92+
# Whether `request_id` is currently in flight, so a transport can refuse a colliding request
93+
# before registering any state of its own for it.
94+
def in_flight?(request_id)
95+
!lookup_in_flight(request_id).nil?
7496
end
7597

7698
def lookup_in_flight(request_id)

test/mcp/server/transports/streamable_http_transport_test.rb

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3431,6 +3431,110 @@ def string
34313431
assert_equal "ok", result[:content][:text]
34323432
end
34333433

3434+
test "refuses a POST reusing an in-flight request id and keeps the original stream" do
3435+
server = Server.new(name: "test", tools: [], prompts: [], resources: [])
3436+
transport = StreamableHTTPTransport.new(server)
3437+
server.transport = transport
3438+
3439+
gate = Queue.new
3440+
server.define_tool(name: "victim_tool") do |server_context:|
3441+
server_context.report_progress(1, message: "first-frame")
3442+
gate.pop
3443+
server_context.report_progress(2, message: "second-frame")
3444+
Tool::Response.new([{ type: "text", text: "done" }])
3445+
end
3446+
3447+
session_id, server_session = start_session(transport)
3448+
3449+
victim = transport.handle_request(colliding_tool_call(session_id, "req-1"))
3450+
victim_stream = TestStream.new
3451+
victim_thread = Thread.new { victim[2].call(victim_stream) }
3452+
sleep(0.01) until server_session.lookup_in_flight("req-1")
3453+
3454+
attacker = transport.handle_request(colliding_tool_call(session_id, "req-1"))
3455+
3456+
assert_equal 409, attacker[0]
3457+
assert_equal(
3458+
JsonRpcHandler::ErrorCode::INVALID_REQUEST,
3459+
JSON.parse(attacker[2][0]).dig("error", "code"),
3460+
)
3461+
3462+
gate << :go
3463+
victim_thread.join
3464+
3465+
# The registration survived the refusal, so the frame emitted after it still reaches
3466+
# the request that asked for it.
3467+
assert_includes victim_stream.string, "first-frame"
3468+
assert_includes victim_stream.string, "second-frame"
3469+
end
3470+
3471+
test "a finishing request leaves a stream another request registered under the same id" do
3472+
server = Server.new(name: "test", tools: [], prompts: [], resources: [])
3473+
transport = StreamableHTTPTransport.new(server)
3474+
server.transport = transport
3475+
3476+
gate = Queue.new
3477+
server.define_tool(name: "victim_tool") do |server_context:|
3478+
gate.pop
3479+
Tool::Response.new([{ type: "text", text: "done" }])
3480+
end
3481+
3482+
session_id, server_session = start_session(transport)
3483+
3484+
victim = transport.handle_request(colliding_tool_call(session_id, "req-1"))
3485+
victim_stream = TestStream.new
3486+
victim_thread = Thread.new { victim[2].call(victim_stream) }
3487+
sleep(0.01) until server_session.lookup_in_flight("req-1")
3488+
3489+
# Stand in for a stream that won the registration in a race the pre-dispatch refusal normally prevents.
3490+
# Finishing the other request must not unregister it.
3491+
foreign_stream = TestStream.new
3492+
sessions = transport.instance_variable_get(:@sessions)
3493+
sessions[session_id][:post_request_streams]["req-1"] = foreign_stream
3494+
3495+
gate << :go
3496+
victim_thread.join
3497+
3498+
assert_same foreign_stream, sessions[session_id][:post_request_streams]["req-1"]
3499+
end
3500+
3501+
test "a broken request-scoped stream drops only itself, even when it is not the registered one" do
3502+
server = Server.new(name: "test", tools: [], prompts: [], resources: [])
3503+
transport = StreamableHTTPTransport.new(server)
3504+
server.transport = transport
3505+
3506+
session_id, = start_session(transport)
3507+
sessions = transport.instance_variable_get(:@sessions)
3508+
registered = TestStream.new
3509+
sessions[session_id][:post_request_streams] = { "req-1" => registered }
3510+
3511+
transport.send(:drop_broken_stream, session_id, TestStream.new, "req-1")
3512+
3513+
assert sessions.key?(session_id), "a request-scoped failure must not tear down the session"
3514+
assert_same registered, sessions[session_id][:post_request_streams]["req-1"]
3515+
end
3516+
3517+
test "allows a request id to be reused once the earlier request has finished" do
3518+
server = Server.new(name: "test", tools: [], prompts: [], resources: [])
3519+
transport = StreamableHTTPTransport.new(server)
3520+
server.transport = transport
3521+
3522+
server.define_tool(name: "victim_tool") do |server_context:|
3523+
Tool::Response.new([{ type: "text", text: "done" }])
3524+
end
3525+
3526+
session_id, = start_session(transport)
3527+
3528+
2.times do
3529+
response = transport.handle_request(colliding_tool_call(session_id, "req-1"))
3530+
3531+
assert_equal 200, response[0]
3532+
stream = TestStream.new
3533+
response[2].call(stream)
3534+
assert_includes stream.string, "done"
3535+
end
3536+
end
3537+
34343538
test "JSON response mode returns accepted when cancellation suppresses response" do
34353539
server = Server.new(name: "test", tools: [], prompts: [], resources: [])
34363540
transport = StreamableHTTPTransport.new(server, enable_json_response: true)
@@ -6199,6 +6303,36 @@ def install_mutex_probe_stream(session_id, related_request_id: nil)
61996303
writes
62006304
end
62016305

6306+
# Initializes `transport` and returns its session id together with the `ServerSession`,
6307+
# which the in-flight request id tests poll to know when a request has really started.
6308+
def start_session(transport)
6309+
request = create_rack_request(
6310+
"POST",
6311+
"/",
6312+
{ "CONTENT_TYPE" => "application/json" },
6313+
{ jsonrpc: "2.0", method: "initialize", id: "init", params: initialize_params }.to_json,
6314+
)
6315+
session_id = transport.handle_request(request)[1]["mcp-session-id"]
6316+
6317+
[session_id, transport.instance_variable_get(:@sessions)[session_id][:server_session]]
6318+
end
6319+
6320+
# A `tools/call` for `victim_tool` under a caller-chosen request id, with a progress token so
6321+
# the tool's `report_progress` has somewhere to go.
6322+
def colliding_tool_call(session_id, request_id)
6323+
create_rack_request(
6324+
"POST",
6325+
"/",
6326+
{ "CONTENT_TYPE" => "application/json", "HTTP_MCP_SESSION_ID" => session_id },
6327+
{
6328+
jsonrpc: "2.0",
6329+
id: request_id,
6330+
method: "tools/call",
6331+
params: { name: "victim_tool", arguments: {}, _meta: { progressToken: "tok" } },
6332+
}.to_json,
6333+
)
6334+
end
6335+
62026336
def create_rack_request(method, path, headers, body = nil)
62036337
default_accept = case method
62046338
when "POST"

test/mcp/server_cancellation_test.rb

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
module MCP
66
class ServerCancellationTest < ActiveSupport::TestCase
77
include InstrumentationTestHelper
8+
include InitializeParamsTestHelper
89

910
class MockTransport < Transport
1011
attr_reader :requests, :notifications, :cancelled_request_ids
@@ -333,6 +334,74 @@ def handle_request(request); end
333334
assert_includes @mock_transport.cancelled_request_ids, "req-9"
334335
end
335336

337+
test "register_in_flight refuses an id that is already in flight" do
338+
first = @session.register_in_flight("req-1")
339+
340+
assert first
341+
assert_nil @session.register_in_flight("req-1"), "a second live request cannot share the id"
342+
assert_same first, @session.lookup_in_flight("req-1"), "the first registration must survive"
343+
end
344+
345+
test "unregister_in_flight leaves a registration it does not own" do
346+
owner = @session.register_in_flight("req-1")
347+
@session.unregister_in_flight("req-1", cancellation: Cancellation.new(request_id: "req-1"))
348+
349+
assert_same owner, @session.lookup_in_flight("req-1")
350+
351+
@session.unregister_in_flight("req-1", cancellation: owner)
352+
353+
assert_nil @session.lookup_in_flight("req-1")
354+
end
355+
356+
test "in_flight? reports whether an id is registered" do
357+
refute @session.in_flight?("req-1")
358+
359+
@session.register_in_flight("req-1")
360+
361+
assert @session.in_flight?("req-1")
362+
end
363+
364+
test "a request reusing an in-flight id is answered with Invalid Request" do
365+
@server.define_tool(name: "slow") do |server_context:|
366+
sleep(0.2)
367+
Tool::Response.new([{ type: "text", text: "ok" }])
368+
end
369+
370+
request = {
371+
jsonrpc: "2.0",
372+
id: "req-1",
373+
method: Methods::TOOLS_CALL,
374+
params: { name: "slow", arguments: {} },
375+
}
376+
377+
in_flight = Thread.new { @session.handle(request) }
378+
sleep(0.01) until @session.lookup_in_flight("req-1")
379+
380+
duplicate = @session.handle(request)
381+
382+
assert_equal(
383+
JsonRpcHandler::ErrorCode::INVALID_REQUEST,
384+
duplicate.dig(:error, :code) || duplicate.dig("error", "code"),
385+
)
386+
in_flight.join
387+
end
388+
389+
test "a refused duplicate initialize leaves an in-flight registration under its reused id" do
390+
@session.handle(jsonrpc: "2.0", id: "init", method: Methods::INITIALIZE, params: initialize_params)
391+
392+
owner = @session.register_in_flight("req-1")
393+
394+
# `initialize` bypasses the duplicate-id refusal (it is never in flight itself), so its
395+
# rejection path is the one place a reused id reaches a handler while the id is still live.
396+
duplicate = @session.handle(jsonrpc: "2.0", id: "req-1", method: Methods::INITIALIZE, params: initialize_params)
397+
398+
assert_equal(
399+
JsonRpcHandler::ErrorCode::INVALID_REQUEST,
400+
duplicate.dig(:error, :code) || duplicate.dig("error", "code"),
401+
)
402+
assert_same owner, @session.lookup_in_flight("req-1"), "the registration must survive the refused initialize"
403+
end
404+
336405
test "parent cancellation propagates to nested server-to-client requests" do
337406
@session.instance_variable_set(:@client_capabilities, { elicitation: {} })
338407

0 commit comments

Comments
 (0)