fix: aggregate every backend in tools/list, once each - #107
Merged
Conversation
Two separate faults in serve mode. register_tools appended to a Vec, so every re-discovery added another copy of a backend's whole tool list, and snapshot_cache_entries then persisted the inflated list, which is why the duplication survived restarts and grew on each one. Separately, discover_for_list only ran when the registry was empty, so a single backend restored from the tool cache was enough to skip discovering all the others. Together they answered tools/list with one backend repeated 28 times and four missing. Registration now replaces instead of appending, and skips names already registered for that server. That second part collapses an already poisoned list on the next load, so nobody has to wipe the cache. A list request discovers whatever is still pending regardless of what the registry already holds, and the existing failure backoff keeps an unreachable backend from being retried on every call. The background refresh resets only what came off the cache instead of clearing the whole discovered set and reconnecting healthy backends. Fixes #106 Signed-off-by: Avelino <31996+avelino@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes mcp serve registry corruption and incomplete discovery in serve mode by making backend registration idempotent (replace instead of append), healing already-poisoned cached tool lists, and ensuring */list triggers discovery for any still-pending backends (respecting existing failure backoff).
Changes:
- Make tool/resource/prompt registration replace existing per-backend entries (and de-dupe same-name entries during a single registration) to stop persistent duplication growth.
- Ensure
tools/list,resources/list, andprompts/listalways attempt discovery of any undiscovered configured backends (rather than only when the registry is empty). - Refine background refresh to re-discover only cache-loaded (disconnected) backends instead of clearing the full discovered set; add targeted tests for both failure modes from #106.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/serve/proxy.rs |
Implements replace-not-append registration, adds de-dupe guards, introduces reset_cache_loaded_for_refresh, and adds regression tests for idempotency + cache healing. |
src/serve/dispatch.rs |
Updates */list flows to discover pending backends even when registries are non-empty; adds test covering partial-cache suppression regression. |
src/serve/http.rs |
Uses reset_cache_loaded_for_refresh() for post-cache background refresh without resetting healthy backends. |
src/serve/stdio.rs |
Uses reset_cache_loaded_for_refresh() in the periodic refresh path instead of clearing all discovered backends. |
Signed-off-by: Avelino <31996+avelino@users.noreply.github.com>
Signed-off-by: Avelino <31996+avelino@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/serve/http.rs:454
- The comment describing this refresh block says it "clear[s] the discovered set", but the code now calls
reset_cache_loaded_for_refresh()(which only resets cache-loaded/disconnected backends). Please update the comment so it matches the new behavior to avoid misleading future changes.
let mut proxy = refresh_proxy.lock().await;
proxy.reset_cache_loaded_for_refresh();
}
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.
Two separate faults in serve mode. register_tools appended to a Vec, so every re-discovery added another copy of a backend's whole tool list, and snapshot_cache_entries then persisted the inflated list, which is why the duplication survived restarts and grew on each one. Separately, discover_for_list only ran when the registry was empty, so a single backend restored from the tool cache was enough to skip discovering all the others. Together they answered tools/list with one backend repeated 28 times and four missing.
Registration now replaces instead of appending, and skips names already registered for that server. That second part collapses an already poisoned list on the next load, so nobody has to wipe the cache. A list request discovers whatever is still pending regardless of what the registry already holds, and the existing failure backoff keeps an unreachable backend from being retried on every call. The background refresh resets only what came off the cache instead of clearing the whole discovered set and reconnecting healthy backends.
Fixes #106