feat: conform FDv1 streaming and polling to RETRY spec - #429
Open
tanderson-ld wants to merge 5 commits into
Open
feat: conform FDv1 streaming and polling to RETRY spec#429tanderson-ld wants to merge 5 commits into
tanderson-ld wants to merge 5 commits into
Conversation
Bring the FDv1 streaming and polling data sources into conformance with the RETRY specification: no HTTP response and no transport-level failure causes the data source to permanently stop. Previously-terminal 4xx statuses (401, 403, 405, other 4xx that aren't 400/408/429) and TLS/cert validation failures now trigger an extended-regime backoff and continue retrying indefinitely. Streaming: classifies each failure via classifyHTTPFailure / classifyTransportFailure into Normal or Unexpected. Unexpected failures activate an extended retry curve on the underlying eventsource stream (base 5 min, max 1 hour, doubling with jitter) instead of stopping. Sixty seconds of continuous healthy operation returns the source to the normal-regime curve. Polling: introduces pollingStrategy, an encapsulated RETRY §1.4 state machine (attempts / regime / rng) with a wait floor at PollInterval per RETRY §1.4.4. Unexpected classifications engage the extended regime with initialDelay = max(configured, PollInterval); two consecutive successful polls reset back to the normal regime per RETRY §1.8. The poll loop uses a dynamically-resettable timer to serve the state machine. Configuration surface (public builders): unchanged. New test-only knobs are exposed via <Builder>.Internal() escape hatches on both builders, with matching free-function wrappers in the new testhelpers/datasourcetest package. Production code must not import that package; it exists so that contract tests can compress the extended regime into an observable window. Testservice: adds ExtendedInitialDelayMS / ResetThresholdMS knobs on the streaming servicedef, ExtendedInitialDelayMS on polling, and declares the retry-conformance-fdv1-streaming and retry-conformance-fdv1-polling capabilities so the sdk-test-harness can drive the new conformance suite. go.mod / testservice/go.mod carry a temporary local replace directive pointing to ../eventsource so this branch can build against the unreleased RetryCurve API in eventsource PR #68. The TODO comment on the replace notes it must be removed once the eventsource release tagging that API ships.
The branch still depends on the unreleased RetryCurve API in eventsource PR #68 to compile, but committing the local replace directive to the PR would mask the fact that this PR cannot merge until that API is released. Reviewers should treat the pending CI failure ("undefined: eventsource. NewRetryCurve" etc.) as the intended signal. To iterate locally in the meantime, add a personal (uncommitted) replace github.com/launchdarkly/eventsource => ../eventsource to go.mod / testservice/go.mod. Once an eventsource release with the RetryCurve API is tagged, bump the eventsource require line here.
tanderson-ld
marked this pull request as ready for review
August 10, 2026 19:32
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| class := classifyHTTPFailure(statusCode) | ||
| if class == FailureClassUnexpected { | ||
| loggers.Errorf("Error %s (%s; will continue retrying with extended backoff): %s", | ||
| errorContext, willRetryMessage, errorDesc) |
There was a problem hiding this comment.
Contradictory polling retry logs
Low Severity
For unexpected polling failures, classifyAndLogHTTPFailure logs both pollingWillRetryMessage (“will retry at next scheduled poll interval”) and “will continue retrying with extended backoff”. After an unexpected failure the next wait comes from the extended regime, so the combined message is factually inconsistent.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit b68347d. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6d2b96e. Configure here.
tanderson-ld
changed the title
pollingStrategy.OnFailure previously incremented the counter unconditionally before applying the regime swap. A sequence of "normal, normal, unexpected" would leave the counter at 3 at the moment of the regime swap, producing a first-extended-regime wait of initialDelay * 2^2 = 20min rather than the intended 5min (bounded by extendedPollMaxDelay = 1h in the worst case). Fix: on the transition from normal into extended regime (detected via initialDelay == normalInterval at the moment of the unexpected failure), reset n to 1 so the formula yields initialDelay * 2^0 = initialDelay. RETRY §1.5.3 explicitly delegates the increment behavior on unexpected failure to the component's own specification, so this is spec-conformant. Also rename the field from `attempts` to `n` to (a) name the field for its role as the formula input (RETRY §1.4.1) and (b) match the naming convention used in the RETRY spec and streaming Confluence spec. Adds two regression tests: - TestPollingStrategy_UnexpectedAfterNormalFailuresStartsAtInitialDelay covers the specific case that was broken (normal, normal, normal, unexpected → first extended wait ∈ [2.5min, 5min]). - TestPollingStrategy_UnexpectedWhileAlreadyExtendedContinuesDoubling covers the counterpart (second unexpected in extended does NOT re-reset n; normal failure in extended increments n without exiting the regime). Contract-test scenarios in sdk-test-harness all begin with an unexpected failure as the first failure of the SDK's lifetime, so the bug was not triggered by the harness. Discovered during retro discussion.
tanderson-ld
force-pushed
the
ta/SDK-2788/retry-conformance-work
branch
from
August 11, 2026 17:29
6d2b96e to
a2972ad
Compare
The eventsource library renamed RetryCurve to RetryProfile in response to review feedback on launchdarkly/eventsource#68. Update the streaming data-source wire-up to consume the new API. - es.NewRetryCurve / RetryCurveBaseDelay / MaxDelay / Jitter → es.NewRetryProfile / RetryProfileBaseDelay / … - result.ActivateCurve → result.ActivateProfile - es.StreamOptionDefaultRetryCurve / RegisterRetryCurve → es.StreamOptionDefaultRetryProfile / RegisterRetryProfile - Local vars defaultCurve / extendedCurve → defaultProfile / extendedProfile - Comment references to "retry curve" / "extended-regime curve" → "profile" go.mod is intentionally left pinned at eventsource v1.10.0. CI will be red on this PR until eventsource releases the renamed API and go.mod is bumped, matching the sequencing the epic assumes.
Consumes the RetryProfile API introduced in launchdarkly/eventsource#68 and released as v1.13.0. This is the final piece of the SDK-2788 chain; CI on this PR should now go green. - go.mod: launchdarkly/eventsource v1.10.0 -> v1.13.0 - go.sum updated accordingly
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
1 participant
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
Brings the FDv1 streaming and polling data sources into conformance with the RETRY specification. Previously-terminal HTTP responses (401, 403, other 4xx that aren't 400/408/429) and TLS/certificate validation failures no longer cause a permanent stop — the data sources engage an extended-regime backoff and continue retrying indefinitely.
eventsourcestream (base 5 min → cap 1 h, doubling with jitter). Sixty seconds of continuous healthy operation reverts to the normal-regime curve.FDv2 adoption of RETRY is out of scope for this ticket (deferred to a future epic).
Public API
Unchanged. New test-only knobs are exposed via `.Internal()` escape hatches on the streaming and polling builders, with matching free-function wrappers in a new `testhelpers/datasourcetest` package. Production code must not import that package — it exists so the sdk-test-harness can compress the extended regime into an observable window during contract tests.
Testservice / contract tests
Test plan
Note
Overview
FDv1 streaming and polling no longer treat 401/403, most other 4xx (except 400/408/429), or TLS/certificate failures as permanent stops. Failures are classified as normal vs unexpected per the RETRY spec; unexpected cases switch to an extended backoff regime and keep retrying indefinitely (data source stays Interrupted, not Off).
Streaming bumps
github.com/launchdarkly/eventsourceto v1.13.0 and wires normal vs extended retry profiles (extended base ~5 min, cap ~1 h). Unexpected HTTP/transport errors activate the extended profile instead of closing the stream withCloseNow: true.Polling replaces the fixed-interval ticker with a
pollingStrategystate machine: exponential waits with jitter, a floor atPollInterval, extended regime on unexpected failures, and return to normal after two consecutive successful polls.Shared
FailureClasshelpers ininternal/datasource/helpers.goreplaceisHTTPErrorRecoverable/ “giving up permanently” logging. Client initialization on auth errors now typically ends withErrInitializationTimeoutwhile background retries continue; tests and testservice add internal knobs (ExtendedInitialDelay, retry reset) and retry-conformance capabilities for the harness.FDv2 data sources are unchanged in this PR.
Reviewed by Cursor Bugbot for commit 2d49623. Bugbot is set up for automated code reviews on this repo. Configure here.