Skip to content

feat: allow a custom retry delay strategy to be provided - #40

Merged
tanderson-ld merged 1 commit into
mainfrom
ta/SDK-2790/retry-delay-strategy-injection
Sep 24, 2026
Merged

tanderson-ld merged 1 commit into
mainfrom
ta/SDK-2790/retry-delay-strategy-injection

Conversation

@tanderson-ld

@tanderson-ld tanderson-ld commented Sep 23, 2026 •

Copy link
Copy Markdown

Summary

Adds a retryDelayStrategy init option that lets the application take full control of reconnection timing, and hardens handling of the SSE retry: 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, so initialRetryDelayMillis, maxBackoffMillis, jitterRatio, and retryResetIntervalMillis have 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 — previously parseInt leniency meant retry: 12abc was 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.supportedOptions includes 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

  • Nine new tests: an injected strategy drives reconnect delays; the built-in options are inert when a strategy is provided; the good-since signal reaches the strategy once per connection, on the first event; exactly one failure is recorded when a read timeout is followed by the connection dropping (double-signal idempotence — verified to fail if the once() wrapper is removed); valid retry: values are forwarded; non-digit and negative values are ignored; over-cap values are clamped to 3600000; and a behavioral pin that a valid retry: governs the next reconnect delay with the built-in strategy (a previously untested path).
  • Full suite: 98 passing, standard clean. Mutation check: the seam tests fail against the unmodified library except the negative cases, which are vacuous without the feature.

Note

Overview
Adds a retryDelayStrategy init option so callers can plug in custom reconnection timing via nextRetryDelay, setGoodSince, and setBaseDelay, fully replacing the built-in initialRetryDelayMillis / backoff / jitter options when set. EventSource.supportedOptions and the README document the new seam.

SSE retry: parsing is tightened: values must be all ASCII digits (rejecting negatives and parseInt-style partial matches), and accepted delays are capped at one hour before updating reconnectInterval and forwarding to the strategy.

Tests cover custom strategy behavior, retry: validation/capping, and built-in delay after a valid retry: field.

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

@tanderson-ld
tanderson-ld requested a review from a team as a code owner September 23, 2026 21:35
@tanderson-ld
tanderson-ld merged commit dd081f0 into main Sep 24, 2026
5 checks passed
@tanderson-ld
tanderson-ld deleted the ta/SDK-2790/retry-delay-strategy-injection branch September 24, 2026 15:54
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>
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