From 2bc2bdbabad91efd361ca9fb6aeb8726176c7bff Mon Sep 17 00:00:00 2001 From: soyuka Date: Wed, 2 Sep 2026 14:40:24 +0200 Subject: [PATCH] [Server][Capability] Defer loading into a custom registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `setLazyLoading()` defaults to true but never applied to a registry supplied through `setRegistry()`: the builder ran the chain loader itself and set `$eagerlyLoaded = true`, because it could not hand a loader to an instance it did not construct. Two consequences under a persistent runtime. The loaders run once at build, so a source that is not ready yet (a cold metadata cache) freezes the registry empty for the whole process and `tools/list` keeps returning `[]`. And `detectCapabilities()` reads `hasTools()` off that cold registry, so the server can advertise `tools: false` and a client that respects capabilities never calls `tools/list` at all. `Registry::deferLoadingFrom()` adopts a loader after construction, so the builder can defer instead of loading eagerly. It chains behind a loader the constructor took while that one is still owed its run, and replaces it once it has already run, since chaining would run it twice and discovery would rescan. Resetting `loaded` is what lets the adopted loader run on the next read: without it, a registry read before `build()` would keep the deferred loader forever unrun. Deferral is not conditional on the registry being empty. Gating it that way would mean a caller who hand-registers a single element before `setRegistry()` silently falls back to eager loading and gets the cold-source bug back. Instead `Registry::isEmpty()` — which reads the element arrays directly and so does not trigger the loader — tells `detectCapabilities()` that a deferred registry already holds elements, and it advertises them as one more opaque source. That over-advertises a registry holding only one kind, which is harmless per MCP semantics and already how custom loaders and discovery are treated. A foreign `RegistryInterface` cannot be deferred into and still loads eagerly. `RegistryInterface` is unchanged: both methods live on `Registry`, which is what the builder already type-checks for `loadFrom()`. --- src/Capability/Registry.php | 25 +++++- src/Server/Builder.php | 35 ++++++--- tests/Unit/Capability/RegistryTest.php | 102 ++++++++++++++++++++++++- tests/Unit/Server/BuilderTest.php | 86 +++++++++++++++++++++ 4 files changed, 233 insertions(+), 15 deletions(-) diff --git a/src/Capability/Registry.php b/src/Capability/Registry.php index 97840431..92c4a4f6 100644 --- a/src/Capability/Registry.php +++ b/src/Capability/Registry.php @@ -11,6 +11,7 @@ namespace Mcp\Capability; +use Mcp\Capability\Registry\Loader\ChainLoader; use Mcp\Capability\Registry\Loader\LoaderInterface; use Mcp\Capability\Registry\PromptReference; use Mcp\Capability\Registry\ResourceReference; @@ -69,7 +70,7 @@ public function __construct( private readonly ?EventDispatcherInterface $eventDispatcher = null, private readonly LoggerInterface $logger = new NullLogger(), private readonly NameValidator $nameValidator = new NameValidator(), - private readonly ?LoaderInterface $loader = null, + private ?LoaderInterface $loader = null, ) { } @@ -114,6 +115,28 @@ public function loadFrom(LoaderInterface $loader): void } } + /** + * Adopts $loader for the deferred load, so a registry the caller constructed can still load at + * first read instead of at build time. Chains behind a loader the constructor already took, + * but only when that loader is still owed its run: once it has already run, chaining would run + * it a second time — discovery would rescan — so $loader replaces it instead. Resetting $loaded + * is what makes the adopted loader actually run on the next read. + */ + public function deferLoadingFrom(LoaderInterface $loader): void + { + $this->loader = null === $this->loader || $this->loaded ? $loader : new ChainLoader([$this->loader, $loader]); + $this->loaded = false; + } + + /** + * True when nothing is registered yet. Reads the backing arrays directly, so unlike has*() it + * never triggers the loader. + */ + public function isEmpty(): bool + { + return [] === $this->tools && [] === $this->resources && [] === $this->resourceTemplates && [] === $this->prompts; + } + public function registerTool(Tool $tool, callable|array|string $handler): ToolReference { if (!$this->nameValidator->isValid($tool->name)) { diff --git a/src/Server/Builder.php b/src/Server/Builder.php index 1a5dd73b..0f42c8c5 100644 --- a/src/Server/Builder.php +++ b/src/Server/Builder.php @@ -522,8 +522,9 @@ 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; its own constructor loader, - * if it has one, still runs on the first read. + * A registry supplied via setRegistry() is deferred the same way, whatever it already holds is + * still advertised by capability detection. Either way its own constructor loader, if it has + * one, still runs on the first read. */ public function setLazyLoading(bool $lazyLoading = true): self { @@ -1043,17 +1044,25 @@ private function resolve(): array } $chainLoader = new ChainLoader($loaders); + $hasPreloadedElements = false; 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; - if ($registry instanceof Registry) { - $registry->loadFrom($chainLoader); + + if ($this->lazyLoading && $registry instanceof Registry) { + $hasPreloadedElements = !$registry->isEmpty(); + $registry->deferLoadingFrom($chainLoader); + $eagerlyLoaded = false; } else { - $chainLoader->load($registry); + // A foreign RegistryInterface cannot be deferred into, so it is loaded eagerly here. + // loadFrom() suppresses the change events the load would otherwise dispatch. + if ($registry instanceof Registry) { + $registry->loadFrom($chainLoader); + } else { + $chainLoader->load($registry); + } + $eagerlyLoaded = true; } - $eagerlyLoaded = true; } else { $registry = new Registry($eventDispatcher, $logger, loader: $chainLoader); if (!$this->lazyLoading) { @@ -1064,7 +1073,7 @@ private function resolve(): array $messageFactory = MessageFactory::make(additional: $this->extensionMessages); - $capabilities = $this->serverCapabilities ?? $this->detectCapabilities($registry, $eagerlyLoaded, $eventDispatcher); + $capabilities = $this->serverCapabilities ?? $this->detectCapabilities($registry, $eagerlyLoaded, $eventDispatcher, $hasPreloadedElements); // Extensions enabled via enableExtension() are folded into caller-supplied // capabilities too, so setCapabilities() does not silently drop them. @@ -1118,9 +1127,11 @@ private function resolve(): array /** * When loaded, capabilities are read from the registry. When deferred, reading it would force * the load, so they are advertised from the configured sources instead — opaque sources (custom - * loaders, discovery) advertise all kinds, and over-advertising is harmless per MCP semantics. + * loaders, discovery) advertise all kinds, and over-advertising is harmless per MCP semantics. A + * custom registry deferred while already holding elements ($hasPreloadedElements) counts as an + * opaque source too, for the same reason: reading it would force the load it is deferred to avoid. */ - private function detectCapabilities(RegistryInterface $registry, bool $eagerlyLoaded, ?EventDispatcherInterface $eventDispatcher): ServerCapabilities + private function detectCapabilities(RegistryInterface $registry, bool $eagerlyLoaded, ?EventDispatcherInterface $eventDispatcher, bool $hasPreloadedElements): ServerCapabilities { // Without a dispatcher the registry announces nothing, so there is no // list-changed notification to advertise. @@ -1143,7 +1154,7 @@ private function detectCapabilities(RegistryInterface $registry, bool $eagerlyLo ); } - $hasOpaqueSources = [] !== $this->loaders || null !== $this->discoveryBasePath; + $hasOpaqueSources = [] !== $this->loaders || null !== $this->discoveryBasePath || $hasPreloadedElements; $hasResources = [] !== $this->resources || [] !== $this->explicitResources || [] !== $this->resourceTemplates || [] !== $this->explicitResourceTemplates || $hasOpaqueSources; return new ServerCapabilities( diff --git a/tests/Unit/Capability/RegistryTest.php b/tests/Unit/Capability/RegistryTest.php index 92782ab9..4a35cbb8 100644 --- a/tests/Unit/Capability/RegistryTest.php +++ b/tests/Unit/Capability/RegistryTest.php @@ -853,6 +853,104 @@ public function testLoadIsANoopWithoutAConfiguredLoader(): void $this->assertFalse($registry->hasTools()); } + public function testDeferLoadingFromDoesNotRunUntilFirstRead(): void + { + $loader = $this->createMock(LoaderInterface::class); + $loader->expects($this->never())->method('load'); + + $registry = new Registry(null, $this->logger); + $registry->deferLoadingFrom($loader); + } + + public function testDeferLoadingFromRunsOnFirstReadAndPopulatesTheRegistry(): void + { + $registry = new Registry(null, $this->logger); + $registry->deferLoadingFrom($this->toolLoader($this->createValidTool('deferred'))); + + $this->assertTrue($registry->hasTools()); + $this->assertArrayHasKey('deferred', $registry->getTools()->references); + } + + public function testDeferLoadingFromRunsTheLoaderExactlyOnceAcrossManyReads(): void + { + $loader = $this->createMock(LoaderInterface::class); + $loader->expects($this->once())->method('load'); + + $registry = new Registry(null, $this->logger); + $registry->deferLoadingFrom($loader); + + $registry->hasTools(); + $registry->getTools(); + $registry->hasResources(); + } + + public function testDeferLoadingFromChainsBehindTheConstructorLoader(): void + { + // Both register 'shared'; last-write-wins proves the run order, since + // ChainLoader lets the later loader overwrite the earlier one's registration. + $constructorLoader = $this->toolLoader($this->createValidTool('shared', null, 'from constructor')); + $deferredLoader = $this->toolLoader($this->createValidTool('shared', null, 'from deferred')); + + $registry = new Registry(null, $this->logger, loader: $constructorLoader); + $registry->deferLoadingFrom($deferredLoader); + + $tools = $registry->getTools()->references; + + $this->assertArrayHasKey('shared', $tools); + $this->assertSame('from deferred', $tools['shared']->description); + } + + public function testDeferLoadingFromRunsTheAdoptedLoaderAfterTheConstructorLoaderAlreadyRan(): void + { + // A read before deferLoadingFrom() runs the constructor loader and sets $loaded, the bug + // this covers: the adopted loader was then stored but never run because load() returned on + // $loaded before consulting it. + $constructorLoader = new class implements LoaderInterface { + public int $calls = 0; + + public function load(RegistryInterface $registry): void + { + ++$this->calls; + } + }; + $adoptedLoader = new class implements LoaderInterface { + public int $calls = 0; + + public function load(RegistryInterface $registry): void + { + ++$this->calls; + } + }; + + $registry = new Registry(null, $this->logger, loader: $constructorLoader); + $registry->hasTools(); + + $registry->deferLoadingFrom($adoptedLoader); + $registry->hasTools(); + $registry->hasResources(); + + $this->assertSame(1, $constructorLoader->calls); + $this->assertSame(1, $adoptedLoader->calls); + } + + public function testIsEmptyIsTrueForAFreshRegistryAndDoesNotTriggerTheLoader(): void + { + $loader = $this->createMock(LoaderInterface::class); + $loader->expects($this->never())->method('load'); + + $registry = new Registry(null, $this->logger, loader: $loader); + + $this->assertTrue($registry->isEmpty()); + } + + public function testIsEmptyIsFalseAfterRegisterTool(): void + { + $registry = new Registry(null, $this->logger); + $registry->registerTool($this->createValidTool('registered'), 'handler'); + + $this->assertFalse($registry->isEmpty()); + } + private function toolLoader(Tool $tool): LoaderInterface { return new class($tool) implements LoaderInterface { @@ -907,7 +1005,7 @@ public function jsonSerialize(): float $this->assertNull($toolRef->extractStructuredContent($result, ProtocolVersion::V2025_11_25)); } - private function createValidTool(string $name, ?array $outputSchema = null): Tool + private function createValidTool(string $name, ?array $outputSchema = null, ?string $description = null): Tool { return new Tool( name: $name, @@ -919,7 +1017,7 @@ private function createValidTool(string $name, ?array $outputSchema = null): Too ], 'required' => null, ], - description: "Test tool: {$name}", + description: $description ?? "Test tool: {$name}", annotations: null, icons: null, meta: null, diff --git a/tests/Unit/Server/BuilderTest.php b/tests/Unit/Server/BuilderTest.php index 790af5f5..8bbe197e 100644 --- a/tests/Unit/Server/BuilderTest.php +++ b/tests/Unit/Server/BuilderTest.php @@ -443,6 +443,92 @@ public function testAThirdPartyRegistryIsStillLoadedThroughThePlainLoader(): voi ->addTool(static fn (): string => 'ok', 'alpha') ->build(); } + + #[TestDox('An empty custom registry with lazy loading defers its configured loader past build(), running it on the first registry read')] + public function testEmptyCustomRegistryDefersLoaderPastBuild(): void + { + $loader = new class implements LoaderInterface { + public int $calls = 0; + + public function load(RegistryInterface $registry): void + { + ++$this->calls; + } + }; + + $registry = new Registry(); + + Server::builder() + ->setRegistry($registry) + ->addLoader($loader) + ->build(); + + $this->assertSame(0, $loader->calls); + + $registry->hasTools(); + + $this->assertSame(1, $loader->calls); + } + + #[TestDox('An empty custom registry with lazy loading advertises tools from the configured loader without forcing a load')] + public function testEmptyCustomRegistryAdvertisesToolsFromConfiguredLoaderWithoutLoading(): void + { + $loader = $this->createMock(LoaderInterface::class); + $loader->expects($this->never())->method('load'); + + $registry = new Registry(); + + $server = Server::builder() + ->setServerInfo('test', '1.0.0') + ->setRegistry($registry) + ->addLoader($loader) + ->build(); + + $capabilities = $this->extractServerCapabilities($server); + + $this->assertTrue($capabilities->tools); + } + + #[TestDox('setLazyLoading(false) with an empty custom registry loads it eagerly during build()')] + public function testSetLazyLoadingFalseWithEmptyCustomRegistryLoadsEagerly(): void + { + $loader = $this->createMock(LoaderInterface::class); + $loader->expects($this->once())->method('load'); + + $registry = new Registry(); + + Server::builder() + ->setRegistry($registry) + ->setLazyLoading(false) + ->addLoader($loader) + ->build(); + } + + #[TestDox('A pre-populated custom registry with lazy loading also defers its configured loader past build(), instead of the old isEmpty() gate loading it eagerly')] + public function testPreloadedCustomRegistryDefersLoaderPastBuild(): void + { + $loader = new class implements LoaderInterface { + public int $calls = 0; + + public function load(RegistryInterface $registry): void + { + ++$this->calls; + } + }; + + $registry = new Registry(); + $registry->registerTool( + new Tool(name: 'preloaded_tool', title: null, inputSchema: ['type' => 'object', 'properties' => [], 'required' => null], description: 'A preloaded tool', annotations: null), + static fn (): string => 'result', + ); + + Server::builder() + ->setRegistry($registry) + ->addLoader($loader) + ->build(); + + $this->assertSame(0, $loader->calls); + } } /**