[Server][Capability] Do not announce an externally loaded registry as changed (#490)
* [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. * Apply batched suggestions from code review Co-authored-by: Christopher Hertel <mail@christopher-hertel.de>
This commit is contained in:
parent
46628fbe6a
commit
c5dbfb64e5
2 changed files with 134 additions and 4 deletions
|
|
@ -753,6 +753,74 @@ class RegistryTest extends TestCase
|
|||
$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);
|
||||
|
|
|
|||
|
|
@ -15,6 +15,7 @@ use Mcp\Capability\Registry;
|
|||
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\Handler\Request\InitializeHandler;
|
|||
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 @@ final class BuilderTest extends TestCase
|
|||
|
||||
$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();
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Reference in a new issue