Skip to content

[Server][Capability] Do not announce an externally loaded registry as changed - #490

Merged
chr-hertel merged 2 commits into
modelcontextprotocol:mainfrom
chr-hertel:registry-load-guard-external-loader
Aug 29, 2026
Merged

[Server][Capability] Do not announce an externally loaded registry as changed#490
chr-hertel merged 2 commits into
modelcontextprotocol:mainfrom
chr-hertel:registry-load-guard-external-loader

Conversation

@chr-hertel

@chr-hertel chr-hertel commented Aug 29, 2026

Copy link
Copy Markdown
Member

Registry suppresses its *ListChangedEvents while loading, but the loading guard is only set by Registry::load(). Builder::resolve() loads a caller-supplied registry by calling $chainLoader->load($registry) directly, which bypasses it — so with a notification bus configured, every build() publishes one list_changed per element for a registry that did not change.

$registry = new Registry(new PublishingEventDispatcher($bus));
$tools(Server::builder()->setRegistry($registry)->setNotificationBus($bus))->build();
echo $bus->cursor();   // 3, and 0 for a registry the Builder constructs itself

InMemoryNotificationBus mostly gets away with it. Psr16NotificationBus does not: under PHP-FPM the server is built per request and the bus is shared, so every request broadcasts its whole element list to every open subscriptions/listen stream and consumes the 256-entry backlog — after which a lagging reader silently skips real notifications.

Splits the guarded body of load() into loadFrom(LoaderInterface $loader) and routes the custom-registry branch through it.

loadFrom() takes only the loading guard; loaded stays with load(). The loader it runs is the caller's, so marking the registry loaded there would retire a constructor loader that never ran, and make a second loadFrom() — one registry handed to two builders — a silent no-op.

RegistryInterface declares no load(), so this stays on the concrete Registry; anything else keeps the path it has today. No BC break.

Found in symfony/mcp-bundle, which always supplies its own registry (symfony/ai#2458).

`Registry` suppresses its `*ListChangedEvent`s while it is loading — `dispatch()`
returns early on the `loading` guard, so the elements a loader registers are the
registry's initial contents rather than a change to them.
`testListChangedEventsAreSuppressedDuringTheDeferredLoad` pins that.

The guard only covers `Registry::load()`, which needs the loader the constructor
took. A registry the caller built cannot be given one that way, so
`Builder::resolve()` loads it from the outside instead — by calling
`$chainLoader->load($registry)` directly, which never sets the guard. Every
element then dispatches on the way in.

With a notification bus configured that is not quiet. `PublishingEventDispatcher`
turns each event into a published notification, so every `build()` puts one
`list_changed` per element on the bus for a registry that did not change. Under
PHP-FPM, where the server is built per request and `Psr16NotificationBus` is
shared and persistent, every request broadcasts its whole element list to every
open `subscriptions/listen` stream and consumes the 256-entry backlog — after
which a reader that fell behind silently skips real notifications.

Split the guarded body of `load()` into `loadFrom(LoaderInterface $loader)` and
route the custom-registry branch through it.

`loadFrom()` takes only the `loading` guard, not the `loaded` bookkeeping, which
stays with `load()`. The loader it runs belongs to the caller, so it cannot stand
in for the one the registry was constructed with: marking the registry loaded
would retire a constructor loader that never ran, and would make a second
`loadFrom()` — one registry handed to two builders — a silent no-op. Keeping
`loaded` out of it also keeps `load()`'s promise that a transient failure is
retried on the next read, including when the failing loader reads the registry
during its own run.

`RegistryInterface` declares no `load()`, so this stays on the concrete
`Registry`, and a third-party implementation keeps the path it has today.
@chr-hertel
chr-hertel force-pushed the registry-load-guard-external-loader branch from 006070e to 40c9eaa Compare August 29, 2026 23:02
Comment thread src/Capability/Registry.php Outdated
Co-authored-by: Christopher Hertel <mail@christopher-hertel.de>
@chr-hertel chr-hertel added bug Something isn't working Server Issues & PRs related to the Server component labels Aug 29, 2026
@chr-hertel chr-hertel changed the title [Capability] Do not announce an externally loaded registry as changed [Server][Capability] Do not announce an externally loaded registry as changed Aug 29, 2026
@chr-hertel
chr-hertel merged commit c5dbfb6 into modelcontextprotocol:main Aug 29, 2026
27 checks passed
@chr-hertel
chr-hertel deleted the registry-load-guard-external-loader branch August 29, 2026 23:11
chr-hertel added a commit to chr-hertel/ai that referenced this pull request Aug 29, 2026
…tion bus

`subscriptions:` configures a notification bus, and the 2026-07-28 protocol reads
it: every `subscriptions/listen` stream polls it for the list-changed
notifications it agreed to carry. Nothing ever writes to it.

The SDK wires the publishing half itself — `Builder` wraps the event dispatcher
in a `PublishingEventDispatcher` when a bus is configured, so that a runtime
`registerTool()` reaches a listening client without the caller knowing a bus
exists. But it can only wrap a registry it constructs, and a registry handed in
through `setRegistry()` is already built. This bundle always supplies one, so the
wrapping never happens: the bus is read by every stream and written to by
nothing.

From a client that is worse than an error. The stream opens, the acknowledgment
names the types the server agreed to carry, keep-alives arrive for the configured
lifetime, and it closes gracefully having carried nothing — whatever changed on
the server. Every observable part of the mechanism works except the one that
matters.

Give the registry the publishing dispatcher where it is registered, which is the
only place the bundle knows both it and the bus. Per server rather than one
publisher on the shared `event_dispatcher`, because the registries are per server
too: a tool appearing on one of them is not news to a client subscribed to
another.

Requires mcp/sdk 0.8.1. Until modelcontextprotocol/php-sdk#490 the SDK loaded a
supplied registry without its `loading` guard, so publishing from the registry
also published every element the load registered — one `list_changed` each, on
every build, for a registry that did not change.
chr-hertel added a commit to chr-hertel/ai that referenced this pull request Aug 29, 2026
…tion bus

`subscriptions:` configures a notification bus, and the 2026-07-28 protocol reads
it: every `subscriptions/listen` stream polls it for the list-changed
notifications it agreed to carry. Nothing ever writes to it.

The SDK wires the publishing half itself — `Builder` wraps the event dispatcher
in a `PublishingEventDispatcher` when a bus is configured, so that a runtime
`registerTool()` reaches a listening client without the caller knowing a bus
exists. But it can only wrap a registry it constructs, and a registry handed in
through `setRegistry()` is already built. This bundle always supplies one, so the
wrapping never happens: the bus is read by every stream and written to by
nothing.

From a client that is worse than an error. The stream opens, the acknowledgment
names the types the server agreed to carry, keep-alives arrive for the configured
lifetime, and it closes gracefully having carried nothing — whatever changed on
the server. Every observable part of the mechanism works except the one that
matters.

Give the registry the publishing dispatcher where it is registered, which is the
only place the bundle knows both it and the bus. Per server rather than one
publisher on the shared `event_dispatcher`, because the registries are per server
too: a tool appearing on one of them is not news to a client subscribed to
another.

Requires mcp/sdk 0.8.1. Until modelcontextprotocol/php-sdk#490 the SDK loaded a
supplied registry without its `loading` guard, so publishing from the registry
also published every element the load registered — one `list_changed` each, on
every build, for a registry that did not change.
chr-hertel added a commit to symfony/ai that referenced this pull request Aug 30, 2026
…ed notification bus (chr-hertel)

This PR was merged into the main branch.

Discussion
----------

[MCP Bundle] Publish the registry's changes to the configured notification bus

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| Docs?         | no
| Issues        | -
| License       | MIT

`subscriptions:` configures a notification bus, and the 2026-07-28 protocol reads it: every `subscriptions/listen` stream polls it for the list-changed notifications it agreed to carry. Nothing ever writes to it.

The SDK wires the publishing half itself — `Builder` wraps the event dispatcher in a `PublishingEventDispatcher` when a bus is configured. But it can only wrap a registry it constructs, and this bundle always supplies one (`mcp.server.<name>.registry`, built with Symfony's `event_dispatcher`), so the wrapping never happens. The two halves are each individually right and never meet.

From a client that is worse than an error: the stream opens, the acknowledgment names the types the server agreed to carry, keep-alives arrive for the configured lifetime, and it closes gracefully having carried nothing — whatever changed on the server.

Gives the registry the publishing dispatcher where it is registered, which is the only place the bundle knows both it and the bus. Per server, because the registries are per server.

Requires **mcp/sdk 0.8.1**. Below that the SDK loads a supplied registry without its `loading` guard (modelcontextprotocol/php-sdk#490), so publishing from the registry also publishes every element the load registers — this patch would turn an inert bus into a noisy one.

Found by a demo application that drives its own MCP servers with its own MCP client over a real transport ([chr-hertel/mcp-demo](https://github.com/chr-hertel/mcp-demo)).

Commits
-------

283b19a [McpBundle] Publish the registry's changes to the configured notification bus
symfony-splitter pushed a commit to symfony/mcp-bundle that referenced this pull request Aug 30, 2026
…tion bus

`subscriptions:` configures a notification bus, and the 2026-07-28 protocol reads
it: every `subscriptions/listen` stream polls it for the list-changed
notifications it agreed to carry. Nothing ever writes to it.

The SDK wires the publishing half itself — `Builder` wraps the event dispatcher
in a `PublishingEventDispatcher` when a bus is configured, so that a runtime
`registerTool()` reaches a listening client without the caller knowing a bus
exists. But it can only wrap a registry it constructs, and a registry handed in
through `setRegistry()` is already built. This bundle always supplies one, so the
wrapping never happens: the bus is read by every stream and written to by
nothing.

From a client that is worse than an error. The stream opens, the acknowledgment
names the types the server agreed to carry, keep-alives arrive for the configured
lifetime, and it closes gracefully having carried nothing — whatever changed on
the server. Every observable part of the mechanism works except the one that
matters.

Give the registry the publishing dispatcher where it is registered, which is the
only place the bundle knows both it and the bus. Per server rather than one
publisher on the shared `event_dispatcher`, because the registries are per server
too: a tool appearing on one of them is not news to a client subscribed to
another.

Requires mcp/sdk 0.8.1. Until modelcontextprotocol/php-sdk#490 the SDK loaded a
supplied registry without its `loading` guard, so publishing from the registry
also published every element the load registered — one `list_changed` each, on
every build, for a registry that did not change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Server Issues & PRs related to the Server component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant