Bound no-arg list operations to prevent unbounded pagination - #1100
ZYZ666-RGB wants to merge 4 commits into
Conversation
The no-arg listTools(), listResources(), listResourceTemplates() and listPrompts() follow the server-provided nextCursor chain via Mono.expand with no page limit, no duplicate-cursor detection and no total deadline. A server that returns an endless stream of non-empty cursors makes the client issue an unbounded number of requests, accumulate unbounded memory, and block synchronous callers forever. Add a client-side pagination guard: each no-arg list operation now tracks the cursors it has already seen and the number of pages fetched, and aborts with a new McpPaginationException when the configured maxPaginationPages (default 100) or paginationTimeout (disabled by default) is exceeded, or when a cursor is returned more than once. The bounds are configurable on both McpClient.SyncSpec and McpClient.AsyncSpec (maxPaginationPages(int), paginationTimeout(Duration)); existing behavior and APIs are unchanged for servers that terminate pagination normally. Adds McpAsyncClientPaginationTests covering normal multi-page aggregation, duplicate-cursor loops, ever-changing cursor loops (page limit and total timeout) and the synchronous client path. Fixes modelcontextprotocol#1084 Signed-off-by: zhaoyuzhe <[email protected]>
|
The page-count and repeated-cursor guards look useful. I think the new aggregate timeout currently has a semantic hole, though.
Example:
The call waits ~4.05s. Only after page 2 arrives does A focused regression would be useful where the last page exceeds To make the builder contract match “total wall-clock time budget”, I’d apply the timeout to the whole pagination publisher (with a This also avoids the worst case where a configured 200ms pagination budget still waits up to the full per-request timeout on an in-flight page. |
|
Hi maintainers, I added a regression test for a slow final page and updated paginationTimeout to cover the entire paginated operation. The new CI and Conformance runs are marked action_required with no jobs started. Could someone with repository access approve the runs? Thank you! |
This branch has not been deployed
2 participants
Motivation and Context
The no-arg list operations (
listTools(),listResources(),listResourceTemplates(),listPrompts()) follow the server-providednextCursorchain viaMono.expandwithno page limit, no duplicate-cursor detection and no total deadline. A server that
returns an endless stream of non-empty cursors makes the client:
McpSyncClient.listTools()has no duration).requestTimeoutdoes not help: it bounds each individual request, not the number ofrequests. The existing #954 fix only stops servers that signal the end with an empty
cursor. Fixes #1084.
Changes
Add a client-side pagination guard applied to all four no-arg list operations:
maxPaginationPages(builder-configurable, default 100)caps the total number of pages fetched.
0disables the limit.the operation aborts immediately — an unambiguous sign of a loop.
paginationTimeout(builder-configurable, disabled by default)bounds the wall-clock time of the whole list operation.
When a bound is exceeded the operation fails with a new
McpPaginationExceptioncarrying a clear message. Configuration is exposed on bothMcpClient.SyncSpecandMcpClient.AsyncSpec(maxPaginationPages(int),paginationTimeout(Duration)). No existing API or behavior changes for servers thatterminate pagination normally — the guard state is created per subscription, so shared
Monos can be subscribed multiple times safely.How Has This Been Tested?
New
McpAsyncClientPaginationTests(7 tests):nullcursor (tools + resources),McpPaginationException,McpPaginationExceptionaftermaxPaginationPagespages,McpPaginationExceptionafterpaginationTimeout,McpPaginationExceptionat the total deadline,McpSyncClient.listTools()) propagates the same exception.The original pagination tests passed locally before this follow-up. The new final-page
regression and current HEAD could not be run locally because Maven Central was unreachable
during dependency resolution; CI for this commit currently awaits repository approval.
Breaking Changes
None. The new builder options are additive; defaults keep the API source-compatible and
only add protection against misbehaving servers.
Types of changes
Checklist
Additional context
Design note:
Mono.expandruns per subscription, so a shared listMonosubscribedtwice would otherwise share stale guard state; the guard is therefore created inside
Mono.deferper subscription.