[Server][Capability] Do not announce an externally loaded registry as changed - #490
Merged
chr-hertel merged 2 commits intoAug 29, 2026
Conversation
chr-hertel
requested review from
CodeWithKyrian,
Nyholm and
soyuka
as code owners
August 29, 2026 22:29
chr-hertel
force-pushed
the
registry-load-guard-external-loader
branch
from
August 29, 2026 22:43
fefe4df to
006070e
Compare
`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
force-pushed
the
registry-load-guard-external-loader
branch
from
August 29, 2026 23:02
006070e to
40c9eaa
Compare
chr-hertel
commented
Aug 29, 2026
Co-authored-by: Christopher Hertel <mail@christopher-hertel.de>
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Registrysuppresses its*ListChangedEvents while loading, but theloadingguard is only set byRegistry::load().Builder::resolve()loads a caller-supplied registry by calling$chainLoader->load($registry)directly, which bypasses it — so with a notification bus configured, everybuild()publishes onelist_changedper element for a registry that did not change.InMemoryNotificationBusmostly gets away with it.Psr16NotificationBusdoes 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 opensubscriptions/listenstream and consumes the 256-entry backlog — after which a lagging reader silently skips real notifications.Splits the guarded body of
load()intoloadFrom(LoaderInterface $loader)and routes the custom-registry branch through it.loadFrom()takes only theloadingguard;loadedstays withload(). 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 secondloadFrom()— one registry handed to two builders — a silent no-op.RegistryInterfacedeclares noload(), so this stays on the concreteRegistry; 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).