From 89772bf123ba16749e68748544584b43e4aff2da Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Fri, 23 Oct 2026 05:17:32 +0700 Subject: [PATCH] Drop a buffered notification that cannot be generated ## Motivation and Context On the 2026-07-28 path the Streamable HTTP transport buffers the notifications a handler emits during a request and writes them as SSE frames ahead of the response once the handler returns. A notification holding a string that is not valid UTF-8, such as log data a tool passed to `notify_log_message`, raised `JSON::GeneratorError` while the frames were written, and the whole exchange was answered with 500, losing the response the handler had produced. The transport now generates each notification when it is buffered. One that cannot be generated is reported to the exception reporter and dropped as non-delivery (`false`), as the buffer already does past its cap, and the other notifications and the response are written as before. ## How Has This Been Tested? `test/mcp/server/transports/streamable_http_transport_test.rb`. Against the previous library the new test answers 500. ## Breaking Changes None. --- .../transports/streamable_http_transport.rb | 13 ++++-- .../streamable_http_transport_test.rb | 42 +++++++++++++++++++ 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/lib/mcp/server/transports/streamable_http_transport.rb b/lib/mcp/server/transports/streamable_http_transport.rb index 37b71760..a2dafd30 100644 --- a/lib/mcp/server/transports/streamable_http_transport.rb +++ b/lib/mcp/server/transports/streamable_http_transport.rb @@ -521,12 +521,19 @@ def send_notification(method, params = nil, session_id: nil, related_request_id: # a per-request sink for its ephemeral session and flushes it as SSE frames ahead of the final response. # Checked before the stateless guard, since modern requests are served in stateless deployments too. # The sink is bounded; past `MAX_MODERN_REQUEST_NOTIFICATIONS` the notification is dropped as - # non-delivery (`false`), never handed to the legacy paths below. + # non-delivery (`false`), never handed to the legacy paths below. It holds each notification as JSON, + # generated here so that one that cannot be generated is dropped on its own instead of failing + # the whole response when the sink is flushed. sink = @mutex.synchronize { session_id && @modern_request_sinks[session_id] } if sink return false if sink.size >= MAX_MODERN_REQUEST_NOTIFICATIONS - sink << notification + begin + sink << notification.to_json + rescue JSON::GeneratorError => e + MCP.configuration.exception_reporter.call(e, { session_id: session_id, error: "Failed to generate notification" }) + return false + end return true end @@ -949,7 +956,7 @@ def handle_modern(request, header_version, body_string: nil) if notifications.empty? [modern_http_status(response), { "content-type" => "application/json" }, [response_json]] else - frames = notifications.map(&:to_json) + [response_json] + frames = notifications + [response_json] sse_body = frames.map { |frame| "event: message\ndata: #{frame}\n\n" }.join [200, SSE_HEADERS.dup, [sse_body]] end diff --git a/test/mcp/server/transports/streamable_http_transport_test.rb b/test/mcp/server/transports/streamable_http_transport_test.rb index e1c78348..b4d335cd 100644 --- a/test/mcp/server/transports/streamable_http_transport_test.rb +++ b/test/mcp/server/transports/streamable_http_transport_test.rb @@ -6561,6 +6561,48 @@ def string assert_equal overflow, delivery_results.count { |result| result == false } end + test "modern POST drops a handler notification that cannot be generated and keeps the response" do + reported = [] + original_reporter = MCP.configuration.exception_reporter + MCP.configuration.exception_reporter = ->(e, context) { reported << [e.class, context[:error]] } + delivery_results = [] + @server.define_tool(name: "logging_tool") do |server_context:| + delivery_results << server_context.notify_log_message(data: "\xFF".dup.force_encoding(Encoding::UTF_8), level: "info") + delivery_results << server_context.notify_log_message(data: "hi", level: "info") + Tool::Response.new([{ type: "text", text: "done" }]) + end + + body = { + jsonrpc: "2.0", + method: "tools/call", + id: 1, + params: { + name: "logging_tool", + arguments: {}, + _meta: { + "io.modelcontextprotocol/protocolVersion": "2026-07-28", + "io.modelcontextprotocol/clientInfo": { name: "modern_client", version: "2.0" }, + "io.modelcontextprotocol/clientCapabilities": {}, + "io.modelcontextprotocol/logLevel": "info", + }, + }, + }.to_json + + response = @transport.handle_request(modern_rack_request(body)) + + assert_equal 200, response[0] + frames = response[2][0].split("\n\n").reject(&:empty?).map do |chunk| + JSON.parse(chunk.sub(/\Aevent: message\ndata: /, "")) + end + assert_equal 2, frames.size + assert_equal "hi", frames.first.dig("params", "data") + assert_equal [{ "type" => "text", "text" => "done" }], frames.last.dig("result", "content") + assert_equal [false, true], delivery_results + assert_equal [[JSON::GeneratorError, "Failed to generate notification"]], reported + ensure + MCP.configuration.exception_reporter = original_reporter + end + test "modern POST without handler notifications keeps the single JSON exchange" do response = @transport.handle_request(modern_rack_request(modern_body("tools/list", {})))