Repository navigation
fix(mcp): stdio id matching, real notifications, drained stderr, call timeout and cancel - #234
Merged
Merged
Conversation
… timeout and cancel (#224) - StdioTransport::send reads until the response with the request's id. It skips server notifications, answers server requests (`ping` succeeds, others get -32601), and drops stale ids and non-JSON lines. - New JsonRpcNotification and McpTransport::notify. The default keeps the old request-and-wait behaviour for custom transports; stdio and HTTP write the notification and return. notifications/initialized uses it, so the handshake no longer waits on a reply a compliant server never sends. - The server's stderr is drained into debug! logs, so it can no longer block on a full pipe. The child process is kill_on_drop. - McpToolAdapter races ctx.cancel and a call timeout (300 s default, with_call_timeout). - Unmodelled content blocks (resource, audio, …) become JSON text instead of failing the call. Tested against a bash-scripted server (tests/mcp_stdio_test.rs). The test fails if id matching is broken. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7iq5hpndSiHQcnAsywKuG
- The stderr drain reads lossily in bounded chunks. It used to stop at
the first non-UTF-8 byte, after which the server blocked again or got
EPIPE. The last 4 KiB are kept, so a server that exits is reported as
Transport("Connection closed: … exited with …; last stderr: …") instead
of a bare ConnectionClosed (it waits up to 500 ms for the drain).
- Non-JSON stdout lines are warn!ed and counted into the close error.
Declined server requests are warn!ed too. Response ids are read in
numeric or string form, and a null-id error is warn!ed when it is
attributed to the current call.
- A write cut off by a cancelled call is flagged, and the next message
starts on a fresh line.
- The call timeout moved to the client: 300 s for stdio, none for HTTP
(its idle timeout already ends a stalled call). from_client copies it,
and the clock starts after the client lock is held. The message says
the server may still carry the call out.
- The handshake is bounded at 60 s. A failed initialized notification
fails the connect, and the HTTP notify error quotes the body.
- Unmodelled content: a text resource becomes its own text; data/blob
over 1 KiB is replaced by its size; warn! once per type.
- close() logs a kill error instead of dropping it, and its comment is
fixed.
- Tests:
- stderr with a non-UTF-8 byte then a 200 KB flood;
- a banner line;
- a declined sampling request;
- a late answer followed by a successful call;
- kill on drop (with a server that ignores EOF);
- a dying server's stderr in the error;
- HTTP initialized sent once with no id, and a rejected one failing
the connect;
- large payloads.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T7iq5hpndSiHQcnAsywKuG
…fter exit Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7iq5hpndSiHQcnAsywKuG
…poll panic; fix the guide snippet - The trait's default notify stays best-effort (logged, Ok), as the old path was; only the built-in transports' errors fail the handshake. - a47ffc2's take() did fix a real panic (a finished JoinHandle polled again). Its test missed it, because an exited server fails at the write first. A server that closes its output but keeps reading reaches the close error each time, and that test panics without the fix. - The guide's custom-timeout snippet keeps the agent's other tools (with_tools replaces them) and imports Arc/Mutex. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7iq5hpndSiHQcnAsywKuG
Stdout can close a moment before the process is reaped, so try_wait() raced: CI saw "closed its output" for a server that had exited. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7iq5hpndSiHQcnAsywKuG
…ies) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7iq5hpndSiHQcnAsywKuG
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #224.
Stdio transport
sendread exactly one line as the response. A logging or progress notification, or aping, sent ahead of the answer became "the response": the call failed, and every later call was off by one.sendnow reads until the response carrying its own id. In between it:ping→{}, anything else (sampling, roots) →-32601, so the server doesn't wait;debug!logs (targetyoagent::mcp::stderr). It was piped and never read, so a server that logged more than a pipe holds blocked mid-call. Found while fixing this; it wasn't in the issue.kill_on_drop, so dropping the client ends the server.Notifications
notifications/initializedwas sent as a request with an id and awaited. A spec-compliant server never answers it, so connecting over stdio could hang.JsonRpcNotificationandMcpTransport::notify.202.notifykeeps the old request-and-wait behaviour, so a custom transport neither breaks nor changes.Tool calls
McpToolAdapter::executenow races the run's cancel token and a call timeout. The default isDEFAULT_CALL_TIMEOUT(300 s), changed withwith_call_timeout(Option<Duration>). The timeout includes waiting for the client lock. Before, a hung server hung the agent, and every tool from that server waited behind the lock.McpToolCallResult.contentis parsed leniently. Aresource,resource_linkoraudioblock becomesMcpContent::Textholding its JSON, instead of failing the whole call. A new enum variant would have been a breaking change.All public changes are additive.
Tests
tests/mcp_stdio_test.rs(unix) runsMcpClient::connect_stdioagainst a bash-scripted server:initialize: a notification and apingarrive before the answer, and the script checks the client'spingreply.initialized: must arrive without an id.tools/list: a stale response for another id comes before the real one.With id matching broken, the test fails, which I checked by breaking it. There are also unit tests for lenient content and the notification shape.
Local checks: fmt, clippy (all features, no default features, wasm32), docs, and
cargo test --all-features(55 binaries).🤖 Generated with Claude Code
https://claude.ai/code/session_01T7iq5hpndSiHQcnAsywKuG