Skip to content

Commit 2200cbd

Browse files
committed
Serialize subscriptions/listen writes so the graceful result is the final message
## Motivation and Context On graceful teardown (`transport.close`), each open `subscriptions/listen` stream receives its `SubscriptionsListenResult` response before closing, signaling a clean end the client can distinguish from an abrupt disconnect. But a delivery that snapshotted the stream from the registry before teardown cleared it performs its write outside the lock, so a change notification could land after the final result. Each subscription entry now carries a per-stream write mutex. Teardown marks the entry closed and writes the result under that mutex; a racing delivery either lands before the result or observes `closed` and skips, keeping the graceful result the stream's final message. The mutex is per stream and held only around the single write call, so deliveries to different streams stay as parallel as before, and no path nests it inside the transport mutex (delivery snapshots release `@mutex` first, and the write-error cleanup takes `@mutex` only after the write mutex is released), ruling out lock-order inversions. Keepalive pings stay outside the mutex: they are SSE comment frames, which clients ignore by specification, so one landing after the result is harmless. ## How Has This Been Tested? With a new regression test pinning the race (an entry captured before teardown and re-presented to delivery afterwards writes nothing after the result), the full suite, RuboCop, and the conformance suite, all green. ## Breaking Changes None. The write mutex is internal to the listen registry; the wire only gains the ordering guarantee.
1 parent 411f6b9 commit 2200cbd

2 files changed

Lines changed: 55 additions & 22 deletions

File tree

‎lib/mcp/server/transports/streamable_http_transport.rb‎

Lines changed: 36 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,8 @@ def initialize(
169169
@allowed_origins = Array(allowed_origins).map(&:downcase).freeze
170170
@pending_responses = {}
171171

172-
# Maps a `subscriptions/listen` request id to `{ stream: stream_object, filter: honored_subscription_filter, active: boolean }` (SEP-2575).
172+
# Maps a `subscriptions/listen` request id to
173+
# `{ stream: stream_object, filter: honored_subscription_filter, active: boolean, write_mutex: Mutex }` (SEP-2575).
173174
# In-process only; a multi-worker deployment needs an external event bus to fan notifications out across processes,
174175
# which is a follow-up.
175176
@listen_subscriptions = {}
@@ -936,7 +937,7 @@ def listen_sse_body(request_id, honored)
936937
(@max_listen_subscriptions && @listen_subscriptions.size >= @max_listen_subscriptions)
937938
rejected = true
938939
else
939-
@listen_subscriptions[request_id] = { stream: stream, filter: honored, active: false }
940+
@listen_subscriptions[request_id] = { stream: stream, filter: honored, active: false, write_mutex: Mutex.new }
940941
end
941942
end
942943

@@ -1066,24 +1067,32 @@ def deliver_to_listen_subscriptions(method, params)
10661067
uris.is_a?(Array) && uris.include?(uri)
10671068
end
10681069

1069-
[request_id, subscription[:stream]] if hit
1070+
[request_id, subscription] if hit
10701071
end
10711072
end
10721073

1073-
matched.each do |request_id, stream|
1074+
matched.each do |request_id, subscription|
10741075
meta = { RequestEnvelope::SUBSCRIPTION_ID_META_KEY.to_sym => request_id }
10751076
notification_params = (params || {}).merge(_meta: meta)
10761077
notification = { jsonrpc: "2.0", method: method, params: notification_params }
10771078

10781079
begin
1079-
send_to_stream(stream, notification)
1080+
# The per-stream write mutex orders this write against a concurrent graceful teardown:
1081+
# once teardown has marked the entry closed and written its `SubscriptionsListenResult`,
1082+
# a delivery that snapshotted the entry before the registry was cleared skips it instead
1083+
# of writing after the final message.
1084+
subscription[:write_mutex].synchronize do
1085+
next if subscription[:closed]
1086+
1087+
send_to_stream(subscription[:stream], notification)
1088+
end
10801089
rescue *STREAM_WRITE_ERRORS => e
10811090
MCP.configuration.exception_reporter.call(
10821091
e,
10831092
{ subscription_id: request_id, error: "Failed to send notification" },
10841093
)
10851094
remove_listen_subscription(request_id)
1086-
close_stream_safely(stream)
1095+
close_stream_safely(subscription[:stream])
10871096
end
10881097
end
10891098
end
@@ -1102,20 +1111,27 @@ def teardown_listen_subscriptions
11021111
end
11031112

11041113
removed.each do |request_id, subscription|
1105-
begin
1106-
send_to_stream(subscription[:stream], {
1107-
jsonrpc: "2.0",
1108-
id: request_id,
1109-
result: {
1110-
# `SubscriptionsListenResult` is served at the transport layer and never
1111-
# passes through the dispatch path, so the REQUIRED 2026-07-28 `resultType` is
1112-
# stamped at its construction site.
1113-
resultType: ResultType::COMPLETE,
1114-
_meta: { RequestEnvelope::SUBSCRIPTION_ID_META_KEY.to_sym => request_id },
1115-
},
1116-
})
1117-
rescue *STREAM_WRITE_ERRORS
1118-
nil
1114+
# Marking the entry closed and writing the result under the stream's write mutex orders
1115+
# this against in-flight deliveries: each one either lands before the result or observes
1116+
# `closed` and skips, keeping the graceful result the stream's final message.
1117+
subscription[:write_mutex].synchronize do
1118+
subscription[:closed] = true
1119+
1120+
begin
1121+
send_to_stream(subscription[:stream], {
1122+
jsonrpc: "2.0",
1123+
id: request_id,
1124+
result: {
1125+
# `SubscriptionsListenResult` is served at the transport layer and never
1126+
# passes through the dispatch path, so the REQUIRED 2026-07-28 `resultType` is
1127+
# stamped at its construction site.
1128+
resultType: ResultType::COMPLETE,
1129+
_meta: { RequestEnvelope::SUBSCRIPTION_ID_META_KEY.to_sym => request_id },
1130+
},
1131+
})
1132+
rescue *STREAM_WRITE_ERRORS
1133+
nil
1134+
end
11191135
end
11201136
close_stream_safely(subscription[:stream])
11211137
end

‎test/mcp/server/transports/streamable_http_transport_test.rb‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6113,7 +6113,7 @@ def string
61136113
# the registry insert and the acknowledgement write, which happens outside the lock.
61146114
io = StringIO.new
61156115
@transport.instance_variable_get(:@listen_subscriptions)["listen-1"] = {
6116-
stream: io, filter: { toolsListChanged: true }, active: false
6116+
stream: io, filter: { toolsListChanged: true }, active: false, write_mutex: Mutex.new
61176117
}
61186118

61196119
@server.notify_tools_list_changed
@@ -6126,6 +6126,23 @@ def string
61266126
assert_equal ["notifications/tools/list_changed"], sse_events(io).map { |event| event["method"] }
61276127
end
61286128

6129+
test "a delivery racing the graceful teardown cannot write after the final result" do
6130+
io = open_listen_stream(id: "listen-1", notifications: { toolsListChanged: true })
6131+
entry = @transport.instance_variable_get(:@listen_subscriptions)["listen-1"]
6132+
6133+
@transport.close
6134+
6135+
# Simulate an in-flight delivery that snapshotted the entry before teardown cleared
6136+
# the registry: the closed flag set under the write mutex makes it a no-op.
6137+
@transport.instance_variable_get(:@listen_subscriptions)["listen-1"] = entry
6138+
@server.notify_tools_list_changed
6139+
6140+
events = sse_events(io)
6141+
6142+
assert_equal "complete", events.last.dig("result", "resultType")
6143+
refute(events.any? { |event| event["method"] == "notifications/tools/list_changed" })
6144+
end
6145+
61296146
test "subscriptions/listen streams for different subscriptions receive their own subscriptionId" do
61306147
first = open_listen_stream(id: "listen-1", notifications: { toolsListChanged: true })
61316148
second = open_listen_stream(id: "listen-2", notifications: { toolsListChanged: true })
@@ -6264,7 +6281,7 @@ def string
62646281
ping = data
62656282
end
62666283
stream.define_singleton_method(:flush) {}
6267-
@transport.instance_variable_get(:@listen_subscriptions)["listen-1"] = { stream: stream, filter: {} }
6284+
@transport.instance_variable_get(:@listen_subscriptions)["listen-1"] = { stream: stream, filter: {}, write_mutex: Mutex.new }
62686285

62696286
@transport.send(:send_listen_keepalive_ping, "listen-1")
62706287

0 commit comments

Comments
 (0)