Repository navigation
fix(mothership): bound best-effort mcp discovery and stop retrying a down server every query - #9273
Conversation
…down server every query Broad chat catalog discovery started best-effort discovery for every usable workspace server at once. The 3s wait only ends the caller's wait, so each attempt kept its sockets open for up to ~2 min, and a workspace with many servers could burst past the connection pool and evict other users' warm connections. Best-effort failures write no shared cooldown, so a down server was reattempted (with its DB and Redis reads) on every query. - Run best-effort discovery through mapSettledWithConcurrency with 8 at once; each server's 3s wait counts from its own start, and servers still queued 3s into the query are skipped unstarted, bounding query latency at ~6s. - Remember, per process and per requester (workspace:server:user, 60s, capped at 1000 entries), a best-effort server that failed or outlived its wait, and skip it until the memo expires or the late discovery succeeds. Nothing is written to shared state. - Named servers are unchanged: listed at once, waited on to completion.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
[Medium impact] The PR appears safe to merge. SummaryBounds broad MCP discovery and remembers failures for each requester.
|
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Heads up: you’re close to your flex budget. Increase your flex budget so reviews don’t pause.
Turn on auto-fix | Re-trigger cubic
…its own attempt settle it - Replace the hand-rolled TTL map with LRUCache (max 1000, ttl 60s), per the repo's caching rule. - A timed-out attempt's late outcome now settles the memo only while the entry is still that attempt's: an older attempt's late success no longer erases a newer attempt's failure, and its late failure no longer recreates a memo after a newer attempt succeeded.
|
@greptile |
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 2 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Heads up: you’re close to your flex budget. Increase your flex budget so reviews don’t pause.
Turn on auto-fix | Re-trigger cubic
Summary
Broad chat catalog discovery (
lib/mothership/integrations/application/catalog.ts) passes every usable workspace server tobuildMcpToolSchemasas best-effort, andlib/mothership/mcp-tools.tsstarted all of them at once withPromise.allSettled.withinDeadlineonly ends the caller's 3s wait. The attempt itself keeps running (connect up to 60s,tools/listup to 60s, plus a retry), so a large workspace opens an unbounded burst of sockets. The connection pool caps retained connections, not pending ones, so a burst of more than 100 LRU-evicts other users' warm connections. Best-effort failures write no shared cooldown, which means a down server was reattempted, with ~4-6 DB reads and 2 Redis reads, on every query. Found in the review of release PR #9270 (Greptile,mcp-tools.ts~111).Changes, all in
lib/mothership/mcp-tools.ts:mapSettledWithConcurrency, 8 at a time (BEST_EFFORT_DISCOVERY_CONCURRENCY). Each server's 3s wait starts when that server starts. A server still queued 3s into the query (BEST_EFFORT_DISCOVERY_START_WINDOW_MS) is skipped without starting, and reported as aDeadlineExceededErrorfailure, which catalog.ts already logs at info. Query latency therefore stays around 6s however many servers there are, and one query leaves at most 8 slow attempts running.workspaceId:serverId:userIdholds a best-effort server that failed or outlived its wait. That requester skips it for 60s, and the stored error is reported again. When a timed-out discovery later succeeds, the entry is cleared, so the next query is served from the cache that discovery warmed. When it later fails, the entry's 60s restarts. The map holds at most 1000 entries and evicts the oldest. It writes no shared state: the best-effort path stays free of side effects on shared status, cooldown, DB and Redis.complete) servers are unchanged: they all start at once and are waited on until they finish.Why the memo lives in mcp-tools rather than McpService: the 3s wait, the start window and the concurrency limit all belong to this module. McpService's
bestEffortonly means "write no shared state". A timeout is something only the caller sees, and a memo in the service could not tell "outlived the wait" apart from "still in flight". Joins of an in-flight attempt are already deduped by the service'sinflightServerDiscovery. The memo also skips the use case's per-query authorization and DB reads, which come before that dedupe.Type of Change
Testing
I wrote the new tests in
lib/mothership/mcp-tools.test.ts(fake timers, mocked discovery) before the fix and ran them against the old code. Five of the six failed there, and all of them pass with the fix:starts at most eight best-effort discoveries at once and skips the rest once the start window closes: fails before the fix (all 20 start)keeps discovering fast servers while slow ones hold their slots: guards against batching (passes before and after)measures each server's wait from its own start, not the query's: fails before the fixlists every named server at once and waits for each however long it takes: fails before the fix (named servers stay unbounded; best-effort servers are capped next to them)does not reattempt a failed best-effort server for the same requester until the memo expires: fails before the fix (also checks that another requester, and the same requester after the TTL, do retry)skips a server whose discovery outlived the wait until that discovery settles, then serves it: fails before the fixEach guard was reverted on its own and a test went red every time: no cap, no start window, wait counted from the query's start, no memo check, a late success that leaves the memo in place, and timeouts left out of the memo.
The touched and dependent suites pass:
mcp-tools,catalog-registry,process-contentsandintegrations/catalog/route(106 tests).apps/simtype-check and biome are clean.Checklist