Skip to content

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

Open
tanderson-ld wants to merge 1 commit into
mainfrom
ta/SDK-2789/retry-conformance-v2
Open

feat!: multi-strategy retry delay API for regime switching#110
tanderson-ld wants to merge 1 commit 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 reconnect delay so callers can register multiple RetryDelayStrategy instances and switch among them at runtime (activateRetryDelayStrategy), for LaunchDarkly RETRY regime switching.

RetryDelayStrategy.apply / Result are replaced by immutable snapshots: getDelayMillis(), getNext(), and optional withBaseDelayMillis for SSE retry: hints. DefaultRetryDelayStrategy now owns its initial delay (initialDelay(...)) and rolls jitter once at construction (ThreadLocalRandom).

Builder.retryDelayStrategy is additive: first call is the default (healthy-op reset target); later calls register extra strategies. Each keeps its own backoff state across activations. Wire retry: values stick across resets. Reconnect delay is computed at sleep time (full delay, no elapsed-time subtraction). getBaseRetryDelayMillis / getNextRetryDelayMillis are removed.

Reviewed by Cursor Bugbot for commit 8fc09bd. 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

@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 2 potential issues.

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 8fc09bd. Configure here.

// default strategy and zero every registered strategy's counter state.
logger.debug("Resetting retry delay strategy to initial state");
currentRetryDelayStrategy = defaultRetryDelayStrategy;
resetAllRegisteredStrategyState();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Healthy-op reset uses wrong duration

High Severity

Deferring reconnect-delay computation moved the healthy-op check to sleep time, but it still measures now - connectedTime. That includes the gap after disconnect, so a short connection plus a pause before reconnect can falsely reset backoff. The duration needs to reflect only the prior connection (for example via disconnectedTime - connectedTime), which matches the old fault-time behavior this PR intentionally deferred.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8fc09bd. Configure here.

nextBase = maxDelayMillis;
}
return new DefaultRetryDelayStrategy(nextBase, maxDelayMillis, backoffMultiplier, jitterMultiplier);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Max delay not applied to current base

Medium Severity

maxDelay is enforced only when building the successor in getNext(), not when constructing the current instance. So getDelayMillis() can exceed the configured maximum whenever initialDelay or withBaseDelayMillis (including sticky retry: hints) sets a base above max. The previous apply() path pinned every attempt, including the first.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8fc09bd. Configure here.

@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.

// Check if deliberatelyClosedConnection might have been set during that wait
if (deliberatelyClosedConnection) {
exception = new StreamClosedByCallerException();
// If interrupt(), stop(), or close() is called while we're waiting, we will

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This comment seems like it should be above the wait line.

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