fix: Streamable HTTP client ping response handling #578
Conversation
WalkthroughAdds server-to-client JSON-RPC "ping" handling in the client, replaces per-request timeouts with cancelable contexts in the Streamable HTTP transport, broadens the transport Accept header to include SSE, and extends Streamable HTTP tests to exercise ping acknowledgements, periodic server pings, and SSE events lacking an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (3 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
havasik
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/transport/streamable_http.go (1)
623-623: Consider implementing exponential backoff for retry logic.The immediate retry approach could lead to excessive resource consumption and server load if the connection repeatedly fails. Consider implementing exponential backoff with jitter to be more considerate of server resources and network conditions.
Example implementation:
retryDelay := time.Second maxRetryDelay := 30 * time.Second for { // ... existing connection attempt code ... if err != nil { c.logger.Errorf("failed to listen to server, retrying after %v: %v", retryDelay, err) // Wait with backoff select { case <-time.After(retryDelay): // Exponential backoff with cap retryDelay = retryDelay * 2 if retryDelay > maxRetryDelay { retryDelay = maxRetryDelay } case <-ctx.Done(): return } } else { // Reset delay on successful connection retryDelay = time.Second } }client/transport/streamable_http_test.go (1)
774-781: Consider making the ping timeout configurable or more robust.The test waits up to 5 seconds for a ping that should arrive every 3 seconds. While this provides some buffer, consider:
- Making the timeout relative to the ping interval (e.g.,
pingInterval * 2)- Waiting for multiple pings to ensure the mechanism works consistently
+ const pingInterval = 3 * time.Second + const pingTimeout = pingInterval * 2 + // Wait for at least one ping to be received (should happen within 3 seconds) select { case <-pingReceived: t.Log("Received ping message successfully") time.Sleep(10 * time.Millisecond) // Allow time for response - case <-time.After(5 * time.Second): - t.Errorf("Expected to receive ping message within 5 seconds, but didn't") + case <-time.After(pingTimeout): + t.Errorf("Expected to receive ping message within %v, but didn't", pingTimeout) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/client.go(2 hunks)client/transport/streamable_http.go(2 hunks)client/transport/streamable_http_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
client/client.go (2)
mcp/types.go (5)
JSONRPCRequest(325-330)JSONRPCResponse(339-343)MethodPing(25-25)JSONRPC_VERSION(111-111)Result(246-250)client/transport/interface.go (2)
JSONRPCRequest(57-62)JSONRPCResponse(64-73)
client/transport/streamable_http_test.go (2)
mcp/types.go (3)
JSONRPCRequest(325-330)JSONRPCResponse(339-343)Result(246-250)client/transport/interface.go (2)
JSONRPCRequest(57-62)JSONRPCResponse(64-73)
🔇 Additional comments (9)
client/client.go (3)
461-461: LGTM! Clear and accurate comment update.The comment appropriately documents that this handler now processes both sampling and ping requests from the server.
466-467: LGTM! Correct ping request routing.The ping request is properly routed to the dedicated handler.
521-530: No validation required — MCP ping has no parameters.
MCP "ping" is a JSON-RPC request with an empty params object; returning an empty JSON result ({}) is correct and no additional request-parameter validation is needed.Likely an incorrect or invalid review comment.
client/transport/streamable_http.go (2)
615-621: LGTM! Proper defensive check before retry.Good addition of the context cancellation check before retrying. This prevents unnecessary retry attempts when the client is shutting down.
768-768: LGTM! Proper Accept header for SSE support.The extended Accept header correctly indicates support for both JSON and SSE responses, which is appropriate for handling ping acknowledgments that may come via SSE.
client/transport/streamable_http_test.go (4)
557-570: LGTM! Proper ping acknowledgment handling.The implementation correctly handles ping responses from the client by checking for a
resultfield and responding with HTTP 202 Accepted status, which aligns with the MCP specification for acknowledging pings.
647-669: LGTM! Well-implemented keepalive mechanism.The periodic ping implementation correctly:
- Uses a 3-second interval for keepalive pings
- Generates unique ping IDs
- Follows the JSON-RPC 2.0 format
- Properly cleans up the ticker on exit
709-724: LGTM! Comprehensive ping handling in tests.The test properly validates the bidirectional ping flow:
- Receives ping requests from the server
- Responds with the correct JSON-RPC format
- Uses an empty JSON object as the result, matching the MCP spec
418-492: LGTM! Good test coverage for SSE edge case.The test properly validates that SSE events without an explicit
event:field are processed as "message" events by default, which is correct according to the SSE specification. This ensures compatibility with servers that may omit the event field.
havasik
force-pushed
the
streamable-handle-ping
branch
from
7b47212 to
6ea7d8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/client.go(2 hunks)client/transport/streamable_http.go(2 hunks)client/transport/streamable_http_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/client.go
- client/transport/streamable_http_test.go
🔇 Additional comments (2)
client/transport/streamable_http.go (2)
615-621: Pre-retry cancellation check: LGTMGood guard to avoid pointless reconnect attempts during shutdown.
769-780: Accept header for POSTed responses: spec-compliantIncluding both application/json and text/event-stream on all client POSTs (requests, notifications, responses) matches the Streamable HTTP spec. Servers that receive only responses/notifications should reply 202 with no body, which your code already handles. (spec.modelcontextprotocol.io)
havasik
force-pushed
the
streamable-handle-ping
branch
from
6ea7d8c to
68e8505
Compare
havasik
force-pushed
the
streamable-handle-ping
branch
from
0309bb8 to
a97bd22
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
client/transport/streamable_http_test.go (3)
557-571: Tighten JSON-RPC response detection to avoid false positivesGate on absence of "method" and presence of "id" when treating POSTs as responses. Prevents misclassifying regular requests with a stray "result" field.
- // Handle ping responses from client - if _, hasResult := request["result"]; hasResult { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusAccepted) - if err := json.NewEncoder(w).Encode(map[string]any{ - "jsonrpc": "2.0", - "id": request["id"], - "result": "response received", - }); err != nil { - http.Error(w, "Failed to encode response acknowledgment", http.StatusInternalServerError) - return - } - return - } + // Handle client JSON-RPC responses (e.g., to server-initiated ping) + if _, hasResult := request["result"]; hasResult { + if _, hasMethod := request["method"]; hasMethod { + // Not a response; fall through to request handling. + } else { + if _, hasID := request["id"]; !hasID { + http.Error(w, "Missing id in JSON-RPC response", http.StatusBadRequest) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusAccepted) + if err := json.NewEncoder(w).Encode(map[string]any{ + "jsonrpc": "2.0", + "id": request["id"], + "result": "response received", + }); err != nil { + http.Error(w, "Failed to encode response acknowledgment", http.StatusInternalServerError) + return + } + return + } + }
709-725: Limit handler scope to ping and avoid emitting errors for unknown methodsReturn nil for non-ping so future server-initiated methods (e.g., sampling) don’t fail this test. Also fix the misleading comment.
- // Setup notification handler + // Setup request handler for ping pingReceived := make(chan struct{}, 10) // Setup request handler for ping requests trans.SetRequestHandler(func(ctx context.Context, request JSONRPCRequest) (*JSONRPCResponse, error) { if request.Method == "ping" { pingReceived <- struct{}{} // Return proper ping response according to MCP specification return &JSONRPCResponse{ JSONRPC: "2.0", ID: request.ID, Result: json.RawMessage("{}"), }, nil } - return nil, fmt.Errorf("unsupported request method: %s", request.Method) + // Ignore other methods in this test. + return nil, nil })
774-781: Remove unnecessary sleep; optionally extend timeout if keeping 3s ticksThe post-receipt sleep adds no value and can slow tests; if the 3s ticker is retained, consider a slightly larger timeout.
case <-pingReceived: t.Log("Received ping message successfully") - time.Sleep(10 * time.Millisecond) // Allow time for response case <-time.After(5 * time.Second): t.Errorf("Expected to receive ping message within 5 seconds, but didn't") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/client.go(2 hunks)client/transport/streamable_http.go(2 hunks)client/transport/streamable_http_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/client.go
- client/transport/streamable_http.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/transport/streamable_http_test.go (1)
client/transport/interface.go (2)
JSONRPCRequest(57-62)JSONRPCResponse(64-73)
🔇 Additional comments (1)
client/transport/streamable_http_test.go (1)
647-670: Shorten ping interval to reduce test flakiness and total wall time3s ticks can be borderline on slow CI; reduce to 500ms without changing semantics.
- pingTicker := time.NewTicker(3 * time.Second) + pingTicker := time.NewTicker(500 * time.Millisecond)Verification inconclusive: the provided test run failed to start the test server (error "failed to start telemetry sidecar: os.Executable: readlink /proc/self/exe: no such file or directory") and showed repeated "dial tcp 127.0.0.1:NNNNN: connect: connection refused". Re-run after fixing test startup:
go test ./client/transport -run ^TestContinuousListening$ -count=50 -timeout 60s -v
havasik
force-pushed
the
streamable-handle-ping
branch
from
a97bd22 to
cb3cb1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client/transport/streamable_http_test.go (2)
647-670: Speed up ping loop to reduce test flakiness and runtime.A 3s ticker can push close to the 5s assertion window below. Shorten the interval for tests to increase determinism.
- pingTicker := time.NewTicker(3 * time.Second) + pingTicker := time.NewTicker(1 * time.Second) // faster/less flaky in testsIf you prefer not to change behavior here, consider injecting the interval (e.g., test-only option) instead of hard-coding.
774-781: Strengthen the assertion: also verify the client posted the ping response back.Right now we assert only receipt of the server “ping”. To cover the full round-trip, consider signaling from the mock server when it receives the client’s JSON-RPC response (e.g., an ackReceived chan set in the POST handler above) and wait on it here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/streamable_http_test.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: leavez
PR: mark3labs/mcp-go#114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
🧬 Code graph analysis (1)
client/transport/streamable_http_test.go (2)
mcp/types.go (3)
JSONRPCRequest(325-330)JSONRPCResponse(339-343)Result(246-250)client/transport/interface.go (2)
JSONRPCRequest(57-62)JSONRPCResponse(64-73)
🔇 Additional comments (1)
client/transport/streamable_http_test.go (1)
709-725: LGTM: request handler correctly responds to server “ping”.Response mirrors the incoming ID and returns a JSON-RPC 2.0 result; channel signal makes the assertion straightforward.
| // Handle ping responses from client | ||
| if _, hasResult := request["result"]; hasResult { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusAccepted) | ||
| if err := json.NewEncoder(w).Encode(map[string]any{ | ||
| "jsonrpc": "2.0", | ||
| "id": request["id"], | ||
| "result": "response received", | ||
| }); err != nil { | ||
| http.Error(w, "Failed to encode response acknowledgment", http.StatusInternalServerError) | ||
| return | ||
| } | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Tighten JSON-RPC response (ping ACK) detection; current check is too loose.
Gate on a valid JSON-RPC response shape (jsonrpc, id, no method) before treating any POST-with-result as an ACK. This avoids accidentally ACKing non-response payloads.
Apply within this block:
- // Handle ping responses from client
- if _, hasResult := request["result"]; hasResult {
+ // Handle client JSON-RPC responses (e.g., ping replies)
+ if request["jsonrpc"] == "2.0" && request["id"] != nil && request["method"] == nil {
+ if _, hasResult := request["result"]; hasResult {
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusAccepted)
if err := json.NewEncoder(w).Encode(map[string]any{
"jsonrpc": "2.0",
"id": request["id"],
"result": "response received",
}); err != nil {
http.Error(w, "Failed to encode response acknowledgment", http.StatusInternalServerError)
return
}
return
- }
+ }
+ }If you want to be strict with MCP spec semantics (HTTP status/body for ACKs), I can confirm against the 2025-03-26 ping utility section.
🌐 Web query:
What HTTP status code and response body (if any) does the MCP Streamable HTTP spec prescribe for client-to-server responses to server-initiated "ping" requests (2025-03-26 spec)?
💡 Result:
Answer: The client must POST a JSON‑RPC response to the server's ping (e.g. {"jsonrpc":"2.0","id":"123","result":{}}). Because that POST consists solely of a JSON‑RPC response, the Streamable HTTP spec requires the server to reply with HTTP 202 Accepted and no response body. [1][2]
Sources:
[1] MCP Streamable HTTP — Transports (Protocol revision 2025‑03‑26).
[2] MCP — Ping utility (Protocol revision 2025‑03‑26).
Tighten JSON-RPC ping ACK detection and return 202 with no body (spec requires empty body).
File: client/transport/streamable_http_test.go:557–571 — Gate on a valid JSON‑RPC response shape (request["jsonrpc"] == "2.0" && request["id"] != nil && request["method"] == nil) before treating a POST-with-"result" as an ACK. When ACKing, respond with HTTP 202 Accepted and no response body (remove the JSON encoding and Content-Type header) per MCP Streamable HTTP spec (2025-03-26).
🤖 Prompt for AI Agents
In client/transport/streamable_http_test.go around lines 557 to 571, the handler
currently treats any POST containing "result" as a ping ACK and returns a 202
with a JSON body and Content-Type; tighten detection to only consider a valid
JSON-RPC response shape (request["jsonrpc"] == "2.0" && request["id"] != nil &&
request["method"] == nil && request["result"] != nil) before treating it as an
ACK, and change the response to set status to http.StatusAccepted (202) with no
response body and no Content-Type header (remove json encoding and any
Write/Encode calls) to match the spec.
There was a problem hiding this comment.
@havasik could you please address this?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
ezynda3
added
status: needs submitter response
Description
Fixes #576
Support for handling https://modelcontextprotocol.io/specification/2025-03-26/basic/utilities/ping has been added for Streamable HTTP client, when clients are created with
transport.WithContinuousListening().Type of Change
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests