fix(mcp): apply the upstream.* connection settings to the rmcp transport - #846
Conversation
The rmcp streamable-http transport was the last outbound path running on a bare default client: rmcp pins its own reqwest line (0.13), so the from_uri/from_config constructors built a client with no connect timeout (kernel SYN retries, worst case ≈127 s), a 90 s pool idle lifetime and TCP keepalive off — the exact gaps the upstream.* config block exists to close, and the reason the no-bare-client guard test was added (AISIX-Cloud#1122/#1126). Only the per-operation deadline (McpServer.timeout_ms, outer tokio wrapper) bounded anything. - New shared_http_client() in aisix-mcp: one client for every MCP upstream, built from upstream_http::config() (connect_timeout, TCP keepalive triple, pool idle, pool max-idle-per-host) against rmcp's reqwest line; all four transport constructions now go through StreamableHttpClientTransport::with_client. - The no_production_code_builds_a_bare_reqwest_client guard now names rmcp_reqwest::Client:: as the sanctioned second construction site instead of (coincidentally, via substring) flagging it. The regression test dials a black-holed upstream (TEST-NET-3) with a 12 s operation timeout: on the default client the dial hung the full 12 s; with the shared client the connect timeout (default 5 s) cuts it.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
…mption Audit follow-ups — the code is unchanged, the story around it was wrong in two places: - rmcp's default_http_client() is not a bare default client: it deliberately DISABLES idle pooling (pool_max_idle_per_host(0)) to dodge ~40 ms delayed-ACK stalls, and reqwest 0.13 has TCP keepalive (15 s/15 s/3) on by default. The real delta of shared_http_client() is: a connect timeout exists at all (the actual fix), pooling turns ON under upstream.* management (reuse beats per-call TCP+TLS to a remote MCP server; upstream.pool_max_idle_per_host: 0 restores rmcp's behaviour), and the keepalive triple moves to the deployment's 60/30/5. Comments now say exactly that. - The Cargo.toml note claiming the rmcp-reqwest dep is 'never a client' is now false; updated. - The guard-test exemption for rmcp_reqwest::Client:: is scoped to aisix-mcp/src/bridge.rs so a bare rmcp client anywhere else still gets flagged.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Problem
The rmcp streamable-http transport's HTTP client was outside the
upstream.*connection-settings contract: rmcp pins its own reqwest line (0.13, vs the workspace's 0.12), so thefrom_uri/from_configconstructors built rmcp'sdefault_http_client()— which sets no connect timeout. A black-holed MCP upstream (SYN swallowed) was bounded only by the coarse per-operationMcpServer.timeout_ms(default 30 s), instead of failing atupstream.connect_timeout_mslike every other outbound path. This is the same class the no-bare-client guard exists for (#1122/#1126); it missed this site because the client is built inside rmcp.Change
shared_http_client()inaisix-mcp: one client shared by every MCP upstream, built against rmcp's reqwest line with theupstream_http::config()values applied —connect_timeout_ms, the TCP keepalive triple,pool_idle_timeout_secs,pool_max_idle_per_host. All four transport constructions (none / bearer / api-key / oauth2) now go throughStreamableHttpClientTransport::with_client. Auth stays per-request (transport-injected), so sharing one client across differently-authenticated upstreams is safe — same posture as the provider bridges.no_production_code_builds_a_bare_reqwest_clientguard namesrmcp_reqwest::Client::inaisix-mcp/src/bridge.rsas the sanctioned second construction site (scoped to that file; a bare rmcp client anywhere else still gets flagged).Behaviour changes (the honest delta vs rmcp's default client)
pool_max_idle_per_host(0)) to dodge ~40 ms delayed-ACK stalls on reused connections. Here reuse wins: every other outbound path pools underupstream.*management (30 s idle expiry), per-call TCP+TLS handshakes to a remote MCP server cost far more than the stall rmcp avoids, and the reference proxy's MCP path (python-sdk over httpx) pools by default too.upstream.pool_max_idle_per_host: 0restores rmcp's exact behaviour.Operation-level deadlines are unchanged:
McpServer.timeout_msstill bounds every MCP operation end-to-end.Tests
connect_timeout_bounds_an_unreachable_upstreamdials a black-holed upstream (TEST-NET-3) with a 12 s operation timeout and asserts the dial is cut well below it (the 5 s connect timeout). Verified to fail without the fix: the dial hangs the full 12 s and surfaces the outer-timeout error instead.