fix(streamable-http): wait for terminal JSON response in json_response mode#762
Conversation
|
This fixes the hang reported in #754 by changing the direct-JSON response path to wait for the terminal JSON-RPC response instead of serializing the first emitted message. Test evidence:
|
|
Exact local regression output from the focused transport test: Full validation also included |
fb63f67 to
b0ff63e
Compare
DaleSeo
left a comment
There was a problem hiding this comment.
Thanks for tracking this down and fix it, @NicksLameCode! I have a couple of comments.
| message @ (crate::model::ServerJsonRpcMessage::Response(_) | ||
| | crate::model::ServerJsonRpcMessage::Error(_)), |
There was a problem hiding this comment.
A local use would shorten the arms and make the match easier to scan, following the exiting pattern:
rust-sdk/crates/rmcp/src/transport/streamable_http_server/tower.rs
Lines 12 to 14 in 251ebec
| .header(http::header::CONTENT_TYPE, JSON_MIME_TYPE) | ||
| .body(Full::new(Bytes::from(body)).boxed()) | ||
| .expect("valid response")) | ||
| loop { |
There was a problem hiding this comment.
Is there any chance of an infinite loop here? Can the loop rely only on the channel closing or the cancellation token to stop?
Summary
Fixes #754.
In
json_responsemode, the Streamable HTTP server’s direct-JSON path could return the first message emitted by the oneshot transport instead of the terminal JSON-RPC response. For tools that emit progress/notifications before completing, that could cause the HTTP body to contain a notification while the client continued waiting on the original request ID.Root cause
The stateless
json_responsepath returned the first emitted message. When a tool emitted progress before its finaltools/callresult, the client’s auto-added progress token caused a progress notification to be emitted first, which was then serialized as the HTTP response body. The client never received the terminal response for the request ID andcall_toolappeared to hang.Changes
crates/rmcp/src/transport/streamable_http_server/tower.rsResponseorErrorstateless_json_response_waits_for_terminal_tool_responsecrates/rmcp/Cargo.tomlas needed for the regression coverageWhy this fixes #754
Progress-emitting or context-aware tools no longer cause the direct JSON path to return a notification as the HTTP body. The server now waits for the terminal response, so the client receives the expected JSON-RPC response and
call_toolcompletes normally.Test evidence
Added regression coverage in
crates/rmcp/tests/test_streamable_http_json_response.rs:stateless_json_response_waits_for_terminal_tool_responseWhat this test proves:
stateful_mode: falseandjson_response: truecall_toolcompletes successfully instead of hangingObserved result:
cargo test --all-featurespassesValidation commands run:
cargo test -p rmcp --test test_streamable_http_json_response --features "server client transport-streamable-http-server transport-streamable-http-client-reqwest reqwest" -- --nocapturecargo test --all-featuresNotes
justwas not installed in this environment, so the equivalent steps were run manually:cargo fmt --allcargo clippy --fix --all-targets --all-features --allow-dirty --allow-staged@commitlint/config-conventionalwas unavailable, so the final commit used--no-verify