Skip to content

fix(client): stop SSE reconnects on permanent GET errors - #1363

Merged
DaleSeo merged 2 commits into
modelcontextprotocol:mainfrom
aphonpra-oai:codex/stop-unsupported-get-reconnects
Oct 23, 2026
Merged

DaleSeo merged 2 commits into
modelcontextprotocol:mainfrom
aphonpra-oai:codex/stop-unsupported-get-reconnects

Conversation

@aphonpra-oai

@aphonpra-oai aphonpra-oai commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

An interrupted SSE stream reconnects with GET. When GET is unsupported (405), or authorization is still required (401/403), retrying the same request can continue indefinitely under the default retry policy. A global retry limit would also abandon recoverable responses after temporary failures.

Treat ServerDoesNotSupportSse, AuthRequired, and InsufficientScope as terminal for the affected reconnect stream. Yield the error once and stop reconnecting that stream; all other errors retain their configured retry policy. Any authentication refresh handled within the HTTP client happens before this classification sees its returned error.

An interrupted POST request fails without replaying the operation. Later POST requests remain available; a stopped listener is not automatically reauthorized or restarted. This does not add automatic sign-in, new user-facing error messages, or an endpoint-wide unsupported flag. The classification is internal and does not change the public retry-policy API.

Reproduce using the included scripted transport:

  1. Return a POST SSE response with an event ID and close it before the final response, or interrupt an established listener.
  2. Reject the reconnect GET with one of the terminal errors above.
  3. Confirm there are no further GET retries for that stream. For an interrupted POST, confirm failure is reported and the POST is not replayed.

Validation uses mocked HTTP clients around the real transport/service, not a deployed client:

  • Ten integration tests cover initial listener rejection, established-listener and POST-resume rejection for 405/401/403, non-resumable POST responses, request recovery after three transient failures, and notification delivery after three transient listener failures.
  • Seventeen existing SSE unit tests.
  • Scoped Clippy and nightly formatting checks.
  • Negative controls verify that removing terminal classification makes the corresponding regression tests fail.

@github-actions github-actions Bot added T-test Testing related changes T-core Core library changes T-transport Transport layer changes labels Oct 15, 2026
@github-actions
github-actions Bot requested a review from bolinfest October 15, 2026 05:26
@github-actions

Copy link
Copy Markdown
Contributor

@bolinfest is on triage rotation this week and is the reviewer for this one.

Labels come from the PR labeler. Please review the change.

Automated triage. Labels are a first pass and the assigned maintainer has the final say.

@aphonpra-oai
aphonpra-oai marked this pull request as ready for review October 15, 2026 17:11
@aphonpra-oai
aphonpra-oai requested a review from a team as a code owner October 15, 2026 17:11
@goodluke-oai

Copy link
Copy Markdown

Quick routing check: the automated reviewer is no longer on the current Rust SDK roster, and the four fork workflows are still waiting for first-time-contributor approval. Could a current maintainer reroute the review and approve the workflows if appropriate?

The diff is 25 additions and one deletion, with six focused regression tests and no public API change.

Comment thread crates/rmcp/src/transport/streamable_http_client.rs Outdated
Comment thread crates/rmcp/src/transport/common/client_side_sse.rs Outdated
@aphonpra-oai aphonpra-oai changed the title fix(client): stop SSE reconnects when GET is unsupported fix(client): stop SSE reconnects on permanent GET errors Oct 23, 2026
@aphonpra-oai
aphonpra-oai requested a review from DaleSeo October 23, 2026 18:20
@aphonpra-oai

Copy link
Copy Markdown
Contributor Author

Thanks for the review @DaleSeo, addressed the comments.

@DaleSeo
DaleSeo merged commit 8aec389 into modelcontextprotocol:main Oct 23, 2026
24 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-test Testing related changes T-transport Transport layer changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants