Merge pull request #629 from koic/answer_an_http_request_whose_reply_cannot_be_encoded

Answer an HTTP request whose reply cannot be generated with Internal error

Author: koic

lib/json_rpc_handler.rb

@@ -53,7 +53,9 @@ def handle(request, id_validation_pattern: DEFAULT_ALLOWED_ID_CHARACTERS, &metho
     end
   end
 
-  def handle_json(request_json, id_validation_pattern: DEFAULT_ALLOWED_ID_CHARACTERS, &method_finder)
+  # `on_generate_error`, when given, is called with the `JSON::GeneratorError` of a response that cannot be generated,
+  # before that response is replaced with Internal error.
+  def handle_json(request_json, id_validation_pattern: DEFAULT_ALLOWED_ID_CHARACTERS, on_generate_error: nil, &method_finder)
     begin
       begin
         request = JSON.parse(request_json, symbolize_names: true)
@@ -77,7 +79,35 @@ def handle_json(request_json, id_validation_pattern: DEFAULT_ALLOWED_ID_CHARACTE
       })
     end
 
-    response&.to_json
+    begin
+      response&.to_json
+    rescue JSON::GeneratorError => e
+      on_generate_error&.call(e)
+      encodable_response(response, id_validation_pattern: id_validation_pattern).to_json
+    end
+  end
+
+  # `response` with each part JSON cannot generate replaced with Internal error. A batch is replaced item by item,
+  # so the other items keep their results.
+  def encodable_response(response, id_validation_pattern: nil)
+    return response.map { |item| encodable_response(item, id_validation_pattern: id_validation_pattern) } if response.is_a?(Array)
+
+    response.to_json
+    response
+  rescue JSON::GeneratorError
+    internal_error_for(response, id_validation_pattern: id_validation_pattern)
+  end
+
+  # The error answering a request whose response cannot be generated: a result or an error holding a string that
+  # is not valid UTF-8, typically one the request carried and the SDK or a handler repeated, such as a tool name
+  # in its Tool not found error. It carries the request id when the response has one, which was validated before
+  # the method ran.
+  def internal_error_for(response, id_validation_pattern: nil)
+    id = response[:id] if response.is_a?(Hash)
+    error_response(id: id.nil? ? :unknown_id : id, id_validation_pattern: id_validation_pattern, error: {
+      code: ErrorCode::INTERNAL_ERROR,
+      message: "Internal error",
+    })
   end
 
   def process_request(request, id_validation_pattern:, &method_finder)

lib/mcp/server.rb

@@ -303,7 +303,8 @@ def handle(request, session: nil, auth_info: nil)
     #   for the current request, as in `handle`.
     # @return [String, nil] The JSON-RPC response as JSON, or `nil` for notifications.
     def handle_json(request, session: nil, auth_info: nil)
-      JsonRpcHandler.handle_json(request, id_validation_pattern: nil) do |method, request_id|
+      on_generate_error = ->(error) { report_exception(error, { error: "Failed to generate response" }) }
+      JsonRpcHandler.handle_json(request, id_validation_pattern: nil, on_generate_error: on_generate_error) do |method, request_id|
         handle_request(request, method, session: session, related_request_id: request_id, auth_info: auth_info)
       end
     end

lib/mcp/server/transports/streamable_http_transport.rb

@@ -931,16 +931,26 @@ def handle_modern(request, header_version, body_string: nil)
           # `nil` covers notifications and cancellation-suppressed responses; ack with 202 like the legacy notification path.
           return handle_accepted if response.nil?
 
+          # A response that cannot be generated answers its request with Internal error, as `JsonRpcHandler.handle_json`
+          # does on the legacy paths, instead of failing the whole exchange with a 500.
+          response_json = begin
+            response.to_json
+          rescue JSON::GeneratorError => e
+            MCP.configuration.exception_reporter.call(e, { error: "Failed to generate response" })
+            response = JsonRpcHandler.encodable_response(response)
+            response.to_json
+          end
+
           # Notifications a handler emitted during the request ride the request's own response stream as SSE frames ahead of
           # the final response (SEP-2575); a request that emitted none keeps the single JSON exchange and its HTTP status ladder.
           # Delivery is buffered: frames flush after the handler returns, preserving order but not real-time interleaving
           # (the TypeScript and Python SDKs stream live), and the sink grows with the handler's notification count.
           # Live streaming can follow without changing the wire shape.
           if notifications.empty?
-            [modern_http_status(response), { "content-type" => "application/json" }, [response.to_json]]
+            [modern_http_status(response), { "content-type" => "application/json" }, [response_json]]
           else
-            frames = notifications + [response]
-            sse_body = frames.map { |frame| "event: message\ndata: #{frame.to_json}\n\n" }.join
+            frames = notifications.map(&:to_json) + [response_json]
+            sse_body = frames.map { |frame| "event: message\ndata: #{frame}\n\n" }.join
             [200, SSE_HEADERS.dup, [sse_body]]
           end
         rescue StandardError => e

test/json_rpc_handler_test.rb

@@ -488,6 +488,38 @@
       }
     end
 
+    it "returns an error with the code set to -32603 and the request id when the result cannot be generated" do
+      register("echo") { |_params| { text: "\xFF".dup.force_encoding(Encoding::UTF_8) } }
+
+      handle_json({ jsonrpc: "2.0", id: 1, method: "echo" }.to_json)
+
+      assert_equal(1, @response[:id])
+      assert_rpc_error expected_error: {
+        code: -32603,
+        message: "Internal error",
+      }
+    end
+
+    it "replaces only the batch item whose result cannot be generated and reports the error" do
+      register("echo") { |_params| { text: "\xFF".dup.force_encoding(Encoding::UTF_8) } }
+      register("ping") { |_params| {} }
+      errors = []
+
+      response_json = JsonRpcHandler.handle_json(
+        [{ jsonrpc: "2.0", id: 1, method: "echo" }, { jsonrpc: "2.0", id: 2, method: "ping" }].to_json,
+        on_generate_error: ->(error) { errors << error.class },
+      ) { |method_name, _request_id| @registry[method_name] }
+
+      assert_equal(
+        [
+          { jsonrpc: "2.0", id: 1, error: { code: -32603, message: "Internal error" } },
+          { jsonrpc: "2.0", id: 2, result: {} },
+        ],
+        JSON.parse(response_json, symbolize_names: true),
+      )
+      assert_equal([JSON::GeneratorError], errors)
+    end
+
     it "returns nil when the method does not exist and the id is nil" do
       handle jsonrpc: "2.0", method: "add", params: { a: 1, b: 2 }
 

test/mcp/server/transports/stdio_transport_test.rb

@@ -735,7 +735,7 @@ class StdioTransportTest < ActiveSupport::TestCase
           assert_equal([false, true, true, false], responses.map { |response| response.key?(:error) })
         end
 
-        test "open answers a batch whose reply cannot be encoded with Internal error without an id" do
+        test "open answers a batch whose reply cannot be encoded with Internal error and the item's id" do
           server = Server.new(name: "test_server")
           server.define_tool(name: "invalid_text") do
             Tool::Response.new([{ type: "text", text: "\xFF".dup.force_encoding(Encoding::UTF_8) }])
@@ -750,7 +750,7 @@ class StdioTransportTest < ActiveSupport::TestCase
           ])
 
           assert_equal(
-            { jsonrpc: "2.0", error: { code: JsonRpcHandler::ErrorCode::INTERNAL_ERROR, message: "Internal error" } },
+            { jsonrpc: "2.0", id: 2, error: { code: JsonRpcHandler::ErrorCode::INTERNAL_ERROR, message: "Internal error" } },
             responses[1],
           )
           assert_equal({ jsonrpc: "2.0", id: 3, result: {} }, responses[2])

test/mcp/server/transports/streamable_http_transport_test.rb

@@ -2792,6 +2792,68 @@ def string
           assert_equal "456", body["id"]
         end
 
+        test "a reply that cannot be generated is answered with Internal error on the legacy SSE path" do
+          reported = []
+          configuration = MCP::Configuration.new
+          configuration.exception_reporter = ->(e, context) { reported << [e.class, context] }
+          @server = Server.new(name: "test_server", configuration: configuration)
+          @server.define_tool(name: "invalid_text") do
+            Tool::Response.new([{ type: "text", text: "\xFF".dup.force_encoding(Encoding::UTF_8) }])
+          end
+          @transport.close
+          @transport = StreamableHTTPTransport.new(@server, listen_keepalive_interval: nil)
+          session_id = initialize_test_session
+
+          response = @transport.handle_request(create_rack_request(
+            "POST",
+            "/",
+            { "CONTENT_TYPE" => "application/json", "HTTP_MCP_SESSION_ID" => session_id },
+            { jsonrpc: "2.0", id: 2, method: "tools/call", params: { name: "invalid_text", arguments: {} } }.to_json,
+          ))
+          io = StringIO.new
+          response[2].call(io)
+
+          assert_equal 200, response[0]
+          assert_equal(
+            { "jsonrpc" => "2.0", "id" => 2, "error" => { "code" => -32603, "message" => "Internal error" } },
+            JSON.parse(io.string[/^data: (.+)$/, 1]),
+          )
+          assert_equal([[JSON::GeneratorError, { error: "Failed to generate response" }]], reported)
+        end
+
+        test "a reply that cannot be generated is answered with Internal error in JSON response mode" do
+          reported = []
+          configuration = MCP::Configuration.new
+          configuration.exception_reporter = ->(e, context) { reported << [e.class, context] }
+          server = Server.new(name: "test", configuration: configuration)
+          server.define_tool(name: "invalid_text") do
+            Tool::Response.new([{ type: "text", text: "\xFF".dup.force_encoding(Encoding::UTF_8) }])
+          end
+          transport = StreamableHTTPTransport.new(server, enable_json_response: true)
+          session_id = transport.handle_request(create_rack_request(
+            "POST",
+            "/",
+            { "CONTENT_TYPE" => "application/json" },
+            { jsonrpc: "2.0", method: "initialize", id: "init", params: initialize_params }.to_json,
+          ))[1]["mcp-session-id"]
+
+          response = transport.handle_request(create_rack_request(
+            "POST",
+            "/",
+            { "CONTENT_TYPE" => "application/json", "HTTP_MCP_SESSION_ID" => session_id },
+            { jsonrpc: "2.0", id: 2, method: "tools/call", params: { name: "invalid_text", arguments: {} } }.to_json,
+          ))
+
+          assert_equal 200, response[0]
+          assert_equal(
+            { "jsonrpc" => "2.0", "id" => 2, "error" => { "code" => -32603, "message" => "Internal error" } },
+            JSON.parse(response[2][0]),
+          )
+          assert_equal([[JSON::GeneratorError, { error: "Failed to generate response" }]], reported)
+        ensure
+          transport.close
+        end
+
         test "JSON response mode returns application/json for POST requests" do
           server = Server.new(name: "test", tools: [], prompts: [], resources: [])
           transport = StreamableHTTPTransport.new(server, enable_json_response: true)
@@ -6280,6 +6342,27 @@ def string
           assert_equal 405, @transport.handle_request(delete_request)[0]
         end
 
+        test "modern POST answers a reply that cannot be generated with Internal error" do
+          reported = []
+          original_reporter = MCP.configuration.exception_reporter
+          MCP.configuration.exception_reporter = ->(e, context) { reported << [e.class, context] }
+          @server.define_tool(name: "invalid_text") do
+            Tool::Response.new([{ type: "text", text: "\xFF".dup.force_encoding(Encoding::UTF_8) }])
+          end
+
+          response = @transport.handle_request(modern_rack_request(
+            modern_body("tools/call", { name: "invalid_text", arguments: {} }),
+          ))
+
+          assert_equal 200, response[0]
+          body = JSON.parse(response[2][0])
+          assert_equal 1, body["id"]
+          assert_equal({ "code" => -32603, "message" => "Internal error" }, body["error"])
+          assert_equal([[JSON::GeneratorError, { error: "Failed to generate response" }]], reported)
+        ensure
+          MCP.configuration.exception_reporter = original_reporter
+        end
+
         test "modern POST maps an unknown method to 404 with -32601" do
           response = @transport.handle_request(modern_rack_request(modern_body("no/such_method", {})))