Repository navigation
fix: Close the connection before reporting a stream error, and keep the reconnect loop alive - #95
Merged
Merged
Conversation
…he reconnect loop alive - Close the failed connection before calling on_error, so a handler that blocks does not hold the dead connection open. - Log a failure to close a connection instead of reporting a second error for the same attempt, and do not let it stop the worker thread. - Return no delay for a zero base interval and stop counting attempts at the maximum, so the backoff can no longer compute NaN.
tanderson-ld
approved these changes
Oct 15, 2026
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.
BEGIN_COMMIT_OVERRIDE
fix: Close the connection before passing a stream error to the error handler
fix: Pass at most one error to the error handler for each connection attempt
fix: Keep reconnecting when closing the previous connection fails
fix: Prevent a reconnect delay of NaN when the retry interval is zero
END_COMMIT_OVERRIDE
Summary
Fixes found while reviewing the Ruby server SDK's RETRY work (ruby-server-sdk#448). That SDK waits inside
on_errorbefore it reconnects, which exposed these paths in the reconnect loop:run_streamcalledon_errorbeforereset_http. A handler that blocks kept the dead connection open for the whole wait, for example after a read timeout. The connection is now closed first.reset_httpwas sent toon_erroras a second error for the same attempt. It is now logged at warn.close_connectionat the top of the loop was outside any rescue, so a raise ended the worker thread and the client never reconnected. It is now logged and the loop continues.0.0base (for example after a serverretry: 0),0.0 * 2**nbecameNaNafter about 1024 attempts, and[NaN, max].minraised on every later attempt. A zero base now returns no delay, and the attempt count stops growing at the maximum.StreamingHTTPConnection, which was removed in 2.0.0. They now nameHTTP::Response.No API change.
Testing
bundle exec rspec spec: 182 examples, 0 failures.bundle exec rubocop: no offenses.Note
Overview
Hardens the SSE client reconnect loop so cleanup and error reporting happen in a safer order, and fixes backoff edge cases that could break long-running reconnects.
In
run_stream, the connection is now closed (reset_http) beforeon_errorruns, so blocking error handlers do not keep a dead socket open. Close failures are handled vialog_close_errors: they are logged only (not sent toon_error), andclose_connectionat the start of each loop is wrapped the same way so a close raise no longer kills the worker thread.Backoff#next_intervalreturns 0 when the base or max interval is non-positive (avoidsNaNafter many attempts with a0.0base fromretry: 0), and stops incrementing the attempt counter once the delay hits the max ceiling.New specs cover close-before-error ordering, a single error per attempt when close fails, worker survival, and backoff behavior. Comments now refer to
HTTP::Responseinstead of the removedStreamingHTTPConnection. No public API changes.Reviewed by Cursor Bugbot for commit dedf9f8. Bugbot is set up for automated code reviews on this repo. Configure here.