feat: Remove permanent failure modes from FDv1 following RETRY spec - #519
Merged
Merged
Conversation
jsonbailey
force-pushed
the
jb/sdk-2792/retry-conformance
branch
from
September 14, 2026 15:02
37cdb27 to
9413155
Compare
…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.
jsonbailey
force-pushed
the
jb/sdk-2792/retry-conformance
branch
from
September 18, 2026 15:39
6c277e3 to
8e11162
Compare
jsonbailey
marked this pull request as ready for review
September 18, 2026 16:56
jsonbailey
force-pushed
the
jb/sdk-2792/retry-conformance
branch
from
September 18, 2026 21:36
8e11162 to
e1f29df
Compare
tanderson-ld
self-requested a review
September 21, 2026 13:18
tanderson-ld
requested changes
Sep 21, 2026
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.
tanderson-ld
approved these changes
Sep 24, 2026
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.
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.
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
normalorunexpected. 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) 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
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. An SDK retrying with a rejected credential consumes 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.poll_intervalorinitial_reconnect_delaynow logs a warning and uses the documented default.poll_intervalwas silently clamped andinitial_reconnect_delaywas 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 becomesINTERRUPTED, and the wait is interruptible bystop(), which matters now that a wait can be an hour long.config.py/async_config.py— both intervals are validated.Configpreviously clampedpoll_intervalwithmax(), which letNaNandinfthrough, because every comparison againstNaNis false. ANaNinterval reachedEvent.wait()and the delay arithmetic. The 30-second minimum still applies on top of validation.impl/util.py—validate_positive_finite, beside the validatorsConfigalready imports, soconfigandretryshare it without either importing the other.impl/retry.py— uses the shared validator; the two configurable defaults now live inconfig.pynext toDEFAULT_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
impl/datasourcev2/**,impl/datasystem/**) still stops permanently on an unexpected response. Tracked as SDK-2776._disabledpermanent stop stays. No spec binding exists for it yet. Also SDK-2776.except. "A data source never stops" holds viald_eventsourceinternals 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 theMakefiledoes 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 viaclassify_http_status); backoff is applied on the polling/stream loops, logs include the actual retry delay, andstop()can interrupt waits (including long extended backoffs). Async SSE creation usessdk_managed_retry=Trueso the library does not sleep on its own.Behavioral shifts:
401/403/most other4xx→INTERRUPTEDand keep retrying (extended regime from ~5m up to 1h) instead ofOFFand giving up;OFFis terminal and only for explicit shutdown (in-flight work cannot update status afterOFF). Server-initiated stream closes are treated asStreamClosedErrorwith normal backoff, not ignored. Invalidpoll_interval/initial_reconnect_delaywarn and fall back toDEFAULT_*inconfig.py;RepeatingTaskcaps waits withTIMEOUT_MAX.Docs and
DataSourceStatesemantics are updated; contract-test services advertiseretry-conformance-fdv1-streamingandretry-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.