fix(transport): bound buffered Streamable HTTP responses - #1246
Conversation
github-actions
Bot
added
T-test
| .text() | ||
| .await | ||
| .unwrap_or_else(|_| "<failed to read response body>".to_owned()); | ||
| let body = match read_bounded_body( |
There was a problem hiding this comment.
Have you considered using read_truncated_body instead? Without it, a legacy server with a 4xx page larger than 64 KiB never reaches legacy_discover_response. That means a client that used to fall back to the initialize handshake now fails to connect entirely. Since we stop reading the stream at the limit either way, rejecting it does not reduce the memory limit.
There was a problem hiding this comment.
You're right: rejecting a diagnostic page broke legacy discovery without improving the buffering bound. Fixed in a94edbc.
Both built-in backends now use read_truncated_body for non-JSON error pages. It retains at most max_error_response_size bytes and stops immediately at that limit, without polling another chunk or waiting for EOF. The existing discovery fallback eligibility checks are unchanged.
The regression tests now complete server/discover -> initialize -> notifications/initialized within two seconds for a >64 KiB page, both fixed-length and unfinished chunked. Unix socket coverage also verifies fallback from an unfinished stream whose prefix is exactly the limit. Auth/session precedence and ineligible 401/403/500 responses remain covered.
JSON error responses are handled separately: the complete bounded body is parsed before any diagnostic truncation, using the same JSON limit as HTTP 200. An oversized JSON protocol message still fails rather than parsing a prefix or triggering fallback. I've corrected the PR description accordingly.
| let body = match read_bounded_body( | ||
| response.bytes_stream(), | ||
| content_length, | ||
| limits.max_error_response_size, |
There was a problem hiding this comment.
The same payload gets 16 MiB with a 200 response and 64 KiB with a 400. (see line 316) Is this intentional?
There was a problem hiding this comment.
That status-based split was a mistake for JSON-RPC errors. Fixed in a94edbc: application/json uses max_json_response_size on both success and failure statuses, with a 64 MiB default. max_error_response_size now only limits diagnostic text.
The full JSON body is bounded and parsed before truncating diagnostics; prefixes are never parsed as complete messages. Tests send the identical JSON-RPC error on HTTP 200 and 400 with fixed/chunked bodies at N-1, N, and N+1, including an independent diagnostic limit of zero.
This applies to both built-in backends. Unix socket HTTP JSON-RPC errors now also surface as Json before synthetic discovery fallback, matching reqwest; that backend behavior change is documented and tested. All 686 selected tests and focused strict Clippy pass locally.
| fn default() -> Self { | ||
| Self { | ||
| max_sse_event_size: DEFAULT_MAX_SSE_EVENT_SIZE, | ||
| max_json_response_size: 16 * 1024 * 1024, |
There was a problem hiding this comment.
I'd raise the default for base64-encoded tool results.
| max_json_response_size: 16 * 1024 * 1024, | |
| max_json_response_size: 64 * 1024 * 1024, |
There was a problem hiding this comment.
Applied in a94edbc: the JSON default is now 64 MiB, including JSON-RPC errors carried by non-2xx responses. The independent SSE-event default remains 16 MiB, and the diagnostic prefix remains 64 KiB.
Added a regression using the existing POST API that accepts a tool result containing 17 MiB of base64 data, plus assertions for all three defaults. Callers can still configure the limits independently; this bounds accumulated body bytes, not total process memory.
Apply the complete JSON limit on every HTTP status, raise its default to 64 MiB, and truncate diagnostic prefixes without waiting for EOF. Align Unix socket JSON-RPC error handling with reqwest and add regression coverage for review feedback.
2 participants
Summary
Refs #1030. Addresses the Streamable HTTP client JSON/error response scope described in #1180 and redirected to #1030 by the maintainer. The stdio work in #1049 remains separate; this PR should not close the combined tracker on its own.
The built-in clients already bound individual SSE events, but buffer JSON and HTTP error bodies before parsing them. This change adds configurable pre-decode bounds without replacing the transports or adding dependencies.
Changes
StreamableHttpResponseLimitsand transport config setters. Defaults: 64 MiB per complete JSON response, regardless of HTTP status, 64 KiB per HTTP diagnostic prefix, and the unchanged independent 16 MiB SSE-event limit.ResponseBodyTooLarge { limit }if Content-Length exceeds its limit or a chunk would exceed it. Count bytes even when the length is missing or understated; do not drain oversized JSON to EOF.read_truncated_bodyretains at most the diagnostic limit and stops immediately when that prefix is full, without polling for another chunk or EOF. Large legacy 4xx pages can still reach the existingserver/discover->initializefallback. The fallback eligibility rules are unchanged.application/jsonerror responses use the same complete-body JSON limit as successful responses. Parse the full bounded body before considering discovery fallback or truncating diagnostic text. Never parse a diagnostic prefix as a complete JSON-RPC message. Malformed/non-error JSON gets only a diagnostic prefix; oversized JSON still fails without fallback/retry.StreamableHttpPostResponse::Json, before synthetic discovery fallback. This also fixes the previous backend discrepancy for these errors.AuthClientpreserves them across token refresh.Compatibility and limits
Custom HTTP backends remain source-compatible, but must override
post_message_with_response_limitsto enforce JSON/diagnostic limits; the default cannot bound a body already decoded by custom code. The API docs explain this distinction and the JSON-versus-diagnostic behavior.Previously accepted JSON bodies above the new default now fail; callers can raise each limit independently. A zero JSON limit accepts only empty bodies. A zero diagnostic limit omits diagnostic text rather than rejecting a non-JSON error response. The defaults are proposed for review.
Existing 202/204, authentication challenge, session-expired, within-limit malformed-success-JSON, and backend-specific success-body-I/O-error behavior is preserved. An error-body I/O failure before the diagnostic limit still uses the existing read-failure placeholder (subject to the diagnostic prefix limit). This does not subsume #1208's separate malformed-JSON proposal.
Limits count bytes yielded by the HTTP backend (decompressed bytes when decompression is enabled). They bound accumulated body length, not backend/decompressor allocations, vector capacity, UTF-8 conversion, or JSON parser allocations, and are not a total RSS bound.
Validation
Linux, Rust 1.96.1 (the repository's pinned 1.96 toolchain), with the same fixed validation lockfile kept out of this PR:
296d1faea6ec1fd3b538eb629e93bd9910725778compile and produce 31 passes / 27 assertion failures; all 58 pass with the revision. This includes the two real loopback legacy startup regressions.server/discover->initialize->notifications/initializedwithin two seconds after a >64 KiB diagnostic error page, with either a fixed body or an unfinished chunked stream. Oversized JSON on HTTP 200 and 400 still fails within two seconds, with exactly one POST and no fallback/retry.cargo +nightly-2026-06-20 fmt --all --checkand the repository hook'scargo +nightly fmt --all --checkpass, as doesgit diff --check.The initial submission's broader server-enabled
--all-targets -D warningsClippy check reported the same 11 diagnostics in unchangedstreamable_http_server/tower.rson both baseline and patch; that broad check was not rerun for this revision. No lint suppression or unrelated server cleanup is included. Full workspace/all-features tests, Python/JS interoperability, and an actual MSRV-1.88 compiler run were not performed locally; CI remains a separate check.Prepared with AI assistance; reviewed and tested locally before submission.