fix(sse): loop instead of recursing when skipping SSE events - #1146
Merged
DaleSeo merged 1 commit intoAug 7, 2026
Merged
Conversation
`SseAutoReconnectStream::poll_next` recursed into itself for every event it skips — control frames, data-less frames, and frames whose data fails to deserialize — plus once more on every state transition. The inner stream returns `Poll::Ready` for each event it can parse out of already-buffered bytes, so a burst of skipped events has no yield point between them. Each one adds a stack frame, and `poll_next` is a large frame. A client connected to a server that emits a run of non-JSON `message` frames overflows the stack and aborts the process. Wrap the body in a `loop` and replace the four `self.poll_next(cx)` tail calls with `continue`. `this` is re-derived from `self.as_mut().project()` at the top of each iteration, so the borrows end cleanly per iteration; no other logic changes. The diff is mostly the resulting re-indent — review with `?w=1`. Adds `skipped_events_do_not_grow_the_stack`, which feeds 50,000 undeserializable frames through the stream. Before this change it aborts with `fatal runtime error: stack overflow` (SIGABRT); after, it passes.
DaleSeo
approved these changes
Aug 7, 2026
DaleSeo
left a comment
Member
There was a problem hiding this comment.
Thanks for the fix, @shoemoney!
This was referenced Aug 7, 2026
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
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.
Summary
SseAutoReconnectStream::poll_nextrecursed into itself for every event it skips, plus once more on every state transition. A burst of skipped events overflows the stack and aborts the process.Four sites, all in
crates/rmcp/src/transport/common/client_side_sse.rs:main)event: ping,endpoint, …)datapresent but fails to deserializedataThe inner
SseStreamreturnsPoll::Readyfor every event it can parse out of already-buffered bytes, so a run of skipped events has no yield point between them. Each one adds apoll_nextframe, and that frame is large — the function is a bigmatchwith several locals.Reproduction
A server that emits a run of non-JSON
messageframes is enough. Onmain, with 50,000 such frames:This is a client-side abort of the whole process, not a stream error the caller can handle.
The change
Wrap the body of
impl Stream for SseAutoReconnectStream::poll_nextin aloopand replace the fourself.poll_next(cx)tail calls withcontinue.thisis re-derived fromself.as_mut().project()at the top of each iteration, so the borrows end cleanly per iteration. No other logic changes — same branches, same order, same returns.Testing
Adds
skipped_events_do_not_grow_the_stackto the existingtestsmodule in the same file, in the style of the neighbouringoversized_event_returns_error_without_reconnecting. It feeds 50,000 undeserializable frames through the stream and asserts it terminates.Verified against both states of the source — the test aborts with
SIGABRTon unmodifiedmainand passes on this branch, so a regression is caught rather than silently reintroduced.Exactly +1, and no pre-existing failure on either side.
cargo fmt -p rmcp -- --checkis clean and touched only this file.cargo clippy -p rmcp --lib --features client-side-sse,clientexits 0 (7never usedwarnings, all pre-existing and all a consequence of building that narrow feature set).Notes
Toolchain. Built with stable 1.97.1;
rust-toolchain.tomlpins 1.96 and.githooksrunscargo +nightly fmt. I have no rustup on this machine, so the formatting was applied with stable rustfmt — the repo's nightly-only options (imports_granularity,group_imports) were skipped with a warning. My hunk changes no imports and the only reflow was line-wrapping, which is identical on both channels, but CI on 1.96 is the authoritative check.Deliberately not fixed. The 405 →
ServerDoesNotSupportSsemapping in the reqwestget_streampath is missing its 401/403WWW-Authenticatecounterparts, which silently disablesAuthClient's token-refresh retry — the siblingpost_messagein the same file handles both. That is a separate concern and belongs in its own PR; happy to send it if useful.AI assistance disclosure (added after merge)
Per the modelcontextprotocol AI policy: this change was made in conjunction with my pair programmer, Claude Code.
I checked this repo's
CONTRIBUTING.mdbefore filing, found no AI clause, and wrongly concluded none applied —AI_POLICY.mdlives in the specification repo and its Scope section covers every repository in the org, SDKs included. My error, corrected here as soon as I found it.Extent: the defect was surfaced by an automated sweep I run across MCP-ecosystem repos, and the patch was written with Claude Code working alongside me. The verification above is my own — the
fatal runtime error: stack overflow/ SIGABRT reproduction on unmodifiedmain, the 226-vs-225 baseline, and the fmt/clippy runs were executed on my machine. I understand the change and can answer questions on it directly.