Skip to content

Commit 2f20580

Browse files
committed
Harden the listen driver against receive-order races and hostile peers
Move the client's listen-stream demux to a synchronous notification intercept on the dispatcher's receive path, where cancelled/progress interception already lives: acks, events, and teardown signals now advance in wire order relative to the listen result, so a graceful close can no longer outrun the events that preceded it or clobber the acknowledged filter with the fabricated empty one. Ack consumption keys on the live route registry alone (the per-session id set is gone), route admission enforces the honored filter - loose on URIs, which the spec lets be sub-resources, with a capped backlog settling the route lost against floods - and the response-cache eviction barrier moved to the consumption point, held pending until it completes so a cancelled consumer cannot lose a level trigger. On the transport, a request whose SSE stream can never answer - a non-resumable drop or an exhausted reconnection budget - now resolves with a synthesized error, contained against teardown races. Docs: a graceful close is not "stop watching" (servers shed load by closing gracefully, including this SDK's ListenHandler); the watch loop re-listens on both endings, and sub.honored is the delivery contract, not catalog state.
1 parent 34d37a3 commit 2f20580

16 files changed

Lines changed: 781 additions & 132 deletions

docs/advanced/subscriptions.md

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,9 @@ Consuming a subscription is one context manager:
8888
```
8989

9090
* `client.listen(...)` takes the filter as keyword arguments — they mirror the wire `SubscriptionFilter` field for field. Entering sends the request and returns once the server's acknowledgment arrives, so `sub.honored` (the subset the server agreed to deliver) is always there before the first event.
91-
* Iteration yields the same four typed events the server publishes: `ToolsListChanged`, `PromptsListChanged`, `ResourcesListChanged`, and `ResourceUpdated(uri=...)`. An event is a cue to refetch — it carries no payload beyond identity, and duplicates pending consumption collapse into one.
91+
* Iteration yields the same four typed events the server publishes: `ToolsListChanged`, `PromptsListChanged`, `ResourcesListChanged`, and `ResourceUpdated(uri=...)` — where the URI may be a sub-resource of one you subscribed to, at the server's discretion. An event is a cue to refetch — it carries no payload beyond identity, and duplicates pending consumption collapse into one.
9292
* Leaving the block ends the subscription, with the transport's own spelling: over streamable HTTP the request's response stream is closed (that is the 2026 cancellation signal), on stream transports `notifications/cancelled` is sent.
93-
* The stream's two endings are control flow. The server closing gracefully simply ends the `async for`; an abrupt drop raises `SubscriptionLost`. There is no replay and no automatic re-listen — a client that reconnects refetches what it depends on:
93+
* The stream's two endings are control flow. The server closing gracefully simply ends the `async for`; an abrupt drop raises `SubscriptionLost`. The distinction is diagnostic — a clean end versus a connection worth suspecting — not a difference in what to do next: either way the stream is gone, nothing is replayed, and a watcher that still cares re-listens and refetches. Servers close streams gracefully for their own reasons — shutdown, or shedding a subscriber whose backlog grew past bounds, as this SDK's `ListenHandler` does — so a graceful close is not a signal to stop watching:
9494

9595
```python
9696
async def watch(client: Client, uri: str) -> None:
@@ -101,12 +101,14 @@ async def watch(client: Client, uri: str) -> None:
101101
async for _event in sub:
102102
await client.read_resource(uri)
103103
except SubscriptionLost:
104-
continue # transport dropped - re-listen
105-
else:
106-
break # the server ended it deliberately
104+
pass
105+
# Graceful close or abrupt drop, the stream is gone either way. Back
106+
# off before re-listening - a graceful close may be the server
107+
# shedding load, and reconnecting instantly recreates the pressure.
108+
await anyio.sleep(1)
107109
```
108110

109-
* Checking the acknowledgment (the spec's client SHOULD) is reading `sub.honored`for example, `if not sub.honored.prompts_list_changed:` the server has no prompts to watch. Multiple subscriptions may be open concurrently; each demultiplexes by its own subscription id.
111+
* Checking the acknowledgment (the spec's client SHOULD) is reading `sub.honored`the kinds this stream will actually receive. A server may narrow the filter it agrees to honor (a multi-tenant server declining a URI, say), and `sub.honored` is that delivery contract — it says nothing about what exists in the catalog. Multiple subscriptions may be open concurrently; each demultiplexes by its own subscription id.
110112
* Tool calls and other requests run freely beside an open stream — from the same task between events, or from sibling tasks sharing the client. A watcher task that refetches inside its event loop is the intended pattern, not a re-entrancy hazard.
111113
* `listen()` requires a 2026-07-28 connection and raises `ListenNotSupportedError` on older ones, steering to the deprecated `subscribe_resource` and `message_handler` spelling those wires use.
112114

src/mcp/client/client.py

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@
5858
SamplingFnT,
5959
)
6060
from mcp.client.streamable_http import streamable_http_client
61-
from mcp.client.subscriptions import Subscription
61+
from mcp.client.subscriptions import ServerEvent, Subscription
6262
from mcp.client.subscriptions import listen as _listen
6363
from mcp.server import Server
6464
from mcp.server.mcpserver import MCPServer
@@ -69,6 +69,7 @@
6969
from mcp.shared.extension import validate_extension_identifier
7070
from mcp.shared.jsonrpc_dispatcher import JSONRPCDispatcher
7171
from mcp.shared.session import RequestResponder
72+
from mcp.shared.subscriptions import event_to_notification
7273

7374
logger = logging.getLogger(__name__)
7475

@@ -684,7 +685,8 @@ def listen(
684685
tools = await client.list_tools() # refetch on change
685686
686687
A graceful server close ends the loop; an abrupt drop raises
687-
`SubscriptionLost` (re-listen and refetch - there is no replay).
688+
`SubscriptionLost`. Either way the stream is gone and there is no
689+
replay: a client that keeps watching re-listens and refetches.
688690
Exiting the context ends the subscription. Multiple subscriptions may
689691
be open concurrently.
690692
@@ -702,8 +704,28 @@ def listen(
702704
prompts_list_changed=prompts_list_changed,
703705
resources_list_changed=resources_list_changed,
704706
resource_subscriptions=resource_subscriptions,
707+
on_event=self._evict_for_listen_event if self._response_cache is not None else None,
705708
)
706709

710+
async def _evict_for_listen_event(self, event: ServerEvent) -> None:
711+
"""Finish response-cache eviction before a listen consumer can refetch.
712+
713+
The eviction wrapper on message_handler runs on a spawned path; the
714+
consumer's iterator would otherwise wake first, refetch through a
715+
still-warm entry, and - events being deduplicated level triggers -
716+
never get a second wake to correct it. The event's notification still
717+
tees to that wrapper, so the same eviction fires twice; that is
718+
deliberate (eviction is idempotent, and the tee-path one covers
719+
non-iterating consumers sharing this cache). Same containment boundary
720+
as `_evicting_message_handler`: a cache fault must not block delivery.
721+
"""
722+
cache = self._response_cache
723+
assert cache is not None # installed as the event barrier only when a cache exists
724+
try:
725+
await cache.evict_for_notification(event_to_notification(event, {}))
726+
except Exception: # boundary: eviction reaches user store code; a cache fault must not block delivery
727+
logger.exception("Response cache eviction failed; the event is still delivered")
728+
707729
@deprecated(
708730
"resources/subscribe is removed as of 2026-07-28; use Client.listen() instead.",
709731
category=MCPDeprecationWarning,

src/mcp/client/session.py

Lines changed: 65 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,10 @@
5050
mcp_param_headers,
5151
x_mcp_header_map,
5252
)
53-
from mcp.shared.jsonrpc_dispatcher import JSONRPCDispatcher
53+
from mcp.shared.jsonrpc_dispatcher import JSONRPCDispatcher, cancelled_request_id_from_params
5454
from mcp.shared.message import ClientMessageMetadata, SessionMessage
5555
from mcp.shared.session import RequestResponder
56-
from mcp.shared.subscriptions import SUBSCRIPTION_ID_META_KEY, event_from_notification
56+
from mcp.shared.subscriptions import SUBSCRIPTION_ID_META_KEY, event_from_wire
5757
from mcp.shared.transport_context import TransportContext
5858

5959
DEFAULT_CLIENT_INFO = types.Implementation(name="mcp", version="0.1.0")
@@ -365,11 +365,11 @@ def __init__(
365365
self._task_group: anyio.abc.TaskGroup | None = None
366366
# subscriptions/listen demux routes, keyed by the listen request's id
367367
# (verbatim-typed: a plain dict already keeps 1 and "1" distinct).
368+
# Fed by `_intercept_notification` on the dispatcher's receive path;
369+
# membership alone decides ack consumption, so a closed subscription
370+
# releases its id completely - raw escape-hatch listens (never
371+
# registered here) keep receiving their acks via message_handler.
368372
self._listen_routes: dict[RequestId, ListenRoute] = {}
369-
# Every id the driver ever minted on this session: a late ack for a
370-
# closed driver listen must be dropped, while raw escape-hatch listens
371-
# (never in this set) keep receiving their acks via message_handler.
372-
self._driver_listen_ids: set[RequestId] = set()
373373
if dispatcher is not None:
374374
if read_stream is not None or write_stream is not None:
375375
raise ValueError("pass read_stream/write_stream or dispatcher, not both")
@@ -398,7 +398,9 @@ async def __aenter__(self) -> Self:
398398
for binding in self._notification_bindings.values():
399399
send, receive = anyio.create_memory_object_stream[BaseModel](_NOTIFICATION_QUEUE_SIZE)
400400
self._binding_queues[binding.method] = (send, receive)
401-
await self._task_group.start(self._dispatcher.run, self._on_request, self._on_notify)
401+
await self._task_group.start(
402+
self._dispatcher.run, self._on_request, self._on_notify, self._intercept_notification
403+
)
402404
for binding in self._notification_bindings.values():
403405
_, receive = self._binding_queues[binding.method]
404406
self._task_group.start_soon(self._deliver_bound_notifications, binding, receive)
@@ -1248,7 +1250,6 @@ def _register_listen_route(self, request_id: RequestId) -> ListenRoute:
12481250
"""Create the demux route for a listen request id; the caller registers BEFORE sending."""
12491251
route = ListenRoute()
12501252
self._listen_routes[request_id] = route
1251-
self._driver_listen_ids.add(request_id)
12521253
return route
12531254

12541255
def _unregister_listen_route(self, request_id: RequestId) -> None:
@@ -1266,21 +1267,60 @@ def _settle_listen_routes_closed(self) -> None:
12661267
route.settle("lost", error=closed)
12671268
self._listen_routes.clear()
12681269

1269-
def _listen_subscription_id(self, notification: types.ServerNotification) -> RequestId | None:
1270-
"""The frame's `_meta` subscription id, shape-checked.
1271-
1272-
The `as_request_id` guard is not a tripwire: on pre-2026 wires the meta
1273-
key is untyped, so a non-id (even unhashable) value is constructible
1274-
and would fail the dict lookup; 2026 surface validation already
1275-
rejects those shapes.
1270+
def _intercept_notification(self, method: str, params: Mapping[str, Any] | None) -> bool:
1271+
"""Wire-order listen demux, run by the dispatcher on its receive path.
1272+
1273+
Route bookkeeping must advance in receive order relative to the listen
1274+
request's own result: the result resolves synchronously on this same
1275+
path, so an ack or event handled on the spawned `_on_notify` path could
1276+
lose the race and be dropped after the stream settled - a graceful
1277+
close swallowing the events that preceded it. Only the synchronous
1278+
bookkeeping happens here; the user-facing tee (message_handler,
1279+
logging) stays on the spawned path.
1280+
1281+
Returns True to consume the frame: an ack for a live driver route is
1282+
driver state, never surfaced. Raw escape-hatch listens have no route
1283+
registered and keep observing their acks via message_handler - as does
1284+
a stray ack for an already-closed driver id, whose registration is gone.
1285+
1286+
The `as_request_id` guard is not a tripwire: this reads raw wire
1287+
dicts, where a non-id (even unhashable) `_meta` value is
1288+
constructible and would fail the dict lookup.
12761289
"""
1277-
meta = notification.params.meta if notification.params is not None else None
1278-
return as_request_id(meta.get(SUBSCRIPTION_ID_META_KEY)) if meta is not None else None
1279-
1280-
def _listen_route_for(self, notification: types.ServerNotification) -> ListenRoute | None:
1281-
"""The route a listen-stream frame belongs to, by its `_meta` subscription id."""
1282-
subscription_id = self._listen_subscription_id(notification)
1283-
return self._listen_routes.get(subscription_id) if subscription_id is not None else None
1290+
if not self._listen_routes:
1291+
return False
1292+
if method == "notifications/cancelled":
1293+
request_id = cancelled_request_id_from_params(params)
1294+
if request_id is not None and (listen_route := self._listen_routes.get(request_id)) is not None:
1295+
# A server-sent cancel naming one of our listen requests is
1296+
# that stream's teardown signal (the stream-transport spelling).
1297+
listen_route.settle("lost")
1298+
return False # _on_notify swallows every cancelled either way (v1 parity)
1299+
if params is None:
1300+
return False
1301+
meta = params.get("_meta")
1302+
if not isinstance(meta, Mapping):
1303+
return False
1304+
subscription_id = as_request_id(cast("Mapping[str, Any]", meta).get(SUBSCRIPTION_ID_META_KEY))
1305+
if subscription_id is None or (listen_route := self._listen_routes.get(subscription_id)) is None:
1306+
return False
1307+
if method == "notifications/subscriptions/acknowledged":
1308+
raw_filter = params.get("notifications")
1309+
if raw_filter is None:
1310+
# The wire shape requires `notifications`; a frame without it is
1311+
# malformed, not an empty filter - leave it to the spawned
1312+
# path's validation warning rather than fabricating honored={}.
1313+
return False
1314+
try:
1315+
honored = types.SubscriptionFilter.model_validate(raw_filter)
1316+
except ValidationError:
1317+
# Malformed ack: leave it to the spawned path's validation warning.
1318+
return False
1319+
listen_route.set_acked(honored)
1320+
return True
1321+
if (event := event_from_wire(method, params)) is not None:
1322+
listen_route.deliver(event)
1323+
return False # events (and any other stamped frame) still tee as usual
12841324

12851325
async def _on_notify(
12861326
self, dctx: DispatchContext[TransportContext], method: str, params: Mapping[str, Any] | None
@@ -1316,24 +1356,10 @@ async def _on_notify(
13161356
logger.warning("Failed to validate notification: %s", method, exc_info=True)
13171357
return
13181358
if isinstance(notification, types.CancelledNotification):
1319-
# A server-sent cancel naming one of our listen requests is that
1320-
# stream's teardown signal (the stream-transport spelling); any
1321-
# other cancellation was already applied by the dispatcher.
1322-
# Neither is surfaced to message_handler.
1323-
cancelled_id = notification.params.request_id
1324-
if cancelled_id is not None and (listen_route := self._listen_routes.get(cancelled_id)) is not None:
1325-
listen_route.settle("lost")
1359+
# Never surfaced to message_handler (v1 parity): the dispatcher
1360+
# already applied any in-flight cancellation, and a cancel naming
1361+
# one of our listen streams was settled by the wire-order intercept.
13261362
return
1327-
if isinstance(notification, types.SubscriptionsAcknowledgedNotification):
1328-
subscription_id = self._listen_subscription_id(notification)
1329-
if subscription_id is not None and subscription_id in self._driver_listen_ids:
1330-
# Driver state, never surfaced: a late ack for an already-closed
1331-
# driver listen is dropped rather than leaking to message_handler.
1332-
# Acks outside the driver's id namespace fall through - a raw
1333-
# escape-hatch listen observes its ack there.
1334-
if (listen_route := self._listen_routes.get(subscription_id)) is not None:
1335-
listen_route.set_acked(notification.params.notifications)
1336-
return
13371363
try:
13381364
if isinstance(notification, types.LoggingMessageNotification):
13391365
await self._logging_callback(notification.params)
@@ -1344,14 +1370,6 @@ async def _on_notify(
13441370
# would otherwise fail the peer's send. A raising logging_callback
13451371
# skips the message_handler tee for that notification (v1 parity).
13461372
logger.exception("notification callback for %r raised", method)
1347-
# Deliver AFTER the tee: the caching layer's eviction wrapper runs in
1348-
# message_handler, and a consumer woken first could refetch into the
1349-
# stale entry - with deduplicated level-trigger events there would be
1350-
# no second wake to correct it.
1351-
if (listen_route := self._listen_route_for(notification)) is not None:
1352-
event = event_from_notification(notification)
1353-
if event is not None:
1354-
listen_route.deliver(event)
13551373

13561374
async def _on_stream_exception(self, exc: Exception) -> None:
13571375
"""Deliver a transport-level fault to message_handler via a spawned task.

src/mcp/client/streamable_http.py

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -447,9 +447,25 @@ async def _handle_sse_response(
447447
# instead of leaving the request pending for the session's lifetime
448448
# (a listen stream's consumer would otherwise hang instead of
449449
# learning the subscription is lost).
450-
error_data = ErrorData(code=CONNECTION_CLOSED, message="SSE stream ended without a response")
451-
error_msg = SessionMessage(JSONRPCError(jsonrpc="2.0", id=original_request_id, error=error_data))
452-
await ctx.read_stream_writer.send(error_msg)
450+
await self._resolve_abandoned_request(
451+
ctx.read_stream_writer, original_request_id, "SSE stream ended without a response"
452+
)
453+
454+
async def _resolve_abandoned_request(
455+
self, read_stream_writer: StreamWriter, request_id: RequestId, message: str
456+
) -> None:
457+
"""Resolve a request whose response can never arrive with a synthesized error.
458+
459+
Best-effort, like the dispatcher's own resolution writes: the read
460+
stream closing first means the session is being torn down and nobody
461+
is waiting on this request anymore.
462+
"""
463+
error_data = ErrorData(code=CONNECTION_CLOSED, message=message)
464+
error_msg = SessionMessage(JSONRPCError(jsonrpc="2.0", id=request_id, error=error_data))
465+
try:
466+
await read_stream_writer.send(error_msg)
467+
except (anyio.BrokenResourceError, anyio.ClosedResourceError):
468+
logger.debug("read stream closed before request %r could be resolved", request_id)
453469

454470
async def _handle_reconnection(
455471
self,
@@ -459,9 +475,22 @@ async def _handle_reconnection(
459475
attempt: int = 0,
460476
) -> None:
461477
"""Reconnect with Last-Event-ID to resume stream after server disconnect."""
462-
# Bail if max retries exceeded
463-
if attempt >= MAX_RECONNECTION_ATTEMPTS: # pragma: no cover
478+
# Only requests reconnect: every caller reaches here from a request's
479+
# response stream (`_handle_sse_response` asserts the same), so the
480+
# original id to map responses onto is always present.
481+
assert isinstance(ctx.session_message.message, JSONRPCRequest)
482+
original_request_id = ctx.session_message.message.id
483+
484+
if attempt >= MAX_RECONNECTION_ATTEMPTS:
485+
# Give up AND resolve: without a synthesized error the waiter is
486+
# pending for the session's lifetime, and a request with no read
487+
# timeout (a listen stream) would hang its caller forever.
464488
logger.debug(f"Max reconnection attempts ({MAX_RECONNECTION_ATTEMPTS}) exceeded")
489+
await self._resolve_abandoned_request(
490+
ctx.read_stream_writer,
491+
original_request_id,
492+
"SSE stream ended and reconnection attempts were exhausted",
493+
)
465494
return
466495

467496
# Always wait - use server value or default
@@ -471,11 +500,6 @@ async def _handle_reconnection(
471500
headers = self._prepare_headers()
472501
headers[LAST_EVENT_ID] = last_event_id
473502

474-
# Extract original request ID to map responses
475-
original_request_id = None
476-
if isinstance(ctx.session_message.message, JSONRPCRequest): # pragma: no branch
477-
original_request_id = ctx.session_message.message.id
478-
479503
try:
480504
async with aconnect_sse(ctx.client, "GET", self.url, headers=headers) as event_source:
481505
event_source.response.raise_for_status()

0 commit comments

Comments
 (0)