feat: conform FDv1 streaming and polling data sources to the RETRY spec - #2059
tanderson-ld wants to merge 4 commits into
Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
Drive the server SDK's FDv1 streaming and polling reconnection from the reusable retry controller via an injected retry-delay strategy: - Classify HTTP and transport failures per the RETRY table; no failure is terminal, so 401/403 now retry in the extended regime instead of permanently stopping. - Surface data-source failures as logs only, keeping the error-event channel reserved for terminal conditions. - Restart the stream and reschedule polling on malformed or empty payloads, recording them as normal failures and guarding against a duplicate reconnect when several arrive in one parse pass. - Wrap deserialization so structurally invalid data cannot escape and stall the data source. - Advertise FDv1 retry-conformance contract-test capabilities and add a nightly workflow that runs the long-running suite.
b77a558 to
f1af537
Compare
…classification Add unit coverage for RETRY behaviors that the shared contract tests do not exercise: - A scheduled reconnect (streaming) and the next poll (polling) are cancelled when the data source is stopped before the timer fires. - A transport error with no HTTP status is classified as a normal, retryable failure and logged at warn level, for both data sources. - A self-initiated stream restart after malformed data records exactly one failure, so the SDK's own close is not counted a second time.
… optional The shared EventSourceInitDict now makes initialRetryDelayMillis and retryResetIntervalMillis optional, since an injected retryDelayStrategy can replace the built-in backoff. The browser event source still uses the built-in backoff, so default those bounds to the standard streaming values (1s initial, 60s reset) where DefaultBackoff is constructed. The browser always supplies concrete values, so behavior is unchanged; the defaults only satisfy the widened type.
|
@launchdarkly/js-client-sdk size report |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ 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 9fbd27a. Configure here.
| agent: this._agent, | ||
| tlsParams: this._tlsOptions, | ||
| maxBackoffMillis: 30 * 1000, | ||
| jitterRatio: 0.5, |
There was a problem hiding this comment.
Streaming backoff options removed too early
High Severity
NodeRequests.createEventSource no longer sets maxBackoffMillis and jitterRatio, and FDv1 StreamingProcessor no longer passes initialRetryDelayMillis or retryResetIntervalMillis. The pinned launchdarkly-eventsource 2.2.0 ignores retryDelayStrategy and falls back to its built-in backoff, which those options used to configure. FDv2 streaming never injects a strategy and still depends on this layer for the 30s cap and jitter, so it loses exponential backoff even after the seam ships. Persistent stream failures can retry much more aggressively than the RETRY spec and the previous Node SDK behavior.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9fbd27a. Configure here.
This branch has not been deployed
1 participant
Summary
Wires the reusable retry controller (added to
@launchdarkly/js-sdk-commonin #2045) into the Node server SDK's FDv1 streaming and polling data sources, bringing them into conformance with the RETRY spec. Streaming injects the controller through the js-eventsourceretryDelayStrategyseam; polling drives the controller from its own scheduled loop. This replaces the old recoverable / permanent-stop model.Scope is FDv1 streaming + polling in the Node server SDK only. FDv2 (the composite data source and its synchronizers) is intentionally untouched and will be conformed in a future effort.
Behavior changes (operator-facing)
waitForInitialization()resolves on success and rejects only on its own timeout or onclose(); it no longer rejects immediately with "Authentication failed…" on a 401. A misconfigured client keeps retrying and stays uninitialized until the timeout.warnfor normal,error-level for unexpected/invalid-data), with the reconnect delay in the streamingonretryinginfo line. The'error'/'failed'events and init-rejection remain reserved for genuinely terminal conditions (today, only the untouched FDv2 composite reaches them).Versioning:
feat:/ minor for all affected packages — matching how Go (7.16.0), Java (7.16.0), and .NET (8.17.0) each shipped RETRY conformance (minor, behavior changes documented in the PR, not marked breaking).What's here
packages/shared/common—EventSourceRetryDelayStrategytype + optionalretryDelayStrategyon the platformEventSourceInitDict(the injection seam;initialRetryDelayMillis/retryResetIntervalMillismade optional since they're unused under injection).packages/shared/sdk-server—StreamingProcessorandPollingProcessorrewired to the controller (classification, always-retry, malformed-data recovery via a guarded close-and-recreate, completion-anchored polling).LDClientImplis intentionally left unchanged; the reserved'error'/'failed'/init-reject path is untouched.packages/sdk/server-node—NodeRequestspasses the strategy through; contract-test capabilitiesretry-conformance-fdv1-streaming/-polling; a nightly workflow (server-node-nightly.yaml) that runs the long-running RETRY suite (schedule + manual dispatch, no push trigger).Testing
Unit suites green:
@launchdarkly/js-server-sdk-common(1053 pass) and@launchdarkly/node-server-sdk(70 pass), covering the classification ladders (via the controller), the streaming close+recreate on malformed data (including a guard against a duplicate reconnect and a deserialization that throws), polling recovery,stop()-cancels-a-pending-retry interruptibility, status-less transport-error classification, single-failure accounting for a self-initiated restart, the reserved terminal'error'/'failed'/init-reject path, and the restoredonretryingdelay log. The behavioral RETRY conformance itself is validated by the harness in the nightly (long-running), which is a post-merge / manual-dispatch activity.Part of SDK-2790.
Note
Overview
Brings FDv1 streaming and polling in the Node server SDK in line with the RETRY spec by driving reconnect/poll timing from the shared
RetryStatecontroller instead of permanent-stop / recoverable-only logic.Streaming injects that state through a new platform
EventSourceRetryDelayStrategyonEventSourceInitDict;StreamingProcessorpasses it viaretryDelayStrategy, handles malformed payloads with a guarded close-and-recreate path, and always retries HTTP/transport failures (401/403 use extended backoff, logs only—no data-sourceerrorevents). Polling schedules the next request fromRetryStateafter each completion (including invalid JSON/deserialization failures) and no longer treats any HTTP status as terminal.Supporting changes: optional default backoff millis in the browser
EventSourcewhen init fields are omitted; NodecreateEventSourcestops overriding transport backoff caps; contract-test service advertisesretry-conformance-fdv1-*capabilities; a nightly workflow runs the long-running RETRY harness (not on every PR).Reviewed by Cursor Bugbot for commit 9fbd27a. Bugbot is set up for automated code reviews on this repo. Configure here.