feat: allow a custom retry delay strategy to be provided - #40
Merged
Merged
Conversation
joker23
approved these changes
Sep 24, 2026
tanderson-ld
added a commit
to launchdarkly/js-core
that referenced
this pull request
Sep 28, 2026
…off (#2045) ## Summary Adds a reusable retry controller to `@launchdarkly/js-sdk-common` that owns retry tracking, health checking, and delay calculation for long-running components. This is the foundation for RETRY-spec conformance in the Node server SDK (SDK-2790); the data-source wiring that consumes it arrives in a follow-up PR. **There are no consumers in this PR** — the additions are unused by product code and exercised only by tests. The design ports the controller pattern from the Python server SDK ([python-server-sdk#522](launchdarkly/python-server-sdk#522), wired in [#519](launchdarkly/python-server-sdk#519)), with deliberate differences noted below. ## What's where - **`src/datasource/retry/`** — the retry mechanics: the `RetryState` interface and its `createRetryState` factory (the controller), the `ResetPolicy` interface with its two implementations (`AfterHealthyFor` for streaming's healthy-duration reset, `AfterConsecutiveSuccesses` for polling's two-in-a-row reset), and the `forStreaming`/`forPolling` factories that bind the standard values (1s→30s normal and 5min→1hr extended regimes; 60s healthy window; two-success polling reset) with warn-and-default validation of the configured delay. - **`src/errors.ts`** — failure classification, placed beside the legacy helper it supersedes: `FailureKind` (`'normal' | 'unexpected'`), `classifyHttpStatus` (400/408/429 and 5xx and non-error statuses are normal; any other 4xx is unexpected), and `classifyTransportFailure` (always normal — in an all-HTTPS system certificate failures can't be reliably distinguished from transient faults). `isHttpRecoverable` now delegates to `classifyHttpStatus` — one table, no drift — and is documented as superseded; it stays undeprecated because the event-delivery pathway still legitimately consumes it until that pathway migrates. ## Design points for review - **`RetryState` is an interface, not a class.** `createRetryState(config)` returns it, backed by a closure over local state; `forStreaming`/`forPolling` return the interface. This keeps the publicly exposed surface an interface (per the repo's prefer-interfaces guideline) and lets future mutators be added additively. The `ResetPolicy` implementations stay classes, since the `ResetPolicy` interface already fronts them everywhere they are consumed. - **Clockless seams.** No method of `RetryState` or `ResetPolicy` takes a timestamp. Time lives in exactly one place: `AfterHealthyFor`'s constructor-injected clock, defaulting to a monotonic source (`performance.now()`, with a `Date.now` closure fallback for exotic runtimes). The controller itself holds no clock; its only injectable is `random`, for deterministic jitter tests. (The parameter is named `clock` rather than the codebase's `timeStamper` deliberately — it is not a timestamp source.) - **Ceiling-bounded backoff, no exponent constant.** The delay computation compares the base against the ceiling scaled *down* (`base >= max / 2**exponent`) rather than scaling the base up, so nothing can overflow the ceiling — the same compare-before-shift structure as the .NET implementation, expressed in lossless power-of-two float math. A zero base (legal: the spec forbids flooring server-directed retry values) short-circuits, closing a `0 × Infinity = NaN` edge otherwise reachable when a zero-valued server-directed retry is followed by very many failures. - **A configured delay above the normal ceiling clamps to it.** In the normal regime the ceiling wins, matching the literal spec (1.3.2 + 1.4.2) and the majority of the SDK fleet (Go, Java, .NET). The extended regime is the opposite: its bounds are floored at the configured delay, which spec requirement 1.5.4.1 mandates ("a delay or ceiling that applies after an `unexpected` failure MUST NOT be less than the component's initial delay"). - **`applyServerDirectedRetry(ms)`** — the SSE `retry:` entry point: sticky base that takes precedence over the regime's initial delay (including the extended regime's), doubling restarted, ceiling still applies, survives a healthy reset. Wire-level validation and the 1-hour cap live in the SSE library ([launchdarkly/js-eventsource#40](launchdarkly/js-eventsource#40)), not here. - **Post-success wait = operating cadence**, even while the retry state is raised — a recovering poller returns to schedule immediately rather than serving one more extended-regime wait. ## Testing 89 tests across three suites (584 package-wide, all green): - `RetryState.test.ts` (48) — exact delay ladders for both regimes under injected clock/random (including the 5m/10m/20m/40m/1h/1h extended ladder), regime transition/ratchet/re-arm, anchor-once healthy-stretch discrimination, reset-before-count ordering, fast-second-poll cadence, poll-interval wait floor under real jitter, jitter range with the maximal-draw boundary (the exact-`T/2` tie) and distinctness assertions, server-directed retry semantics (replace/clamp/persist/reject-invalid/later-wins, precedence over both regimes' initial delays, `retry: 0` staying finite through 1,100 failures), normal-ceiling clamp and regime-collapse cases, the extended-floor mandate under direct construction, flapping-never-ratchets, high-n robustness, and factory validation matrices. - `ResetPolicy.test.ts` (8) — the policy seam directly: threshold boundary, anchor-once under repeated healthy reports, failure-clears, consecutive-success counting, and both default-clock closures (monotonic and the `Date.now` fallback). - `errors.test.ts` (33) — the full classification matrix including boundary and non-error statuses, transport classification, and parity pins on the legacy `isHttpRecoverable` truth table through the delegation. The `src/datasource/retry` module is at 100% statement, branch, function, and line coverage. Coverage was cross-checked against the Python, Java, Go, and .NET RETRY test suites; the one technique deliberately not ported is .NET's `BigInteger`/randomized reference sweeps, which exist to exercise 64-bit integer shift surfaces that JS float arithmetic does not have. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Introduces a **RETRY-spec-oriented retry controller** in `@launchdarkly/js-sdk-common` as shared library code only—**no data sources or SDKs call it yet**; follow-up PRs will wire streaming/polling. > > Adds `src/datasource/retry/` with **`RetryState`** (`createRetryState`, `forStreaming`, `forPolling`): exponential backoff with jitter, normal vs **extended** regimes after `unexpected` failures, pluggable **`ResetPolicy`** (healthy-for duration for streaming, consecutive successes for polling), and **`applyServerDirectedRetry`** for SSE `retry:` values. **`errors.ts`** gains **`FailureKind`** plus **`classifyHttpStatus`** / **`classifyTransportFailure`**; **`isHttpRecoverable`** now delegates to the same rules. > > New retry APIs are re-exported from the datasource barrel and package **`index`**. CI **package size limit** for common ESM rises **29 000 → 29 500** bytes. Coverage is **89 new tests** across retry and error classification. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 7f529cf. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
tanderson-ld
added a commit
that referenced
this pull request
Sep 29, 2026
…ions (#43) ## What Adds an explicit `permissions:` block (`contents: write`, `pull-requests: write`) to the `release-please` job in `.github/workflows/release-please.yml`. ## Why release-please has failed on every push to `main` since 2026-09-23 (the runs for #38, #40, #42), all with the same error: ``` POST /repos/launchdarkly/js-eventsource/git/refs → 403 "Resource not accessible by integration" (x-accepted-github-permissions: contents=write) ✖ Error when creating branch release-please failed: Error creating Pull Request: Resource not accessible by integration ``` The `release-please` job passes `token: ${{secrets.GITHUB_TOKEN}}` but had **no `permissions:` block**, so it inherited the repository's *default* `GITHUB_TOKEN` permissions. It worked for every release through 2.2.0 (Apr 2025) because that default was read-write; it broke once the default was tightened to read-only, which removed the write access the job needs to create the release branch/PR. The workflow file itself has not changed since 2024-04-26 — only the environment did. (The lone "success" on 2026-09-23 was a `chore:` push, which release-please treats as non-release-worthy, so it never reached the branch-creation step. The next release-worthy commits — #38, #40, #42 — all hit the 403.) ## Fix Grant the two write scopes explicitly on the job that needs them, so the workflow no longer depends on the repo default. This brings the job to parity with the js-core monorepo's `release-please` job, which already sets `contents: write` + `pull-requests: write` explicitly and continues to run successfully under the same `GITHUB_TOKEN`. ## Effect Once merged, the push to `main` re-runs release-please with the corrected permissions and it will open the pending release PR (covering the currently-unreleased #38, #40, #42), which on merge publishes the next version. ## Notes / possible follow-ups (intentionally not in this PR) - If the org has *also* disabled "Allow GitHub Actions to create and approve pull requests," a later run could 403 on the PR-create step (a separate setting from token scope). That setting was clearly enabled through the 2.2.0 release, so it is likely still fine. - The action reference here (`google-github-actions/release-please-action@v4`) is the old org name on a floating tag; js-core uses the renamed `googleapis/release-please-action` pinned to a SHA. Modernizing it is optional and left out to keep this fix focused. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Fixes release-please 403 failures** by giving the `release-please` job explicit `GITHUB_TOKEN` scopes instead of relying on the repo default (which was tightened to read-only). > > The job now declares `contents: write` and `pull-requests: write` so it can create the release branch/tag and open or update the release PR. The downstream `release-package` job already had these permissions; only `release-please` was missing them, which caused `Resource not accessible by integration` on recent pushes to `main`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 97f01e5. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
tanderson-ld
pushed a commit
that referenced
this pull request
Sep 29, 2026
🤖 I have created a release *beep* *boop* --- ## [2.3.0](2.2.0...2.3.0) (2026-09-29) ### Features * allow a custom retry delay strategy to be provided ([#40](#40)) ([dd081f0](dd081f0)) ### Bug Fixes * anchor retry backoff reset to the first event of each connection ([#38](#38)) ([9d46860](9d46860)) * destroy non-200 responses so their sockets close ([c0ba67c](c0ba67c)) * destroy non-200 responses so their sockets close ([#42](#42)) ([988a7b3](988a7b3)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > This PR **bumps the package to 2.3.0** via Release Please: `package.json`, `.release-please-manifest.json`, and a new **2.3.0** section in `CHANGELOG.md`. > > There are **no runtime or library code changes** in the diff itself. The changelog records what ships in this tag: **custom `retryDelayStrategy`** for reconnect timing, **retry backoff reset tied to the first event on each connection**, and **destroying non-200 HTTP responses** so underlying sockets close cleanly. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit ebf94e0. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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
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 a
retryDelayStrategyinit option that lets the application take full control of reconnection timing, and hardens handling of the SSEretry:field.retryDelayStrategy: an object implementing the same three-method contract as the built-in strategy (nextRetryDelay(currentTimeMillis),setGoodSince(goodSinceTimeMillis),setBaseDelay(delayMillis)). When provided it fully replaces the built-in delay behavior, soinitialRetryDelayMillis,maxBackoffMillis,jitterRatio, andretryResetIntervalMillishave no effect (documented in the README). Behavior is unchanged when the option is absent.retry:field hardening: the value must now be entirely ASCII digits, per the SSE specification — previouslyparseIntleniency meantretry: 12abcwas applied as 12, and a negative value produced an immediate-reconnect tight loop — and accepted values are capped at one hour, so an enormous reconnection time cannot behave like a permanent stop. Existing semantics are otherwise preserved (the value replaces the base delay, restarts the backoff progression, persists until the server sends another, and never changes the maximum).EventSource.supportedOptionsincludes the new option.This is the library half of the Node server SDK's RETRY-conformance work (SDK-2790): retry policy moves into the SDK behind this seam, while the library keeps the connection and timer mechanics it already has (including cancellable reconnect timers and one-failure-per-connection idempotence).
Testing
once()wrapper is removed); validretry:values are forwarded; non-digit and negative values are ignored; over-cap values are clamped to 3600000; and a behavioral pin that a validretry:governs the next reconnect delay with the built-in strategy (a previously untested path).standardclean. Mutation check: the seam tests fail against the unmodified library except the negative cases, which are vacuous without the feature.Note
Overview
Adds a
retryDelayStrategyinit option so callers can plug in custom reconnection timing vianextRetryDelay,setGoodSince, andsetBaseDelay, fully replacing the built-ininitialRetryDelayMillis/ backoff / jitter options when set.EventSource.supportedOptionsand the README document the new seam.SSE
retry:parsing is tightened: values must be all ASCII digits (rejecting negatives andparseInt-style partial matches), and accepted delays are capped at one hour before updatingreconnectIntervaland forwarding to the strategy.Tests cover custom strategy behavior,
retry:validation/capping, and built-in delay after a validretry:field.Reviewed by Cursor Bugbot for commit 7918fc5. Bugbot is set up for automated code reviews on this repo. Configure here.