Skip to content

Commit 5ce308b

Browse files
committed
Deliver subscriptions/listen notifications only after the acknowledgement
## Motivation and Context Per SEP-2575, the server MUST NOT send any notification on a subscription before `notifications/subscriptions/acknowledged`. The listen stream was registered in the delivery registry before its acknowledgement was written (the write happens outside the lock so a stalled reader cannot block the transport), so a change notification published concurrently from another thread could be written to the stream ahead of the acknowledgement. The entry is now inserted inactive - still reserving the id and the cap slot atomically - and becomes eligible for delivery only after the acknowledgement write has completed. A concurrent notification inside the window skips the inactive entry, which is indistinguishable from the change happening just before the subscription opened. ## How Has This Been Tested? With a new regression test pinning the window (an entry injected in the registered-but-unacknowledged state receives nothing until activation), the full suite, RuboCop, and the conformance suite, all green. ## Breaking Changes None. Activation happens synchronously inside the same response body proc, so an acknowledged stream behaves exactly as before.
1 parent 2a31e0b commit 5ce308b

2 files changed

Lines changed: 40 additions & 2 deletions

File tree

lib/mcp/server/transports/streamable_http_transport.rb

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ def initialize(
162162
@allowed_origins = Array(allowed_origins).map(&:downcase).freeze
163163
@pending_responses = {}
164164

165-
# Maps a `subscriptions/listen` request id to `{ stream: stream_object, filter: honored_subscription_filter }` (SEP-2575).
165+
# Maps a `subscriptions/listen` request id to `{ stream: stream_object, filter: honored_subscription_filter, active: boolean }` (SEP-2575).
166166
# In-process only; a multi-worker deployment needs an external event bus to fan notifications out across processes,
167167
# which is a follow-up.
168168
@listen_subscriptions = {}
@@ -883,6 +883,12 @@ def too_many_listen_subscriptions_response(request_id)
883883

884884
# The proc registers the stream and returns, leaving the response open like
885885
# the legacy GET stream (`create_sse_body`).
886+
#
887+
# Registration and activation are split on purpose: the entry is inserted inactive
888+
# (reserving the id and the cap slot atomically), the acknowledgement is written outside the lock,
889+
# and only then does the entry become eligible for delivery. A concurrent notification between
890+
# the insert and the acknowledgement write skips the inactive entry,
891+
# enforcing the SEP-2575 rule that no notification precedes the acknowledgement.
886892
def listen_sse_body(request_id, honored)
887893
proc do |stream|
888894
rejected = false
@@ -891,7 +897,7 @@ def listen_sse_body(request_id, honored)
891897
(@max_listen_subscriptions && @listen_subscriptions.size >= @max_listen_subscriptions)
892898
rejected = true
893899
else
894-
@listen_subscriptions[request_id] = { stream: stream, filter: honored }
900+
@listen_subscriptions[request_id] = { stream: stream, filter: honored, active: false }
895901
end
896902
end
897903

@@ -909,6 +915,7 @@ def listen_sse_body(request_id, honored)
909915

910916
begin
911917
send_to_stream(stream, acknowledgement)
918+
activate_listen_subscription(request_id)
912919
start_listen_keepalive_thread(request_id)
913920
rescue *STREAM_WRITE_ERRORS
914921
remove_listen_subscription(request_id)
@@ -918,6 +925,15 @@ def listen_sse_body(request_id, honored)
918925
end
919926
end
920927

928+
# Marks a listen subscription eligible for delivery once its acknowledgement write has completed.
929+
# The entry may already be gone when the transport closed concurrently.
930+
def activate_listen_subscription(request_id)
931+
@mutex.synchronize do
932+
subscription = @listen_subscriptions[request_id]
933+
subscription[:active] = true if subscription
934+
end
935+
end
936+
921937
# Periodically writes an SSE keepalive comment frame to a listen stream so a silently dropped
922938
# connection is detected and its slot freed, rather than held until the next fan-out write.
923939
# Mirrors the legacy GET stream's `start_keepalive_thread`; a comment frame (not a data frame)
@@ -999,6 +1015,10 @@ def deliver_to_listen_subscriptions(method, params)
9991015
# a slow or stalled subscriber must not block the transport, matching the legacy delivery paths.
10001016
matched = @mutex.synchronize do
10011017
@listen_subscriptions.filter_map do |request_id, subscription|
1018+
# An inactive entry has not finished writing its acknowledgement yet;
1019+
# delivering to it would put a notification ahead of the acknowledgement.
1020+
next unless subscription[:active]
1021+
10021022
hit = if field
10031023
subscription[:filter][field]
10041024
else

test/mcp/server/transports/streamable_http_transport_test.rb

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6108,6 +6108,24 @@ def string
61086108
transport.close
61096109
end
61106110

6111+
test "notifications are delivered only after the listen acknowledgement" do
6112+
# Inject an entry in the registered-but-not-yet-acknowledged state: the window between
6113+
# the registry insert and the acknowledgement write, which happens outside the lock.
6114+
io = StringIO.new
6115+
@transport.instance_variable_get(:@listen_subscriptions)["listen-1"] = {
6116+
stream: io, filter: { toolsListChanged: true }, active: false
6117+
}
6118+
6119+
@server.notify_tools_list_changed
6120+
6121+
assert_empty sse_events(io)
6122+
6123+
@transport.send(:activate_listen_subscription, "listen-1")
6124+
@server.notify_tools_list_changed
6125+
6126+
assert_equal ["notifications/tools/list_changed"], sse_events(io).map { |event| event["method"] }
6127+
end
6128+
61116129
test "subscriptions/listen streams for different subscriptions receive their own subscriptionId" do
61126130
first = open_listen_stream(id: "listen-1", notifications: { toolsListChanged: true })
61136131
second = open_listen_stream(id: "listen-2", notifications: { toolsListChanged: true })

0 commit comments

Comments
 (0)