feat: expose narrow setters on EventSource for RETRY-spec regime switching - #109
Closed
tanderson-ld wants to merge 2 commits into
Closed
feat: expose narrow setters on EventSource for RETRY-spec regime switching#109tanderson-ld wants to merge 2 commits into
tanderson-ld wants to merge 2 commits into
Conversation
… EventSource Adds two narrow public setter methods on EventSource for SDK-driven regime switching. Motivating use case is RETRY-spec conformance in server SDKs: on classification of a failure as "unexpected" (per RETRY §1.6 / §1.7), the SDK's data source needs to shift the retry timing into an extended regime (e.g. initial 5 min, max 1 hr), and shift back after healthy operation. Cross-referenced in launchdarkly/sdk-scratchpad's server-sdk-guide.md, and tracked as SDK-2789 (Java) under the RETRY-conformance epic SDK-2775. API: - setInitialRetryDelayMillis(long) updates the existing volatile baseRetryDelayMillis field (the same field wire-side SetRetryDelayEvent already updates). If the current strategy is a DefaultRetryDelayStrategy, the exponent counter is also reset so the first subsequent apply() uses the new base directly. Non-Default strategies just see the new base on the next apply() call. - setMaxRetryDelayMillis(long) constructs a new DefaultRetryDelayStrategy with the specified max delay and the exponent counter reset to 0, preserving the current backoff multiplier and jitter multiplier, and atomically swaps the reference. No-op for custom strategy impls (which don't expose a max-delay concept via the abstract interface). Both setters realize the "reset n when delays change" invariant from the LaunchDarkly server-SDK implementation guide: the first attempt in a new regime uses the new initial delay directly rather than newBase * 2^oldN. Under the hood: DefaultRetryDelayStrategy gains two package-private helpers (withResetCounter and withMaxDelayMillisAndResetCounter) that build copies with a fresh counter. The public builder methods (maxDelay, backoffMultiplier, jitterMultiplier) are unchanged. currentRetryDelayStrategy is now volatile to support the "caller can invoke setters from any thread" contract. Tests: 6 new tests covering direct setter effects, counter-reset behavior, composed extended-regime sequence, wire-hint interaction, and no-op behavior on non-Default strategies. Full unit suite and sse-contract-tests both green.
An SDK error handler running under ErrorStrategy.alwaysContinue calls setInitialRetryDelayMillis / setMaxRetryDelayMillis inside its handleError callback, in response to a fault that has just been classified as UNEXPECTED per the RETRY specification. The immediately-prior computeReconnectDelay() call had already stored nextReconnectDelayMillis using the pre-transition strategy, so without a recompute the upcoming reconnect would use the OLD regime's timing (e.g., 1 ms normal-regime delay) and the extended-regime backoff would kick in only starting from the NEXT fault. Fix: - Track a pendingReconnectWait flag: set by computeReconnectDelay after a fault, cleared by tryStart on successful reconnect. - setInitialRetryDelayMillis and setMaxRetryDelayMillis, if pendingReconnectWait is true, recompute nextReconnectDelayMillis with the just-updated strategy. Do NOT advance the strategy (prior computeReconnectDelay already did that; advancing here would double-increment the counter for the next fault). Verified via the sdk-test-harness RETRY-conformance streaming/retry test "enters extended-regime backoff after unexpected HTTP error", which is now green (was failing prior to this fix because the SDK reconnected at normal-regime timing after the first 401).
4 tasks
tanderson-ld
changed the title
Contributor
Author
|
Superseded by a follow-up PR taking a different design approach — replacing the narrow setInitialRetryDelayMillis / setMaxRetryDelayMillis API with a multi-strategy activation model. Link will be added once the new PR is opened. |
Contributor
Author
|
Superseded by #110, which takes a different design approach — multi-strategy activation instead of narrow setters. |
tanderson-ld
added a commit
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 -->
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
1 participant
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.
Summary
Adds two narrow public setter methods on
EventSource—setInitialRetryDelayMillis(long)andsetMaxRetryDelayMillis(long)— for SDK-driven regime switching. The motivating use case is RETRY-spec conformance in server SDKs: on classification of a failure as "unexpected" (per RETRY §1.6 / §1.7), the SDK's data source shifts retry timing into an extended regime (e.g., initial 5 min, max 1 hr), and shifts back after healthy operation.What ships
Two public methods on
EventSource:setInitialRetryDelayMillis(long millis)— updates the existing volatilebaseRetryDelayMillisfield (same field the wire-sideSetRetryDelayEventalready updates); if the current strategy is aDefaultRetryDelayStrategy, the exponent counter is reset so the first subsequentapply()uses the new base directly.setMaxRetryDelayMillis(long millis)— constructs a new immutableDefaultRetryDelayStrategywith the specified max delay and the exponent counter reset to 0, preserving the current backoff multiplier and jitter multiplier, and atomically swaps the reference. No-op for custom strategy impls (which don't expose a max-delay concept via the abstract interface).Both realize the "reset
nwhen delays change" invariant from the LaunchDarkly server-SDK implementation guide: the first attempt in a new regime uses the new initial delay directly rather thannewBase × 2^oldN.Two package-private helpers on
DefaultRetryDelayStrategy:withResetCounter()andwithMaxDelayMillisAndResetCounter(long). Existing public builder methods (maxDelay,backoffMultiplier,jitterMultiplier) are unchanged.currentRetryDelayStrategyfield made volatile to support the "caller can invoke setters from any thread" contract.Design notes
Alternative considered: expose a single
setRetryDelayStrategy(RetryDelayStrategy)method that lets the caller replace the whole strategy. Rejected because it gives consumers too much rope — an SDK doing regime switching only needs to move min/max between regimes; it shouldn't be able to accidentally change jitter or backoff-multiplier as part of the same knob. The narrow-setter API keeps the public surface minimal.The
EventSource.baseRetryDelayMillishandling of server-directedretry:hints is orthogonal to the new setters and remains unchanged. A later wire hint continues to overwrite the SDK-set base delay, matching WHATWG HTML Living Standard EventSource semantics.Testing
EventSourceRetryDelayStrategyUsageTest:setInitialRetryDelayMillisUpdatesGetBaseRetryDelayMillissetInitialRetryDelayMillisResetsExponentCountersetMaxRetryDelayMillisClampsAndResetsExponentCountersetInitialAndSetMaxComposeForExtendedRegimeSequence(asserts the RETRY spec's extended-regime doubling shape at ms-scale for test speed)wireRetryHintStillTakesEffectAfterSdkSideSetterssettersOnCustomRetryDelayStrategyDoNotThrowmake contract-tests(sse-contract-tests harness): "All tests passed" end-to-end.Test plan for reviewers
currentRetryDelayStrategybeing volatile is acceptable (the field was previously non-volatile with a comment noting it should only be accessed from the reading thread; the setters lift that invariant, and the volatile write is a defensive publication guarantee).DefaultRetryDelayStrategyare appropriately scoped.wireRetryHintStillTakesEffectAfterSdkSideSetters) — the wire hint continues to override the SDK's initial delay after both setters have been used, which is intentional (SDK regime state ≠ wire-authoritative reconnect time).Downstream
Consumed by java-server-sdk PR (SDK-2789) — Java SDK's RETRY-conformance work. That PR's CI will be red until this PR ships to Maven Central.