feat: Retry FDv1 data sources after unexpected errors instead of stopping

[beekld]

Summary

The FDv1 streaming and polling data sources no longer stop permanently on unexpected errors, such as a 401 or 403. They back off into a longer delay regime instead, until the source is healthy again.

FDv2 synchronizers are unchanged, consistent with Java, Go, and Python.

• Requires eventsource 5.0.0 for runtime retry strategy swapping.
• Declares the FDv1 retry conformance capabilities.
• Removes TaskExecutor.startRepeatingTask, now without a caller.

---

Note

Overview
FDv1 streaming and polling keep retrying after “unexpected” HTTP failures (e.g. 401/403) instead of stopping the data source, calling shutDown() on the sink, or blocking further start() calls.

Streaming wires normal and extended RetryDelayStrategy instances (okhttp-eventsource 5.0.0). On non-recoverable stream HTTP errors, the connection error handler calls activateRetryDelayStrategy(extendedRetryDelay) and always PROCEEDs with reconnect; the old connection401Error guard and permanent shutdown path are removed. Normal reconnect max delay is tightened to 30s; extended backoff starts at 5 minutes up to 1 hour.

Polling drops fixed startRepeatingTask intervals in favor of one-shot scheduleTask chains driven by new PollingRetryState: routine failures keep the poll interval; non-recoverable HTTP errors enter extended backoff until consecutive successes reset the regime.

Housekeeping: TaskExecutor.startRepeatingTask is removed from the interface and all executors/tests; contract test service advertises retry-conformance-fdv1-streaming and retry-conformance-fdv1-polling. FDv2 streaming synchronizer only updates retry builder API for eventsource 5.x (behavior unchanged per PR description).

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

[cursor]

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 4a6076d. Configure here.}

[cursor]

Stream never leaves extended backoff

Medium Severity

The connection error handler activates extendedRetryDelay on an unexpected HTTP failure and never activates normalRetryDelay again. After one 401 or 403, later reconnects stay on the 5-minute to 1-hour schedule, including ordinary disconnects after the stream has become healthy.

Additional Locations (1)

• launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/StreamingDataSource.java#L94-L100

 

^{Reviewed by Cursor Bugbot for commit 4a6076d. Configure here.}

[tanderson-ld]

We should deprecate isHttpErrorRecoverable and define a new function or classification util for expected / unexpected. I'm not sure that the mapping from recoverable/unrecoverable to expected/unexpected will hold long term and may lead to confusion down the road.

[tanderson-ld]

Is it possible to get past the stopped guard, a call to stop() occurs which clears and cancels the task, and then schedule task / task.set runs? I think this would be a race between scheduling the next task and close. Perhaps it doesn't matter because the request in flight race is unavoidable.

Will the response get processed? I think ConnectivityManager has generation counting protections.

[tanderson-ld]

I think you just need the stop check after delays / sleeps / IO, so you can probably get away with just putting it in the closure and making sure there is a close check for the response handling.

The close check happening after schedulePoll is called from start without a sleep / delay is what leads me to think it can live in a better spot.

[tanderson-ld]

I think there should be a close check in this callback.

[tanderson-ld]

That's a nice result.

[tanderson-ld]

The chain's continuation is the last statement after the result callback, and these callbacks run on OkHttp's dispatcher thread, outside AndroidTaskExecutor's error wrapper. If resultCallback.onSuccess/onError throws — or fetcher.fetch never invokes the callback at all (HttpFeatureFlagFetcher.fetch silently no-ops when ldContext == null, and its onFailure path calls callback.onError outside any try/catch) — schedulePoll is never reached and polling ends permanently: no log, no status change, flags frozen until restart. The removed startRepeatingTask was structurally immune to this since the next tick was already queued. It's also a RETRY spec 1.9.1/1.9.2 problem (work disposition must not stop the component).

Suggest rescheduling in a finally on both branches:

@Override
public void onError(Throwable e) {
    try {
        retryState.recordFailure(e);
        resultCallback.onError(e);
    } finally {
        schedulePoll(resultCallback, retryState.nextDelayMillis());
    }
}

plus some guard for the fetch-never-calls-back case (scheduling before dispatching the callback, or a watchdog). A unit test that throws a RuntimeException from the result callback of the first poll reproduces it: one poll observed, then silence forever.

[tanderson-ld]

What these two capabilities actually do at the harness version CI resolves today (v2 → v2.41.0): the streaming one only skips the two legacy "do not retry after unrecoverable HTTP error" client-side subtest groups (6 subtests removed, none added — the positive retry-conformance assertions are all registered under the server-side suites), and retry-conformance-fdv1-polling has no client-side consumer at all, so it gates nothing. Net effect: streaming loses coverage and the new polling behavior ships with zero end-to-end verification.

Declaring them is directionally right. Suggest (a) noting in the PR body that conformance is currently unverified by contract tests, and (b) tracking client-side positive retry subtests in the harness as a follow-up. (Separately, the harness capability doc claims TLS/cert failures trigger extended backoff — the merged RETRY spec classifies all transport failures as normal per 1.7.1 — so that doc needs a harness-side fix.)

[tanderson-ld]

This flips LDInvalidResponseCodeFailure.isRetryable() to true for 401/403/405 — app-visible via ConnectionInformation.getLastFailure() and LDStatusListener.onInternalFailure, and the deleted tests asserted the old false. Related fallout from removing the shutdown path, worth tidying in this PR or an immediate follow-up:

• DataSourceUpdateSink.shutDown() now has no production caller, so ConnectionMode.SHUTDOWN becomes an unreachable public enum value.
• Stale javadocs: DataSourceUpdateSink.shutDown ("if it receives an error such as HTTP 401"), DataSourceUpdateSinkV2, and ConnectionMode.SET_OFFLINE ("…as a result of failed authentication to LaunchDarkly").
• The actionable guidance log dropped ERROR→INFO and no longer includes the status code (the generic per-attempt error log still carries it, but the "Verify correct Mobile Key" guidance is now below ERROR).

Deserves deprecation annotations / doc updates and a CHANGELOG entry describing the behavior change.

[tanderson-ld]

Four tests covering the old 401 behavior are deleted here without replacements asserting the new behavior: nothing now verifies that a 401 retries rather than shutting down the sink, that the extended strategy is engaged, or that 5xx stays on the 30s normal ceiling. On the polling side, PollingRetryState — all of the new transition logic — has no tests, and PollingDataSourceTest.setupErrorResponse is defined but never called, so the error path / extended-regime entry / two-success reset have zero coverage.

Cheap additions that don't require waiting out real delays: direct unit tests on PollingRetryState (it's package-private), and a streaming test asserting assertFalse(dataSourceUpdateSink.shutDownCalled) + assertTrue(failure.isRetryable()) for a 401. For actual regime-timing tests, the extended constants would need to be injectable (e.g. a @VisibleForTesting constructor overload), since 2.5-minute minimum waits aren't unit-testable.