fix(client/streamable_http): bound SSE reconnect loop with exponential backoff + jitter - #4
Open
annasclaw wants to merge 2 commits into
Open
fix(client/streamable_http): bound SSE reconnect loop with exponential backoff + jitter#4annasclaw wants to merge 2 commits into
annasclaw wants to merge 2 commits into
Conversation
…er (issue modelcontextprotocol#3356) StreamableHTTPTransport.handle_get_stream and the recursive _handle_reconnection both reset their attempt counter to 0 on normal SSE EOF, bypassing MAX_RECONNECTION_ATTEMPTS and looping forever when an upstream keeps cleanly closing the stream. Increment the attempt counter on normal EOF instead, and apply bounded exponential backoff (factor 2, 30s cap) with +/-25% jitter on the reconnect delay to avoid synchronized reconnect storms against a recovering upstream. Adds a _compute_backoff_delay_ms helper with parametric + smoke tests verifying the growth/cap/jitter envelope.
annasclaw
commented
Sep 2, 2026
annasclaw
left a comment
Owner
Author
There was a problem hiding this comment.
Reviewed f14da60: fixed the new import ordering/formatting lint finding, verified the reconnect backoff tests (33/33 passed), and ran ruff check/format successfully. The bounded attempt-count fix remains sound; maintainer approval is still required.
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
Fixes #3356:
StreamableHTTPTransport.handle_get_streamand the recursive_handle_reconnectionboth reset their attempt counter to 0 on every normal SSE EOF, bypassingMAX_RECONNECTION_ATTEMPTSand looping forever when an upstream keeps cleanly closing the stream. This PR also adds bounded exponential backoff with jitter to the reconnect delay so a recovering upstream is not pummelled by synchronized client reconnects.Root cause
When a server repeatedly closes a fresh SSE stream (e.g. crashloop, drained read buffer, deliberate close) the loop never increments
attempt, so thewhile attempt < MAX_RECONNECTION_ATTEMPTSguard never fires. The recursive_handle_reconnectionhad the same bug (attempt=0on the recursive call after a clean EOF).Fix
attempton normal EOF instead of resetting it (handle_get_stream) or passing 0 to the recursive call (_handle_reconnection)._compute_backoff_delay_ms(attempt, base)helper that returns a bounded exponential delay (base * 2**attempt, capped atMAX_RECONNECTION_DELAY_MS = 30s) with ±25% jitter.The server-supplied
retry_interval_ms(per the SSE specretry:field) is still used as thebasefor the backoff, so well-behaved servers still pace their own reconnects.Tests
test_compute_backoff_delay_ms_grows_then_caps— parametric over attempts[0, 1, 3, 10], samples jitter, asserts the growth/cap envelope.test_compute_backoff_delay_ms_no_regression_for_zero_attempt— smoke test, attempt 0 stays near the base delay.The loop-termination semantic is enforced by the source change: the
attempt += 1on normal EOF guarantees thewhile attempt < MAX_RECONNECTION_ATTEMPTSguard fires afterMAX_RECONNECTION_ATTEMPTS - 1clean disconnects. This is provable by inspection: existingtest_exhausted_reconnection_attempts_resolve_the_request_with_an_erroralready exercises the>= MAX_RECONNECTION_ATTEMPTSbranch on_handle_reconnectionand now serves as a regression guard for the recursive variant.Diffstat
Notes
MAX_RECONNECTION_ATTEMPTS = 2attempts. This was the user's stated expected behaviour in the issue: "The client should respect MAX_RECONNECTION_ATTEMPTS on stream disconnects". If reviewers want behaviour-preservation for the "one-and-done" case, that's a separate follow-up.handle_get_streamand_handle_reconnectionshare the same backoff algorithm and the same attempt-counter semantics; previously they differed subtly (the former incremented in the except branch only, the latter in the recursive call only — both reset on EOF).