feat: Remove permanent failure modes from FDv1 following RETRY spec by jsonbailey · Pull Request #519 · launchdarkly/python-server-sdk

jsonbailey · 2026-09-10T14:37:13Z

BEGIN_COMMIT_OVERRIDE
feat: Retry indefinitely after a data source failure instead of stopping permanently in FDv1
fix: Warn and use the documented default for an invalid poll interval or initial reconnect delay
END_COMMIT_OVERRIDE

Summary

Brings the FDv1 streaming and polling data sources into conformance with the RETRY specification. No HTTP response and no transport-level failure stops a data source permanently any more.

Every failure is classified normal or unexpected. A normal failure retries on the existing curve — 1s doubling to a 30s ceiling for streaming, the poll interval for polling. An unexpected failure (401, 403, any other 4xx outside 400/408/429) moves to a longer regime starting at 5 minutes and doubling to a 1-hour ceiling, and keeps retrying there until the condition clears.

The retry state machine itself landed in #522; the delay-source plumbing landed in #521. This PR connects them to the four data sources and removes the permanent-stop paths.

Behaviour changes for release notes

A bad or revoked SDK key no longer fails fast. LDClient(config, start_wait=N), postfork(start_wait=N) and await client.start(start_wait=N) now block for the full start_wait and return with is_initialized() false, rather than returning at once. The SDK keeps retrying in the background.
DataSourceState.OFF is now reserved for explicit shutdown and unparseable configuration. HTTP errors produce INTERRUPTED.
A rejected SDK key now logs at error roughly hourly, indefinitely, rather than once. An SDK retrying with a rejected credential consumes resources, and the condition needs a person to fix it.
A server-initiated stream close now logs a warning where it previously logged nothing, and backs off rather than reconnecting immediately.
Failure logs now state the actual delay — Received HTTP error 401 (invalid SDK key) for stream connection - will retry in 300.0s. Previously the SDK said only "will retry", and the delay was logged separately at info by the SSE client, so it was invisible at default log levels.
An invalid poll_interval or initial_reconnect_delay now logs a warning and uses the documented default. poll_interval was silently clamped and initial_reconnect_delay was not checked at all.

What changed

impl/datasource/{streaming,async_streaming,polling,async_polling}.py — the permanent-stop paths are gone. Each failure is classified, the retry state advances, status becomes INTERRUPTED, and the wait is interruptible by stop(), which matters now that a wait can be an hour long.
config.py / async_config.py — both intervals are validated. Config previously clamped poll_interval with max(), which let NaN and inf through, because every comparison against NaN is false. A NaN interval reached Event.wait() and the delay arithmetic. The 30-second minimum still applies on top of validation.
impl/util.py — validate_positive_finite, beside the validators Config already imports, so config and retry share it without either importing the other.
impl/retry.py — uses the shared validator; the two configurable defaults now live in config.py next to DEFAULT_STREAM_URI.
impl/datasource/datasource_common.py, interfaces.py, client.py / async_client.py — docstrings and status handling updated for the above.
impl/aio/transport.py — the async transport's own retry is driven by the shared state.
contract-tests/ — both services declare the conformance capability.

Known gaps, deliberately out of scope

FDv2 (impl/datasourcev2/**, impl/datasystem/**) still stops permanently on an unexpected response. Tracked as SDK-2776.
The event processor's _disabled permanent stop stays. No spec binding exists for it yet. Also SDK-2776.
SSE action loops have no except. "A data source never stops" holds via ld_eventsource internals rather than by construction. No reachable escape path was found at the pinned version.

Testing

make test: 1681 passed. make lint: clean across 228 files.

Contract tests were run out of band against harness v2.41.0 — streaming's eight conformance subtests and polling's four all pass. Note the conformance scenarios need -enable-long-running-tests, which the Makefile does not pass, so they do not run in CI.

Note

Overview
Aligns FDv1 streaming and polling (sync and async) with the RETRY spec: failures no longer shut the data source down permanently.

Retry ownership moves from the SSE client into the SDK via shared RetryState (for_streaming / for_polling). Each failure is classified normal vs unexpected (HTTP via classify_http_status); backoff is applied on the polling/stream loops, logs include the actual retry delay, and stop() can interrupt waits (including long extended backoffs). Async SSE creation uses sdk_managed_retry=True so the library does not sleep on its own.

Behavioral shifts: 401/403/most other 4xx → INTERRUPTED and keep retrying (extended regime from ~5m up to 1h) instead of OFF and giving up; OFF is terminal and only for explicit shutdown (in-flight work cannot update status after OFF). Server-initiated stream closes are treated as StreamClosedError with normal backoff, not ignored. Invalid poll_interval / initial_reconnect_delay warn and fall back to DEFAULT_* in config.py; RepeatingTask caps waits with TIMEOUT_MAX.

Docs and DataSourceState semantics are updated; contract-test services advertise retry-conformance-fdv1-streaming and retry-conformance-fdv1-polling. Test coverage expands for the new retry, logging, and shutdown paths.

^{Reviewed by Cursor Bugbot for commit b2f719e. Bugbot is set up for automated code reviews on this repo. Configure here.}

…erage

…loor Streaming's operating cadence is zero rather than absent, so a healthy stream schedules no wait and a retry delay has no floor. Polling is the only data source that reads next_delay as a DelaySource, and its cadence floor keeps that from reaching zero. Make the seven observational properties private. No data source reads them; they are test instrumentation, and as public API they invite callers to attach logic to unsynchronized state.

…formance # Conflicts: # ldclient/impl/datasource/async_polling.py # ldclient/impl/datasource/polling.py # ldclient/testing/impl/datasource/test_async_polling.py

Config accepted any initial_reconnect_delay and clamped poll_interval with max(), which let NaN and inf through because every comparison against NaN is false. A NaN interval reaches Event.wait() and the delay arithmetic downstream. Both are now validated, warn, and fall back to the documented default; the poll interval keeps its 30s minimum on top. Move the check to impl/util.py as validate_positive_finite, next to the validators Config already imports, so config and retry share it without either importing the other. The retry factories keep their own call: a RetryState can be built without going through Config. Also name the delay bounds after the spec's ceiling vocabulary.

…e delay The exponent driver was _n and a second counter held the name attempts, which is what Requirement 1.4.1 calls the exponent driver. A reviewer reading self.attempts against the spec was reading the wrong field. The second counter is gone. It had no reader outside tests, not even a logger, and every test that used it recorded only normal failures -- where the two counters are equal by construction.

…formance # Conflicts: # ldclient/impl/retry.py # ldclient/testing/impl/test_retry.py

Config now validates both intervals, so the factories' guards are no longer the only check. They still matter -- a RetryState can be built without going through Config -- but the docstrings described the old state, where Config ignored initial_reconnect_delay and only clamped poll_interval.

…wait

Addresses review feedback on #519. A new stream connection now clears _interrupted_by_sdk. interrupt() is a no-op when the connection has already gone, so no Fault arrives to clear the flag and it could swallow the next genuine server close, losing one failure and skipping one backoff. Both streaming sources get a test that fails without the change. _connection_attempt_start_time is now read after the retry wait instead of predicted before it, so a clock change during the wait cannot skew the stream-init latency we report. Also drops the certificate-classification comment from all four data sources, says why the bare RetryDelayStrategy is passed to both SSE clients, and rewords AsyncPollingUpdateProcessor.stop() so it is clear that the wait after OFF only drains a poll already in flight.

Addresses review feedback on #519. AsyncPollingUpdateProcessor.stop() now carries one line saying why it waits before closing the transport. The previous wording read as if the processor kept working after OFF, when it is shutting down. The comment on the bare RetryDelayStrategy keeps only the fact a reader needs: the strategy must be passed, or the SSE client picks its own backoff.

Addresses review feedback on #519. Both FDv1 data source update sinks now ignore every status after OFF. A poll or stream connection that was still in flight when stop() ran could report VALID or INTERRUPTED afterwards, so a listener saw the data source come back from a shutdown it will never come back from. The latch lives in the sink rather than in each data source because a store write that fails reports INTERRUPTED from __monitor_store_update, which no data source can guard. Both sinks are built once per client and FDv1 never switches data sources, so OFF is terminal for their whole lifetime. StreamingUpdateProcessor declares _sse, so a stop() before the first run no longer raises AttributeError, and run() gives up if a stop landed before the client existed. Without that check stop() had nothing to close and the run went on to read a connection nobody was left to close. The action loop is also wrapped in try/finally, so a raise the loop does not catch can no longer leak the connection pool. The async source already had both, which is why only the sync one changes here. Both streaming sources now report OFF before teardown rather than after. OFF answers "will more data arrive?", not "is every socket closed?", so a slow close must not hold back the status that tells a waiter to give up. The DataSourceState.OFF docstring says what the state now guarantees.

Addresses TUI review feedback on 496830f.

Addresses TUI review feedback on 7001f39.

The shared helper took unbound TypeVars, which accepted any sink beside any store and returned a union, so it checked nothing. Splitting it into sink_or_store and async_sink_or_store lets each name its own sink and store types. main did not need this because the async caller sat in an unannotated method, whose body mypy skips. Real types surfaced a latent mismatch in _process_message: FEATURES and SEGMENTS are VersionedDataKindWithOrdering, and Mapping's key type is invariant, so the inferred dict did not satisfy init's parameter. The literal now carries the declared type.

…off (#2045) ## Summary Adds a reusable retry controller to `@launchdarkly/js-sdk-common` that 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](launchdarkly/python-server-sdk#522), wired in [#519](launchdarkly/python-server-sdk#519)), with deliberate differences noted below. ## What's where - **`src/datasource/retry/`** — the retry mechanics: the `RetryState` interface and its `createRetryState` factory (the controller), the `ResetPolicy` interface with its two implementations (`AfterHealthyFor` for streaming's healthy-duration reset, `AfterConsecutiveSuccesses` for polling's two-in-a-row reset), and the `forStreaming`/`forPolling` factories 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), and `classifyTransportFailure` (always normal — in an all-HTTPS system certificate failures can't be reliably distinguished from transient faults). `isHttpRecoverable` now delegates to `classifyHttpStatus` — 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 - **`RetryState` is an interface, not a class.** `createRetryState(config)` returns it, backed by a closure over local state; `forStreaming`/`forPolling` return the interface. This keeps the publicly exposed surface an interface (per the repo's prefer-interfaces guideline) and lets future mutators be added additively. The `ResetPolicy` implementations stay classes, since the `ResetPolicy` interface already fronts them everywhere they are consumed. - **Clockless seams.** No method of `RetryState` or `ResetPolicy` takes a timestamp. Time lives in exactly one place: `AfterHealthyFor`'s constructor-injected clock, defaulting to a monotonic source (`performance.now()`, with a `Date.now` closure fallback for exotic runtimes). The controller itself holds no clock; its only injectable is `random`, for deterministic jitter tests. (The parameter is named `clock` rather than the codebase's `timeStamper` deliberately — it is not a timestamp source.) - **Ceiling-bounded backoff, no exponent constant.** The delay computation compares the base against the ceiling scaled *down* (`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 a `0 × Infinity = NaN` edge otherwise reachable when a zero-valued server-directed retry is followed by very many failures. - **A configured delay above the normal ceiling clamps to it.** In the normal regime the ceiling wins, matching the literal spec (1.3.2 + 1.4.2) and the majority of the SDK fleet (Go, Java, .NET). The extended regime is the opposite: its bounds are floored at the configured delay, which spec requirement 1.5.4.1 mandates ("a delay or ceiling that applies after an `unexpected` failure MUST NOT be less than the component's initial delay"). - **`applyServerDirectedRetry(ms)`** — the SSE `retry:` 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](launchdarkly/js-eventsource#40)), not here. - **Post-success wait = operating cadence**, even while the retry state is raised — a recovering poller returns to schedule immediately rather than serving one more extended-regime wait. ## 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/2` tie) and distinctness assertions, server-directed retry semantics (replace/clamp/persist/reject-invalid/later-wins, precedence over both regimes' initial delays, `retry: 0` staying 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 the `Date.now` fallback). - `errors.test.ts` (33) — the full classification matrix including boundary and non-error statuses, transport classification, and parity pins on the legacy `isHttpRecoverable` truth table through the delegation. The `src/datasource/retry` module 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's `BigInteger`/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-common` as shared library code only—**no data sources or SDKs call it yet**; follow-up PRs will wire streaming/polling. > > Adds `src/datasource/retry/` with **`RetryState`** (`createRetryState`, `forStreaming`, `forPolling`): exponential backoff with jitter, normal vs **extended** regimes after `unexpected` failures, pluggable **`ResetPolicy`** (healthy-for duration for streaming, consecutive successes for polling), and **`applyServerDirectedRetry`** for SSE `retry:` values. **`errors.ts`** gains **`FailureKind`** plus **`classifyHttpStatus`** / **`classifyTransportFailure`**; **`isHttpRecoverable`** now 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. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 7f529cf. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>

jsonbailey added 3 commits September 9, 2026 17:24

feat: Remove permanent failure modes from FDv1 following RETRY spec

ae7581a

cleanup and simplify based on feedback

fa8d44b

reset attempt count on reset

9413155

jsonbailey force-pushed the jb/sdk-2792/retry-conformance branch from 37cdb27 to 9413155 Compare September 14, 2026 15:02

jsonbailey added 10 commits September 14, 2026 13:49

refactor: Report the next delay through the property, not a return value

30c2f08

chore: Remove spec citations and passthrough test helpers

23474a7

fix: Address review findings on input guards, next delay and test cov…

fbd8f85

…erage

addressing feedback

da72713

Merge remote-tracking branch 'origin/main' into jb/sdk-2792/retry-con…

e51858c

…formance # Conflicts: # ldclient/impl/datasource/async_polling.py # ldclient/impl/datasource/polling.py # ldclient/testing/impl/datasource/test_async_polling.py

Merge remote-tracking branch 'origin/main' into jb/sdk-2792/retry-con…

ea5d3c5

…formance # Conflicts: # ldclient/impl/retry.py # ldclient/testing/impl/test_retry.py

jsonbailey force-pushed the jb/sdk-2792/retry-conformance branch from 6c277e3 to 8e11162 Compare September 18, 2026 15:39

jsonbailey marked this pull request as ready for review September 18, 2026 16:56

jsonbailey requested a review from a team as a code owner September 18, 2026 16:56

fix: Validate data source intervals in the retry state and bound the …

e1f29df

…wait

jsonbailey force-pushed the jb/sdk-2792/retry-conformance branch from 8e11162 to e1f29df Compare September 18, 2026 21:36

jsonbailey added 2 commits September 18, 2026 16:54

refactor: Drop the unused ceiling parameter from the delay validator

0047a65

fix: Measure the start_wait tests against a monotonic clock and a bound

30bb8ec

tanderson-ld self-requested a review September 21, 2026 13:18

tanderson-ld requested changes Sep 21, 2026

View reviewed changes

jsonbailey added 3 commits September 22, 2026 12:47

tanderson-ld mentioned this pull request Sep 24, 2026

feat: add a reusable retry state controller for RETRY-conformant backoff launchdarkly/js-core#2045

Merged

jsonbailey added 2 commits September 24, 2026 11:04

fix: Shorten the teardown comment and say why two statuses follow OFF

7001f39

Addresses TUI review feedback on 496830f.

fix: Reword the teardown and latch test comments

816df7a

Addresses TUI review feedback on 7001f39.

jsonbailey requested a review from tanderson-ld September 24, 2026 16:43

tanderson-ld approved these changes Sep 24, 2026

View reviewed changes

jsonbailey merged commit e17e173 into main Sep 25, 2026
16 checks passed

jsonbailey deleted the jb/sdk-2792/retry-conformance branch September 25, 2026 22:29

Navigation Menu

feat: Remove permanent failure modes from FDv1 following RETRY spec - #519

feat: Remove permanent failure modes from FDv1 following RETRY spec#519
jsonbailey merged 22 commits into
mainfrom
jb/sdk-2792/retry-conformance

jsonbailey commented Sep 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jsonbailey commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Behaviour changes for release notes

What changed

Known gaps, deliberately out of scope

Testing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jsonbailey commented Sep 10, 2026 •

edited

Loading