Make custom notifications first-class: typed send, observable drops - #3173
Make custom notifications first-class: typed send, observable drops#3173maxisbey wants to merge 1 commit into
Conversation
Widen ClientSession.send_notification and ServerSession.send_notification to accept any Notification subclass, mirroring send_request's open Request arm, so extensions can emit their own methods without casts. Raise dropped-notification logging from debug to a once-per-method warning that names the remedy: on the client when no NotificationBinding observes the method, on the server when a spec notification is not defined at the negotiated version or a custom method has no registered handler (an unserved spec notification stays at debug; repeat drops of a method log at debug so a stream cannot flood the log). Document the message_handler routing change and the binding channel in the migration guide, fix the callbacks page's catch-all claim, and cover the vendor-notification round trip with an interaction test, un-deferring its requirement.
📚 Documentation preview
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/advanced/extensions.md">
<violation number="1" location="docs/advanced/extensions.md:221">
P3: Repeated unbound notifications are logged at DEBUG, not WARNING. Qualify this as the first drop per method so extension authors do not expect a warning for every dropped event.</violation>
</file>
<file name="src/mcp/client/session.py">
<violation number="1" location="src/mcp/client/session.py:1345">
P2: A peer sending unique unknown method names grows this per-session set without bound, turning the log-deduplication path into a memory-exhaustion vector. Bound the remembered method cache (or cap deduplication) so untrusted notification names are not retained indefinitely.</violation>
</file>
<file name="docs/advanced/low-level-server.md">
<violation number="1" location="docs/advanced/low-level-server.md:174">
P3: The new notification example cannot run as shown: `NotificationParams`, `Notification`, and `Literal` are undefined. Include their imports so readers can use the typed surface demonstrated here.</violation>
</file>
<file name="docs/client/callbacks.md">
<violation number="1" location="docs/client/callbacks.md:138">
P3: Repeated unbound vendor notifications are dropped with a debug log, not a warning. Qualify this as a first-occurrence warning so the documented observability behavior matches the log-flood suppression implementation.</violation>
<violation number="2" location="docs/client/callbacks.md:138">
P3: `notifications/cancelled` does not reach `message_handler`, despite being spec traffic: `_on_notify` explicitly swallows `CancelledNotification`. Document this exception so catch-all handlers are not expected to observe cancellation frames.</violation>
</file>
<file name="src/mcp/server/runner.py">
<violation number="1" location="src/mcp/server/runner.py:167">
P2: Repeated dropped custom notifications on the modern stream will still log at `WARNING` every time, because `_serve_modern_stream` creates a new `ServerRunner` for each notification and this set is reset with it. Consider keeping the warned-method state in the long-lived modern-stream driver (and sharing it with each transient runner) so repeats are downgraded to debug as promised.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| logger.debug("dropped %r: not defined at %s", method, version) | ||
| # The first drop of a method warns; repeats log at debug so a stream cannot flood the log. | ||
| level = logging.WARNING if method not in self._warned_notification_drops else logging.DEBUG | ||
| self._warned_notification_drops.add(method) |
There was a problem hiding this comment.
P2: A peer sending unique unknown method names grows this per-session set without bound, turning the log-deduplication path into a memory-exhaustion vector. Bound the remembered method cache (or cap deduplication) so untrusted notification names are not retained indefinitely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/client/session.py, line 1345:
<comment>A peer sending unique unknown method names grows this per-session set without bound, turning the log-deduplication path into a memory-exhaustion vector. Bound the remembered method cache (or cap deduplication) so untrusted notification names are not retained indefinitely.</comment>
<file context>
@@ -1334,7 +1340,12 @@ async def _on_notify(
- logger.debug("dropped %r: not defined at %s", method, version)
+ # The first drop of a method warns; repeats log at debug so a stream cannot flood the log.
+ level = logging.WARNING if method not in self._warned_notification_drops else logging.DEBUG
+ self._warned_notification_drops.add(method)
+ logger.log(
+ level, "dropped %r: not defined at %s and no notification binding is registered", method, version
</file context>
| init_options: InitializationOptions | None = None | ||
| """`InitializeResult` payload. Defaults to `server.create_initialization_options()`.""" | ||
|
|
||
| _warned_notification_drops: set[str] = field(default_factory=lambda: set(), init=False, repr=False) |
There was a problem hiding this comment.
P2: Repeated dropped custom notifications on the modern stream will still log at WARNING every time, because _serve_modern_stream creates a new ServerRunner for each notification and this set is reset with it. Consider keeping the warned-method state in the long-lived modern-stream driver (and sharing it with each transient runner) so repeats are downgraded to debug as promised.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/runner.py, line 167:
<comment>Repeated dropped custom notifications on the modern stream will still log at `WARNING` every time, because `_serve_modern_stream` creates a new `ServerRunner` for each notification and this set is reset with it. Consider keeping the warned-method state in the long-lived modern-stream driver (and sharing it with each transient runner) so repeats are downgraded to debug as promised.</comment>
<file context>
@@ -164,6 +164,14 @@ class ServerRunner(Generic[LifespanT]):
init_options: InitializationOptions | None = None
"""`InitializeResult` payload. Defaults to `server.create_initialization_options()`."""
+ _warned_notification_drops: set[str] = field(default_factory=lambda: set(), init=False, repr=False)
+
+ def _log_dropped_notification(self, method: str, message: str, *args: object) -> None:
</file context>
| notification as a `mcp_types.Notification` subclass and sends it with | ||
| `ctx.session.send_notification(..., related_request_id=ctx.request_id)`, as shown in | ||
| **[The low-level Server](low-level-server.md#a-method-of-your-own)**. A client without the binding | ||
| drops the notification with a warning. |
There was a problem hiding this comment.
P3: Repeated unbound notifications are logged at DEBUG, not WARNING. Qualify this as the first drop per method so extension authors do not expect a warning for every dropped event.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/advanced/extensions.md, line 221:
<comment>Repeated unbound notifications are logged at DEBUG, not WARNING. Qualify this as the first drop per method so extension authors do not expect a warning for every dropped event.</comment>
<file context>
@@ -214,7 +214,11 @@ def notifications(self) -> Sequence[NotificationBinding[Any]]:
+notification as a `mcp_types.Notification` subclass and sends it with
+`ctx.session.send_notification(..., related_request_id=ctx.request_id)`, as shown in
+**[The low-level Server](low-level-server.md#a-method-of-your-own)**. A client without the binding
+drops the notification with a warning.
Two quiet rules. Claims are active on 2026-07-28 connections only, and the capability
</file context>
| drops the notification with a warning. | |
| drops the first notification for each method with a warning; repeated drops are logged at debug. |
| Outbound, `ctx.session.send_notification(...)` accepts any `mcp_types.Notification` subclass, not just the spec-defined union, so the notifying half of a vendor protocol is one model away: | ||
|
|
||
| ```python | ||
| class ReindexProgressParams(NotificationParams): |
There was a problem hiding this comment.
P3: The new notification example cannot run as shown: NotificationParams, Notification, and Literal are undefined. Include their imports so readers can use the typed surface demonstrated here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/advanced/low-level-server.md, line 174:
<comment>The new notification example cannot run as shown: `NotificationParams`, `Notification`, and `Literal` are undefined. Include their imports so readers can use the typed surface demonstrated here.</comment>
<file context>
@@ -162,10 +162,33 @@ The constructor covers the methods MCP defines. `add_request_handler` covers eve
+Outbound, `ctx.session.send_notification(...)` accepts any `mcp_types.Notification` subclass, not just the spec-defined union, so the notifying half of a vendor protocol is one model away:
+
+```python
+class ReindexProgressParams(NotificationParams):
+ percent: float
+
</file context>
| `logging_callback` receives every `notifications/message` a server sends, as `LoggingMessageNotificationParams` (`level`, `logger`, `data`). Protocol logging is itself deprecated by the 2026-07-28 spec (**[Logging](../handlers/logging.md)** has what to do instead), so this callback exists for the servers that still emit it. | ||
|
|
||
| `message_handler` is the catch-all: every server notification reaches it (as well as its specific callback), and on a stream-backed transport so does every transport-level `Exception`. The one pattern worth knowing is `if isinstance(message, Exception): raise message`, so a broken connection fails loudly instead of vanishing. | ||
| `message_handler` is the catch-all for spec traffic: every notification the negotiated protocol version defines reaches it (as well as its specific callback), and on a stream-backed transport so does every transport-level `Exception`. Vendor and extension methods are outside that set; they are delivered only to a matching `NotificationBinding` (**[Extensions](../advanced/extensions.md)**), and without one they are dropped with a warning. The one pattern worth knowing is `if isinstance(message, Exception): raise message`, so a broken connection fails loudly instead of vanishing. |
There was a problem hiding this comment.
P3: Repeated unbound vendor notifications are dropped with a debug log, not a warning. Qualify this as a first-occurrence warning so the documented observability behavior matches the log-flood suppression implementation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/client/callbacks.md, line 138:
<comment>Repeated unbound vendor notifications are dropped with a debug log, not a warning. Qualify this as a first-occurrence warning so the documented observability behavior matches the log-flood suppression implementation.</comment>
<file context>
@@ -135,7 +135,7 @@ Two more. Neither declares anything.
`logging_callback` receives every `notifications/message` a server sends, as `LoggingMessageNotificationParams` (`level`, `logger`, `data`). Protocol logging is itself deprecated by the 2026-07-28 spec (**[Logging](../handlers/logging.md)** has what to do instead), so this callback exists for the servers that still emit it.
-`message_handler` is the catch-all: every server notification reaches it (as well as its specific callback), and on a stream-backed transport so does every transport-level `Exception`. The one pattern worth knowing is `if isinstance(message, Exception): raise message`, so a broken connection fails loudly instead of vanishing.
+`message_handler` is the catch-all for spec traffic: every notification the negotiated protocol version defines reaches it (as well as its specific callback), and on a stream-backed transport so does every transport-level `Exception`. Vendor and extension methods are outside that set; they are delivered only to a matching `NotificationBinding` (**[Extensions](../advanced/extensions.md)**), and without one they are dropped with a warning. The one pattern worth knowing is `if isinstance(message, Exception): raise message`, so a broken connection fails loudly instead of vanishing.
## Recap
</file context>
| `message_handler` is the catch-all for spec traffic: every notification the negotiated protocol version defines reaches it (as well as its specific callback), and on a stream-backed transport so does every transport-level `Exception`. Vendor and extension methods are outside that set; they are delivered only to a matching `NotificationBinding` (**[Extensions](../advanced/extensions.md)**), and without one they are dropped with a warning. The one pattern worth knowing is `if isinstance(message, Exception): raise message`, so a broken connection fails loudly instead of vanishing. | |
| `message_handler` is the catch-all for spec traffic: every notification the negotiated protocol version defines reaches it (as well as its specific callback), and on a stream-backed transport so does every transport-level `Exception`. Vendor and extension methods are outside that set; they are delivered only to a matching `NotificationBinding` (**[Extensions](../advanced/extensions.md)**), and without one they are dropped with a warning on the first occurrence (then logged at debug to avoid floods). The one pattern worth knowing is `if isinstance(message, Exception): raise message`, so a broken connection fails loudly instead of vanishing. |
| `logging_callback` receives every `notifications/message` a server sends, as `LoggingMessageNotificationParams` (`level`, `logger`, `data`). Protocol logging is itself deprecated by the 2026-07-28 spec (**[Logging](../handlers/logging.md)** has what to do instead), so this callback exists for the servers that still emit it. | ||
|
|
||
| `message_handler` is the catch-all: every server notification reaches it (as well as its specific callback), and on a stream-backed transport so does every transport-level `Exception`. The one pattern worth knowing is `if isinstance(message, Exception): raise message`, so a broken connection fails loudly instead of vanishing. | ||
| `message_handler` is the catch-all for spec traffic: every notification the negotiated protocol version defines reaches it (as well as its specific callback), and on a stream-backed transport so does every transport-level `Exception`. Vendor and extension methods are outside that set; they are delivered only to a matching `NotificationBinding` (**[Extensions](../advanced/extensions.md)**), and without one they are dropped with a warning. The one pattern worth knowing is `if isinstance(message, Exception): raise message`, so a broken connection fails loudly instead of vanishing. |
There was a problem hiding this comment.
P3: notifications/cancelled does not reach message_handler, despite being spec traffic: _on_notify explicitly swallows CancelledNotification. Document this exception so catch-all handlers are not expected to observe cancellation frames.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/client/callbacks.md, line 138:
<comment>`notifications/cancelled` does not reach `message_handler`, despite being spec traffic: `_on_notify` explicitly swallows `CancelledNotification`. Document this exception so catch-all handlers are not expected to observe cancellation frames.</comment>
<file context>
@@ -135,7 +135,7 @@ Two more. Neither declares anything.
`logging_callback` receives every `notifications/message` a server sends, as `LoggingMessageNotificationParams` (`level`, `logger`, `data`). Protocol logging is itself deprecated by the 2026-07-28 spec (**[Logging](../handlers/logging.md)** has what to do instead), so this callback exists for the servers that still emit it.
-`message_handler` is the catch-all: every server notification reaches it (as well as its specific callback), and on a stream-backed transport so does every transport-level `Exception`. The one pattern worth knowing is `if isinstance(message, Exception): raise message`, so a broken connection fails loudly instead of vanishing.
+`message_handler` is the catch-all for spec traffic: every notification the negotiated protocol version defines reaches it (as well as its specific callback), and on a stream-backed transport so does every transport-level `Exception`. Vendor and extension methods are outside that set; they are delivered only to a matching `NotificationBinding` (**[Extensions](../advanced/extensions.md)**), and without one they are dropped with a warning. The one pattern worth knowing is `if isinstance(message, Exception): raise message`, so a broken connection fails loudly instead of vanishing.
## Recap
</file context>
| `message_handler` is the catch-all for spec traffic: every notification the negotiated protocol version defines reaches it (as well as its specific callback), and on a stream-backed transport so does every transport-level `Exception`. Vendor and extension methods are outside that set; they are delivered only to a matching `NotificationBinding` (**[Extensions](../advanced/extensions.md)**), and without one they are dropped with a warning. The one pattern worth knowing is `if isinstance(message, Exception): raise message`, so a broken connection fails loudly instead of vanishing. | |
| `message_handler` is the catch-all for spec traffic: every notification the negotiated protocol version defines reaches it (as well as its specific callback), except `notifications/cancelled`, which the session consumes internally. On a stream-backed transport it also receives every transport-level `Exception`. Vendor and extension methods are outside that set; they are delivered only to a matching `NotificationBinding` (**[Extensions](../advanced/extensions.md)**), and without one they are dropped with a warning on the first occurrence (then logged at debug to avoid floods). The one pattern worth knowing is `if isinstance(message, Exception): raise message`, so a broken connection fails loudly instead of vanishing. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, one other candidate was examined and ruled out this run: unbounded growth of the client-side _warned_notification_drops dedup set under peer-controlled unique method names (src/mcp/client/session.py) — the set is session-scoped and holds only small method strings, so its lifetime and cost are bounded by the session itself.
Extended reasoning...
Bugs were found and are posted as inline comments (the modern in-process notification delivery gap, the per-message ServerRunner dedup-scope issue, plus two doc/observability nits), so the inline comments already signal that human review is needed. This note only records the one additional candidate that finder agents raised and verifiers refuted — the client-side warn-dedup set growing under peer-controlled method names — so a later pass does not re-explore it. I confirmed the set at src/mcp/client/session.py:366 is per-ClientSession, making the growth bounded by session lifetime; a peer flooding unique methods costs more in notification processing than in set entries.
| async def send_notification(self, notification: types.ClientNotification | types.Notification[Any, Any]) -> None: | ||
| """Send a one-way notification. Usable before entering the context manager. | ||
|
|
||
| Spec notifications are the `types.ClientNotification` union members; any | ||
| other `types.Notification` subclass is sent as-is, so extensions can emit | ||
| their own methods (a python-sdk server routes those to handlers registered | ||
| with `Server.add_notification_handler`). |
There was a problem hiding this comment.
🔴 Custom client->server notifications sent via the newly-widened ClientSession.send_notification are silently discarded on the default Client(server) in-process (modern) path: the server side of the dispatcher pair uses the no-op _no_inbound_client_notifications (src/mcp/client/client.py:117), so a handler registered with Server.add_notification_handler never fires and no log line is emitted at any level — contradicting this PR's new docstring and the low-level-server.md "dropped with a warning" promise (the identical send with mode='legacy' is delivered). Fix by giving the modern in-process connect an on_notify that dispatches through a ServerRunner (mirroring _serve_modern_stream.on_notify in src/mcp/server/runner.py), or at minimum warn on drop there and scope the docstring claim.
Extended reasoning...
The bug
This PR widens ClientSession.send_notification to accept any mcp_types.Notification subclass, and its new docstring (src/mcp/client/session.py:531-537) promises that "a python-sdk server routes those to handlers registered with Server.add_notification_handler". The new docs make matching promises: docs/advanced/low-level-server.md says "a custom notification nobody registered a handler for is dropped with a warning". Neither claim holds on the SDK's flagship in-memory testing path — Client(server) in its default (modern) mode — where a custom client→server notification is neither delivered to a registered handler nor logged at any level. This is precisely the silent-drop failure mode the PR description says it eliminates ("dropping one now leaves a trace in the logs").
The code path
In _connect_inproc (src/mcp/client/client.py:113-118), the modern in-process connect builds a DirectDispatcher pair and starts the server side with:
on_request = modern_on_request(server, lifespan_state)
await tg.start(server_disp.run, on_request, _no_inbound_client_notifications)_no_inbound_client_notifications (client.py:185-193) is a docstring-only no-op — no ServerRunner, no Server.get_notification_handler lookup, no logger call. Its rationale ("the spec defines no client→server notifications") predates this PR's premise, which is exactly notifications outside the spec. modern_on_request (src/mcp/server/runner.py) covers requests only. DirectDispatcher.notify just invokes the peer's on_notify, so nothing else in the chain logs the drop either.
Why existing safeguards miss it
The PR's new warn-on-drop logic lives in ServerRunner._log_dropped_notification, reached from ServerRunner._on_notify. Every other path builds a runner for inbound notifications: the legacy in-memory path drives server.run → serve_dual_era_loop → the runner's on_notify, and the modern stream path builds a ServerRunner per notification in _serve_modern_stream.on_notify (runner.py, pinned by test_dual_era_loop_modern_notification_dispatches_at_the_served_version). Only the modern in-process path never constructs a runner for notifications. The PR's new interaction test (test_vendor_notifications_reach_the_owning_extensions_binding_in_order) exercises the server→client direction only, so the gap is untested.
Step-by-step proof
- Build a low-level
Serverand registerserver.add_notification_handler('notifications/vendor/ping', PingParams, handler). - Set the root logger to
DEBUG. - Connect with
async with Client(server) as client:— default mode negotiates 2026-07-28 over theDirectDispatcherpair; the server side'sOnNotifyis_no_inbound_client_notifications. - Send
await client.session.send_notification(VendorPing(params=PingParams()))— the exact usage this PR's docstring documents. DirectDispatcher.notifyinvokes the peer'son_notify=_no_inbound_client_notifications, whose body is empty. The handler never fires; no log record is emitted at any level.- Repeat with
Client(server, mode='legacy'): the notification flows throughserve_dual_era_loop→ServerRunner._on_notify→ the registered handler, which fires. (Independently reproduced by two runtime verifiers on this branch.)
So the same public call is delivered on legacy in-memory and modern stdio, and silently vanishes on default Client(server) — the path the docs themselves call the testing story ("Client(server) treated both servers identically").
Impact
A developer following this PR's new low-level-server.md guidance (define a Notification subclass, register add_notification_handler, test in memory) will conclude the feature is broken, with zero log trace to explain why — trial-and-error debugging is the very complaint that motivated this PR. The no-op itself predates the PR, but the PR legalizes the typed send, documents the delivery contract, and promises observable drops, so the contract violation is introduced by this PR.
Fix
Give the modern in-process connect an on_notify that dispatches through a ServerRunner — mirroring _serve_modern_stream.on_notify, which builds a Connection.from_envelope(LATEST_MODERN_VERSION, ...) and a runner per notification — so add_notification_handler handlers fire and unserved custom methods get the new warn-on-drop. At minimum, log a warning in _no_inbound_client_notifications and scope the docstring/docs claims to the transports where they hold.
Not a duplicate of the warn-once-dedup finding: that one concerns the _warned_notification_drops set's lifetime on paths where a runner IS built per notification; here no runner, no dispatch, and no log exist at all.
| _warned_notification_drops: set[str] = field(default_factory=lambda: set(), init=False, repr=False) | ||
|
|
||
| def _log_dropped_notification(self, method: str, message: str, *args: object) -> None: | ||
| """The first drop of a method warns; repeats log at debug so a stream cannot flood the log.""" | ||
| level = logging.WARNING if method not in self._warned_notification_drops else logging.DEBUG | ||
| self._warned_notification_drops.add(method) | ||
| logger.log(level, message, method, *args) |
There was a problem hiding this comment.
🔴 The once-per-method warn dedup for dropped notifications never engages on the modern (2026-07-28) stream path: _serve_modern_stream.on_notify builds a fresh ServerRunner (and Connection.from_envelope) for every inbound notification, so _warned_notification_drops is always empty and every repeat of the same unhandled custom method (or version-gated spec method) logs at WARNING — exactly the log flood this mechanism was added to prevent, and noisier than the pre-PR debug-only behavior on that path. The dedup set needs dispatcher-loop or server scope (per-runner and per-connection are both per-message here), e.g. hoisted onto the modern stream loop or Server.
Extended reasoning...
The bug. This PR introduces ServerRunner._log_dropped_notification (src/mcp/server/runner.py:167-173), which keys its warn-once/repeat-at-debug behavior on _warned_notification_drops, an init=False instance field of ServerRunner. That works only if the runner lives as long as the connection. On the legacy loop path it does (serve_connection/_serve_legacy_stream build one runner per connection), but on the modern stream path it does not: _serve_modern_stream.on_notify (runner.py:814-822) constructs a fresh Connection.from_envelope(LATEST_MODERN_VERSION, None, None, outbound=outbound) and a fresh ServerRunner for every inbound notification, then tears both down via aclose_shielded. The dedup set is therefore empty on every call, level = WARNING if method not in set else DEBUG always picks WARNING, and the repeat-suppression never happens.\n\nThe code path is live and production-reachable. Server.run drives serve_dual_era_loop for stdio (and any duplex stream pair); a client whose opening request carries the 2026-07-28 _meta envelope locks the connection into _serve_modern_stream, and every subsequent notification on that connection goes through the per-message on_notify. Both drop branches are reachable there: from_envelope connections are born-ready (initialize_accepted=True), so a custom notification with no registered handler reaches the _log_dropped_notification(method, "dropped %r: no notification handler is registered") branch, and a spec notification absent at LATEST_MODERN_VERSION reaches the version-gate branch.\n\nStep-by-step proof. (1) A modern client connects over stdio and sends its first enveloped request; serve_dual_era_loop routes to _serve_modern_stream. (2) The client emits notifications/vendor/heartbeat — a custom method the server never registered. (3) on_notify builds runner #1; its _warned_notification_drops is a fresh empty set; "notifications/vendor/heartbeat" not in set → WARNING logged; the method is added to the set; the runner is discarded. (4) The client sends the same notification again. (5) on_notify builds runner #2 with another fresh empty set; the membership test is again false → WARNING again. (6) Repeat N times → N WARNING lines. The intended behavior (and what the docstring at runner.py:170 promises: "repeats log at debug so a stream cannot flood the log") is 1 WARNING + (N−1) DEBUG.\n\nWhy nothing catches it. The PR's level-pinning test, test_runner_on_notify_drops_before_init_and_unknown_methods (asserting [logging.WARNING, logging.DEBUG]), runs on the legacy connected_runner path where the runner genuinely is per-connection, so the dedup works there and the modern-path regression is unobserved. test_dual_era_loop_modern_notification_dispatches_at_the_served_version drives the affected on_notify but only for a handled notification, never asserting drop log levels. No other server-side dedup layer exists; the client-side twin (ClientSession._warned_notification_drops, client/session.py:366) is correctly session-scoped — only the server side is broken.\n\nImpact. The invariant is peer-controlled: a chatty (or malicious) modern client repeatedly sending an unhandled vendor notification produces unbounded WARNING-level log lines. That both defeats the PR's stated flood protection and is a regression versus pre-PR behavior on this path, where these drops logged at logger.debug and could not flood a WARNING-level log.\n\nFix direction. The set needs a scope that outlives one message on the modern stream path. Options: hoist the set to the _serve_modern_stream closure and pass it into (or share) the per-notification runner; share one ServerRunner across the modern loop's notifications; or move the set onto Server. Note that Connection scope would not fix this path, since the Connection is also rebuilt per message here.
| async def send_notification( | ||
| self, | ||
| notification: types.ServerNotification, | ||
| notification: types.ServerNotification | types.Notification[Any, Any], | ||
| related_request_id: types.RequestId | None = None, | ||
| ) -> None: | ||
| """Send a typed server-to-client notification.""" | ||
| """Send a typed server-to-client notification. | ||
| Spec notifications are the `types.ServerNotification` union members; any | ||
| other `types.Notification` subclass is sent as-is, so extensions can emit | ||
| their own methods (a python-sdk client observes those by registering a | ||
| `NotificationBinding`). The 2026-07-28 revision gives standalone | ||
| notifications no channel outside `subscriptions/listen`, so on modern | ||
| connections pass `related_request_id` to ride the originating request's | ||
| stream. | ||
| """ | ||
| channel = self._request_outbound if related_request_id is not None else self._connection.outbound | ||
| data = notification.model_dump(by_alias=True, mode="json", exclude_none=True) |
There was a problem hiding this comment.
🟡 The server-to-client vendor-notification flow this PR documents and type-legalizes can still drop silently on the modern streamable-HTTP entry: with json_response=True, handle_modern_request never sets dctx.sink, so ctx.session.send_notification(..., related_request_id=ctx.request_id) — the exact pattern the new docs prescribe — is discarded with no log at any level; and without related_request_id, _NoChannelOutbound.notify drops at debug only, while this PR upgraded the receive-side equivalent to a once-per-method WARNING. The drop sites pre-date this PR, but since the PR's headline is observable drops and the new docs state no json_response caveat, consider applying the same warn-once logging to both outbound drop sites and/or caveating the docs.
Extended reasoning...
What happens
This PR promotes server-emitted vendor notifications to a first-class, documented flow: docs/advanced/low-level-server.md and docs/advanced/extensions.md now tell server authors to emit them with ctx.session.send_notification(..., related_request_id=ctx.request_id), the widened ServerSession.send_notification signature legalizes it, and the PR description headlines that "dropping one now leaves a trace in the logs." On the modern (2026-07-28) streamable-HTTP entry, the emit half of exactly this flow can still drop without the promised trace, at two sites:
1. Zero-log drop under json_response=True, even when the documented pattern is followed. In src/mcp/server/_streamable_http_modern.py, handle_modern_request takes the JSON early-return branch for non-listen methods (lines 403–411) and calls serve_one with dctx.sink still None — dctx.sink = send_ch is only assigned on the SSE path (line 414). _SingleExchangeDispatchContext.notify then hits if self.sink is None: return (lines 105–106) and returns without logging at any level. So a tool that follows the new docs to the letter silently loses every notification on a json_response=True deployment, and neither side logs anything.
2. Debug-only drop when related_request_id is omitted. The modern entry builds Connection.from_envelope(...) whose default outbound is _NO_CHANNEL; _NoChannelOutbound.notify (src/mcp/server/connection.py:113-114) logs only logger.debug("dropped %s: no standalone channel", method). The same call is delivered on legacy connections and on the dual-era stdio modern path (NotifyOnlyOutbound), so a server developed there silently loses its notifications when deployed on modern HTTP — while this PR upgraded the receive-side equivalent of this exact situation to a once-per-method WARNING (ServerRunner._log_dropped_notification, ClientSession._on_notify).
Step-by-step proof (case 1)
- Deploy an
MCPServerviastreamable_http_app()/run()withjson_response=True(a supported option). - A 2026-07-28 client calls a tool that does
await ctx.session.send_notification(ReceiptIssued(params=...), related_request_id=ctx.request_id)— the exact code in the PR's new interaction test and docs. handle_modern_requestseesjson_response and req.method != "subscriptions/listen"→ early-return JSON branch at line 403;dctx.sinkis never assigned.send_notificationselectsself._request_outbound(theDispatchContext, sincerelated_request_idis set) →_SingleExchangeDispatchContext.notify→if self.sink is None: returnat line 105.- The notification vanishes. The client's
NotificationBindinghandler never fires; no log is written on either end — the precise "found by trial and error" failure mode the PR was written to eliminate, now on the emitting half of the same feature.
Why existing safeguards miss it
The PR's warn-on-drop logic lives only on the inbound side (ServerRunner._on_notify, ClientSession._on_notify). No test covers the outbound drop sites, and the new interaction test (test_vendor_notifications_reach_the_owning_extensions_binding_in_order) runs on a transport where the sink exists. The new docs say unconditionally that related_request_id makes the notification "ride the originating request's stream" — no json_response caveat.
Impact and fix
Nothing that worked before this PR breaks: the drop lines pre-date it and are untouched, and the json_response drop is structurally forced (a plain JSON response carries no stream). But the PR increases the surface area for exactly the silent failure it set out to eliminate, and its observability claim doesn't hold for the outbound direction. Suggested fix: apply the same once-per-method WARNING at both outbound drop sites (sink-less _SingleExchangeDispatchContext.notify and _NoChannelOutbound.notify), and/or add a json_response caveat to the newly added docs in low-level-server.md and extensions.md. Filed as one finding: one root cause (outbound notification drops remain unobservable), two sites.
| `logging_callback` receives every `notifications/message` a server sends, as `LoggingMessageNotificationParams` (`level`, `logger`, `data`). Protocol logging is itself deprecated by the 2026-07-28 spec (**[Logging](../handlers/logging.md)** has what to do instead), so this callback exists for the servers that still emit it. | ||
|
|
||
| `message_handler` is the catch-all: every server notification reaches it (as well as its specific callback), and on a stream-backed transport so does every transport-level `Exception`. The one pattern worth knowing is `if isinstance(message, Exception): raise message`, so a broken connection fails loudly instead of vanishing. | ||
| `message_handler` is the catch-all for spec traffic: every notification the negotiated protocol version defines reaches it (as well as its specific callback), and on a stream-backed transport so does every transport-level `Exception`. Vendor and extension methods are outside that set; they are delivered only to a matching `NotificationBinding` (**[Extensions](../advanced/extensions.md)**), and without one they are dropped with a warning. The one pattern worth knowing is `if isinstance(message, Exception): raise message`, so a broken connection fails loudly instead of vanishing. |
There was a problem hiding this comment.
🟡 The rewritten sentence overclaims: "every notification the negotiated protocol version defines reaches it" is contradicted by the SDK itself — notifications/cancelled (defined at every negotiated version) is deliberately swallowed by ClientSession._on_notify before the message_handler tee, and a well-formed notifications/subscriptions/acknowledged owned by a live listen() route is consumed as driver state and never tees either. Since this PR re-scoped the sentence to be precise about message_handler delivery, consider carving out these two exceptions.
Extended reasoning...
What the doc claims vs. what the code does
The PR rewrites the message_handler sentence in docs/client/callbacks.md from the old "every server notification reaches it" to a more precise-sounding "message_handler is the catch-all for spec traffic: every notification the negotiated protocol version defines reaches it (as well as its specific callback)". The rewrite deliberately narrows the claim to spec-defined notifications so the new vendor/NotificationBinding split can be documented — but the narrowed claim is still stated as exceptionless, and the SDK contradicts it in two places.
Exception 1: notifications/cancelled never reaches message_handler
In src/mcp/client/session.py, _on_notify parses the inbound frame through the per-version notification tables and then returns early for types.CancelledNotification with the comment "Never surfaced (v1 parity): the dispatcher already applied it" (lines 1369–1371) — before the message_handler tee. The intercept path confirms the same contract at line 1306: "_on_notify swallows every cancelled either way (v1 parity)". notifications/cancelled is defined at every negotiated protocol version — that is precisely why it parses through parse_server_notification and hits the isinstance branch — so it squarely falls inside the doc's "every notification the negotiated protocol version defines" set while never being delivered.
Exception 2: owned subscription acks are consumed as driver state
_intercept_notification consumes (returns True for) a well-formed notifications/subscriptions/acknowledged whose _meta subscription id matches a live listen() route (session.py:1316–1326): the ack becomes the route's honored filter and never reaches the normal notification path, so it never tees to message_handler. Only unowned or malformed acks fall through and tee.
Why this is pinned SDK behavior, not an accident
Both behaviors are guarded by the SDK's own tests: tests/client/test_session.py::test_receive_loop_consumes_server_cancelled_without_reaching_message_handler asserts a server-sent notifications/cancelled is swallowed while a follow-up notifications/tools/list_changed does reach the handler, and test_intercept_consumes_acks_for_live_routes_and_leaves_malformed_ones asserts the owned-ack consumption (is True), with test_a_late_ack_for_a_closed_driver_listen_reaches_message_handler pinning that only unowned acks tee. So the doc cannot be brought true by changing the code without breaking pinned contracts.
Step-by-step proof (cancelled case)
- A server sends
{"method": "notifications/cancelled", "params": {"requestId": 1, "reason": "timed out"}}— a notification the negotiated version defines. _intercept_notificationreturnsFalse(no listen route named), so the frame proceeds to_on_notify._on_notifyvalidates it intotypes.CancelledNotificationvia the per-version tables — validation succeeds because the method is defined at the version.- The
isinstance(notification, types.CancelledNotification)branch returns at session.py:1371, before the tee at line 1375. - A user's
message_handler— which the doc promises receives "every notification the negotiated protocol version defines" — never sees it. The pinned test asserts exactly this:seencontains only the follow-upToolListChangedNotification.
Why this is fair to flag on this PR (not pre-existing)
The old sentence had the same flaw for notifications/cancelled, but this PR rewrote the exact sentence with the express purpose of making the message_handler delivery contract precise (spec traffic vs. vendor traffic). Having taken ownership of the sentence's precision, leaving a residual exceptionless universal is a residue worth fixing in the same pass.
Impact and fix
Doc-only: a user writing a message_handler that exhaustively matches spec notifications (or one that watches for cancellations) would be misled into expecting frames that never arrive. Nothing breaks at runtime. The fix is a small carve-out clause, e.g.: "…every notification the negotiated protocol version defines reaches it (as well as its specific callback), except notifications/cancelled (consumed by the dispatcher) and subscription acks owned by an open listen() stream."
1 participant
Custom (non-spec) notifications get a typed send surface, and dropping one now leaves a trace in the logs.
Motivation and Context
Feedback from a downstream framework: a server-to-client notification whose method isn't in the negotiated version's core tables only reaches a registered
NotificationBinding, and otherwise is dropped atlogger.debug. That drop is silent enough that the fix on the framework's side was found by trial and error, and even once you know the channel,send_notificationon both sessions is typed to the closed spec unions, so emitting a vendor notification needs acast()or# type: ignoreeven though the runtime already sends whatever it's given.Two changes:
ClientSession.send_notificationandServerSession.send_notificationaccept anymcp_types.Notificationsubclass, matching the openRequest[Any, Any]armsend_requestalready has in both classes. The runtime path is unchanged; this legalizes the existing behaviour.debugto a once-per-methodwarningthat names the remedy: the client when no binding observes the method, the server when a spec notification isn't defined at the negotiated version or a custom method has no registered handler. Repeats of the same method fall back todebugso a chatty stream can't flood the log. An unserved spec notification stays atdebug(most servers legitimately ignorenotifications/roots/list_changedand friends).Nothing is teed to
message_handler: its contract is the typed spec union, and v1 dropped unknown methods before it too (atWARNING). The one v1 delivery difference isnotifications/tasks/status, which was a member of v1's union; the migration guide now says where it went and how to observe it.How Has This Been Tested?
New unit tests pin the log levels (first drop
WARNING, repeatDEBUG, unserved spec methodDEBUG) on both sides, plus a wire test for a custom model going throughClientSession.send_notificationunchanged. A new interaction test drives the full loop: anMCPServertool emits vendor notifications throughctx.session.send_notification(..., related_request_id=ctx.request_id)and the declaringClientExtension's binding receives them in order; that un-defers the previously deferredextensions:client:notification-binding-deliveryrequirement. Also exercised end-to-end against a running streamable HTTP server at both2025-11-25and2026-07-28.Breaking Changes
None. The widened annotations only accept more; the log-level change is observability.
Types of changes
Checklist
Additional context
Deliberately out of scope, happy to file follow-ups: a catch-all handler for unobserved notifications, a
Client(notification_bindings=...)kwarg (a one-off binding on the high-levelClientcurrently needs aClientExtension, which also advertises its identifier), anotifications()contribution on the serverExtension, and outbound era-gating for spec notifications.AI Disclaimer