fix(mcp): bound + parallelize provider discovery so one bad provider can't stall the aggregate#373
Merged
Conversation
…can't stall the aggregate The hub-side federation already skipped providers that errored (EOF/405) and recovered AddTool panics, so a crashing provider couldn't poison the aggregate tool list. The remaining gap was a SLOW/HUNG provider: discovery was a sequential loop with the client's 15s per-request timeout, so one hung provider stalled the whole tools/list — and serialized every healthy provider behind it. Fan out per-provider tools/list concurrently, each under a short providerDiscoveryTimeout (8s, well under the 15s call timeout). A hung provider now costs at most that one deadline, in parallel, then drops out of this round and reappears on the next once it recovers. Tools are still registered sequentially in the original provider order (AddTool isn't guaranteed goroutine-safe) so the aggregate tool list stays deterministic. Discovery goroutines also recover panics individually. Adds a regression test: a healthy provider + a provider whose /mcp hangs; asserts the healthy provider's tool still registers, the hung one drops out, and the aggregate builds in well under the client timeout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 #372. That PR fixed the kuery provider so it stops crashing; this one hardens the hub-side aggregator so any misbehaving provider — present or future — can't take the whole MCP query path down.
What was already safe
registerProviderToolsalready isolates a provider that errors: a failedtools/list(EOF, 405, …) is logged andcontinued, andAddToolpanics are recovered per-tool. So a crashing provider couldn't poison the aggregate tool list.The remaining gap: a slow / hung provider
Discovery was a sequential loop, and each
tools/listwas bounded only by the federation client's 15s timeout. So a single provider whose/mcphangs would:tools/listfor up to 15s, andFrom the caller's perspective, one hung provider takes the whole query down.
Fix
Fan out per-provider discovery concurrently, each under a short
providerDiscoveryTimeout(8s, well under the 15s call timeout):AddToolisn't guaranteed goroutine-safe), so the aggregate tool list stays deterministic.Test
TestRegisterProviderTools_HungProviderDoesNotBlockAggregate: a healthy provider + a provider whose/mcphangs (released only at cleanup). Asserts the healthy provider's tool still registers, the hung one drops out, and the aggregate builds in well under the client timeout.providerDiscoveryTimeoutis avarso the test can lower it to run in ms.🤖 Generated with Claude Code