From 40c9eaa547ef8d4cdc9ce1f7c1cf7d5e881b550e Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Sun, 30 Aug 2026 00:28:58 +0200 Subject: [PATCH 1/2] [Capability] Do not announce an externally loaded registry as changed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- src/Capability/Registry.php | 22 ++++++++- src/Server/Builder.php | 10 +++- tests/Unit/Capability/RegistryTest.php | 68 ++++++++++++++++++++++++++ tests/Unit/Server/BuilderTest.php | 39 +++++++++++++++ 4 files changed, 135 insertions(+), 4 deletions(-) diff --git a/src/Capability/Registry.php b/src/Capability/Registry.php index 794193f8..73c583d1 100644 --- a/src/Capability/Registry.php +++ b/src/Capability/Registry.php @@ -88,10 +88,28 @@ public function load(): void return; } + $this->loadFrom($this->loader); + + // Only on success: a failure propagates, so it is retried on the next read. + $this->loaded = true; + } + + /** + * Runs $loader with the change events its registrations would dispatch suppressed, since they + * describe the registry filling up rather than changing. + * + * Re-entrant-safe, and failure propagates. Does not mark the registry loaded: $loader is the + * caller's, and the one the constructor took is still owed its run. + */ + public function loadFrom(LoaderInterface $loader): void + { + if ($this->loading) { + return; + } + $this->loading = true; try { - $this->loader->load($this); - $this->loaded = true; + $loader->load($this); } finally { $this->loading = false; } diff --git a/src/Server/Builder.php b/src/Server/Builder.php index 90272ef3..1a5dd73b 100644 --- a/src/Server/Builder.php +++ b/src/Server/Builder.php @@ -522,7 +522,8 @@ public function setRegistry(RegistryInterface $registry): self * * Lazy (the default) defers loading to the first registry read so a persistent runtime does not * freeze the registry to a source not yet ready at build time. Disable to load eagerly at build. - * A registry supplied via setRegistry() is always loaded eagerly. + * A registry supplied via setRegistry() is always loaded eagerly; its own constructor loader, + * if it has one, still runs on the first read. */ public function setLazyLoading(bool $lazyLoading = true): self { @@ -1045,8 +1046,13 @@ private function resolve(): array if ($this->hasCustomRegistry) { // Builder can't inject the loader into an already-constructed instance, so load it eagerly. + // Via loadFrom(), which suppresses the change events the load would otherwise dispatch. $registry = $this->registry; - $chainLoader->load($registry); + if ($registry instanceof Registry) { + $registry->loadFrom($chainLoader); + } else { + $chainLoader->load($registry); + } $eagerlyLoaded = true; } else { $registry = new Registry($eventDispatcher, $logger, loader: $chainLoader); diff --git a/tests/Unit/Capability/RegistryTest.php b/tests/Unit/Capability/RegistryTest.php index cb325565..92782ab9 100644 --- a/tests/Unit/Capability/RegistryTest.php +++ b/tests/Unit/Capability/RegistryTest.php @@ -753,6 +753,74 @@ public function load(RegistryInterface $registry): void $this->assertTrue($registry->hasPrompts()); } + public function testListChangedEventsAreSuppressedWhenTheLoaderIsSuppliedFromOutside(): void + { + // As quiet as the deferred load above: these are the initial contents, not a change. + $eventDispatcher = $this->createMock(EventDispatcherInterface::class); + $eventDispatcher->expects($this->never())->method('dispatch'); + + $registry = new Registry($eventDispatcher, $this->logger); + $registry->loadFrom($this->toolLoader($this->createValidTool('loaded'))); + + $this->assertTrue($registry->hasTool('loaded')); + } + + public function testARuntimeRegistrationAfterAnExternalLoadIsStillDispatched(): void + { + $eventDispatcher = $this->createMock(EventDispatcherInterface::class); + $eventDispatcher->expects($this->once()) + ->method('dispatch') + ->with($this->isInstanceOf(ToolListChangedEvent::class)) + ->willReturnArgument(0); + + $registry = new Registry($eventDispatcher, $this->logger); + $registry->loadFrom($this->toolLoader($this->createValidTool('loaded'))); + + $registry->registerTool($this->createValidTool('runtime'), 'handler'); + } + + public function testAnExternalLoadDoesNotConsumeTheRegistrysOwnLoader(): void + { + $registry = new Registry($this->createMock(EventDispatcherInterface::class), $this->logger, loader: $this->toolLoader($this->createValidTool('own'))); + $registry->loadFrom($this->toolLoader($this->createValidTool('external'))); + + $this->assertTrue($registry->hasTool('external')); + $this->assertTrue($registry->hasTool('own')); + } + + public function testAnExternalLoadCanRunMoreThanOnce(): void + { + $registry = new Registry($this->createMock(EventDispatcherInterface::class), $this->logger); + $registry->loadFrom($this->toolLoader($this->createValidTool('first'))); + $registry->loadFrom($this->toolLoader($this->createValidTool('second'))); + + $this->assertTrue($registry->hasTool('first')); + $this->assertTrue($registry->hasTool('second')); + } + + public function testAFailedExternalLoadLeavesTheRegistryRetryable(): void + { + $registry = new Registry($this->createMock(EventDispatcherInterface::class), $this->logger); + $failing = new class implements LoaderInterface { + public function load(RegistryInterface $registry): void + { + // Reads during its own run, as discovery's identity check does. + $registry->hasTool('anything'); + + throw new \RuntimeException('data source not ready'); + } + }; + + foreach ([1, 2] as $attempt) { + try { + $registry->loadFrom($failing); + $this->fail('The loader was expected to fail.'); + } catch (\RuntimeException $e) { + $this->assertSame('data source not ready', $e->getMessage()); + } + } + } + public function testListChangedEventsAreStillDispatchedForRuntimeRegistrations(): void { $eventDispatcher = $this->createMock(EventDispatcherInterface::class); diff --git a/tests/Unit/Server/BuilderTest.php b/tests/Unit/Server/BuilderTest.php index 9cdf3292..790af5f5 100644 --- a/tests/Unit/Server/BuilderTest.php +++ b/tests/Unit/Server/BuilderTest.php @@ -15,6 +15,7 @@ use Mcp\Capability\Registry\ElementReference; use Mcp\Capability\Registry\Loader\LoaderInterface; use Mcp\Capability\Registry\ReferenceHandlerInterface; +use Mcp\Capability\RegistryInterface; use Mcp\Exception\InvalidArgumentException; use Mcp\Exception\LogicException; use Mcp\Schema\Content\TextContent; @@ -32,6 +33,8 @@ use Mcp\Server\Protocol; use Mcp\Server\Session\SessionInterface; use Mcp\Server\Stateless\StatelessProtocol; +use Mcp\Server\Subscription\InMemoryNotificationBus; +use Mcp\Server\Subscription\PublishingEventDispatcher; use Mcp\Tests\Unit\Server\Extension\ThingExtension; use Mcp\Tests\Unit\Server\Extension\ThingListHandler; use Mcp\Tests\Unit\Server\Extension\ThingListRequest; @@ -404,6 +407,42 @@ private function callTool(Server $server, string $toolName): mixed $this->fail('CallToolHandler not found in request handlers'); } + + public function testBuildingWithASuppliedRegistryDoesNotAnnounceTheLoadAsAChange(): void + { + // Otherwise every build publishes one list_changed per element, for no change. + $bus = new InMemoryNotificationBus(); + $registry = new Registry(new PublishingEventDispatcher($bus)); + + Server::builder() + ->setRegistry($registry) + ->setNotificationBus($bus) + ->addTool(static fn (): string => 'ok', 'alpha') + ->addTool(static fn (): string => 'ok', 'beta') + ->build(); + + $this->assertSame(0, $bus->cursor()); + $this->assertTrue($registry->hasTool('alpha')); + $this->assertTrue($registry->hasTool('beta')); + + // A real change after the build is still published. + $registry->unregisterTool('alpha'); + + $this->assertSame(1, $bus->cursor()); + } + + public function testAThirdPartyRegistryIsStillLoadedThroughThePlainLoader(): void + { + $registry = $this->createMock(RegistryInterface::class); + $registry->expects($this->once()) + ->method('registerTool') + ->with($this->callback(static fn (Tool $tool): bool => 'alpha' === $tool->name)); + + Server::builder() + ->setRegistry($registry) + ->addTool(static fn (): string => 'ok', 'alpha') + ->build(); + } } /** From 32ca4f5540468ed93676b42c0564e0df4cab434c Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Sun, 30 Aug 2026 01:07:52 +0200 Subject: [PATCH 2/2] Apply batched suggestions from code review Co-authored-by: Christopher Hertel --- src/Capability/Registry.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Capability/Registry.php b/src/Capability/Registry.php index 73c583d1..97840431 100644 --- a/src/Capability/Registry.php +++ b/src/Capability/Registry.php @@ -90,7 +90,6 @@ public function load(): void $this->loadFrom($this->loader); - // Only on success: a failure propagates, so it is retried on the next read. $this->loaded = true; }