feat: add a reusable retry state controller for RETRY-conformant backoff - #2045
Merged
Merged
Conversation
Contributor
|
@launchdarkly/js-sdk-common size report |
Contributor
|
@launchdarkly/js-client-sdk size report |
Contributor
|
@launchdarkly/js-client-sdk-common size report |
tanderson-ld
marked this pull request as ready for review
September 25, 2026 18:33
joker23
reviewed
Sep 25, 2026
Export the retry controller (RetryState, factories, and reset policies) from the package entry point so consumers can reach it. Clamp the normal regime at its ceiling when a configured initial delay exceeds it, matching the majority of the SDK fleet and the literal spec; the mandated extended floor stays. Document the config trust boundary, the server-directed hint precedence and ceiling bound, and the read-in-same-turn contract, and pin the hint-precedence and jitter-boundary behaviors and the reset-policy and default-cadence paths with tests. Part of SDK-2790.
Convert RetryState from an exported class to an interface plus a closure-backed createRetryState factory, per the prefer-interfaces guideline for publicly exposed types. State lives in closure locals, so the returned object is minifiable without the private-field convention and future mutators are additive. The forStreaming/forPolling factories return the interface; the reset-policy classes are unchanged, since the ResetPolicy interface already fronts them. No behavioral change. Part of SDK-2790.
Contributor
Author
|
Need to discuss file size impact with @joker23 |
joker23
reviewed
Sep 28, 2026
joker23
approved these changes
Sep 28, 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.
Summary
Adds a reusable retry controller to
@launchdarkly/js-sdk-commonthat owns retry tracking, health checking, and delay calculation for long-running components. This is the foundation for RETRY-spec conformance in the Node server SDK (SDK-2790); the data-source wiring that consumes it arrives in a follow-up PR. There are no consumers in this PR — the additions are unused by product code and exercised only by tests.The design ports the controller pattern from the Python server SDK (python-server-sdk#522, wired in #519), with deliberate differences noted below.
What's where
src/datasource/retry/— the retry mechanics: theRetryStateinterface and itscreateRetryStatefactory (the controller), theResetPolicyinterface with its two implementations (AfterHealthyForfor streaming's healthy-duration reset,AfterConsecutiveSuccessesfor polling's two-in-a-row reset), and theforStreaming/forPollingfactories that bind the standard values (1s→30s normal and 5min→1hr extended regimes; 60s healthy window; two-success polling reset) with warn-and-default validation of the configured delay.src/errors.ts— failure classification, placed beside the legacy helper it supersedes:FailureKind('normal' | 'unexpected'),classifyHttpStatus(400/408/429 and 5xx and non-error statuses are normal; any other 4xx is unexpected), andclassifyTransportFailure(always normal — in an all-HTTPS system certificate failures can't be reliably distinguished from transient faults).isHttpRecoverablenow delegates toclassifyHttpStatus— one table, no drift — and is documented as superseded; it stays undeprecated because the event-delivery pathway still legitimately consumes it until that pathway migrates.Design points for review
RetryStateis an interface, not a class.createRetryState(config)returns it, backed by a closure over local state;forStreaming/forPollingreturn the interface. This keeps the publicly exposed surface an interface (per the repo's prefer-interfaces guideline) and lets future mutators be added additively. TheResetPolicyimplementations stay classes, since theResetPolicyinterface already fronts them everywhere they are consumed.RetryStateorResetPolicytakes a timestamp. Time lives in exactly one place:AfterHealthyFor's constructor-injected clock, defaulting to a monotonic source (performance.now(), with aDate.nowclosure fallback for exotic runtimes). The controller itself holds no clock; its only injectable israndom, for deterministic jitter tests. (The parameter is namedclockrather than the codebase'stimeStamperdeliberately — it is not a timestamp source.)base >= max / 2**exponent) rather than scaling the base up, so nothing can overflow the ceiling — the same compare-before-shift structure as the .NET implementation, expressed in lossless power-of-two float math. A zero base (legal: the spec forbids flooring server-directed retry values) short-circuits, closing a0 × Infinity = NaNedge otherwise reachable when a zero-valued server-directed retry is followed by very many failures.unexpectedfailure MUST NOT be less than the component's initial delay").applyServerDirectedRetry(ms)— the SSEretry:entry point: sticky base that takes precedence over the regime's initial delay (including the extended regime's), doubling restarted, ceiling still applies, survives a healthy reset. Wire-level validation and the 1-hour cap live in the SSE library (launchdarkly/js-eventsource#40), not here.Testing
89 tests across three suites (584 package-wide, all green):
RetryState.test.ts(48) — exact delay ladders for both regimes under injected clock/random (including the 5m/10m/20m/40m/1h/1h extended ladder), regime transition/ratchet/re-arm, anchor-once healthy-stretch discrimination, reset-before-count ordering, fast-second-poll cadence, poll-interval wait floor under real jitter, jitter range with the maximal-draw boundary (the exact-T/2tie) and distinctness assertions, server-directed retry semantics (replace/clamp/persist/reject-invalid/later-wins, precedence over both regimes' initial delays,retry: 0staying finite through 1,100 failures), normal-ceiling clamp and regime-collapse cases, the extended-floor mandate under direct construction, flapping-never-ratchets, high-n robustness, and factory validation matrices.ResetPolicy.test.ts(8) — the policy seam directly: threshold boundary, anchor-once under repeated healthy reports, failure-clears, consecutive-success counting, and both default-clock closures (monotonic and theDate.nowfallback).errors.test.ts(33) — the full classification matrix including boundary and non-error statuses, transport classification, and parity pins on the legacyisHttpRecoverabletruth table through the delegation.The
src/datasource/retrymodule is at 100% statement, branch, function, and line coverage. Coverage was cross-checked against the Python, Java, Go, and .NET RETRY test suites; the one technique deliberately not ported is .NET'sBigInteger/randomized reference sweeps, which exist to exercise 64-bit integer shift surfaces that JS float arithmetic does not have.Note
Overview
Introduces a RETRY-spec-oriented retry controller in
@launchdarkly/js-sdk-commonas shared library code only—no data sources or SDKs call it yet; follow-up PRs will wire streaming/polling.Adds
src/datasource/retry/withRetryState(createRetryState,forStreaming,forPolling): exponential backoff with jitter, normal vs extended regimes afterunexpectedfailures, pluggableResetPolicy(healthy-for duration for streaming, consecutive successes for polling), andapplyServerDirectedRetryfor SSEretry:values.errors.tsgainsFailureKindplusclassifyHttpStatus/classifyTransportFailure;isHttpRecoverablenow delegates to the same rules.New retry APIs are re-exported from the datasource barrel and package
index. CI package size limit for common ESM rises 29 000 → 29 500 bytes. Coverage is 89 new tests across retry and error classification.Reviewed by Cursor Bugbot for commit 7f529cf. Bugbot is set up for automated code reviews on this repo. Configure here.