Skip to content

Commit fefe4df

Browse files
committed
[Capability] Do not announce an externally loaded registry as changed
`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. `RegistryInterface` declares no `load()`, so this stays on the concrete `Registry`, and a third-party implementation keeps the path it has today.
1 parent 46628fb commit fefe4df

4 files changed

Lines changed: 78 additions & 3 deletions

File tree

src/Capability/Registry.php

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,13 +84,31 @@ public function __construct(
8484
*/
8585
public function load(): void
8686
{
87-
if ($this->loaded || $this->loading || null === $this->loader) {
87+
if (null === $this->loader) {
88+
return;
89+
}
90+
91+
$this->loadFrom($this->loader);
92+
}
93+
94+
/**
95+
* Runs a loader supplied from outside, under the same guard {@see load()} uses.
96+
*
97+
* A registry the caller constructed cannot be given a loader through the constructor, so
98+
* {@see \Mcp\Server\Builder} loads it from the outside instead. Doing that by calling the
99+
* loader directly skips the guard, and then every element registered during the load is
100+
* announced as a change — which on a configured notification bus is a burst of `list_changed`
101+
* notifications on every build, for a registry that did not change.
102+
*/
103+
public function loadFrom(LoaderInterface $loader): void
104+
{
105+
if ($this->loaded || $this->loading) {
88106
return;
89107
}
90108

91109
$this->loading = true;
92110
try {
93-
$this->loader->load($this);
111+
$loader->load($this);
94112
$this->loaded = true;
95113
} finally {
96114
$this->loading = false;

src/Server/Builder.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1045,8 +1045,10 @@ private function resolve(): array
10451045

10461046
if ($this->hasCustomRegistry) {
10471047
// Builder can't inject the loader into an already-constructed instance, so load it eagerly.
1048+
// Through loadFrom() where possible: loading a registry from the outside otherwise skips the
1049+
// guard that keeps the load itself from being announced as a change.
10481050
$registry = $this->registry;
1049-
$chainLoader->load($registry);
1051+
$registry instanceof Registry ? $registry->loadFrom($chainLoader) : $chainLoader->load($registry);
10501052
$eagerlyLoaded = true;
10511053
} else {
10521054
$registry = new Registry($eventDispatcher, $logger, loader: $chainLoader);

tests/Unit/Capability/RegistryTest.php

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -753,6 +753,34 @@ public function load(RegistryInterface $registry): void
753753
$this->assertTrue($registry->hasPrompts());
754754
}
755755

756+
public function testListChangedEventsAreSuppressedWhenTheLoaderIsSuppliedFromOutside(): void
757+
{
758+
// A registry the caller constructed cannot take a loader through the constructor, so
759+
// Builder loads it from the outside. That has to be as quiet as the deferred load above:
760+
// the elements arriving are the registry's initial contents, not a change to them.
761+
$eventDispatcher = $this->createMock(EventDispatcherInterface::class);
762+
$eventDispatcher->expects($this->never())->method('dispatch');
763+
764+
$registry = new Registry($eventDispatcher, $this->logger);
765+
$registry->loadFrom($this->toolLoader($this->createValidTool('loaded')));
766+
767+
$this->assertTrue($registry->hasTool('loaded'));
768+
}
769+
770+
public function testARuntimeRegistrationAfterAnExternalLoadIsStillDispatched(): void
771+
{
772+
$eventDispatcher = $this->createMock(EventDispatcherInterface::class);
773+
$eventDispatcher->expects($this->once())
774+
->method('dispatch')
775+
->with($this->isInstanceOf(ToolListChangedEvent::class))
776+
->willReturnArgument(0);
777+
778+
$registry = new Registry($eventDispatcher, $this->logger);
779+
$registry->loadFrom($this->toolLoader($this->createValidTool('loaded')));
780+
781+
$registry->registerTool($this->createValidTool('runtime'), 'handler');
782+
}
783+
756784
public function testListChangedEventsAreStillDispatchedForRuntimeRegistrations(): void
757785
{
758786
$eventDispatcher = $this->createMock(EventDispatcherInterface::class);

tests/Unit/Server/BuilderTest.php

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@
3232
use Mcp\Server\Protocol;
3333
use Mcp\Server\Session\SessionInterface;
3434
use Mcp\Server\Stateless\StatelessProtocol;
35+
use Mcp\Server\Subscription\InMemoryNotificationBus;
36+
use Mcp\Server\Subscription\PublishingEventDispatcher;
3537
use Mcp\Tests\Unit\Server\Extension\ThingExtension;
3638
use Mcp\Tests\Unit\Server\Extension\ThingListHandler;
3739
use Mcp\Tests\Unit\Server\Extension\ThingListRequest;
@@ -404,6 +406,31 @@ private function callTool(Server $server, string $toolName): mixed
404406

405407
$this->fail('CallToolHandler not found in request handlers');
406408
}
409+
410+
public function testBuildingWithASuppliedRegistryDoesNotAnnounceTheLoadAsAChange(): void
411+
{
412+
// The registry a caller supplies carries its own dispatcher, which the Builder has no
413+
// handle on — so the load has to be quiet at the registry, or every build publishes one
414+
// list_changed per element. On a shared bus that is a burst per process, for no change.
415+
$bus = new InMemoryNotificationBus();
416+
$registry = new Registry(new PublishingEventDispatcher($bus));
417+
418+
Server::builder()
419+
->setRegistry($registry)
420+
->setNotificationBus($bus)
421+
->addTool(static fn (): string => 'ok', 'alpha')
422+
->addTool(static fn (): string => 'ok', 'beta')
423+
->build();
424+
425+
$this->assertSame(0, $bus->cursor());
426+
$this->assertTrue($registry->hasTool('alpha'));
427+
$this->assertTrue($registry->hasTool('beta'));
428+
429+
// What the bus is for still works: a change after the build is published.
430+
$registry->unregisterTool('alpha');
431+
432+
$this->assertSame(1, $bus->cursor());
433+
}
407434
}
408435

409436
/**

0 commit comments

Comments
 (0)