feat: Retry indefinitely after a data source failure instead of stopping permanently - #448
jsonbailey wants to merge 3 commits into
Conversation
…ing permanently The FDv1 streaming and polling data sources follow the RETRY spec. No HTTP or transport failure stops them. An unexpected status such as 401 moves them to a longer backoff, and the SDK keeps retrying. OFF is reported only after shutdown.
| end | ||
| end | ||
|
|
||
| def self.http_error_message(status, context, recoverable_message) |
There was a problem hiding this comment.
Still used in FDv2? Consider marking deprecated.
| reconnect_time: @config.initial_reconnect_delay, | ||
| # The SDK waits in the failure handlers instead. This must be an Integer: the SSE client | ||
| # multiplies it by a power of two, and a Float 0.0 becomes NaN once that power overflows. | ||
| reconnect_time: 0, |
There was a problem hiding this comment.
The mixture of floats and ints throughout the logic is a tad concerning if we anticipate edge cases like this comment describes.
| old_status = @current_status | ||
|
|
||
| # A poll or stream connection that was still in flight when the data source stopped must not | ||
| # report after OFF. |
There was a problem hiding this comment.
This comment is true, but also perhaps too specific. It doesn't have to be in flight or stopped, just that nothing else can be reported after Off.
| MAX_WAIT = 1_000_000_000 | ||
|
|
||
| # The 4xx statuses that are still normal failures. Every other 4xx is unexpected. | ||
| NORMAL_4XX_STATUSES = [400, 408, 429].freeze |
There was a problem hiding this comment.
Should these be defined where other http error handling is and not tied to retry?
|
|
||
| # The longest wait a caller should pass to `Concurrent::Event#wait`, in seconds. A much longer | ||
| # wait raises `RangeError`. This is about 31 years, so the bound has no effect in practice. | ||
| MAX_WAIT = 1_000_000_000 |
There was a problem hiding this comment.
This feels silly. Is it just avoiding a thrown error?
| end | ||
|
|
||
| # Integer exponentiation does not overflow, and a Float product that becomes Infinity is | ||
| # still cut down to the ceiling. |
There was a problem hiding this comment.
"Integer exponentiation does not overflow". What black magic is this? Quick, patent it!
This branch has not been deployed
2 participants
Note
This requires ld-eventsource 3.0.0 (launchdarkly/ruby-eventsource#91), which adds
SSE::Errors::StreamClosedByServerError. The gemspec now pins it exactly.BEGIN_COMMIT_OVERRIDE
feat: Retry indefinitely after a data source failure instead of stopping permanently
fix: Warn and use the documented default for an invalid poll interval or initial reconnect delay
fix: Measure a repeating task's interval from when the task returns
END_COMMIT_OVERRIDE
Requirements
Related issues
Describe the solution you've provided
The FDv1 streaming and polling data sources now follow the RETRY spec. No HTTP or transport failure stops a data source.
Each failure is
normalorunexpected:400,408,429, any5xx4xx, including401,403initial_reconnect_delay(1s), doubling to 30spoll_interval(flat)poll_intervalThe SDK owns the reconnect delay. The SSE client gets
reconnect_time: 0, and the SDK waits inside its error handler on an event thatstopsets, sostopends even an hour-long wait at once. A server close arrives in the error handler asSSE::Errors::StreamClosedByServerError.Behavior changes (for release notes)
LDClient.newandpostforkwait the fullwait_for_secand return uninitialized, and the SDK keeps retrying.OFFonly after shutdown. HTTP errors giveINTERRUPTED.HTTP error 401 (invalid SDK key) for streaming connection - will retry in 300.0s.poll_intervalorinitial_reconnect_delaylogs a warning and uses the default.What changed
impl/retry_state.rb(new): the delay calculation, failure classification, both reset policies, and input validation.impl/data_source/stream.rb: one failure path for every error; the wait on a stop event;record_successon each successful event;reconnect_time: 0as an Integer (a Float0.0becomesNaNinside the gem after about 1024 failures).impl/data_source/polling.rb: the retry state drives the task interval; the stop-on-unexpected-status branch is gone.impl/repeating_task.rb: the interval can be a callable; every wait starts when the run returns.impl/data_source.rb: the FDv1 update sink ignores every status afterOFF, so a request still in flight at shutdown cannot report after it.impl/data_system/streaming.rb(FDv2, one line):StreamClosedByServerErrorreports asNETWORK_ERROR, notUNKNOWN.impl/util.rb: a log message helper that states the delay. The old helper stays for FDv2 and events.interfaces/data_source.rb,ldclient.rb:INTERRUPTED,OFFandinitialized?docs.contract-tests/service.rb: declaresretry-conformance-fdv1-streamingandretry-conformance-fdv1-polling.Known gaps / out of scope
retry:field would bring back the gem's own sleep, whichclosecannot interrupt. LaunchDarkly does not send that field.Testing
bundle exec rspec spec: 1170 examples, 0 failures. RuboCop: 189 files, no offenses. Run against the gem at 03c7b1a.-enable-long-running-tests, run out of band:mainwith the two capabilities declared: 13 failures, exactly the 9 streaming and 4 polling conformance tests.-enable-long-running-tests, so the conformance tests do not run in CI. Declaring the capabilities also turns off the legacy "do not retry" tests.Describe alternatives you've considered
Retry profiles inside the gem (the Go and Java approach). Rejected: the algorithm would then exist twice, in the gem for streaming and in the SDK for polling.
Note
Overview
FDv1 streaming and polling no longer shut down on HTTP or transport failures; they follow the RETRY spec with
Impl::RetryStatedriving exponential backoff, jitter, and separate “normal” vs “unexpected” (e.g. 401/403) delay regimes.OFFis reserved for client shutdown, while failures surface asINTERRUPTEDwith logs that include the actual next retry delay.Streaming sets
reconnect_time: 0onld-eventsource3.0.0 and waits in the error handler on a stop event sostopcan interrupt long backoffs; server-initiated closes useStreamClosedByServerError. Polling drivesRepeatingTaskvia a callable interval from retry state instead of stopping on “unrecoverable” statuses.RepeatingTasknow reads the interval after each run (full wait after the task returns).UpdateSinkignores status updates onceOFF, avoiding races from in-flight handlers at shutdown. Contract tests advertiseretry-conformance-fdv1-streamingandretry-conformance-fdv1-polling.Reviewed by Cursor Bugbot for commit e2c2237. Bugbot is set up for automated code reviews on this repo. Configure here.