[Server][Capability] Defer loading into a custom registry - #494
Closed
soyuka wants to merge 1 commit into
Closed
Conversation
soyuka
requested review from
CodeWithKyrian,
Nyholm and
chr-hertel
as code owners
September 2, 2026 12:40
`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()`.
soyuka
force-pushed
the
feat/lazy-custom-registry
branch
from
September 2, 2026 13:55
0e0f251 to
2bc2bdb
Compare
Contributor
Author
|
Closing this — I'd rather not push |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #389, which made loading lazy for the registry the builder constructs. This does the same for one the caller supplies.
The gap
setLazyLoading()defaults totrue, but it never applied to a registry passed throughsetRegistry().resolve()ran the chain loader itself and set$eagerlyLoaded = true, because the builder could not hand a loader to an instance it did not construct.Two consequences under a persistent runtime (FrankenPHP worker mode, Laravel Octane):
tools/listkeeps returning[].detectCapabilities()takes the$eagerlyLoaded === truebranch and readshasTools()off that cold registry, so the server can advertisetools: false. A client that respects capabilities then never callstools/listat all, which the first point alone would not cause.Not hypothetical:
symfony/mcp-bundleregisters its registry as aRegistryservice and callssetRegistry()on it, and API Platform does the same on its Laravel side. Every consumer that supplies its own registry service is affected, and API Platform carried a customtools/listhandler purely to work around it.The change
Registry::deferLoadingFrom()adopts a loader after construction, so the builder can defer instead of loading eagerly and report$eagerlyLoaded = false.It chains behind a loader the constructor took while that one is still owed its run, and replaces it once it has already run — chaining there would run it twice and discovery would rescan. Resetting
loadedis what lets the adopted loader run on the next read; without it a registry that was read beforebuild()(a warmup, a debug command, a health check) would keep the deferred loader forever unrun, which is the same cold-source bug this PR exists to fix.Deferral is deliberately 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 bug back. InsteadRegistry::isEmpty()— which reads the element arrays directly and so does not trigger the loader while answering — tellsdetectCapabilities()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. Per the doctrine already stated in
detectCapabilities()'s docblock, over-advertising is harmless under MCP semantics, and it is already how custom loaders and discovery are treated.Two guards remain:
$this->lazyLoading—setLazyLoading(false)must still mean eager for a supplied registry.$registry instanceof Registry— a foreignRegistryInterfacecannot be deferred into and keeps the eager path, astestAThirdPartyRegistryIsStillLoadedThroughThePlainLoaderpins.Notes
RegistryInterfaceis untouched. Both new methods live onRegistry, which is what the builder already type-checks forloadFrom(); adding them to the interface would break third-party implementations.load(), which delegates toloadFrom().$loaderlosesreadonly(still private) so it can be replaced.CHANGELOG.mdentry, matching [Server][Capability] Do not announce an externally loaded registry as changed #490.Tests
RegistryTest:deferLoadingFrom()does not run until the first read, runs exactly once across many reads, chains behind a constructor loader, and — the regression guard for theloadedreset — runs the adopted loader even when the constructor loader had already run.isEmpty()is true for a fresh registry without triggering a configured loader, and false afterregisterTool().BuilderTest: an empty custom registry defers its loader pastbuild()and runs it on the first read; a pre-populated one defers too; the deferred setup advertisestools: truewhile the loader is asserted never to run;setLazyLoading(false)still loads eagerly.testBuildAdvertisesToolsForPreloadedCustomRegistrypasses unchanged.