Skip to content

feat!: multi-strategy retry delay API for regime switching - #110

Merged
tanderson-ld merged 4 commits into
mainfrom
ta/SDK-2789/retry-conformance-v2
Aug 26, 2026
Merged

feat!: multi-strategy retry delay API for regime switching#110
tanderson-ld merged 4 commits into
mainfrom
ta/SDK-2789/retry-conformance-v2

Conversation

@tanderson-ld

@tanderson-ld tanderson-ld commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Redesigns the retry-delay API on EventSource to support the LaunchDarkly RETRY-specification regime-switching pattern. Replaces the narrow setInitialRetryDelayMillis / setMaxRetryDelayMillis shape from the previous PR (#109) with a multi-strategy activation model.

Draft while the downstream consumer (java-server-sdk via java-core PR #200) is reworked to validate the new API end-to-end.

Tracks SDK-2789 under the RETRY-conformance epic SDK-2775.

Motivation

The previous PR's narrow setters had two fatal design smells:

  • EventSource.setMaxRetryDelayMillis had to reach through the abstract RetryDelayStrategy to a concrete DefaultRetryDelayStrategy via instanceof. Custom strategies got a silent no-op.
  • The apply(long baseDelayMillis) argument conflated wire retry hints with backoff progression, forcing every caller to pass a base value the strategy usually ignored.

The multi-strategy shape resolves both: no instanceof, no argument coupling, and per-strategy initial delays become expressible so an extended-regime strategy can start at 5 min while normal starts at 1 s.

What ships

RetryDelayStrategy (breaking)

  • apply(long) and Result are removed.
  • getDelayMillis() returns the delay for the current retry.
  • getNext() returns the successor instance (immutable-progression pattern).
  • withBaseDelayMillis(long) (default no-op) is the mutation channel for server-directed retry: hints. Custom strategies without a base concept opt out by not overriding.

DefaultRetryDelayStrategy

  • New initialDelay(long, TimeUnit) builder method for per-strategy initial delay.
  • Consolidated to a single baseDelayMillis field.
  • Jitter uses ThreadLocalRandom instead of SecureRandom (backoff jitter doesn't need cryptographic entropy).

EventSource

  • activateRetryDelayStrategy(RetryDelayStrategy) swaps the active registered strategy at runtime. Null / unregistered = silent no-op.
  • Builder.retryDelayStrategy(RetryDelayStrategy) has additive semantics: first call sets the default (initially active AND the healthy-op reset target); subsequent calls register additional strategies for later activation.
  • Each registered strategy retains its own backoff progression state across activations.
  • Server-directed retry: hints are stored in serverDirectedInitialDelayMillis and applied to every registered strategy's reset instance — sticky across activations, matching WHATWG semantics.
  • Reconnect-delay compute is deferred to sleep time so activation or wire hints received during the fault window affect the impending reconnect, not the one after.
  • Removed getBaseRetryDelayMillis() / getNextRetryDelayMillis(). The reconnect delay is observable via the "Waiting X milliseconds before reconnecting" log message.
  • Removed the historical delayNow = nextDelay - (now - disconnectedTime) subtraction — aligned with Go, .NET, and Swift SSE clients which sleep for the full computed delay.

Consumer example

```java
RetryDelayStrategy normal = RetryDelayStrategy.defaultStrategy()
.initialDelay(1, TimeUnit.SECONDS)
.maxDelay(30, TimeUnit.SECONDS);

RetryDelayStrategy extended = RetryDelayStrategy.defaultStrategy()
.initialDelay(5, TimeUnit.MINUTES)
.maxDelay(1, TimeUnit.HOURS);

EventSource es = new EventSource.Builder(...)
.retryDelayStrategy(normal) // first call = default
.retryDelayStrategy(extended) // second call = additional
.build();

// On extended-regime classification:
es.activateRetryDelayStrategy(extended);

// On healthy-op reset (revert to normal):
es.activateRetryDelayStrategy(normal);
```

Testing

  • All ~215 existing tests pass; jacoco coverage passes.
  • New tests in EventSourceRetryDelayStrategyUsageTest cover: activation swap, per-strategy state preservation across activations, healthy-op reset reverting to default, null/unregistered no-op.
  • Test observability of reconnect delays migrated from es.nextReconnectDelayMillis field reads to a readReconnectDelayFromLog() helper that consumes the info log.

Downstream

Consumed by java-core PR #200 for the Java Server SDK's RETRY-conformance work. That PR's CI will be red until this ships to Maven Central.


Note

Overview
Breaking redesign of SSE reconnect timing: retry configuration moves from Builder.retryDelay(...) and RetryDelayStrategy.apply(long) into immutable strategy instances (getDelayMillis() / getNext()), with optional initialDelay(...) on DefaultRetryDelayStrategy and wire overrides via withBaseDelayMillis(long).

EventSource now registers multiple strategies at build time (first call = default + healthy-op reset target; later calls = additional). Runtime switching uses activateRetryDelayStrategy, with per-strategy backoff state preserved across swaps. Server retry: hints are clamped (1h cap), applied to all registered strategies, and stay sticky through resets. Reconnect sleep is computed at sleep time (not at fault time), healthy-op duration uses connection length only, and getBaseRetryDelayMillis / getNextRetryDelayMillis are removed.

Contract tests and the full test suite migrate to retryDelayStrategy(defaultStrategy().initialDelay(...)) and log-based delay assertions.

Reviewed by Cursor Bugbot for commit e6f2675. Bugbot is set up for automated code reviews on this repo. Configure here.

Introduces a multi-strategy retry-delay API on EventSource so SDKs
adopting the LaunchDarkly RETRY specification can register normal- and
extended-regime strategies at build time and activate between them at
runtime. Server-directed retry: hints from the SSE wire remain sticky
across activations, matching WHATWG "reconnection time is set until
updated" semantics.

RetryDelayStrategy is now a snapshot-oriented immutable value: each
instance exposes getDelayMillis() for the current retry's delay and
getNext() for the successor instance. The single previous method
apply(long) is removed along with the Result wrapper class; the base
delay is no longer an out-of-band parameter but lives on the strategy
itself and is updated via the new withBaseDelayMillis(long) method
(default no-op for custom strategies without a base concept).

DefaultRetryDelayStrategy gains initialDelay(long, TimeUnit) so each
strategy can carry its own initial delay, letting normal- and
extended-regime strategies coexist with different starting points.

EventSource.activateRetryDelayStrategy(RetryDelayStrategy) swaps the
active strategy at runtime; each registered strategy retains its own
backoff progression state across activations. Passing null or an
unregistered strategy is a silent no-op. Builder.retryDelayStrategy has
additive semantics: the first call sets the default (initially active
and reset target); subsequent calls register additional strategies.

The reconnect-delay compute is deferred to sleep time so activation and
wire-hint changes received during the fault window take effect on the
impending reconnect, not the one after.

BREAKING CHANGE: RetryDelayStrategy.apply(long) is replaced by
getDelayMillis() + getNext() + withBaseDelayMillis(long). The Result
class is removed. Custom RetryDelayStrategy implementations must
migrate to the new abstract shape. EventSource no longer exposes
getBaseRetryDelayMillis() or getNextRetryDelayMillis(); observability
is via the "Waiting X milliseconds before reconnecting" log message.
tanderson-ld added a commit to launchdarkly/java-core that referenced this pull request Aug 21, 2026
…polling data sources (SDK-2789)

Guided by the server-sdk-guide.md in sdk-scratchpad; analogous to the
Go server SDK's reference implementation.

The behavioral change: HTTP responses that today cause a data source to
permanently stop (notably 401, 403, other 4xx) and TLS/certificate
validation failures are no longer terminal. Streaming enters an extended
backoff regime (5 min -> 1 hour, doubling); polling continues at its
configured cadence with extended-regime waits between failing polls.
Recovery from either regime uses a healthy-operation reset (60 s of
continuous connectivity for streaming; two consecutive successful polls
for polling).

Scope: FDv1 streaming and polling data sources under
`lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/`. FDv2 is out
of scope for this epic and is deferred to a future one; nothing in
`datasourcev2/` or the DataSystem-related code paths is touched.

Highlights:
- FailureClass enum + classifier helpers in
  launchdarkly-java-sdk-internal's HttpErrors: NORMAL for HTTP
  400/408/429 and 5xx and ordinary transport failures; UNEXPECTED for
  other 4xx (401/403/etc.) and TLS/certificate validation failures.
- PollingStrategy: new state-machine encapsulation with
  onFailure(class) / onSuccess() / nextWait() methods. State: n
  (formula input), initialDelay, maxDelay, priorPollWasSuccessful.
  Wait floor: max(pollInterval, T - J). Two-consecutive-successes
  returns from extended to normal regime.
- PollingProcessor: rewired to a self-driven loop using
  strategy.nextWait(). Removed the State.OFF permanent-stop path
  entirely; state stays INITIALIZING/INTERRUPTED with a lastError.
- StreamProcessor: consumes okhttp-eventsource's new multi-strategy
  retry API (see launchdarkly/okhttp-eventsource#110). On UNEXPECTED
  classification, activates the extended-regime RetryDelayStrategy on
  the underlying EventSource; the library's built-in healthy-op reset
  returns to normal-regime timing after 60 s of continuous
  connectivity.
- Constructor plumbing: PollingProcessor and StreamProcessor take
  extendedInitialReconnectDelay, extendedStreamMaxRetryDelay,
  retryResetInterval, and extendedInitialDelay as constructor
  parameters; package-private defaults threaded through
  ComponentsImpl.
- Contract test service: declares retry-conformance-fdv1-streaming
  and retry-conformance-fdv1-polling capabilities.

Tests:
- Unit tests: full test suite green. New coverage for classifier
  (HttpErrorsClassificationTest), strategy state machine
  (PollingStrategyTest), and extended-regime timing observation in
  StreamProcessorTest. Existing 401/403 tests rewritten to assert
  extended-regime retry rather than permanent stop.
- Contract tests via sdk-test-harness PR #404 (RETRY-conformance
  tests): 7/7 parallel shards pass end-to-end at production timing
  (5-minute extended-initial-delay), ~12 min wall clock.

CI: intentionally red on this PR until
launchdarkly/okhttp-eventsource#110 and
launchdarkly-java-sdk-internal 1.11.0 are released to Maven Central.
The multi-strategy retry API this SDK relies on is only in that PR's
branch, and the classifier helpers are only in the 1.11.0 branch.
Once released, bump both versions in lib/sdk/server/build.gradle.
@tanderson-ld
tanderson-ld marked this pull request as ready for review August 21, 2026 20:36
@tanderson-ld
tanderson-ld requested a review from a team as a code owner August 21, 2026 20:36
Comment thread src/main/java/com/launchdarkly/eventsource/EventSource.java

@jsonbailey jsonbailey left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks good. I'll hold off on approval until the cursor comments are addressed.

Comment thread src/main/java/com/launchdarkly/eventsource/EventSource.java Outdated
tanderson-ld added a commit to launchdarkly/java-core that referenced this pull request Aug 24, 2026
…polling data sources (SDK-2789)

Guided by the server-sdk-guide.md in sdk-scratchpad; analogous to the
Go server SDK's reference implementation.

The behavioral change: HTTP responses that today cause a data source to
permanently stop (notably 401, 403, other 4xx) and TLS/certificate
validation failures are no longer terminal. Streaming enters an extended
backoff regime (5 min -> 1 hour, doubling); polling continues at its
configured cadence with extended-regime waits between failing polls.
Recovery from either regime uses a healthy-operation reset (60 s of
continuous connectivity for streaming; two consecutive successful polls
for polling).

Scope: FDv1 streaming and polling data sources under
`lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/`. FDv2 is out
of scope for this epic and is deferred to a future one; nothing in
`datasourcev2/` or the DataSystem-related code paths is touched. The
classifier this depends on (`FailureClass` + `HttpErrors.classify*`)
lives in `launchdarkly-java-sdk-internal` and ships in its own PR.

Highlights:
- PollingStrategy: new state-machine encapsulation with
  onFailure(class) / onSuccess() / nextWait() methods. State: n
  (formula input), initialDelay, maxDelay, priorPollWasSuccessful.
  Wait floor: max(pollInterval, T - J). Two-consecutive-successes
  returns from extended to normal regime.
- PollingProcessor: rewired to a self-driven loop using
  strategy.nextWait(). Removed the State.OFF permanent-stop path
  entirely; state stays INITIALIZING/INTERRUPTED with a lastError.
- StreamProcessor: consumes okhttp-eventsource's new multi-strategy
  retry API (see launchdarkly/okhttp-eventsource#110). On UNEXPECTED
  classification, activates the extended-regime RetryDelayStrategy on
  the underlying EventSource; the library's built-in healthy-op reset
  returns to normal-regime timing after 60 s of continuous
  connectivity.
- Constructor plumbing: PollingProcessor and StreamProcessor take
  extendedInitialReconnectDelay, extendedStreamMaxRetryDelay,
  retryResetInterval, and extendedInitialDelay as constructor
  parameters; package-private defaults threaded through
  ComponentsImpl.
- DataSourceStatusProvider Javadocs: State.INITIALIZING, State.OFF,
  State.INTERRUPTED, and getStateSince OFF-case updated to reflect the
  new semantics (no HTTP-error -> OFF transition).
- LDClient constructor Javadoc: describes an SDK-key rejection as
  ongoing background retry rather than an "unsuccessful initialization"
  that reads as terminal.
- Contract test service: declares retry-conformance-fdv1-streaming
  and retry-conformance-fdv1-polling capabilities.

Tests:
- Unit tests: full test suite green. New coverage for the strategy
  state machine (PollingStrategyTest) and extended-regime timing
  observation in StreamProcessorTest. Existing 401/403 tests rewritten
  to assert extended-regime retry rather than permanent stop.
- Contract tests via sdk-test-harness PR #404 (RETRY-conformance
  tests): 7/7 parallel shards pass end-to-end at production timing
  (5-minute extended-initial-delay), ~12 min wall clock.

CI: intentionally red on this PR until
launchdarkly/okhttp-eventsource#110 releases okhttp-eventsource 5.0.0
and #204 releases launchdarkly-java-sdk-internal
1.11.0. The multi-strategy retry API this SDK relies on is only in
that eventsource PR's branch, and the classifier helpers are only in
that internal-artifact PR's branch. Once both are released, bump both
versions in lib/sdk/server/build.gradle.
- remove Builder.retryDelay(long, TimeUnit) in favor of
  retryDelayStrategy(defaultStrategy().initialDelay(...))
- reject null in Builder.retryDelayStrategy with IllegalArgumentException
- clamp server-directed retry hints at MAX_SERVER_DIRECTED_RETRY_DELAY_MILLIS
  (1 hour), matching the Go SDK's ApplyRetryTime
- treat null return from RetryDelayStrategy.getNext() as "reuse this"
  instance so naive migration from v4's Result.next=null does not NPE
- healthy-op reset now uses disconnectedTime - connectedTime instead of
  now - connectedTime, so consumer processing between fault delivery and
  the sleep-time compute does not spuriously reset backoff
- clamp base against maxDelay in DefaultRetryDelayStrategy constructor
  so the first getDelayMillis() respects the configured max, matching
  the pre-PR apply() behavior
- document race semantics on activateRetryDelayStrategy
- migrate all tests and the contract-test service off the removed
  retryDelay method

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ 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 4999d92. Configure here.

Comment thread src/main/java/com/launchdarkly/eventsource/DefaultRetryDelayStrategy.java Outdated
CI jacocoTestCoverageVerification flagged the throw branch in
Builder.retryDelayStrategy as uncovered.
Storing baseDelayMillis clamped against maxDelayMillis at construction
destroyed the caller's intent whenever a builder chain called
initialDelay before maxDelay (e.g. .initialDelay(5, MINUTES).maxDelay(1, HOURS)
on defaultStrategy(), which carries DEFAULT_MAX_DELAY_MILLIS=30s until
the maxDelay call replaces it). The first constructor invocation would
clamp base to 30s, and no subsequent call could recover it.

Preserve baseDelayMillis as-configured and apply the max clamp only when
computing delayMillis. getNext() continues to clamp on the successor as
before.
@tanderson-ld

Copy link
Copy Markdown
Contributor Author

Waiting to merge/release until java-server PR is done with review.

@tanderson-ld
tanderson-ld merged commit f19f3fd into main Aug 26, 2026
8 checks passed
@tanderson-ld
tanderson-ld deleted the ta/SDK-2789/retry-conformance-v2 branch August 26, 2026 20:34
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.

2 participants