Skip to content

fix(client): stop SSE reconnects when GET is unsupported - #1363

Open
aphonpra-oai wants to merge 1 commit into
modelcontextprotocol:mainfrom
aphonpra-oai:codex/stop-unsupported-get-reconnects
Open

aphonpra-oai wants to merge 1 commit 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

An SSE response can end before its final JSON-RPC result after supplying an event ID. The Streamable HTTP transport then resumes it with GET. If the server does not support GET and returns 405, the reconnect loop currently applies its retry policy to ServerDoesNotSupportSse and can keep issuing rejected requests. The same problem occurs when an established server-event listener later reconnects to a server that rejects GET.

This change treats ServerDoesNotSupportSse as terminal for the affected stream. An interrupted request reports failure without replaying its POST, and later POST requests remain usable. Other reconnect errors retain the configured retry policy, so temporary failures can still recover. The initial optional listener GET already treats 405 as unsupported and keeps its existing behavior.

The classification is internal to the reconnect connector; it does not change the public retry-policy API or add a shared endpoint-wide unsupported flag.

Reproduce with the included scripted client:

  1. Return an SSE POST response containing an event ID, then close it before its final result.
  2. Return ServerDoesNotSupportSse from the resume GET.
  3. Before this change, GET retries continue. Afterward, the request fails after that rejected GET; a subsequent POST succeeds.

Validation covers initial listener rejection, established listener rejection, interrupted POSTs with and without event IDs, request recovery after three transient failures without POST replay, and delivery of a server notification after three transient listener failures.

  • Six new transport integration tests and 17 existing SSE unit tests pass.
  • Against the unpatched base, both reconnect-405 regression tests fail as expected.
  • Scoped Clippy passes with the existing feature-selection warnings. Validation used mocked transports, not a live deployment.

Draft for maintainer review of this error-classification approach.

Checks:

  • cargo test -p rmcp --features client,transport-streamable-http-client --test test_streamable_http_reconnect_classification -- --test-threads 2: 6 passed.
  • cargo test -p rmcp --features client,transport-streamable-http-client --lib transport::common::client_side_sse -- --test-threads 2: 17 passed.
  • Negative control: restoring the unpatched reconnect implementation makes both reconnect-405 regression tests fail.
  • cargo +nightly fmt --all: passed.
  • Scoped cargo clippy -p rmcp --features client,transport-streamable-http-client --test test_streamable_http_reconnect_classification: passed with the same eight existing unused/dead-code warnings seen on the unpatched implementation under this feature set.

@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

This branch has not been deployed

No deployments
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.

1 participant