Skip to content

fix(sse): loop instead of recursing when skipping SSE events - #1146

Merged
DaleSeo merged 1 commit into
modelcontextprotocol:mainfrom
shoemoney:fix/sse-skip-loop-not-recursion
Aug 7, 2026
Merged

fix(sse): loop instead of recursing when skipping SSE events#1146
DaleSeo merged 1 commit into
modelcontextprotocol:mainfrom
shoemoney:fix/sse-skip-loop-not-recursion

Conversation

@shoemoney

@shoemoney shoemoney commented Aug 7, 2026

Copy link
Copy Markdown

Summary

SseAutoReconnectStream::poll_next recursed 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:

Line (on main) Path
400 control frame handled (event: ping, endpoint, …)
415 data present but fails to deserialize
422 event carries no data
517 state-transition tail call

The inner SseStream returns Poll::Ready for 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 a poll_next frame, and that frame is large — the function is a big match with several locals.

Reproduction

A server that emits a run of non-JSON message frames is enough. On main, with 50,000 such frames:

thread '...::skipped_events_do_not_grow_the_stack' has overflowed its stack
fatal runtime error: stack overflow, aborting
error: test failed ... (signal: 6, SIGABRT: process abort signal)

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_next 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 — same branches, same order, same returns.

Review with ?w=1. The functional change is four keywords; the rest of the diff is the resulting re-indent of the body.

Testing

Adds skipped_events_do_not_grow_the_stack to the existing tests module in the same file, in the style of the neighbouring oversized_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 SIGABRT on unmodified main and passes on this branch, so a regression is caught rather than silently reintroduced.

cargo test -p rmcp --lib --features client-side-sse,client
  this branch:  226 passed; 0 failed
  main:         225 passed; 0 failed

Exactly +1, and no pre-existing failure on either side. cargo fmt -p rmcp -- --check is clean and touched only this file. cargo clippy -p rmcp --lib --features client-side-sse,client exits 0 (7 never used warnings, all pre-existing and all a consequence of building that narrow feature set).

Notes

Toolchain. Built with stable 1.97.1; rust-toolchain.toml pins 1.96 and .githooks runs cargo +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 → ServerDoesNotSupportSse mapping in the reqwest get_stream path is missing its 401/403 WWW-Authenticate counterparts, which silently disables AuthClient's token-refresh retry — the sibling post_message in 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.md before filing, found no AI clause, and wrongly concluded none applied — AI_POLICY.md lives 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 unmodified main, 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.

`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.
@github-actions github-actions Bot added T-core Core library changes T-transport Transport layer changes labels Aug 7, 2026

@DaleSeo DaleSeo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, @shoemoney!

@DaleSeo
DaleSeo merged commit 3c8fb2a into modelcontextprotocol:main Aug 7, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-core Core library changes T-transport Transport layer changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants