Skip to content

Commit 40c9eaa

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. `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.
1 parent 46628fb commit 40c9eaa

4 files changed

Lines changed: 135 additions & 4 deletions

File tree

src/Capability/Registry.php

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,10 +88,28 @@ public function load(): void
8888
return;
8989
}
9090

91+
$this->loadFrom($this->loader);
92+
93+
// Only on success: a failure propagates, so it is retried on the next read.
94+
$this->loaded = true;
95+
}
96+
97+
/**
98+
* Runs $loader with the change events its registrations would dispatch suppressed, since they
99+
* describe the registry filling up rather than changing.
100+
*
101+
* Re-entrant-safe, and failure propagates. Does not mark the registry loaded: $loader is the
102+
* caller's, and the one the constructor took is still owed its run.
103+
*/
104+
public function loadFrom(LoaderInterface $loader): void
105+
{
106+
if ($this->loading) {
107+
return;
108+
}
109+
91110
$this->loading = true;
92111
try {
93-
$this->loader->load($this);
94-
$this->loaded = true;
112+
$loader->load($this);
95113
} finally {
96114
$this->loading = false;
97115
}

src/Server/Builder.php

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -522,7 +522,8 @@ public function setRegistry(RegistryInterface $registry): self
522522
*
523523
* Lazy (the default) defers loading to the first registry read so a persistent runtime does not
524524
* freeze the registry to a source not yet ready at build time. Disable to load eagerly at build.
525-
* A registry supplied via setRegistry() is always loaded eagerly.
525+
* A registry supplied via setRegistry() is always loaded eagerly; its own constructor loader,
526+
* if it has one, still runs on the first read.
526527
*/
527528
public function setLazyLoading(bool $lazyLoading = true): self
528529
{
@@ -1045,8 +1046,13 @@ private function resolve(): array
10451046

10461047
if ($this->hasCustomRegistry) {
10471048
// Builder can't inject the loader into an already-constructed instance, so load it eagerly.
1049+
// Via loadFrom(), which suppresses the change events the load would otherwise dispatch.
10481050
$registry = $this->registry;
1049-
$chainLoader->load($registry);
1051+
if ($registry instanceof Registry) {
1052+
$registry->loadFrom($chainLoader);
1053+
} else {
1054+
$chainLoader->load($registry);
1055+
}
10501056
$eagerlyLoaded = true;
10511057
} else {
10521058
$registry = new Registry($eventDispatcher, $logger, loader: $chainLoader);

tests/Unit/Capability/RegistryTest.php

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

756+
public function testListChangedEventsAreSuppressedWhenTheLoaderIsSuppliedFromOutside(): void
757+
{
758+
// As quiet as the deferred load above: these are the initial contents, not a change.
759+
$eventDispatcher = $this->createMock(EventDispatcherInterface::class);
760+
$eventDispatcher->expects($this->never())->method('dispatch');
761+
762+
$registry = new Registry($eventDispatcher, $this->logger);
763+
$registry->loadFrom($this->toolLoader($this->createValidTool('loaded')));
764+
765+
$this->assertTrue($registry->hasTool('loaded'));
766+
}
767+
768+
public function testARuntimeRegistrationAfterAnExternalLoadIsStillDispatched(): void
769+
{
770+
$eventDispatcher = $this->createMock(EventDispatcherInterface::class);
771+
$eventDispatcher->expects($this->once())
772+
->method('dispatch')
773+
->with($this->isInstanceOf(ToolListChangedEvent::class))
774+
->willReturnArgument(0);
775+
776+
$registry = new Registry($eventDispatcher, $this->logger);
777+
$registry->loadFrom($this->toolLoader($this->createValidTool('loaded')));
778+
779+
$registry->registerTool($this->createValidTool('runtime'), 'handler');
780+
}
781+
782+
public function testAnExternalLoadDoesNotConsumeTheRegistrysOwnLoader(): void
783+
{
784+
$registry = new Registry($this->createMock(EventDispatcherInterface::class), $this->logger, loader: $this->toolLoader($this->createValidTool('own')));
785+
$registry->loadFrom($this->toolLoader($this->createValidTool('external')));
786+
787+
$this->assertTrue($registry->hasTool('external'));
788+
$this->assertTrue($registry->hasTool('own'));
789+
}
790+
791+
public function testAnExternalLoadCanRunMoreThanOnce(): void
792+
{
793+
$registry = new Registry($this->createMock(EventDispatcherInterface::class), $this->logger);
794+
$registry->loadFrom($this->toolLoader($this->createValidTool('first')));
795+
$registry->loadFrom($this->toolLoader($this->createValidTool('second')));
796+
797+
$this->assertTrue($registry->hasTool('first'));
798+
$this->assertTrue($registry->hasTool('second'));
799+
}
800+
801+
public function testAFailedExternalLoadLeavesTheRegistryRetryable(): void
802+
{
803+
$registry = new Registry($this->createMock(EventDispatcherInterface::class), $this->logger);
804+
$failing = new class implements LoaderInterface {
805+
public function load(RegistryInterface $registry): void
806+
{
807+
// Reads during its own run, as discovery's identity check does.
808+
$registry->hasTool('anything');
809+
810+
throw new \RuntimeException('data source not ready');
811+
}
812+
};
813+
814+
foreach ([1, 2] as $attempt) {
815+
try {
816+
$registry->loadFrom($failing);
817+
$this->fail('The loader was expected to fail.');
818+
} catch (\RuntimeException $e) {
819+
$this->assertSame('data source not ready', $e->getMessage());
820+
}
821+
}
822+
}
823+
756824
public function testListChangedEventsAreStillDispatchedForRuntimeRegistrations(): void
757825
{
758826
$eventDispatcher = $this->createMock(EventDispatcherInterface::class);

tests/Unit/Server/BuilderTest.php

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use Mcp\Capability\Registry\ElementReference;
1616
use Mcp\Capability\Registry\Loader\LoaderInterface;
1717
use Mcp\Capability\Registry\ReferenceHandlerInterface;
18+
use Mcp\Capability\RegistryInterface;
1819
use Mcp\Exception\InvalidArgumentException;
1920
use Mcp\Exception\LogicException;
2021
use Mcp\Schema\Content\TextContent;
@@ -32,6 +33,8 @@
3233
use Mcp\Server\Protocol;
3334
use Mcp\Server\Session\SessionInterface;
3435
use Mcp\Server\Stateless\StatelessProtocol;
36+
use Mcp\Server\Subscription\InMemoryNotificationBus;
37+
use Mcp\Server\Subscription\PublishingEventDispatcher;
3538
use Mcp\Tests\Unit\Server\Extension\ThingExtension;
3639
use Mcp\Tests\Unit\Server\Extension\ThingListHandler;
3740
use Mcp\Tests\Unit\Server\Extension\ThingListRequest;
@@ -404,6 +407,42 @@ private function callTool(Server $server, string $toolName): mixed
404407

405408
$this->fail('CallToolHandler not found in request handlers');
406409
}
410+
411+
public function testBuildingWithASuppliedRegistryDoesNotAnnounceTheLoadAsAChange(): void
412+
{
413+
// Otherwise every build publishes one list_changed per element, for no change.
414+
$bus = new InMemoryNotificationBus();
415+
$registry = new Registry(new PublishingEventDispatcher($bus));
416+
417+
Server::builder()
418+
->setRegistry($registry)
419+
->setNotificationBus($bus)
420+
->addTool(static fn (): string => 'ok', 'alpha')
421+
->addTool(static fn (): string => 'ok', 'beta')
422+
->build();
423+
424+
$this->assertSame(0, $bus->cursor());
425+
$this->assertTrue($registry->hasTool('alpha'));
426+
$this->assertTrue($registry->hasTool('beta'));
427+
428+
// A real change after the build is still published.
429+
$registry->unregisterTool('alpha');
430+
431+
$this->assertSame(1, $bus->cursor());
432+
}
433+
434+
public function testAThirdPartyRegistryIsStillLoadedThroughThePlainLoader(): void
435+
{
436+
$registry = $this->createMock(RegistryInterface::class);
437+
$registry->expects($this->once())
438+
->method('registerTool')
439+
->with($this->callback(static fn (Tool $tool): bool => 'alpha' === $tool->name));
440+
441+
Server::builder()
442+
->setRegistry($registry)
443+
->addTool(static fn (): string => 'ok', 'alpha')
444+
->build();
445+
}
407446
}
408447

409448
/**

0 commit comments

Comments
 (0)