Skip to content

feat(server): RETRY-spec conformance in FDv1 streaming and polling - #200

Open
tanderson-ld wants to merge 9 commits into
mainfrom
ta/SDK-2789/retry-conformance-work
Open

feat(server): RETRY-spec conformance in FDv1 streaming and polling#200
tanderson-ld wants to merge 9 commits into
mainfrom
ta/SDK-2789/retry-conformance-work

Conversation

@tanderson-ld

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

Copy link
Copy Markdown
Contributor

Summary

Implements RETRY-spec conformance in the Java server SDK's FDv1 streaming and polling data sources (SDK-2789). This PR is scoped to the server SDK only; the classifier helpers it consumes ship separately in #204.

Behavioral change. HTTP responses that today cause an FDv1 data source to permanently stop (notably 401 / 403 / other 4xx) and TLS / certificate validation failures are no longer terminal. Streaming enters an extended-regime backoff (5 min initial → 1 hr max, doubling); polling continues at its configured cadence but engages the extended-regime wait after an UNEXPECTED failure. Either regime returns to normal after 60 s of continuous healthy operation (streaming) or two consecutive successful polls (polling).

Scope. FDv1 streaming and polling under lib/sdk/server/. FDv2, event delivery, and other network callers are unchanged.

What changed

  • PollingStrategy — New state-machine encapsulation with onFailure(class) / onSuccess() / nextWait(). State: n (formula input), initialDelay, maxDelay, priorPollWasSuccessful. Wait floor is max(pollInterval, T − J); two consecutive successes reset from extended → normal.
  • PollingProcessor — Rewired to a self-driven loop that consults strategy.nextWait() between attempts. The State.OFF permanent-stop path is removed; the state stays INITIALIZING or INTERRUPTED with a lastError.
  • StreamProcessor — Consumes okhttp-eventsource's new multi-strategy retry API from launchdarkly/okhttp-eventsource#110. On UNEXPECTED classification it calls activateRetryDelayStrategy on the underlying EventSource to switch into the extended-regime RetryDelayStrategy. The library's built-in healthy-op reset returns the SDK to normal-regime timing after 60 s of continuous connectivity.
  • DataSourceStatusProvider docsState.INITIALIZING, State.OFF, State.INTERRUPTED, and getStateSince OFF-case Javadocs updated to reflect the new semantics (no HTTP-error → OFF transition). Aligned with the Go server SDK's parallel doc adjustments.
  • LDClient constructor Javadoc — Wording tightened so a "wrong SDK key" scenario is described as ongoing retry in the background, not as an "unsuccessful initialization" that reads as terminal.
  • Contract-test service — Declares retry-conformance-fdv1-streaming and retry-conformance-fdv1-polling capabilities.

The classifier this SDK depends on — FailureClass and HttpErrors.classify* helpers — ships in #204.

Testing

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

Test plan for reviewers

  • Verify PollingStrategy transition semantics: normal → extended fires exactly once per UNEXPECTED failure; two consecutive successful polls fully reset (n = 0, delay bounds back to normal, inExtended cleared so a subsequent UNEXPECTED re-triggers the transition).
  • Verify StreamProcessor.handleError ordering: classifier → regime switch → updateStatus(INTERRUPTED, …), unconditionally returning true so the eventsource keeps retrying.
  • Review the DataSourceStatusProvider Javadoc changes for accuracy vs. the current state machine.
  • Code comments deliberately describe current behavior only — no spec section refs, no historical framing ("previously", "no longer"), no cross-SDK references. Confirm you'd expect a reader to find that acceptable.

Dependencies (why CI is red)

Two unreleased upstream artifacts:

Once both are released, bump their versions in lib/sdk/server/build.gradle and CI will go green. Locally, the branch builds against mavenLocal() snapshots of both.


Note

Overview
FDv1 data sources no longer treat HTTP auth and other “unexpected” failures as terminal. Streaming and polling keep retrying in the background with classified backoff instead of moving to State.OFF (e.g. 401/403).

Polling switches from fixed-rate scheduling to a self-driven loop backed by new PollingStrategy: normal cadence at pollInterval, extended exponential backoff (default 5 min → 1 hr cap, jitter) after FailureClass.UNEXPECTED, reset after two consecutive successes.

Streaming adopts okhttp-eventsource 5.x multi-regime RetryDelayStrategy: normal reconnect caps at 30s; on unexpected failures the SDK activates an extended strategy (5 min → 1 hr) and relies on a 60s healthy-connection threshold to return to normal timing.

Public docs and tests align with the new model: DataSourceStatusProvider Javadoc and LDClient init wording no longer describe HTTP errors as permanent shutdown; contract-test service advertises retry-conformance capabilities; unit/e2e tests assert continued retry and extended-regime behavior. Dependencies bump launchdarkly-java-sdk-internal and okhttp-eventsource for HttpErrors classification and strategy APIs.

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

@tanderson-ld
tanderson-ld force-pushed the ta/SDK-2789/retry-conformance-work branch from f87cd37 to a754251 Compare August 21, 2026 20:32
@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
@tanderson-ld tanderson-ld changed the title feat(server): RETRY-spec conformance in FDv1 streaming and polling (SDK-2789) feat(server): RETRY-spec conformance in FDv1 streaming and polling 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. 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.
@tanderson-ld
tanderson-ld force-pushed the ta/SDK-2789/retry-conformance-work branch from 9394a65 to 6d0c138 Compare August 24, 2026 13:26
@tanderson-ld tanderson-ld changed the title feat(server): RETRY-spec conformance in FDv1 streaming and polling feat(server): RETRY-spec conformance in FDv1 streaming and polling (SDK-2789) Aug 24, 2026
@tanderson-ld

Copy link
Copy Markdown
Contributor Author

CI Failures will be resolved once okhttp-eventsource and internal release. But waiting on initial review in this PR to make sure all the APIs on okhttp-eventsource and internal aren't going to need changes.

Aligns with StreamProcessor.close() and FDv2DataSource.close(), both of
which call updateStatus(State.OFF, null) after closing their upstream
I/O. Also aligns with the PR's updated Javadoc for State.OFF, which now
describes it as the time the data source "stopped operation" (i.e.,
close), rather than the pre-PR meaning of "encountered an unrecoverable
error".
tanderson-ld added a commit that referenced this pull request Aug 25, 2026
…rrors (SDK-2789) (#204)

## Summary

Introduces a shared network-failure classifier in
`launchdarkly-java-sdk-internal` that categorizes failures as either
**NORMAL** (typically transient) or **UNEXPECTED** (indicative of a
longer-lived condition — e.g., invalid SDK key, TLS misconfiguration).
Downstream SDKs can use the classification to select between
normal-regime and extended-regime backoff.

Enables the server SDK's RETRY-spec conformance work in
[SDK-2789](https://launchdarkly.atlassian.net/browse/SDK-2789) (see PR
#200), which needs these helpers to select its
retry regime.

## What changed

- **`FailureClass` enum** — `NORMAL` and `UNEXPECTED`, with a
package-private cause-chain scan for TLS / certificate exceptions.
- **`HttpErrors.classifyHTTPFailure(int)`** — Returns `NORMAL` for 400 /
408 / 429, 5xx, and any other status the SDK treats as a failure;
returns `UNEXPECTED` for other 4xx (401 / 403 / etc.).
- **`HttpErrors.classifyTransportFailure(Throwable)`** — Returns
`UNEXPECTED` if TLS or certificate validation appears anywhere in the
exception chain; `NORMAL` otherwise.
- **`HttpErrors.classifyAndLogHTTPFailure` /
`classifyAndLogTransportFailure`** — Classify, log at the appropriate
level (Error for `UNEXPECTED`, Warn for `NORMAL`), and return the
classification.
- **`HttpErrors.isHttpErrorRecoverable` and
`checkIfErrorIsRecoverableAndLog` are now `@Deprecated`.** The boolean
"give up permanently" contract doesn't fit callers that keep retrying
regardless of classification; the Javadoc points migrations at the new
helpers. Existing callers keep working — deprecation is
source-compatible.

## Testing

- New `HttpErrorsClassificationTest` covers the full HTTP status matrix
(400/408/429/5xx = NORMAL, 401/403/404/418/451 = UNEXPECTED,
non-4xx/non-5xx = NORMAL) and the transport-exception matrix (ordinary
I/O = NORMAL, SSL/certificate = UNEXPECTED, TLS as a cause of a wrapper
exception = UNEXPECTED).
- Existing `lib/shared/internal` tests continue to pass; the deprecated
methods still exercise their original behavior.

## Test plan for reviewers

- [ ] Confirm the 4xx / 5xx split (400 / 408 / 429 = NORMAL, other 4xx =
UNEXPECTED) matches expectations.
- [ ] Confirm transport-level TLS / certificate failures at any depth in
the cause chain are surfaced as `UNEXPECTED`.
- [ ] Sanity-check the deprecation Javadocs — they name the migration
target concretely and give a one-line reason (existing boolean contract
vs. new tri-state usage).

## Downstream

Once this PR merges and release-please publishes
`launchdarkly-java-sdk-internal 1.11.0`, launchdarky/java-core#200 will
bump its `build.gradle` dep to 1.11.0 and consume the classifier from
the server SDK's data sources.

[SDK-2789]:
https://launchdarkly.atlassian.net/browse/SDK-2789?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> Adds shared **internal** network failure classification so data
sources can pick **normal** vs **extended** backoff instead of treating
some errors as “stop retrying forever.”
> 
> Introduces a `FailureClass` enum (`NORMAL` vs `UNEXPECTED`) and
`HttpErrors` helpers to classify HTTP status codes (e.g. 400/408/429 and
5xx → `NORMAL`; most other 4xx including 401/403 → `UNEXPECTED`) and
transport exceptions (TLS/certificate causes anywhere in the chain →
`UNEXPECTED`). New `classifyAndLogHttpFailure` /
`classifyAndLogTransportFailure` classify, log at **Error** vs **Warn**,
and return the class for the caller.
> 
> `isHttpErrorRecoverable` and `checkIfErrorIsRecoverableAndLog` are
**deprecated** (behavior unchanged); Javadoc points callers at the new
APIs. Unit tests cover the HTTP and transport classification matrices.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
f67fd5e. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

@jsonbailey jsonbailey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed this alongside #204 and okhttp-eventsource#110 to check the API assumptions against the source rather than take the description's word for them. Broadly this holds up well — I specifically tried to break the two-consecutive-successes reset, the activateRetryDelayStrategy timing, and the polling loop lifecycle, and all three survived. The #110 integration is correct on every point I checked, including the one I most expected to be wrong: activation is a pointer swap that does not reset progression, so repeated 401s really do double 5→10→20→40→60 min. Using an explicit inExtended flag instead of comparing initialDelay == normalInterval avoids a real bug, and nInExtendedDoublesEvenWhenPollIntervalEqualsExtendedInitial is a good regression test for exactly that case.

I have a longer list of medium/low observations I'll hold for now. Two things I think block merge:

1. Won't compile against #204 HEAD. classifyAndLogHTTPFailure was renamed to classifyAndLogHttpFailure in #204's f67fd5e ("address review feedback on classifier helpers"). This branch predates that rename, and the local mavenLocal() snapshot is what hides it. Two call sites, inline below. Calling it out separately because it is not the unreleased-dependency-pin issue the description covers — it survives releasing both artifacts.

2. Worth one test before deciding anything else: the TLS classification may be far broader than intended. FailureClass.hasTlsOrCertificateCause (in #204) returns UNEXPECTED for any SSLException or GeneralSecurityException anywhere in the cause chain, and StreamIOException sets the underlying IOException as its cause, so the chain is reachable from StreamProcessor.handleError. Since all LD traffic is HTTPS, every transport error against the real endpoint arrives through the TLS layer — so SSLHandshakeException: Remote host terminated the handshake from a load balancer or proxy dropping mid-handshake reads as UNEXPECTED and pushes streaming into 5 min–1 hr backoff for a transient blip. The compounding part: a flapping stream never accumulates the 60 s of continuous connectivity the healthy-op reset needs, so it cannot climb back out.

I could not settle whether JSSE actually throws SSLException for these cases (no JVM available where I was reviewing), and about 20 lines answers it: open an SSLServerSocket, complete the handshake, then setSoLinger(true, 0) + close(), and read from the client — observe the exception type. Worth running before deciding: if the classification is as broad as it reads, this is the largest production-impact item in the change; if it isn't, the test costs nothing. Narrowing to SSLHandshakeException / SSLPeerUnverifiedException / CertificateException, or excluding causes that are themselves SocketException / SocketTimeoutException, would settle it either way.

On your test plan: the PollingStrategy transition semantics and the handleError ordering both check out — I traced the reset across intervening-failure and mixed NORMAL/UNEXPECTED orderings and found no off-by-one, and inExtended is cleared so a later UNEXPECTED re-fires the transition. No objection to the comment style.

Comment thread lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/PollingProcessor.java Outdated
Comment thread lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/StreamProcessor.java Outdated
tanderson-ld added a commit to launchdarkly/okhttp-eventsource that referenced this pull request Aug 26, 2026
## 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](#109))
with a **multi-strategy activation** model.

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

Tracks [SDK-2789](https://launchdarkly.atlassian.net/browse/SDK-2789)
under the RETRY-conformance epic
[SDK-2775](https://launchdarkly.atlassian.net/browse/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](launchdarkly/java-core#200) for the Java
Server SDK's RETRY-conformance work. That PR's CI will be red until this
ships to Maven Central.

[SDK-2789]:
https://launchdarkly.atlassian.net/browse/SDK-2789?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[SDK-2775]:
https://launchdarkly.atlassian.net/browse/SDK-2775?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

<!-- CURSOR_SUMMARY -->
---

> [!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.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
e6f2675. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
The classifier helper was renamed from classifyAndLogHTTPFailure to
classifyAndLogHttpFailure in review of the internal-artifact PR. This branch
was written against the earlier revision, so it would have failed compileJava
once internal 1.11.x published -- independent of the unreleased-dependency
pins, since it survives releasing both artifacts.

Two call sites: StreamProcessor.handleError and PollingProcessor.poll.
@tanderson-ld tanderson-ld changed the title feat(server): RETRY-spec conformance in FDv1 streaming and polling (SDK-2789) feat(server): RETRY-spec conformance in FDv1 streaming and polling Aug 27, 2026
PollingProcessor.close() now reports State.OFF, so it dereferences the
update sink. The two builder-configuration tests construct a processor from
a bare ClientContext, whose getDataSourceUpdateSink() returns null, then
dispose it via try-with-resources -- which NPE'd.

A null sink is not a supported state: every SDK path that builds a data
source calls withDataSourceUpdateSink() first (FDv1DataSystem,
FDv2DataSystem), that method is package-private so callers outside the SDK
cannot populate it, and poll() dereferences the sink unconditionally, so
such a processor could never run anyway. These tests were relying on
close() incidentally not touching the sink.

Supplies the sink instead, matching the equivalent tests in
StreamProcessorTest, which have always done this because
StreamProcessor.close() has always reported OFF.
@tanderson-ld

tanderson-ld commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @jsonbailey — both blocking items handled.

The classifyAndLogHttpFailure rename is fixed in 14de006 (both call sites), and d345b08 fixes a builder-test regression the earlier State.OFF-on-close change introduced. On the TLS breadth: you were right, and it reproduces — a peer closing mid-handshake with a FIN yields SSLHandshakeException: Remote host terminated the handshake (caused by EOFException) and classified UNEXPECTED, while the same fault as a RST yields SocketException and classified NORMAL. It's also a divergence from Go, which matches only certificate errors; the fix is in #206, narrowing to CertificateException / CertPathValidatorException / CertPathBuilderException / SSLPeerUnverifiedException.

Bugbot's two-consecutive-successes latency comment is resolved as working-as-specified per the polling spec — detail in that thread.

Still blocked on releases of #206 and launchdarkly/okhttp-eventsource#110 before the version pins can drop and CI can compile — note that means no tests run here today, which is how the builder-test regression stayed hidden. Happy to take the medium/low list whenever.

@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 92b58e1. Configure here.

tanderson-ld added a commit that referenced this pull request Aug 28, 2026
…#206)

## Summary

`FailureClass.hasTlsOrCertificateCause` matched any `SSLException`
**or** `GeneralSecurityException` anywhere in the cause chain. Since all
SDK traffic is HTTPS, every transport error arrives through the TLS
layer — so this swept in transient faults unrelated to certificate
validity and classified them `UNEXPECTED`, pushing data sources into
extended-regime backoff (5 min – 1 hr).

Found during review of #200 by @jsonbailey, who
flagged the breadth but could not run a JVM to confirm what JSSE
actually throws. Confirmed empirically below.

## The problem, measured

Against a real `SSLServerSocket`:

| Scenario | JSSE exception | Old classification |
|---|---|---|
| Peer sends **FIN** mid-handshake | `SSLHandshakeException: Remote host
terminated the handshake`<br>← caused by `EOFException: SSL peer shut
down incorrectly` | **UNEXPECTED** ❌ |
| Peer sends **RST** mid-handshake | `SocketException: Broken pipe` |
NORMAL ✓ |
| Untrusted certificate chain | `SSLHandshakeException` →
`ValidatorException` → `SunCertPathBuilderException` | UNEXPECTED ✓ |

The regime therefore depended on whether an intermediary sent FIN or RST
— an arbitrary implementation detail. Real triggers for
FIN-mid-handshake are all transient: load balancer draining during a
rolling restart, a connection-limit polite close, an idle timeout during
a slow handshake.

**The compounding case is worse than a single stall.** A connection that
flaps faster than the 60 s healthy-operation reset window never
accumulates enough continuous connectivity to reset, so it ratchets 5 m
→ 10 m → 20 m → 40 m → 1 hr and stays there.

## The fix

Match only genuinely long-lived certificate problems:

```java
c instanceof CertificateException           // expired, not-yet-valid, hostname mismatch;
                                            //   also covers ValidatorException
  || c instanceof CertPathValidatorException  // untrusted chain
  || c instanceof CertPathBuilderException
  || c instanceof SSLPeerUnverifiedException  // hostname mismatch
```

Verified that a genuinely untrusted chain still classifies `UNEXPECTED`
— JSSE's `ValidatorException` is a `CertificateException` and
`SunCertPathBuilderException` is a `CertPathBuilderException`, so two
links of the real chain match.

## Parity with Go

This aligns Java with the Go server SDK, whose
`classifyTransportFailure` enumerates only certificate errors and treats
everything else as normal:

```go
tls.CertificateVerificationError
x509.UnknownAuthorityError
x509.HostnameError
x509.CertificateInvalidError
// everything else -> FailureClassNormal
```

The previous Java behavior was a divergence from that reference
implementation, not a different reading of the spec.

## Tests

- **Replaces** `sslHandshakeIsUnexpected`, which asserted the over-broad
behavior, with `bareSslHandshakeFailureIsNormal` and
`peerClosedMidHandshakeIsNormal` (the latter reproducing the real
`SSLHandshakeException` → `EOFException` shape).
- **Adds** `certPathValidatorFailureIsUnexpected`,
`certPathBuilderFailureIsUnexpected`,
`certificateNotYetValidIsUnexpected`,
`untrustedChainWrappedInHandshakeExceptionIsUnexpected`,
`sslExceptionFromConnectionResetIsNormal`.
- Full `lib/shared/internal` suite green; `checkstyleMain` clean.

## Test plan for reviewers

- [x] Confirm the four matched types are the right set — in particular
that `CertificateException` is the correct catch-all for validator
failures, and that nothing in Go's four cases lacks a Java counterpart
here.
- [x] Consider whether a bare `SSLHandshakeException` with a
*cipher/protocol* mismatch cause (e.g. `handshake_failure` alert, "No
appropriate protocol") should be `UNEXPECTED`. It is persistent like a
cert problem, but it is not a certificate error and is now classified
`NORMAL`. I left it as `NORMAL` to avoid re-widening, and because a
persistent mismatch keeps retrying at 1–30 s rather than stalling — but
it is a judgment call.
- [x] Sanity-check that no other caller depends on the old broad
behavior.

## Downstream

#200 consumes this classifier. It needs an
internal release (1.11.1) before it can pick this up, in addition to the
`classifyAndLogHttpFailure` rename already noted in review there.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> **Narrows when HTTPS transport failures trigger extended-regime
backoff** by changing `FailureClass.hasTlsOrCertificateCause` to walk
the exception chain for **certificate validation problems only**
(`CertificateException`, `CertPathValidatorException`,
`CertPathBuilderException`, `SSLPeerUnverifiedException`), instead of
any `SSLException` or `GeneralSecurityException`.
> 
> Because all SDK traffic is TLS, the old rule treated many
**transient** handshake faults (e.g. peer FIN mid-handshake, bare
`SSLHandshakeException`, connection-reset `SSLException`) as
**UNEXPECTED**, which could push data sources into multi-minute backoff.
Genuine cert issues (expired/not-yet-valid, untrusted chain wrapped in
`SSLHandshakeException`) still classify **UNEXPECTED**.
> 
> Tests are updated to match: removed the expectation that every SSL
handshake failure is unexpected, added cases for cert-path errors and
normal transient SSL shapes, and kept wrapped-certificate-cause
coverage.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
fa662f2. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
tanderson-ld and others added 3 commits August 28, 2026 11:26
close() reports the terminal State.OFF, but poll() wrote status without
consulting isClosed. An in-flight poll -- unblocked by close()'s own call to
requestor.close() -- could then write INTERRUPTED or VALID afterward. The sink
does not treat OFF as terminal, so a listener could observe a shut-down data
source revive, which is the more damaging case of the two.

Routes poll()'s six status writes through tryUpdateStatus(), which checks
isClosed first. StreamProcessor.handleEvent guards its own writes the same way.

The read is deliberately unlocked. DataSourceUpdateSink is an interface with no
bound on how long updateStatus may take, so holding a lock across that call
could stall close() for an arbitrary period. This narrows the window to the gap
between the volatile read and the write rather than eliminating it -- the same
residual exposure the streaming path has. Closing it entirely would mean making
OFF terminal in DataSourceUpdatesImpl, which is shared with FDv2 and out of
scope here.
…rce 5.0.0

Replaces the local mavenLocal snapshot pins now that both artifacts are on
Maven Central:

- launchdarkly-java-sdk-internal 1.10.0 -> 1.11.1, for the FailureClass
  enum and the HttpErrors classify helpers.
- okhttp-eventsource 4.2.0 -> 5.0.0, for the multi-strategy retry API
  (activateRetryDelayStrategy, getDelayMillis/getNext/withBaseDelayMillis).

The eventsource upgrade removes EventSource.Builder.retryDelay(long, TimeUnit),
which the FDv2 streaming synchronizer still used. Ported that one call site to
retryDelayStrategy(defaultStrategy().initialDelay(...)). This is a like-for-like
translation: defaultStrategy() keeps the same 30s ceiling and 0.5 jitter the old
setter produced, so FDv2 retry behavior is unchanged and none of the
RETRY-regime switching is introduced there.

Verified against the released artifacts rather than the snapshots: compileJava,
compileTestJava, checkstyleMain, and the full lib/sdk/server suite all pass.
This matters because the snapshot built from the eventsource PR branch still
had Builder.retryDelay, which the final 5.0.0 dropped -- so earlier local runs
were validating an API surface that did not ship.

@jsonbailey jsonbailey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One follow-up on the init future.

Comment thread lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/PollingStrategy.java Outdated

@jsonbailey jsonbailey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Three more, all questions rather than blockers — one on diagnostics, two on polling test coverage.

Orphaned helper. verifyHttpErrorCausedShutdown (PollingProcessorTest.java:343) has no callers left — the rewrite removed all of them, since asserting State.OFF on an HTTP error is exactly the behavior this PR removes. Worth deleting rather than leaving as dead code. (Commenting here rather than inline because line 343 isn't in the diff.)

Relatedly, nothing asserts close() produces State.OFF. That's the entire purpose of d3b56a6, and d345b08 shows it's load-bearing — those two builder tests only needed a sink because close() now publishes status. A small test would cover both.

Complete the start() future when a data source is closed. No failure
permanently stops either data source now, so shutdown is the only remaining
exit for a caller that waits on that future without a timeout; previously such
a caller waited forever, since close() never completed it and the error-path
completion went away with the permanent-stop paths. Both processors now
complete it in close(), after the OFF status is published so a released thread
observes a consistent state. isInitialized() still reports false, so this
signals "done waiting" rather than "initialized", and it gives the constructor
a fast exit if another thread closes the client during startWaitTime.
StreamProcessor's future was a local in start() and unreachable from close(),
so it becomes a field as PollingProcessor already had; repeated start() calls
consequently share one future, which is how PollingProcessor has always
behaved and LDClient only calls start() once.

Replace the paired compareTo clamps in PollingStrategy.onFailure with a max()
helper. Assign-then-maybe-overwrite twice obscured that both bounds are simply
floored at the poll interval, and compareTo at the call site made the operand
order hard to read. Behavior is unchanged.

Add polling coverage that distinguishes the extended regime from the normal
cadence. testUnexpectedHttpErrorKeepsPolling used a 30ms extended initial delay
against a 20ms poll interval, so after the wait floor the extended wait landed
in 20-30ms and every assertion in it passed whether or not the extended regime
engaged -- its comment claimed the waits were observable but nothing observed
them. The gap that matters is broader: PollingStrategy is covered in isolation,
but the wiring from poll() through onFailure and nextWait into scheduleNext had
no coverage at all, which is where a mis-wired classifier call would hide.

unexpectedFailureUsesExtendedCadence and oneSuccessfulPollDoesNotLeaveExtended-
Cadence assert observed request spacing with a 500ms extended initial delay,
giving a 250-500ms wait against a 20ms normal cadence. Both use a lower bound
only, so CI slowness cannot fail them. Verified non-vacuous by swapping the
injected 401 for a 500: the observed gap drops to 33ms and the assertion fails,
which is what the previous test could not detect. Also adds the missing
"engaging extended backoff" log assertion, which the streaming counterpart
already had.
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