Repository navigation
feat: Retry indefinitely after a data source failure instead of stopping permanently - #448
Open
jsonbailey wants to merge 3 commits into
Open
jsonbailey wants to merge 3 commits into
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.
jsonbailey
marked this pull request as ready for review
September 28, 2026 15:00
| end | ||
| end | ||
|
|
||
| def self.http_error_message(status, context, recoverable_message) |
Contributor
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, |
Contributor
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. |
Contributor
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 |
Contributor
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 |
Contributor
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. |
Contributor
There was a problem hiding this comment.
"Integer exponentiation does not overflow". What black magic is this? Quick, patent it!
tanderson-ld
added a commit
that referenced
this pull request
Oct 6, 2026
## Summary Replaces wall-clock time with monotonic time everywhere the SDK measures an interval, duration, or deadline. Wall clocks (`Time.now`, epoch millis) remain only for values that leave the process or are compared against externally supplied absolute times. Implements SDK-3104. A wall-clock step (NTP correction, manual clock set, container/VM time resync, the time correction when a frozen environment thaws) previously could cause: - **`RepeatingTask`** (polling, event flush, big-segment status polling, store-status polling): an immediate re-run on a forward step, or up to interval-plus-step of silence on a backward step. - **`ExpiringCache`** (big-segments membership cache, persistent-store read-through cache): entries served past their TTL after a backward step; whole-cache eviction (thundering herd against Redis/DynamoDB) on a forward step. A backward step could also strand genuinely expired entries behind the eviction scan's early exit until capacity pressure removed them. - **Migration op latency:** inflated values on a forward step; silently *dropped* measurements on a backward step (the tracker discards negatives), skewing old/new latency comparisons. - **FDv2 fallback/recovery verdicts** (60s/10s/300s windows): a step inside the window could trigger or suppress synchronizer failover. - **Stream-init diagnostics:** FDv1 sent *negative* `durationMillis` after a backward step; FDv2 clamped negatives but still inflated forward steps. ## Changes - `Impl::Util.monotonic_seconds` (`Process.clock_gettime(CLOCK_MONOTONIC, :float_second)`) is the single monotonic seam — now the only clock read in `lib/` besides `Util.current_time_millis`. - `RepeatingTask` measures each run monotonically; fixed-rate semantics unchanged. - `ExpiringCache` stamps and evicts on an injectable monotonic clock (`clock:` kwarg). This also makes the eviction scan's "stop at first unexpired entry" early exit a true invariant (stamps are now non-decreasing in insertion order). - `Migrations::Executor` measures operation latency monotonically. - `StatusProviderV2` keeps a monotonic twin of `state_since`, stamped under the same write lock on state change, exposed via the atomic `status_and_seconds_in_state` (one read-lock acquisition returns `[status, seconds]`, so a verdict can never pair a stale state with a fresh duration). Public `status.state_since` remains a wall-clock `Time`. Also fixed here: a `nil` state from a custom synchronizer previously fell into the change path and could permanently disable FDv2 fallback/recovery; it is now ignored, matching the FDv1 sink. - FDv2 `fallback_condition` / `recovery_condition` are pure functions of `(status, seconds_in_state)`, with a truth-table spec pinning the 60/10/300s thresholds and the strict-`>` boundary semantics. - FDv1/FDv2 stream-init `durationMillis` is measured monotonically (integer ms); the reported `timestamp` stays wall-clock. The monotonic stamp is also the "attempt in flight" sentinel, so lifecycle and measurement share one typed value. **Deliberately unchanged** (wall clock is correct): analytics event `creationDate`, diagnostic `creationDate`/`dataSinceDate`, `Status#state_since`, `ErrorInfo#time`, `debugEventsUntilDate` comparisons (server-supplied absolute dates), and big-segments staleness — `last_up_to_date` is written by another process, so only a wall clock is comparable across hosts; it remains sensitive to host-to-host skew, which no local clock choice can fix. ## Behavioral notes **Not a breaking change.** All modified classes are `Impl`-private; additions are optional kwargs and a `@private` accessor; behavior under a stable clock is identical. Three nuances: 1. **Suspend semantics.** Linux `CLOCK_MONOTONIC` does not advance during system suspend, so TTLs and the FDv2 state windows now measure *process-visible* time. We considered `CLOCK_BOOTTIME` and chose `CLOCK_MONOTONIC` everywhere for cross-platform consistency (macOS/JRuby lack BOOTTIME, and macOS's monotonic clock already counts sleep). Notably, this costs nothing in AWS Lambda: a frozen execution environment pauses the entire microVM with no guest-visible suspend, so BOOTTIME and MONOTONIC behave identically there. The practical consequence in warm Lambdas is that TTLs become cumulative *active* time — persistent-store/big-segments cache entries can live longer in wall time than the configured TTL under sporadic short invocations (the old wall-clock behavior instead re-fetched everything at thaw, since the thaw time-sync is a forward step). If that ever matters in practice, dual-stamping entries (wall + monotonic, expire on whichever elapses first) is the known escape hatch. 2. **Application test suites that time-travel.** Apps using Timecop (or similar) to drive SDK-internal timing — e.g. jumping `Time.now` to expire the SDK's caches — will find the SDK no longer follows the wall clock. Timecop's opt-in `Timecop.mock_process_clock = true` with the offset forms (`Timecop.travel(3600)`) still works, since it mocks `Process.clock_gettime`. Our own `ExpiringCache` specs needed exactly this rewrite. 3. **Measurement/report split.** Where a wall timestamp and a monotonic duration appear side by side (stream inits), `timestamp + durationMillis` will not equal the wall end time if a step occurred mid-measurement. The duration is the trustworthy value. One correction to the originating audit's framing: a Ruby `Time` is an absolute epoch instant (the zone affects display only), so `Time` subtraction was never DST-affected — Ruby's actual exposure was NTP/manual/VM clock steps. The fixes are identical either way. ## Review A three-agent review (general / security / adversarial) ran against these changes: no Critical or High findings. All seven adversarial attack vectors failed against production code — including an exhaustive 4096-sequence brute force showing the wall/monotonic stamps cannot diverge through any `update_status` input sequence, and a 3.1M-sample race probe of the status/duration pairing (zero violations, zero negative durations). Each new regression test was verified to fail against the pre-fix implementation. Review-driven hardening is included here (atomic accessor, nil-state guard, condition truth table, spec Timecop hygiene). Two pre-existing issues surfaced by the review are intentionally out of scope and will be ticketed separately: no minimum gap between `RepeatingTask` runs when a task overruns its interval (reachable via an unvalidated `status_poll_interval`), and interval type validation. ## Relationship to #448 The `RepeatingTask` rewrite in #448 removes the elapsed-time measurement entirely (fixed-delay semantics), which supersedes this PR's `repeating_task.rb` change. The fix here is deliberate insurance on main's current code: whichever PR lands second should resolve the conflict in #448's favor for that one file. No other files overlap. ## Precedent Same change-class as the .NET fixes in launchdarkly/dotnet-core#339 (propagated by launchdarkly/dotnet-core#343). ## Testing Full suite passes (1118 examples; the one failure, `launchdarkly-server-sdk_spec.rb`'s `Bundler.require` test, is pre-existing and environment-dependent — it shells out to a bare `ruby` outside the bundle — and touches nothing in this change). New regression tests simulate wall-clock steps for the scheduler, cache, migrator, status provider, and both stream-init paths. rubocop clean on all changed files. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Wall-clock steps (NTP, manual set, VM resync) could skew polling intervals, cache TTLs, migration latency, FDv2 synchronizer failover/recovery, and stream-init diagnostics. This PR routes **all internal interval and duration measurement** through new `Impl::Util.monotonic_seconds`, while keeping wall time for outward-facing timestamps like `Status#state_since`. > > **`StatusProviderV2`** tracks a monotonic twin of state entry time, exposes atomic `status_and_seconds_in_state`, and ignores `nil` state updates. **FDv2** uses that duration for the 60s / 10s / 300s fallback and recovery thresholds instead of `Time.now - state_since`. **FDv1 and FDv2 streaming** compute diagnostic `durationMillis` from monotonic stamps; reported init timestamps stay wall-clock. **`ExpiringCache`**, **`RepeatingTask`**, and **migration `Executor` latency** also switch to monotonic measurement (cache accepts an optional injectable `clock:` for tests). > > Regression specs cover wall-clock manipulation via Timecop, FDv2 condition boundaries, and global `Timecop.return` after each example. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 49aa78a. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
This branch has not been deployed
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
2 participants
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.
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.