Paginate MCP tools/list and prompts/list - #1336
Open
sugmanue wants to merge 2 commits into
Open
Conversation
listTools()/listPrompts() only read the first page, so any server that paged (commonly ~30 items) was silently truncated. Follow nextCursor to the end, threading each page's cursor back into the next request. Guards for misbehaving servers: a blank or absent cursor ends paging, a repeated or cycling cursor aborts, and a 1000-page cap backstops a runaway server. Returns an immutable list; single-page servers still make one call. Also add a builder-configurable per-request timeout to StdioProxy (default 5m, symmetric with HttpMcpProxy) so a silent server can't block forever.
The registries were mutated by compound (multi-step) operations from several threads with no mutual exclusion, and the tools/list_changed refresh ran inline on the transport reader thread, which deadlocks: the refresh calls listTools() whose response is read by that same thread. Hold each registry as a volatile immutable snapshot (copy-on-write). Readers (tools/list, prompts/list, tools/call dispatch, shutdown) read the current snapshot with no locking; every mutation rebuilds the affected snapshot under a single lock and publishes it atomically, with network I/O kept outside the lock. Run the list_changed refresh on a dedicated thread so it can't deadlock the reader, using add-then-prune so a concurrent tools/list never sees a gap. Snapshots are insertion-ordered (LinkedHashMap) so tools/list and prompts/list return a stable order across refreshes and dynamic additions. getProxies() now returns an immutable snapshot. Integration-test helpers that assumed the first listed tool was McpEcho now look it up by name, since order is deterministic rather than hash-based.
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
1 participant
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.
What behavior changes?
McpServerProxy.listTools()/listPrompts()now follow MCP cursor pagination. Before, they issued a single JSON-RPC call and ignorednextCursor, so a server that paged its results (commonly ~30 items per page) was silently truncated to its first page everywhereMcpServerProxyis used. After, they return the union of all pages. Single-page servers are unchanged and still make exactly one round-trip.Two related fixes ship alongside it:
tools/list_changedrefresh inMcpServiceno longer runs inline on the transport reader thread. Previously that path could deadlock: the refresh callslistTools()whose response is read by the same reader thread. It now runs on a dedicated single-thread executor.McpService's tool/prompt/proxy registries are now thread-safe. They were mutated by compound (multi-step) operations from several threads with no mutual exclusion.Two intentional, minor observable differences reviewers should be aware of:
tools/listandprompts/listnow return a stable, deterministic order (insertion order) instead of an unspecified hash-map order.McpService.getProxies()now returns an immutable snapshot of the proxies at the time of the call. It previously returned the live internal map; mutating the returned map now throws, and the snapshot does not reflect proxies added afterward.Why is this change needed?
Bug. MCP
tools/listandprompts/listare cursor-paginated, but the client only ever read the first page, so any server or gateway federating more than one page of tools had its tool set truncated with no error and no log, which made it hard to notice.This also re-lands the pagination fix from #1308 (reverted in #1312) together with a proper fix for the thread-safety concern that caused that revert: moving the
tools/list_changedrefresh off the reader thread requires the registries it mutates to be safe for concurrent access, which they were not.How was this validated?
./gradlew :mcp:mcp-server:build(unit + integration tests, Spotless, SpotBugs)green: 200 tests, 0 failures.
Tests added or updated:
McpServerProxyTest(new, 12 cases): multi-page union with exactnextCursor->cursorthreading, single-page regression, blank-cursor termination, repeated-cursor abort, cycling (A -> B -> A) abort, page-cap boundary, error on the first page, error on a later page, null-resulthandling,listPromptsguard path, and immutable return.HttpMcpProxyTest(+1): a real localhostHttpServerpagestools/listwith anextCursorand assertslistTools()follows it, reading the cursor off the actually-deserialized response.StdioProxyTest(new, 1): a silent server times out via the per-request timeout instead of hanging.McpServiceTest(new, 3): thetools/list_changedrefresh runs off the notifying thread (deadlock regression), add-then-prune adds new tools while pruning stale ones, and a concurrency stress test (4 adders + 4 refreshers + 4 readers x 200 iterations) asserts no lost updates and no exceptions.McpServerTest: the cache-invalidation test was updated for the now-async refresh (it polls for the refresh instead of assuming inline completion).McpServerIntegrationTest: helpers that assumed the first listed tool wasMcpEchonow look it up by name, since tool order is deterministic rather than hash-based.What should reviewers focus on?
McpServerProxy#listPaginated(...)(mcp/mcp-server/.../McpServerProxy.java): the pagination loop and its three termination guards (blank/absent cursor ends, repeated or cycling cursor aborts,maxListPages()cap of 1000), the null-resultguard, and the immutable return.mcp/mcp-schemas/model/main.smithy: the additive optionalnextCursoronListToolsResult/ListPromptsResult.McpService(mcp/mcp-server/.../McpService.java): the copy-on-write registries (tools/prompts/proxies/servicesheld as volatile immutable snapshots mutated under a singleregistryLock, with network I/O kept outside the lock), the off-reader-threadtools/list_changedrefresh with add-then-prune, andgetProxies()returning a snapshot.StdioProxy#rpc(mcp/mcp-server/.../StdioProxy.java): the builder-configurable per-request timeout (default 5m, symmetric withHttpMcpProxy) and pending-request cleanup.Additional Links
nextCursor/cursor): https://modelcontextprotocol.io/specificationBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.