Skip to content

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

Draft
jsonbailey wants to merge 2 commits into
mainfrom
jb/sdk-2792/retry-conformance
Draft

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

Conversation

@jsonbailey

@jsonbailey jsonbailey commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

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 now classified as either 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) engages a longer regime starting at 5 minutes and doubling to a 1-hour ceiling, and keeps retrying there until the condition clears.

Behavior changes worth calling out in release notes

  1. 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.
  2. DataSourceState.OFF is now reserved for explicit shutdown and unparseable configuration. HTTP errors produce INTERRUPTED.
  3. A rejected SDK key now logs at error roughly hourly, indefinitely, rather than once. That is deliberate: an SDK retrying with a rejected credential is consuming resources, and the condition needs a person to fix it.
  4. A server-initiated stream close now logs a warning where it previously logged nothing, and backs off rather than reconnecting immediately.
  5. Failure logs now state the actual delayReceived 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.

What changed

  • ldclient/impl/retry.py (new) — one RetryState serving all four data sources. Streaming and polling differ only by construction parameters and a ResetPolicy: AfterHealthyFor(60) for streaming, AfterConsecutiveSuccesses(2) for polling.
  • 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.
  • impl/repeating_task.py / impl/aio/concurrency.py — both primitives take a DelaySource (read-only, one property) instead of a fixed interval, with an at_interval factory for the ten fixed-interval callers. This replaces three copies of the same scheduling arithmetic — the two data sources hand-rolled their own loops — and both waits are now gap-based: the wait starts when the callback returns rather than when it began. A slow poll no longer eats its own backoff.
  • contract-tests/{service,async_service}.py — declare retry-conformance-fdv1-streaming and retry-conformance-fdv1-polling.

Not in scope

The event processor's _disabled permanent stop, and FDv2 (impl/datasourcev2/**, impl/datasystem/**). Both still stop permanently on an unexpected response and are tracked separately.

is_http_error_recoverable, http_error_message and check_if_error_is_recoverable_and_log are deprecated but retained, because those two components are their only remaining callers and both genuinely do still stop — rewording them now would make those log lines untrue.

Testing

make test: 1647 passed. make lint: clean across 226 files.

Contract tests pass, run out of band against harness v2.41.0 with -enable-long-running-tests:

  • Streaming: all eight retry-conformance subtests plus the ten legacy recoverable-error cases, zero failures. Includes enters extended-regime backoff after unexpected HTTP error and does not permanently stop under sustained unexpected HTTP errors.
  • Polling: 43 total, 14 skipped, 29 ran, all passed. Includes returns to normal-regime cadence after two consecutive successful polls.

Three things a reviewer should know about how that suite is gated:

  • The conformance scenarios need -enable-long-running-tests, which the Makefile does not pass, so they do not run in CI. They exercise real production timing, so the SDK sits silent for minutes at a stretch and a CI runner has to tolerate that.
  • Declaring the capability also deactivates the harness's legacy "do not retry" tests. So until the long-running suite runs somewhere automatically, contract coverage of retry behaviour is zero and CI stays green regardless.
  • The harness -run flag matches top-level group names only. -run 'retry' reports All tests passed / 14 total, 14 skipped, 0 ran with exit code 0 — a silent false pass worth knowing about before anyone wires a filtered run into CI.

Known open items

  • for_streaming does not clamp the extended initial delay up to initial_reconnect_delay, where for_polling does. With initial_reconnect_delay=600, an unexpected failure waits 300s while a normal one waits 600s — inverted. Reaching it needs a configuration the streaming spec already disallows (it says the value should not exceed 30 seconds) and which this SDK does not enforce, so it is not reachable in a supported setup. One-line fix, still being decided.
  • The SSE action loops have no except, so "a data source never stops" holds by virtue of ld_eventsource's internals rather than by construction. No reachable escape path was found at the pinned version, but a while True plus except Exception around each loop would make the guarantee ours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant