feat: Remove permanent failure modes from FDv1 following RETRY spec - #519
Draft
jsonbailey wants to merge 2 commits into
Draft
feat: Remove permanent failure modes from FDv1 following RETRY spec#519jsonbailey wants to merge 2 commits into
jsonbailey wants to merge 2 commits into
Conversation
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
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 other4xxoutside400/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
LDClient(config, start_wait=N),postfork(start_wait=N)andawait client.start(start_wait=N)now block for the fullstart_waitand return withis_initialized()false, rather than returning at once. The SDK keeps retrying in the background.DataSourceState.OFFis now reserved for explicit shutdown and unparseable configuration. HTTP errors produceINTERRUPTED.errorroughly 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.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 atinfoby the SSE client, so it was invisible at default log levels.What changed
ldclient/impl/retry.py(new) — oneRetryStateserving all four data sources. Streaming and polling differ only by construction parameters and aResetPolicy: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 becomesINTERRUPTED, and the wait is interruptible bystop(), which matters now that a wait can be an hour.impl/repeating_task.py/impl/aio/concurrency.py— both primitives take aDelaySource(read-only, one property) instead of a fixed interval, with anat_intervalfactory 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— declareretry-conformance-fdv1-streamingandretry-conformance-fdv1-polling.Not in scope
The event processor's
_disabledpermanent 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_messageandcheck_if_error_is_recoverable_and_logare 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:enters extended-regime backoff after unexpected HTTP erroranddoes not permanently stop under sustained unexpected HTTP errors.43 total, 14 skipped, 29 ran, all passed. Includesreturns to normal-regime cadence after two consecutive successful polls.Three things a reviewer should know about how that suite is gated:
-enable-long-running-tests, which theMakefiledoes 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.-runflag matches top-level group names only.-run 'retry'reportsAll tests passed / 14 total, 14 skipped, 0 ranwith exit code 0 — a silent false pass worth knowing about before anyone wires a filtered run into CI.Known open items
for_streamingdoes not clamp the extended initial delay up toinitial_reconnect_delay, wherefor_pollingdoes. Withinitial_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.except, so "a data source never stops" holds by virtue ofld_eventsource's internals rather than by construction. No reachable escape path was found at the pinned version, but awhile Trueplusexcept Exceptionaround each loop would make the guarantee ours.