Repository navigation
Conversation
|
|
||
| # Applies the same response-reading limit as `send_request` to bodies the client does not use. | ||
| def bounded_on_data | ||
| SSEStream.new(abortable: false, max_message_bytes: @max_message_bytes).on_data |
There was a problem hiding this comment.
SSEStream#on_data parses a text/event-stream body, so these discard paths now lazily require event_stream_parser, which the docs allow leaving out against a server that never sends SSE; the resulting LoadError passes both rescues here. The parser also bounds only the unterminated event, so a stream of small events keeps send_notification and close from returning.
A reader that only counts bytes and raises MessageTooLargeError past @max_message_bytes, whatever the content type, avoids both.
There was a problem hiding this comment.
Replaced SSEStream on the discard paths with a fresh byte counter in bounded_on_data. It adds each chunk's bytesize and raises past max_message_bytes:, regardless of content type. It does not parse or retain the body, or load event_stream_parser. Added notification, pong, and DELETE tests with many small SSE-framed events and the parser unavailable.
| assert_equal(:internal_error, error.error_type) | ||
| end | ||
|
|
||
| def test_close_rejects_response_body_exceeding_max_message_bytes_and_still_clears_state |
There was a problem hiding this comment.
A DELETE answered 404 or 405 with an over-limit body now raises before raise_error classifies the status, so a case close used to tolerate becomes a RequestHandlerError. If that precedence is intended, a test should pin it; the PR body's "dependency requirements do not change" also needs the parser note above.
There was a problem hiding this comment.
The order is intentional and pinned by test_close_applies_response_size_limit_before_status_errors: over-limit 404, 405, and 500 replies raise RequestHandlerError before status handling. test_close_tolerates_404_and_405_response_bodies_within_max_message_bytes pins the unchanged normal-size tolerance. Local session state is cleared in both cases. The PR body and docs now state that order and explain that these discard reads do not need event_stream_parser.
|
Accurate problem definition, and failing the exchange on an over-limit pong acknowledgement is the right call given the 202-no-body rule. One change before merge: read the discarded bodies with a byte-counting reader instead of |
Apply the configured response limit to notification acknowledgements, replies to server-to-client requests, and session DELETE responses. Count incoming bytes without parsing or retaining these bodies. These discard reads do not load event_stream_parser. Report an over-limit body as RequestHandlerError, as send_request does. Check the size before DELETE status handling, and clear local session state even when close raises.
4f279e2 to
bee8724
Compare
|
Done. The three discard paths now use a byte counter rather than |
Motivation and Context
This completes the client-side response limit introduced in 459 and extended to OAuth requests in 520.
MCP::Client::HTTPalready appliesmax_message_bytes:to message responses. Notification posts, replies to server-to-client requests (including pong), and session DELETE requests now apply it to the bodies they discard.A fresh callback adds each incoming chunk's
bytesizeand raisesMessageTooLargeErrorpast the configured limit. The callers translate this toRequestHandlerError. Unlike an SSE message reader, it counts the whole discarded body, whatever its content type. It does not parse or retain the body and does not loadevent_stream_parser. Message-response and listening streams still use that parser for SSE. The option, default, and dependency floors do not change.The size check deliberately precedes DELETE status classification. The review asks, “If that precedence is intended, a test should pin it”. Tests now pin over-limit 404, 405, and 500 replies raising
RequestHandlerError, with local session state cleared. Normal-size 404/405 replies are still tolerated. This order also works with streaming callbacks that provide no response status.The 2025-11-25 transport rules require accepted notification and response posts to receive 202 with no body. Its session rules recommend DELETE and permit 405. The 2026-07-28 transport rules retain the notification acknowledgement rule but prohibit client-sent JSON-RPC responses; sessions and DELETE remain legacy behavior. This change does not add those exchanges to modern connections.
The TypeScript HTTP client has no equivalent response-body cap. Its
terminateSessiondrains the body before checking status, accepts 405, and raises for other non-success statuses. Python also reads DELETE bodies without a byte cap; it accepts 405 and logs other unexpected statuses or request errors. Python's separate SSE-event limit does not apply to these bodies. Ruby keeps its existing normal-size 404/405 tolerance. The numeric limit and its error precedence are SDK-side choices, not protocol requirements.How Has This Been Tested?
test/mcp/client/http_test.rb: 154 tests, 492 assertions, no failures, errors, or skips. Cases cover notification and pong acknowledgements, DELETE cleanup, many small SSE-framed events, an unavailable parser, chunk-by-chunk byte counting, multibyte strings, exact-limit acceptance, independent response counters, and DELETE status precedence.The original three non-SSE limit tests fail on the unchanged base because no error is raised. Against the previous PR head, the new parser-unavailable cases fail with
LoadError, and the completed-small-events case fails because no size error is raised. They pass with the byte counter.RuboCop with
--force-exclusion: both changed Ruby files pass.git diff --checkpasses. The full local suite was not run because the laptop was on battery power.Breaking Changes
A 404 or 405 DELETE reply whose body exceeds
max_message_bytes:raisesRequestHandlerErrorinstead of being tolerated byclose. This is intentional: the configured byte limit wins before status handling. Replies with those statuses within the limit remain tolerated, and local session state is cleared either way. The limit option and its default do not change.Types of changes
Checklist