Skip to content

fix: stop the standalone GET stream reconnecting forever on empty connections#3087

Draft
heyhayes wants to merge 2 commits into
modelcontextprotocol:mainfrom
heyhayes:fix/get-stream-empty-reconnect-loop
Draft

fix: stop the standalone GET stream reconnecting forever on empty connections#3087
heyhayes wants to merge 2 commits into
modelcontextprotocol:mainfrom
heyhayes:fix/get-stream-empty-reconnect-loop

Conversation

@heyhayes

Copy link
Copy Markdown

Summary

Split out from #2122. Since that PR was opened, its core fix — propagating an error to a waiting request when an SSE stream ends without a response — has landed upstream independently via the _resolve_abandoned_request helper (introduced in #3047), which resolves an abandoned request with a synthesized error both when an id-bearing stream ends unresumably and when reconnection attempts are exhausted.

This PR carries forward only the two fixes from #2122 that are not covered by that work. Relates to #1401.

What changed

Stop the standalone GET stream reconnecting forever on empty connections. handle_get_stream reset its attempt counter to 0 whenever a connection ended "normally" — i.e. the async for over the SSE events completed without raising. A server that accepts the GET but immediately closes the stream with no events hits exactly this path, so the counter never advances and the client reconnects in an unbounded loop (throttled only by the reconnect delay). The counter now resets only when events were actually received; empty connections count toward MAX_RECONNECTION_ATTEMPTS, so the loop gives up. That give-up branch was previously marked unreachable (# pragma: no cover) — it is now reachable and covered by a test.

Log unhandled exceptions in the default message handler. Transport-level errors not tied to a specific pending request (e.g. a GET stream failure) reach _default_message_handler as an Exception item and were silently discarded. It now logs a warning as an observability safety net.

Testing

  • New regression test drives repeated empty GET streams and asserts the loop terminates after MAX_RECONNECTION_ATTEMPTS rather than reconnecting forever.
  • New test asserts _default_message_handler logs a warning for an Exception item.
  • Full suite green; both changed files at 100% branch coverage.

heyhayes added 2 commits July 10, 2026 20:22
Only reset the attempt counter when events were actually received
during the connection. Connections that close immediately without
delivering events now count toward MAX_RECONNECTION_ATTEMPTS.

Github-Issue:modelcontextprotocol#1401
Transport errors that are not tied to a specific pending request (e.g.,
GET stream failures) were silently swallowed by the default message
handler. Add a warning log so these exceptions are at least visible in
logs as an observability safety net.

Github-Issue: modelcontextprotocol#1401
@shivamsingh-007

Copy link
Copy Markdown

Welcome @heyhayes — thanks for this contribution! The fix for the GET stream reconnection loop looks well thought out, and the root cause analysis is clear.

If you haven't already, take a look at the CONTRIBUTING.md — it covers things like the PR checklist, AI disclosure policy, and testing conventions that help move reviews along smoothly. Happy to help if anything's unclear.

AI assistance was used to draft this comment; I reviewed it and take responsibility.

@shivamsingh-007

Copy link
Copy Markdown

Reviewed the diff — the logic looks correct.

Empty-connection path:
eceived_events = False → �ttempt += 1 per empty close → loop terminates after MAX_RECONNECTION_ATTEMPTS. Previously it would reconnect indefinitely.

Event-received path: �ttempt = 0 on normal close preserves the existing good-behavior reset.

Exception path: if _handle_sse_event or the connection itself raises, the except block increments �ttempt and the if/else after the �sync for is never reached — no double-counting.

The _default_message_handler warning is a good observability addition, and both new tests cover the previously-uncovered paths in a deterministic way (monkeypatched delay = 0, ail_after guard). The # pragma: no cover removal on line 234 is correctly matched by the new coverage.

One small note: the standard PR template includes a checklist (Fixes #NNNN, test confirmation, etc.) — it's not required for small fixes like this, but worth keeping in mind for future PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants