From b67350b73df163dab9099f03739844087583e472 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 2 Aug 2026 23:15:04 -0700 Subject: [PATCH 1/3] Give the backoff formula a ceiling in all six SDKs, and retry a Swift Transport's own network error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #577 was filed as a Kotlin bug. It is six SDKs, in three distinct shapes, and the Swift one is strictly the worst and was tracked nowhere. Signed 64-bit wrap Kotlin (1L shl), Go (1<<) The product goes negative and the sleep primitive treats a non-positive delay as "no delay". Kotlin attempt 55 measured -432,345,564,227,567,518ms; Go attempts 62-64 measured 18-58ms, which is the jitter alone. Trap Swift (UInt64 multiply) The process CRASHES. `<<` on an unsigned integer is a smart shift, so an over-shift quietly yields 0, but at `1 << 63` the multiply overflows UInt64 and traps. The red-proof run died with signal 5. Infinity TypeScript (Math.pow) `Math.pow(2, 1024)` is Infinity, and setTimeout clamps an out-of-range delay to 1ms. Unbounded integer Python (2 **), Ruby (2**) No wrap: the multiplier promotes to a bignum. Python raised OverflowError converting it to a float; Ruby produced Float::INFINITY, and sleep(Float::INFINITY) never returns. Three of the four shapes are the same failure: a client that has stopped backing off and is hammering a server already answering 429/503, which is the exact traffic pattern backoff exists to prevent. The fourth is a crash. All of it is reachable. Kotlin's builder validates maxRetries >= 0 with no upper bound; Go's WithMaxRetries only rejects n < 1. It needs a long genuine failure streak, so this is a robustness gap rather than a live incident — but "crash" and "tight-loop a struggling server" are bad enough consequences to close. SPEC §7 declared no delay ceiling at all, so six implementations were making six independent judgment calls about a number the spec never named. It now names one. MAX_BACKOFF_DELAY = 30,000ms, adopted rather than invented: Go's generated client has capped at RetryConfig.MaxDelay = 30s since that retry loop was first templated, so the constant describes shipping behavior. The new "Backoff Ceiling" subsection states the four requirements — saturate rather than wrap or diverge; the ceiling bounds the backoff term with jitter added on top; it applies to linear and constant backoff and therefore to base_delay_ms itself; Retry-After is server-directed and exempt — and tabulates the four overflow shapes so the next implementation knows what it is defending against. No shipped path changes. behavior-model.json tops out at base_delay_ms 2000 over at most three attempts, so the largest delay any operation computes is 4000ms and the ceiling is unreachable by default. Each SDK pins that explicitly: the exponential values below the ceiling are asserted exact. Every implementation compares the multiplier against MAX_BACKOFF_DELAY / base before multiplying, so no intermediate ever leaves the host's numeric range — the clamp is not a Math.min applied after the damage. TypeScript needed two sites. `services/base.ts` runs its own retry loop for the raw multipart transport and computed `Math.pow(2, attempt)` inline; both loops now share `saturatingBackoff`, so the ceiling cannot be honored on one path and not the other. Python needed two too — sync and async each carry a copy of _calculate_delay — and the test drives both. #567: Swift's Transport is public and documented as a seam, but both retry loops classified EVERY BasecampError out of the transport as terminal. A consumer normalizing connectivity failures into BasecampError.network — the most natural thing to reach for, and exactly the classification §6 defines for the condition with retryable: true — got one attempt with retries enabled, while a transport throwing an arbitrary foreign error retried normally. The more carefully an implementer read the error taxonomy, the more certainly they disabled their own retries. The catch could not simply be deleted: it also swallowed the response-type guard, which raised BasecampError.network("Invalid response type") for a non-HTTP URLResponse. That one is a deterministic programming error — retrying it three times just repeats it. So the guard now raises a private InvalidResponseTypeError and is converted back at every boundary, leaving the public error contract byte-for-byte unchanged, and only .network moves to the retry branch. Both loops changed together; splitting them would put two classifications of the same error inside one SDK, which is worse than the gap. On exhaustion the transport's own error surfaces instead of being re-wrapped in a second, vaguer .network that would discard the only diagnostic the caller has. The retry gate is unchanged: network-error retry still runs behind SPEC §7's three gates, so a non-idempotent POST is still attempted exactly once. Red proofs, all captured before the fix: Kotlin RetryTest#backoffDelaySaturatesInsteadOfOverflowing expected every delay in 30000..30100 ms, got attempt 53 -> 4503599627370496020ms, attempt 55 -> -432345564227567518ms, attempt 62 -> 48ms, attempt 2147483647 -> 64ms Go TestBackoffDelaySaturates backoffDelay(32) = 596523h14m8s; backoffDelay(63) = 58.219404ms; backoffDelay(1073741824) = 37.702901ms Swift BackoffCeilingTests — xctest exited with signal code 5 (SIGTRAP) partway through testExponentialBackoffSaturates, after reporting attempt 40 produced 549755813888000ms TS backoff ceiling > saturates the exponential term attempt 40 -> 1099511627776087.8ms, attempt 1024 -> Infinityms Python test_calculate_delay_saturates attempt 64 produced 9.223372036854776e+18s; attempt 2147483648 raised OverflowError: int too large to convert to float Ruby BackoffCeilingTest#test_calculate_delay_saturates_at_the_ceiling attempt 1024 -> 8.98846567431158e+307s, attempt 10000 -> Infinitys Swift TransportNetworkErrorRetryTests — 3 failures, both retry loops #567 giving exactly 1 attempt where 2 and 3 were expected The two-sided bound in each is deliberate. A one-sided "never too long" check passes on exactly the attempts that tight-loop: 1000 * (1L shl 63) wraps to precisely 0, which looks perfectly in range while being the bug itself. Verified green: go-check, kt-check, ts-check, py-check, rb-check, swift-check (319 tests, ran rather than printing its macOS SKIP line), all six conformance runners, check-retry-metadata-parity, check-idempotency-parity. Closes #577 Closes #567 --- SPEC.md | 52 ++- go/pkg/basecamp/backoff_ceiling_test.go | 61 ++++ go/pkg/basecamp/client.go | 38 +- go/pkg/basecamp/http.go | 9 + .../basecamp/sdk/http/BasecampHttpClient.kt | 31 +- .../kotlin/com/basecamp/sdk/RetryTest.kt | 51 +++ python/src/basecamp/_async_http.py | 5 +- python/src/basecamp/_http.py | 5 +- python/src/basecamp/config.py | 26 ++ python/tests/test_backoff_ceiling.py | 83 +++++ ruby/lib/basecamp/config.rb | 30 ++ ruby/lib/basecamp/http.rb | 6 +- ruby/test/basecamp/backoff_ceiling_test.rb | 62 ++++ swift/Sources/Basecamp/HTTP/HTTPClient.swift | 134 ++++++- .../BasecampTests/BackoffCeilingTests.swift | 67 ++++ .../TransportNetworkErrorRetryTests.swift | 332 ++++++++++++++++++ typescript/src/retry.ts | 49 ++- typescript/src/services/base.ts | 5 +- typescript/tests/backoff-ceiling.test.ts | 76 ++++ 19 files changed, 1087 insertions(+), 35 deletions(-) create mode 100644 go/pkg/basecamp/backoff_ceiling_test.go create mode 100644 python/tests/test_backoff_ceiling.py create mode 100644 ruby/test/basecamp/backoff_ceiling_test.rb create mode 100644 swift/Tests/BasecampTests/BackoffCeilingTests.swift create mode 100644 swift/Tests/BasecampTests/TransportNetworkErrorRetryTests.swift create mode 100644 typescript/tests/backoff-ceiling.test.ts diff --git a/SPEC.md b/SPEC.md index d33d2a77e4..aa0a386ec6 100644 --- a/SPEC.md +++ b/SPEC.md @@ -572,7 +572,7 @@ Kotlin, and Ruby. - **TypeScript** implements the three-gate algorithm with the retry loop beneath the openapi-fetch middleware chain (the client's custom `fetch`), so attempts run to each operation's declared `retry.max` and network errors retry under the same idempotency gate; caller aborts and request timeouts are terminal. **Kotlin** implements the three-gate algorithm for both HTTP status and network-error retries (POST retries only when `idempotent: true`, full exponential backoff): one eligibility gate covers both failure shapes, with the whole-request timeout (Ktor's `HttpRequestTimeoutException`) deliberately not retried and auth headers re-attached per attempt. - **Go** implements the three-gate on its generated operation path: it retries operations classified idempotent at generation time — GET/HEAD by method, plus any operation carrying `x-basecamp-idempotent` (the naturally-idempotent PUT/DELETE mutations like `UpdateProject`/`TrashProject`, and the flagged-idempotent POSTs like `CompleteTodo`) — with exponential backoff; non-idempotent operations (e.g. `CreateTodo`) are single-attempt. The separate hand-written `doRequestURL` helper remains GET-only for ordinary retries, with a mutation-specific single re-attempt after successful 401 token refresh. - **Ruby** is stricter: only GET retries; all non-GET methods do not retry. Governed GETs (those carrying their canonical operation ID) are bounded by the per-op ceiling and status-gated on the declared `retryOn`; ungoverned GETs (`get_absolute`, OAuth discovery) keep the taxonomy-driven pre-metadata contract. Ruby is acceptably conservative. -- **Swift** implements the three-gate algorithm: the transport retries only when the method is naturally idempotent (GET/HEAD/PUT/DELETE) **or** the operation is marked `idempotent: true`, so non-idempotent POSTs like `CreateProject` are attempted exactly once while the seven idempotent POSTs (`CompleteTodo`, `CreateBookmark`, `EnableCardColumnOnHold`, `PauseQuestion`, `PrioritizeAssignment`, `Subscribe`, `SubscribeToCardColumn`) keep retrying. The gate covers both retry paths — HTTP status (`429`/`503`) and network errors — so Swift retries network errors but only for retry-eligible operations. `BaseService` threads the per-operation flag from generated `Metadata` into the transport; the naturally-idempotent method set is allowlisted so PATCH/OPTIONS and future methods stay fail-closed. +- **Swift** implements the three-gate algorithm: the transport retries only when the method is naturally idempotent (GET/HEAD/PUT/DELETE) **or** the operation is marked `idempotent: true`, so non-idempotent POSTs like `CreateProject` are attempted exactly once while the seven idempotent POSTs (`CompleteTodo`, `CreateBookmark`, `EnableCardColumnOnHold`, `PauseQuestion`, `PrioritizeAssignment`, `Subscribe`, `SubscribeToCardColumn`) keep retrying. The gate covers both retry paths — HTTP status (`429`/`503`) and network errors — so Swift retries network errors but only for retry-eligible operations. A network error is classified by *meaning*, not by type: a `Transport` that reports connectivity failure as the SDK's own `BasecampError.network` reaches the retry branch exactly as a raw `URLError` does (#567). `Transport` is `public`, so that normalization is the natural implementation and must not be the one that disables retry. Any other `BasecampError` out of the transport (`.auth`, `.usage`, `.api`, …) stays terminal on sight, and the non-HTTP-response guard raises a distinct internal error so a deterministic programming fault is never mistaken for a transport blip. `BaseService` threads the per-operation flag from generated `Metadata` into the transport; the naturally-idempotent method set is allowlisted so PATCH/OPTIONS and future methods stay fail-closed. - The spec prescribes the three-gate algorithm. ### Retry Algorithm @@ -635,15 +635,62 @@ The loop always terminates via step 3e (raise on network error), 3f (return non- ### Backoff Formula ``` -delay = base_delay_ms * 2^(retry_index) + random(0, max_jitter) +delay = min(base_delay_ms * 2^(retry_index), MAX_BACKOFF_DELAY_MS) + random(0, max_jitter) ``` Where `retry_index` is the 0-indexed retry count (first retry = 0, second retry = 1, etc.). In the `executeWithRetry` loop, `retry_index = attempt` — when the initial request (attempt=0) fails and reaches step 3h, it computes the delay for the first retry using `2^0 = 1×base_delay_ms`. Default constants (from `retry_config` or Config): - `base_delay_ms` = 1000 (from `retry_config.base_delay_ms`) - `max_jitter` = 100ms (from Config; not part of `retry_config` — sourced from the client's Config RECORD) +- `MAX_BACKOFF_DELAY_MS` = 30,000 (30s) — the ceiling on the backoff term, below Retry-After header value takes precedence when present and valid. +### Backoff Ceiling `[CONFLICT]` + +The backoff term is **saturating**: it grows exponentially up to `MAX_BACKOFF_DELAY_MS` +and then stops. This is a correctness requirement, not a politeness one — an unbounded +`2^n` is not merely a long sleep, it is a different failure in every host language, and +each of the six SDKs demonstrated one before #577: + +| Overflow shape | SDKs | What actually happens | +|---|---|---| +| Signed 64-bit wrap | Kotlin (`1L shl`), Go (`1<<`) | The product goes negative and the sleep primitive treats a non-positive delay as "no delay" — the client tight-loops against a server that is already answering 429/503, which is the exact traffic pattern backoff exists to prevent | +| Trap on overflow | Swift (`UInt64` multiply) | The process **crashes**. Swift's `<<` is a smart shift, so an over-shift silently yields `0` (tight loop), but at `1 << 63` the multiply overflows `UInt64` and traps | +| Saturate to infinity | TypeScript (`Math.pow`) | `Infinity` reaches `setTimeout`, which clamps an out-of-range delay to **1ms** — a tight loop again | +| Unbounded integer | Python (`2 **`), Ruby (`2**`) | No wrap: the multiplier becomes an arbitrary-precision bignum. Python raises `OverflowError` converting it to a float; Ruby coerces to `Float::INFINITY` and `sleep` never returns | + +Requirements: + +1. **Saturate, never wrap, never diverge.** An implementation must not evaluate an + unbounded `2^retry_index`. Clamp the exponent, or compare the multiplier against + `MAX_BACKOFF_DELAY_MS / base_delay_ms` before multiplying — either keeps every + intermediate inside the host's numeric range. +2. **The ceiling bounds the backoff term, not the total sleep.** Jitter is added after + clamping, so the longest single backoff sleep is `MAX_BACKOFF_DELAY_MS + max_jitter`. + This matches Go's generated client, which has capped at `RetryConfig.MaxDelay = 30s` + since it was first templated; 30s is adopted as the cross-SDK constant because it is + the one value already shipping. +3. **The ceiling applies to `linear` and `constant` backoff too**, and therefore to + `base_delay_ms` itself: a caller configuring a base delay above the ceiling gets the + ceiling. The rule is "no single computed backoff sleep exceeds `MAX_BACKOFF_DELAY_MS`", + with no carve-out for the first one. No shipped configuration approaches it — + `behavior-model.json` tops out at `base_delay_ms: 2000`, and the default three + attempts never compute past `2000 × 2 = 4000ms`, so the ceiling is unreachable on + default paths and changes no shipped behavior. +4. **`Retry-After` is exempt.** It is server-directed and takes precedence per step 3h; + the ceiling governs the locally-computed formula only. Implementations may still + bound it against host limits — Swift clamps its seconds→nanoseconds conversion to + 86,400s because `UInt64(_:)` on an out-of-range `Double` is a trap. + +**Reachability.** Every SDK exposes a path to a high attempt count: Kotlin's builder +validates `maxRetries >= 0` with no upper bound, Go's `WithMaxRetries` only rejects +`n < 1`, and Python/Ruby take a caller cap that is intersected with — never raised +above — the per-operation max, so a caller who *lowers* the cap is fine but the +operation ceiling itself is whatever `behavior-model.json` says. Reaching the overflow +needs a long genuine failure streak, so this is a robustness gap rather than a live +incident; the consequences (crash, tight loop, infinite sleep) are severe enough that +the gate belongs in the spec rather than in six independent judgment calls. + ### Default and No-Retry Configs ``` @@ -1904,6 +1951,7 @@ All magic numbers in one place, derived from shipping SDK code (not `rubric-audi | `DEFAULT_MAX_RETRIES` | 3 | — | All six SDKs | | `DEFAULT_BASE_DELAY` | 1000 | milliseconds | All six SDKs | | `DEFAULT_MAX_JITTER` | 100 | milliseconds | All six SDKs | +| `MAX_BACKOFF_DELAY` | 30,000 (30s) | milliseconds | All six SDKs; ceiling on the §7 backoff term, jitter added on top. Was Go's generated `RetryConfig.MaxDelay` before #577 generalized it | | `DEFAULT_MAX_PAGES` | 10,000 | — | All six SDKs | | `MAX_CACHE_ENTRIES` | 1000 | entries | `typescript/src/client.ts` | | `MAX_TOKEN_HASH_ENTRIES` | 100 | entries | `typescript/src/client.ts` | diff --git a/go/pkg/basecamp/backoff_ceiling_test.go b/go/pkg/basecamp/backoff_ceiling_test.go new file mode 100644 index 0000000000..311ae0bc9c --- /dev/null +++ b/go/pkg/basecamp/backoff_ceiling_test.go @@ -0,0 +1,61 @@ +package basecamp + +import ( + "testing" + "time" +) + +// TestBackoffDelaySaturates pins SPEC §7's backoff ceiling (#577). +// +// Before the fix, backoffDelay computed base * time.Duration(1<<(attempt-1)). +// The shift is evaluated in int: at attempt 64 it is 1<<63, which is negative, +// and past 64 Go defines an over-wide shift as 0. Multiplying a 1s base by +// either produces a delay that is negative or zero, and time.After / time.Sleep +// treat a non-positive duration as "no wait" — so a long failure streak stopped +// backing off entirely and hammered a server already answering 429/503. +// +// WithMaxRetries only rejects n < 1, so a caller can set the attempt count that +// reaches this. The bound is two-sided on purpose: a one-sided "never too long" +// check passes on exactly the attempts that tight-loop. +func TestBackoffDelaySaturates(t *testing.T) { + client := &Client{httpOpts: HTTPOptions{ + BaseDelay: DefaultBaseDelay, + MaxJitter: DefaultMaxJitter, + }} + + // With a 1s base, attempt 6 is the first whose unclamped term (32s) exceeds + // the 30s ceiling, so every attempt from there on must sit at the ceiling. + for _, attempt := range []int{6, 10, 32, 33, 62, 63, 64, 65, 128, 1 << 30} { + delay := client.backoffDelay(attempt) + if delay < MaxBackoffDelay || delay > MaxBackoffDelay+DefaultMaxJitter { + t.Errorf("backoffDelay(%d) = %v, want within [%v, %v]", + attempt, delay, MaxBackoffDelay, MaxBackoffDelay+DefaultMaxJitter) + } + } + + // The unsaturated attempts keep their exact exponential values, so the + // ceiling changes nothing on any path a shipped configuration reaches. + for attempt, want := range map[int]time.Duration{1: time.Second, 2: 2 * time.Second, 3: 4 * time.Second} { + delay := client.backoffDelay(attempt) + if delay < want || delay > want+DefaultMaxJitter { + t.Errorf("backoffDelay(%d) = %v, want within [%v, %v]", attempt, delay, want, want+DefaultMaxJitter) + } + } +} + +// TestBackoffDelayEdgeConfigurations covers the two configurations that make +// the clamp arithmetic itself interesting: a base delay above the ceiling +// (SPEC §7 requirement 3 — it is clamped, with no carve-out for the first +// sleep) and a zero base delay (must stay at zero rather than saturating, +// since MaxBackoffDelay/0 would otherwise divide by zero). +func TestBackoffDelayEdgeConfigurations(t *testing.T) { + above := &Client{httpOpts: HTTPOptions{BaseDelay: 10 * time.Minute, MaxJitter: DefaultMaxJitter}} + if delay := above.backoffDelay(1); delay < MaxBackoffDelay || delay > MaxBackoffDelay+DefaultMaxJitter { + t.Errorf("base delay above the ceiling: backoffDelay(1) = %v, want the ceiling %v", delay, MaxBackoffDelay) + } + + zero := &Client{httpOpts: HTTPOptions{BaseDelay: 0, MaxJitter: DefaultMaxJitter}} + if delay := zero.backoffDelay(90); delay > DefaultMaxJitter { + t.Errorf("zero base delay: backoffDelay(90) = %v, want at most the jitter %v", delay, DefaultMaxJitter) + } +} diff --git a/go/pkg/basecamp/client.go b/go/pkg/basecamp/client.go index 7ac5dcf842..a7eed812b8 100644 --- a/go/pkg/basecamp/client.go +++ b/go/pkg/basecamp/client.go @@ -906,8 +906,7 @@ func (c *Client) assertCredentialOrigin(req *http.Request) error { } func (c *Client) backoffDelay(attempt int) time.Duration { - // Exponential backoff: base * 2^(attempt-1) - delay := c.httpOpts.BaseDelay * time.Duration(1<<(attempt-1)) + delay := saturatingBackoff(c.httpOpts.BaseDelay, attempt) // Add jitter jitter := time.Duration(rand.Int63n(int64(c.httpOpts.MaxJitter))) // #nosec G404 -- jitter doesn't need cryptographic randomness @@ -915,6 +914,41 @@ func (c *Client) backoffDelay(attempt int) time.Duration { return delay + jitter } +// saturatingBackoff computes base * 2^(attempt-1) for a 1-based attempt, +// saturating at MaxBackoffDelay (SPEC §7, "Backoff Ceiling"). +// +// The clamp is load-bearing. `1 << (attempt-1)` is evaluated in int: at attempt +// 64 that is 1<<63, which is negative, and Go defines a shift at or past the +// operand width as 0. Either way the product stops being a delay — and both +// time.Sleep and time.After treat a non-positive duration as "no wait", so an +// unclamped formula turned a long failure streak into a retry loop with no +// backoff at all, against a server already answering 429/503. +// +// The multiplier is compared against MaxBackoffDelay/base before multiplying, +// so no intermediate can overflow int64. +func saturatingBackoff(base time.Duration, attempt int) time.Duration { + if base <= 0 { + return 0 + } + shift := attempt - 1 + if shift < 0 { + shift = 0 + } + // 62 is the largest shift that stays positive in int64; anything at or past + // it is far above any ceiling worth computing. + if shift >= 62 { + return MaxBackoffDelay + } + // The multiplier is a plain count, not a duration — multiplying two + // time.Durations would be a unit error (and golangci's durationcheck says + // so), so the arithmetic happens in int64 nanoseconds. + multiplier := int64(1) << uint(shift) + if multiplier > int64(MaxBackoffDelay)/int64(base) { + return MaxBackoffDelay + } + return time.Duration(int64(base) * multiplier) +} + // parseNextLink extracts the next URL from a Link header. func parseNextLink(linkHeader string) string { if linkHeader == "" { diff --git a/go/pkg/basecamp/http.go b/go/pkg/basecamp/http.go index b4e772dea8..969258ef6e 100644 --- a/go/pkg/basecamp/http.go +++ b/go/pkg/basecamp/http.go @@ -16,6 +16,15 @@ const ( DefaultMaxPages = 10000 ) +// MaxBackoffDelay is the ceiling on the exponential backoff term (SPEC §7, +// "Backoff Ceiling"). Jitter is added after the clamp, so the longest single +// backoff sleep is MaxBackoffDelay + MaxJitter. +// +// It matches the generated client's RetryConfig.MaxDelay, which has capped at +// 30s since that retry loop was first templated; #577 generalized the value to +// every backoff site in every SDK. +const MaxBackoffDelay = 30 * time.Second + // HTTPOptions configures the HTTP client behavior. type HTTPOptions struct { // Timeout is the request timeout (default: 30s). diff --git a/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/http/BasecampHttpClient.kt b/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/http/BasecampHttpClient.kt index 371904decb..32b5e07c15 100644 --- a/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/http/BasecampHttpClient.kt +++ b/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/http/BasecampHttpClient.kt @@ -334,9 +334,36 @@ internal class BasecampHttpClient( private const val MAX_JITTER_MS = 100L - /** Exponential backoff: base * 2^(attempt-1) + jitter. */ + /** + * Ceiling on the backoff term (SPEC §7 "Backoff Ceiling"). Jitter is + * added on top, so the longest single backoff sleep is this plus + * [MAX_JITTER_MS]. + */ + internal const val MAX_BACKOFF_DELAY_MS = 30_000L + + /** + * Exponential backoff: `min(base * 2^(attempt-1), MAX_BACKOFF_DELAY_MS) + jitter`. + * + * The clamp is load-bearing, not defensive. `1L shl 63` is + * [Long.MIN_VALUE] and `1L shl 64` wraps the shift back to 1, so an + * unclamped product goes negative at attempt 54, lands on exactly 0 at + * 63, and collapses to the base value past 64 — and `delay()` returns + * immediately for any non-positive argument. The result is a client + * hammering a server that is already answering 429/503, which is the + * precise traffic pattern backoff exists to prevent. + * + * The multiplier is compared against `MAX_BACKOFF_DELAY_MS / base` + * before multiplying, so no intermediate ever leaves `Long` range. The + * shift itself is bounded at 62 for the same reason. + */ internal fun calculateBackoffDelay(baseDelayMs: Long, attempt: Int): Long { - val delay = baseDelayMs * (1L shl (attempt - 1)) + val base = baseDelayMs.coerceAtLeast(0L) + val multiplier = 1L shl (attempt - 1).coerceIn(0, 62) + val delay = when { + base == 0L -> 0L + multiplier > MAX_BACKOFF_DELAY_MS / base -> MAX_BACKOFF_DELAY_MS + else -> base * multiplier + } val jitter = (kotlin.random.Random.nextLong(MAX_JITTER_MS)) return delay + jitter } diff --git a/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/RetryTest.kt b/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/RetryTest.kt index b4752ccc4e..a00db9bb84 100644 --- a/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/RetryTest.kt +++ b/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/RetryTest.kt @@ -36,6 +36,57 @@ class RetryTest { assert(delay3 in 4000..4100) { "Expected ~4000, got $delay3" } } + /** + * #577: the backoff term saturates at [BasecampHttpClient.MAX_BACKOFF_DELAY_MS] + * instead of overflowing. + * + * Before the fix `baseDelayMs * (1L shl (attempt - 1))` wrapped: `1L shl 63` + * sets the sign bit, so with the 1000ms default the product went NEGATIVE at + * attempt 54, and `delay()` returns immediately for a non-positive argument. + * The client stopped backing off entirely and hammered a server that was + * already answering 429/503. `1L shl 64` wraps the shift itself back to 1, + * so past that the delay collapsed to the base value. + * + * The builder validates `maxRetries >= 0` with no upper bound, so a caller + * setting a large cap reaches these attempt numbers on a long failure streak. + * + * The bound asserted is two-sided, and that matters: a one-sided "never too + * long" check would pass on the two worst attempts. `1L shl 63` is + * `Long.MIN_VALUE`, and `1000 * Long.MIN_VALUE` wraps to exactly 0 — a 0ms + * backoff that looks perfectly in range while being the tight loop itself. + * Once the exponential term is past the ceiling the delay must SIT at the + * ceiling, so 0ms and a collapsed 1000ms both fail. + */ + @Test + fun backoffDelaySaturatesInsteadOfOverflowing() { + val base = 1000L + val ceiling = BasecampHttpClient.MAX_BACKOFF_DELAY_MS + 100L + + // With base 1000, attempt 6 is the first whose unclamped term (32,000ms) + // exceeds the 30,000ms ceiling, so every attempt from here on must land + // in [MAX_BACKOFF_DELAY_MS, MAX_BACKOFF_DELAY_MS + MAX_JITTER_MS]. + // Collected rather than asserted one at a time so a failure names every + // overflow shape at once. + val violations = listOf(6, 10, 53, 54, 55, 62, 63, 64, 65, 128, Int.MAX_VALUE) + .map { it to BasecampHttpClient.calculateBackoffDelay(base, it) } + .filter { (_, delay) -> delay !in BasecampHttpClient.MAX_BACKOFF_DELAY_MS..ceiling } + assert(violations.isEmpty()) { + "expected every delay in ${BasecampHttpClient.MAX_BACKOFF_DELAY_MS}..$ceiling ms, got " + + violations.joinToString { (attempt, delay) -> "attempt $attempt -> ${delay}ms" } + } + + // The ceiling binds base_delay_ms itself (SPEC §7 requirement 3). + val hugeBase = BasecampHttpClient.calculateBackoffDelay(600_000L, 1) + assert(hugeBase in 0..ceiling) { + "a base delay above the ceiling must clamp to it, got $hugeBase" + } + + // A zero/negative base delay stays at zero rather than saturating. + assert(BasecampHttpClient.calculateBackoffDelay(0L, 90) in 0..100L) { + "a zero base delay must not produce a ceiling-length sleep" + } + } + @Test fun retryOn429ForGet() = runTest { var requestCount = 0 diff --git a/python/src/basecamp/_async_http.py b/python/src/basecamp/_async_http.py index 8fd7acc888..4fca584ed2 100644 --- a/python/src/basecamp/_async_http.py +++ b/python/src/basecamp/_async_http.py @@ -8,6 +8,7 @@ from basecamp import _security from basecamp._version import API_VERSION, VERSION +from basecamp.config import saturating_backoff from basecamp.errors import ( ApiError, AuthError, @@ -373,9 +374,11 @@ def _build_url(self, path: str) -> str: return f"{self._config.base_url}{path}" def _calculate_delay(self, attempt: int, server_retry_after: int | None = None) -> float: + # Retry-After is server-directed and exempt from the ceiling (SPEC + # section 7); only the locally-computed term saturates. if server_retry_after and server_retry_after > 0: return float(server_retry_after) - base = self._config.base_delay * (2 ** (attempt - 1)) + base = saturating_backoff(self._config.base_delay, attempt) jitter = random.random() * self._config.max_jitter return base + jitter diff --git a/python/src/basecamp/_http.py b/python/src/basecamp/_http.py index 5a6f63de49..6b63cb4031 100644 --- a/python/src/basecamp/_http.py +++ b/python/src/basecamp/_http.py @@ -7,6 +7,7 @@ from basecamp import _security from basecamp._version import API_VERSION, VERSION +from basecamp.config import saturating_backoff from basecamp.errors import ( ApiError, AuthError, @@ -371,9 +372,11 @@ def _build_url(self, path: str) -> str: return f"{self._config.base_url}{path}" def _calculate_delay(self, attempt: int, server_retry_after: int | None = None) -> float: + # Retry-After is server-directed and exempt from the ceiling (SPEC + # section 7); only the locally-computed term saturates. if server_retry_after and server_retry_after > 0: return float(server_retry_after) - base = self._config.base_delay * (2 ** (attempt - 1)) + base = saturating_backoff(self._config.base_delay, attempt) jitter = random.random() * self._config.max_jitter return base + jitter diff --git a/python/src/basecamp/config.py b/python/src/basecamp/config.py index 59ffcccd6c..f7dc2fe056 100644 --- a/python/src/basecamp/config.py +++ b/python/src/basecamp/config.py @@ -12,6 +12,32 @@ DEFAULT_MAX_JITTER = 0.1 DEFAULT_MAX_PAGES = 10_000 +# Ceiling on the backoff term (SPEC section 7, "Backoff Ceiling"), in seconds. +# Jitter is added after the clamp, so the longest single backoff sleep is this +# plus max_jitter. +MAX_BACKOFF_DELAY = 30.0 + +# Largest exponent evaluated before the clamp takes over. 2**64 is 1.8e19, so +# with any base delay at all the ceiling is long since reached; the bound exists +# because Python integers are unbounded and 2 ** 10_000 is a real three-kilobyte +# integer, not an overflow. +_MAX_BACKOFF_EXPONENT = 64 + + +def saturating_backoff(base_delay: float, attempt: int) -> float: + """Exponential backoff for a 1-based attempt, saturating at MAX_BACKOFF_DELAY. + + The clamp is load-bearing rather than defensive. Python's ``**`` does not + overflow — it promotes — so ``base_delay * (2 ** (attempt - 1))`` for a long + failure streak either raises ``OverflowError`` converting an arbitrary- + precision integer to a float, or hands ``time.sleep`` a delay measured in + geological time. Neither is a retry. + """ + if base_delay <= 0: + return 0.0 + exponent = min(max(attempt - 1, 0), _MAX_BACKOFF_EXPONENT) + return min(base_delay * (2**exponent), MAX_BACKOFF_DELAY) + @dataclass(frozen=True) class Config: diff --git a/python/tests/test_backoff_ceiling.py b/python/tests/test_backoff_ceiling.py new file mode 100644 index 0000000000..24961cfed7 --- /dev/null +++ b/python/tests/test_backoff_ceiling.py @@ -0,0 +1,83 @@ +"""SPEC section 7 "Backoff Ceiling" (#577). + +Python's ``**`` promotes rather than overflowing, so ``base_delay * (2 ** (attempt - 1))`` +does not wrap the way the compiled SDKs do — it builds an arbitrary-precision +integer. The two failure shapes that produces are both worse than a long sleep: +``OverflowError`` out of the float conversion, aborting the retry with an +exception the caller never sees documented, and, before it gets that big, a +``time.sleep`` measured in millennia. + +Both the sync and async transports carry their own copy of ``_calculate_delay``, +so both are exercised here — fixing one and not the other is the shape of bug +this file exists to prevent. +""" + +from __future__ import annotations + +import pytest + +from basecamp._async_http import AsyncHttpClient +from basecamp._http import HttpClient +from basecamp.config import MAX_BACKOFF_DELAY, Config, saturating_backoff + +# With the 1.0s default base, attempt 6 is the first whose unclamped term (32s) +# exceeds the 30s ceiling, so every attempt from there on must sit at the cap. +SATURATED_ATTEMPTS = [6, 10, 64, 128, 1024, 10_000, 2**31] + + +def _clients() -> list[HttpClient | AsyncHttpClient]: + """A sync and an async client sharing one default Config. + + Built via ``__new__`` because ``_calculate_delay`` reads only + ``self._config``; constructing real clients would open an httpx pool for a + pure arithmetic assertion. + """ + config = Config() + clients: list[HttpClient | AsyncHttpClient] = [] + for cls in (HttpClient, AsyncHttpClient): + client = cls.__new__(cls) + client._config = config + clients.append(client) + return clients + + +@pytest.mark.parametrize("attempt", SATURATED_ATTEMPTS) +def test_calculate_delay_saturates(attempt: int) -> None: + """Every transport's computed delay lands within the ceiling plus jitter. + + The bound is two-sided: once the exponential term passes the ceiling the + delay must SIT at the ceiling, so a formula that collapsed to zero would + fail here too rather than sneaking past a one-sided check. + """ + config = Config() + for client in _clients(): + delay = client._calculate_delay(attempt) # type: ignore[attr-defined] + assert MAX_BACKOFF_DELAY <= delay <= MAX_BACKOFF_DELAY + config.max_jitter, ( + f"{type(client).__name__}: attempt {attempt} produced {delay}s" + ) + + +def test_calculate_delay_unchanged_below_the_ceiling() -> None: + """The delays a shipped configuration actually reaches are untouched.""" + config = Config() + for client in _clients(): + for attempt, want in ((1, 1.0), (2, 2.0), (3, 4.0)): + delay = client._calculate_delay(attempt) # type: ignore[attr-defined] + assert want <= delay <= want + config.max_jitter, ( + f"{type(client).__name__}: attempt {attempt} produced {delay}s, want ~{want}s" + ) + + +def test_retry_after_is_exempt_from_the_ceiling() -> None: + """SPEC section 7 requirement 4: the ceiling governs the local formula only.""" + for client in _clients(): + assert client._calculate_delay(1, 120) == 120.0 # type: ignore[attr-defined] + + +def test_saturating_backoff_edges() -> None: + # SPEC section 7 requirement 3: the ceiling binds base_delay itself. + assert saturating_backoff(600.0, 1) == MAX_BACKOFF_DELAY + # A zero base delay stays at zero rather than saturating. + assert saturating_backoff(0.0, 10_000) == 0.0 + # Below the ceiling the exponential term is exact. + assert saturating_backoff(1.0, 3) == 4.0 diff --git a/ruby/lib/basecamp/config.rb b/ruby/lib/basecamp/config.rb index 673983e996..5cd9985ea4 100644 --- a/ruby/lib/basecamp/config.rb +++ b/ruby/lib/basecamp/config.rb @@ -45,6 +45,36 @@ class Config DEFAULT_MAX_JITTER = 0.1 DEFAULT_MAX_PAGES = 10_000 + # Ceiling on the backoff term (SPEC §7, "Backoff Ceiling"), in seconds. + # Jitter is added after the clamp, so the longest single backoff sleep is + # this plus +max_jitter+. + MAX_BACKOFF_DELAY = 30.0 + + # Largest exponent evaluated before the clamp takes over. 2**64 is 1.8e19, + # so with any base delay at all the ceiling is long since reached; the bound + # exists because Ruby Integers are unbounded — 2**10_000 is a real + # three-kilobyte number, not an overflow. + MAX_BACKOFF_EXPONENT = 64 + + # Exponential backoff for a 1-based attempt, saturating at MAX_BACKOFF_DELAY. + # + # The clamp is load-bearing rather than defensive. Ruby's +**+ promotes + # instead of overflowing, so +base_delay * (2**(attempt - 1))+ on a long + # failure streak coerces to +Float::INFINITY+ — and +sleep(Float::INFINITY)+ + # never returns. A retry that never happens is not backoff. + # + # @param base_delay [Float] initial backoff delay in seconds + # @param attempt [Integer] 1-based attempt number + # @return [Float] the backoff term in seconds + def self.saturating_backoff(base_delay, attempt) + if base_delay <= 0 + 0.0 + else + exponent = [ [ attempt - 1, 0 ].max, MAX_BACKOFF_EXPONENT ].min + [ base_delay * (2**exponent), MAX_BACKOFF_DELAY ].min.to_f + end + end + # Creates a new configuration with the given options. # # @param base_url [String] API base URL diff --git a/ruby/lib/basecamp/http.rb b/ruby/lib/basecamp/http.rb index d3b3b2aa49..206640b55e 100644 --- a/ruby/lib/basecamp/http.rb +++ b/ruby/lib/basecamp/http.rb @@ -666,10 +666,12 @@ def assert_credential_origin!(url, allow_cross_origin) end def calculate_delay(attempt, server_retry_after) + # Retry-After is server-directed and exempt from the ceiling (SPEC §7); + # only the locally-computed term saturates. return server_retry_after if server_retry_after&.positive? - # Exponential backoff: base_delay * 2^(attempt-1) + jitter - base = @config.base_delay * (2**(attempt - 1)) + # Exponential backoff: min(base_delay * 2^(attempt-1), ceiling) + jitter + base = Config.saturating_backoff(@config.base_delay, attempt) jitter = rand * @config.max_jitter base + jitter end diff --git a/ruby/test/basecamp/backoff_ceiling_test.rb b/ruby/test/basecamp/backoff_ceiling_test.rb new file mode 100644 index 0000000000..379ab086b7 --- /dev/null +++ b/ruby/test/basecamp/backoff_ceiling_test.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require "test_helper" + +# SPEC §7 "Backoff Ceiling" (#577). +# +# Ruby Integers are unbounded, so +base_delay * (2**(attempt - 1))+ does not wrap +# the way the compiled SDKs do — it promotes. Coerced against a Float base delay +# the product becomes +Float::INFINITY+, and +sleep(Float::INFINITY)+ never +# returns: the retry loop stops being a retry loop. Before it gets that far the +# delays are already measured in geological time. +class BackoffCeilingTest < Minitest::Test + include TestHelper + + # With the 1.0s default base, attempt 6 is the first whose unclamped term + # (32s) exceeds the 30s ceiling, so every attempt from there on must sit at + # the cap. + SATURATED_ATTEMPTS = [ 6, 10, 64, 128, 1024, 10_000, 2**31 ].freeze + + def setup + @config = Basecamp::Config.new + @http = Basecamp::Http.new(config: @config, token_provider: test_token_provider) + end + + # The bound is two-sided on purpose: once the exponential term passes the + # ceiling the delay must SIT at the ceiling, so a formula that collapsed to + # zero would fail here too rather than sneaking past a one-sided check. + def test_calculate_delay_saturates_at_the_ceiling + ceiling = Basecamp::Config::MAX_BACKOFF_DELAY + violations = SATURATED_ATTEMPTS.filter_map do |attempt| + delay = @http.send(:calculate_delay, attempt, nil) + "attempt #{attempt} -> #{delay}s" unless delay.between?(ceiling, ceiling + @config.max_jitter) + end + + assert_empty violations, + "every backoff must land within #{ceiling}..#{ceiling + @config.max_jitter}s" + end + + def test_delays_below_the_ceiling_are_unchanged + { 1 => 1.0, 2 => 2.0, 3 => 4.0 }.each do |attempt, want| + delay = @http.send(:calculate_delay, attempt, nil) + + assert_operator delay, :>=, want + assert_operator delay, :<=, want + @config.max_jitter + end + end + + # SPEC §7 requirement 4: the ceiling governs the locally-computed formula, not + # the server-directed Retry-After. + def test_retry_after_is_exempt_from_the_ceiling + assert_equal 120, @http.send(:calculate_delay, 1, 120) + end + + def test_saturating_backoff_edges + # SPEC §7 requirement 3: the ceiling binds base_delay itself. + assert_equal Basecamp::Config::MAX_BACKOFF_DELAY, Basecamp::Config.saturating_backoff(600.0, 1) + # A zero base delay stays at zero rather than saturating. + assert_in_delta 0.0, Basecamp::Config.saturating_backoff(0.0, 10_000) + # Below the ceiling the exponential term is exact. + assert_in_delta 4.0, Basecamp::Config.saturating_backoff(1.0, 3) + end +end diff --git a/swift/Sources/Basecamp/HTTP/HTTPClient.swift b/swift/Sources/Basecamp/HTTP/HTTPClient.swift index 5660f44494..27bc264d42 100644 --- a/swift/Sources/Basecamp/HTTP/HTTPClient.swift +++ b/swift/Sources/Basecamp/HTTP/HTTPClient.swift @@ -18,6 +18,11 @@ package final class HTTPClient: Sendable { private static let maxJitterMs: UInt64 = 100 private static let defaultBaseDelayMs: UInt64 = 1_000 + /// Ceiling on the backoff term (SPEC §7, "Backoff Ceiling"). Jitter is + /// added after the clamp, so the longest single backoff sleep is this plus + /// ``maxJitterMs``. + static let maxBackoffDelayMs: UInt64 = 30_000 + /// HTTP methods that are naturally idempotent and therefore always /// retry-eligible (SPEC §7). Hoisted to a static constant so the retry gate /// on the hot path does not allocate a `Set` per request. @@ -32,6 +37,45 @@ package final class HTTPClient: Sendable { private static let downloadMaxAttempts = 3 private static let downloadRetryOn: Set = [429, 502, 503, 504] + /// Raised by the response-type guard when a `Transport` hands back a + /// `URLResponse` that is not an `HTTPURLResponse`. + /// + /// It needs its own type rather than reusing `BasecampError.network`. + /// Since #567 the retry loops route a transport-thrown `.network` to the + /// retry branch, and a non-HTTP response is a deterministic programming + /// error — retrying it three times just repeats it. This type never + /// escapes the SDK: every site that raises it surfaces + /// ``invalidResponseType`` instead, so the public error contract is + /// exactly what it was. + private struct InvalidResponseTypeError: Error {} + + /// The caller-facing error for a non-HTTP `URLResponse`. Unchanged from + /// before #567 — same case, same message. + private static var invalidResponseType: BasecampError { + .network(message: "Invalid response type", cause: nil) + } + + /// Whether a thrown error is the transport reporting a connectivity + /// failure, and therefore eligible for the retry branch (SPEC §7). + /// + /// `Transport` is `public` and documented as a seam, so a consumer wiring + /// in their own networking stack will normalize connection resets and + /// timeouts into `BasecampError.network` — that is precisely the + /// classification §6 defines for the condition, `retryable: true`. Before + /// #567 both retry loops classified *every* `BasecampError` out of the + /// transport as terminal, so the more carefully an implementer read the + /// error taxonomy, the more certainly they disabled their own retries. + /// + /// Any other `BasecampError` (`.auth`, `.usage`, `.api`, …) is the + /// transport's own final verdict on the request and stays terminal on + /// sight, as does ``InvalidResponseTypeError``, which is not a transport + /// failure at all. + private static func isTransportNetworkFailure(_ error: any Error) -> Bool { + guard let basecampError = error as? BasecampError else { return true } + if case .network = basecampError { return true } + return false + } + /// Whether an error represents cooperative cancellation. /// /// Swift concurrency raises `CancellationError`, but `URLSession` reports a @@ -147,7 +191,7 @@ package final class HTTPClient: Sendable { let (data, response) = try await transport.data(for: request) guard let httpResponse = response as? HTTPURLResponse else { - throw BasecampError.network(message: "Invalid response type", cause: nil) + throw InvalidResponseTypeError() } let durationMs = Int((CFAbsoluteTimeGetCurrent() - startTime) * 1000) @@ -198,12 +242,20 @@ package final class HTTPClient: Sendable { directive = .done(data, httpResponse) } } - } catch let error as BasecampError { - // A BasecampError thrown by the transport (or the response-type - // guard) is rethrown untouched, with no request-end event. + } catch is InvalidResponseTypeError { + // A deterministic programming error, not a transport blip: + // terminal on sight, with no request-end event, exactly as it + // behaved when the guard raised BasecampError.network directly. + directive = .fail(Self.invalidResponseType) + } catch let error as BasecampError where !Self.isTransportNetworkFailure(error) { + // A non-network BasecampError thrown by the transport is its + // own final verdict: rethrown untouched, with no request-end + // event. directive = .fail(error) } catch { - // Network-level error + // Network-level error — a raw transport failure, or a Transport + // that reports connectivity failure as BasecampError.network + // (#567). Both mean the same thing, so both retry. let durationMs = Int((CFAbsoluteTimeGetCurrent() - startTime) * 1000) safeInvokeHooks { $0.onRequestEnd(info, result: RequestResult(statusCode: 0, durationMs: durationMs)) @@ -226,7 +278,12 @@ package final class HTTPClient: Sendable { ) directive = .retry(error: error, delaySeconds: delaySeconds) } else { - directive = .fail(BasecampError.network(message: "Network error", cause: error)) + // A transport that already speaks BasecampError keeps its + // own message; wrapping it in a second, vaguer .network + // would discard the only diagnostic the caller has. + directive = .fail( + error as? BasecampError + ?? BasecampError.network(message: "Network error", cause: error)) } } @@ -272,7 +329,10 @@ package final class HTTPClient: Sendable { let (data, response) = try await transport.data(for: request) guard let httpResponse = response as? HTTPURLResponse else { - throw BasecampError.network(message: "Invalid response type", cause: nil) + // Neither of these two helpers retries, so the guard can surface + // the caller-facing error directly. Routed through the shared + // constant so the message stays defined in exactly one place. + throw Self.invalidResponseType } return (data, httpResponse) @@ -322,7 +382,7 @@ package final class HTTPClient: Sendable { let (data, response) = try await transport.dataNoRedirect(for: request) guard let httpResponse = response as? HTTPURLResponse else { - throw BasecampError.network(message: "Invalid response type", cause: nil) + throw InvalidResponseTypeError() } let durationMs = Int((CFAbsoluteTimeGetCurrent() - startTime) * 1000) @@ -349,9 +409,15 @@ package final class HTTPClient: Sendable { } else { directive = .done(data, httpResponse) } - } catch let error as BasecampError { + } catch is InvalidResponseTypeError { + directive = .fail(Self.invalidResponseType) + } catch let error as BasecampError where !Self.isTransportNetworkFailure(error) { directive = .fail(error) } catch { + // Both loops classify identically (#567): a transport-thrown + // BasecampError.network is a connectivity failure and retries, + // the same as a raw URLError would. Splitting the two loops + // here would be worse than the gap it closes. let durationMs = Int((CFAbsoluteTimeGetCurrent() - startTime) * 1000) safeInvokeHooks { $0.onRequestEnd(info, result: RequestResult(statusCode: 0, durationMs: durationMs)) @@ -374,7 +440,9 @@ package final class HTTPClient: Sendable { ) directive = .retry(error: error, delaySeconds: delaySeconds) } else { - directive = .fail(BasecampError.network(message: "Network error", cause: error)) + directive = .fail( + error as? BasecampError + ?? BasecampError.network(message: "Network error", cause: error)) } } @@ -414,7 +482,10 @@ package final class HTTPClient: Sendable { let (data, response) = try await transport.data(for: request) guard let httpResponse = response as? HTTPURLResponse else { - throw BasecampError.network(message: "Invalid response type", cause: nil) + // Neither of these two helpers retries, so the guard can surface + // the caller-facing error directly. Routed through the shared + // constant so the message stays defined in exactly one place. + throw Self.invalidResponseType } return (data, httpResponse) @@ -451,19 +522,46 @@ package final class HTTPClient: Sendable { return TimeInterval(retryAfter) } - let base: UInt64 + let base = Self.backoffDelayMs(baseDelayMs: baseDelayMs, backoff: backoff, attempt: attempt) + + // Add jitter (0-100ms) + let jitter = UInt64.random(in: 0...Self.maxJitterMs) + return TimeInterval(base + jitter) / 1000.0 + } + + /// The backoff term in milliseconds for a 1-based attempt, saturating at + /// ``maxBackoffDelayMs`` (SPEC §7, "Backoff Ceiling"). + /// + /// The clamp is load-bearing rather than defensive, and Swift's failure + /// without it is the worst of the six SDKs: `baseDelayMs * (1 << ...)` + /// **traps**. `<<` on an unsigned integer is a smart shift, so an + /// over-shift silently yields `0` — the tight retry loop against a server + /// already answering 429/503 — but at `1 << 63` the multiply overflows + /// `UInt64` and the process dies. `UInt64(attempt - 1)` traps on a + /// negative operand for the same reason, so the exponent is floored, not + /// converted. + /// + /// The multiplier is compared against `maxBackoffDelayMs / baseDelayMs` + /// before multiplying, so no intermediate can leave `UInt64` range. + static func backoffDelayMs(baseDelayMs: UInt64, backoff: RetryBackoff, attempt: Int) -> UInt64 { + guard baseDelayMs > 0 else { return 0 } + + let multiplier: UInt64 switch backoff { case .exponential: - base = baseDelayMs * (1 << UInt64(attempt - 1)) + // 63 is the first shift that sets the sign bit's worth of + // magnitude; at or past it the ceiling is reached regardless. + let exponent = max(attempt - 1, 0) + guard exponent < 63 else { return maxBackoffDelayMs } + multiplier = 1 << UInt64(exponent) case .linear: - base = baseDelayMs * UInt64(attempt) + multiplier = UInt64(max(attempt, 1)) case .constant: - base = baseDelayMs + multiplier = 1 } - // Add jitter (0-100ms) - let jitter = UInt64.random(in: 0...Self.maxJitterMs) - return TimeInterval(base + jitter) / 1000.0 + guard multiplier <= maxBackoffDelayMs / baseDelayMs else { return maxBackoffDelayMs } + return baseDelayMs * multiplier } private func safeInvokeHooks(_ invoke: (any BasecampHooks) -> Void) { diff --git a/swift/Tests/BasecampTests/BackoffCeilingTests.swift b/swift/Tests/BasecampTests/BackoffCeilingTests.swift new file mode 100644 index 0000000000..d1fafcbe6e --- /dev/null +++ b/swift/Tests/BasecampTests/BackoffCeilingTests.swift @@ -0,0 +1,67 @@ +import XCTest +@testable import Basecamp + +/// SPEC §7 "Backoff Ceiling" (#577). +/// +/// Swift's failure mode is the worst of the six SDKs and was tracked nowhere: +/// `baseDelayMs * (1 << UInt64(attempt - 1))` **traps**. `<<` on an unsigned +/// integer is a smart shift, so an over-shift silently yields `0` — a tight +/// retry loop against a server already answering 429/503 — but at `1 << 63` the +/// multiply overflows `UInt64` and the process dies with +/// "Swift runtime failure: arithmetic overflow". A crash is never the right +/// answer to a run of 503s. +final class BackoffCeilingTests: XCTestCase { + + /// With the 1000ms default base, attempt 6 is the first whose unclamped + /// term (32,000ms) exceeds the 30,000ms ceiling, so every attempt from + /// there on must sit at the cap. + /// + /// The bound is two-sided on purpose: a one-sided "never too long" check + /// would pass on the over-shift attempts, which return 0 and are the tight + /// loop itself. + func testExponentialBackoffSaturates() { + for attempt in [6, 10, 40, 62, 63, 64, 65, 128, Int.max] { + let delay = HTTPClient.backoffDelayMs( + baseDelayMs: 1_000, backoff: .exponential, attempt: attempt) + XCTAssertEqual( + delay, HTTPClient.maxBackoffDelayMs, + "attempt \(attempt) produced \(delay)ms") + } + } + + /// The delays a shipped configuration actually reaches are untouched: + /// `behavior-model.json` tops out at `base_delay_ms: 2000` over at most + /// three attempts, so nothing on a default path approaches the ceiling. + func testDelaysBelowTheCeilingAreExact() { + for (attempt, want) in [(1, UInt64(1_000)), (2, 2_000), (3, 4_000)] { + XCTAssertEqual( + HTTPClient.backoffDelayMs(baseDelayMs: 1_000, backoff: .exponential, attempt: attempt), + want) + } + } + + func testLinearAndConstantBackoffAreClampedToo() { + XCTAssertEqual( + HTTPClient.backoffDelayMs(baseDelayMs: 1_000, backoff: .linear, attempt: Int.max), + HTTPClient.maxBackoffDelayMs) + // SPEC §7 requirement 3: the ceiling binds base_delay_ms itself, with + // no carve-out for the first sleep. + XCTAssertEqual( + HTTPClient.backoffDelayMs(baseDelayMs: 600_000, backoff: .constant, attempt: 1), + HTTPClient.maxBackoffDelayMs) + } + + /// A zero base delay must stay at zero rather than saturating — and must + /// not divide by zero on the way there. + func testZeroBaseDelayStaysZero() { + XCTAssertEqual( + HTTPClient.backoffDelayMs(baseDelayMs: 0, backoff: .exponential, attempt: 90), 0) + } + + /// `UInt64(attempt - 1)` traps on a negative operand, so a zero or negative + /// attempt must be floored rather than converted. + func testNonPositiveAttemptDoesNotTrap() { + XCTAssertEqual( + HTTPClient.backoffDelayMs(baseDelayMs: 1_000, backoff: .exponential, attempt: 0), 1_000) + } +} diff --git a/swift/Tests/BasecampTests/TransportNetworkErrorRetryTests.swift b/swift/Tests/BasecampTests/TransportNetworkErrorRetryTests.swift new file mode 100644 index 0000000000..3fc980ebe1 --- /dev/null +++ b/swift/Tests/BasecampTests/TransportNetworkErrorRetryTests.swift @@ -0,0 +1,332 @@ +import XCTest +@testable import Basecamp + +/// Thread-safe counter for use in `@Sendable` closures. +private final class AttemptCounter: @unchecked Sendable { + private let lock = NSLock() + private var _value = 0 + + var value: Int { lock.withLock { _value } } + + @discardableResult + func increment() -> Int { + lock.withLock { + _value += 1 + return _value + } + } +} + +/// A transport that reports connectivity failure as the SDK's own error type. +/// +/// This is the natural implementation, not a contrived one: `Transport` is +/// `public`, and an app wiring its existing networking stack into the seam +/// reaches for `BasecampError.network` because that is exactly the +/// classification §6 defines for the condition — `retryable: true`. +private final class BasecampErrorTransport: Transport, @unchecked Sendable { + private let counter: AttemptCounter + private let failuresBeforeSuccess: Int + private let error: BasecampError + + init(counter: AttemptCounter, failuresBeforeSuccess: Int, error: BasecampError) { + self.counter = counter + self.failuresBeforeSuccess = failuresBeforeSuccess + self.error = error + } + + private func respond(to request: URLRequest) throws -> (Data, URLResponse) { + if counter.increment() <= failuresBeforeSuccess { + throw error + } + let response = HTTPURLResponse( + url: request.url!, statusCode: 200, + httpVersion: "HTTP/1.1", headerFields: [:] + )! + return (Data("{}".utf8), response) + } + + func data(for request: URLRequest) async throws -> (Data, URLResponse) { + try respond(to: request) + } + + func dataNoRedirect(for request: URLRequest) async throws -> (Data, URLResponse) { + try respond(to: request) + } +} + +/// A transport whose failures are its own error type — the shape that already +/// retried, kept here as the control the `BasecampError.network` cases are +/// compared against. +private struct ForeignTransportError: Error {} + +/// #567: a `Transport` that throws `BasecampError.network` must be retried. +/// +/// Both retry loops classified **every** `BasecampError` out of the transport as +/// terminal, so a transport that normalized connectivity failures into the SDK's +/// own retryable classification got exactly one attempt with retries enabled, +/// while a transport throwing an arbitrary foreign error retried normally. The +/// behavior was inverted from what an implementer would predict, and the more +/// carefully the implementer read §6, the more likely they were to hit it. +/// +/// The catch could not simply be deleted: it also swallowed the response-type +/// guard, which throws `BasecampError.network("Invalid response type")` for a +/// non-HTTP `URLResponse`. That is a deterministic programming error — retrying +/// it three times just repeats it — so it now raises a distinct internal error +/// and is converted back at the boundary, keeping the public error contract. +final class TransportNetworkErrorRetryTests: XCTestCase { + + private var retryConfig: RetryConfig { + RetryConfig(maxAttempts: 3, baseDelayMs: 1, backoff: .constant, retryOn: [429, 503]) + } + + // MARK: - performRequest + + func testTransportBasecampNetworkErrorIsRetried() async throws { + let counter = AttemptCounter() + let transport = BasecampErrorTransport( + counter: counter, failuresBeforeSuccess: 1, + error: .network(message: "Connection reset", cause: nil)) + + let client = BasecampClient( + tokenProvider: StaticTokenProvider("test-token"), + userAgent: "test-suite", + config: BasecampConfig(baseURL: "https://3.basecampapi.com", enableRetry: true), + transport: transport + ) + + let (_, response) = try await client.forAccount("999999999").httpClient.performRequest( + method: "GET", + url: "https://3.basecampapi.com/999999999/projects.json", + retryConfig: retryConfig + ) + + XCTAssertEqual(response.statusCode, 200) + XCTAssertEqual(counter.value, 2, "a transport-thrown .network must reach the retry branch") + } + + /// On exhaustion the transport's own error surfaces rather than being + /// re-wrapped in a second, less informative `.network`. + func testTransportBasecampNetworkErrorSurfacesUnwrappedOnExhaustion() async throws { + let counter = AttemptCounter() + let transport = BasecampErrorTransport( + counter: counter, failuresBeforeSuccess: .max, + error: .network(message: "Connection reset", cause: nil)) + + let client = BasecampClient( + tokenProvider: StaticTokenProvider("test-token"), + userAgent: "test-suite", + config: BasecampConfig(baseURL: "https://3.basecampapi.com", enableRetry: true), + transport: transport + ) + + do { + _ = try await client.forAccount("999999999").httpClient.performRequest( + method: "GET", + url: "https://3.basecampapi.com/999999999/projects.json", + retryConfig: retryConfig + ) + XCTFail("Expected the transport's error") + } catch let error as BasecampError { + XCTAssertEqual(error.message, "Connection reset") + } + + XCTAssertEqual(counter.value, 3, "should have spent the whole retry budget") + } + + /// The gate is idempotency, not error type: SPEC §7's network-error retry + /// runs behind the same three-gate check as the status retry, so a + /// non-idempotent POST is still attempted exactly once. + func testNonIdempotentPostIsNotRetriedOnBasecampNetworkError() async { + let counter = AttemptCounter() + let transport = BasecampErrorTransport( + counter: counter, failuresBeforeSuccess: .max, + error: .network(message: "Connection reset", cause: nil)) + + let client = BasecampClient( + tokenProvider: StaticTokenProvider("test-token"), + userAgent: "test-suite", + config: BasecampConfig(baseURL: "https://3.basecampapi.com", enableRetry: true), + transport: transport + ) + + _ = try? await client.forAccount("999999999").httpClient.performRequest( + method: "POST", + url: "https://3.basecampapi.com/999999999/projects.json", + retryConfig: retryConfig, + idempotent: false + ) + + XCTAssertEqual(counter.value, 1, "a non-idempotent POST is attempted exactly once") + } + + /// Only `.network` moves. Every other `BasecampError` out of the transport + /// is the transport's own final verdict and stays terminal on sight. + func testNonNetworkBasecampErrorsStayTerminal() async { + for error in [ + BasecampError.auth(message: "nope", hint: nil, requestId: nil), + BasecampError.usage(message: "bad config", hint: nil), + BasecampError.api(message: "boom", httpStatus: 500, hint: nil, requestId: nil), + ] { + let counter = AttemptCounter() + let transport = BasecampErrorTransport( + counter: counter, failuresBeforeSuccess: .max, error: error) + + let client = BasecampClient( + tokenProvider: StaticTokenProvider("test-token"), + userAgent: "test-suite", + config: BasecampConfig(baseURL: "https://3.basecampapi.com", enableRetry: true), + transport: transport + ) + + _ = try? await client.forAccount("999999999").httpClient.performRequest( + method: "GET", + url: "https://3.basecampapi.com/999999999/projects.json", + retryConfig: retryConfig + ) + + XCTAssertEqual(counter.value, 1, "\(error) must not consume retry budget") + } + } + + // MARK: - Download hop 1 + + /// Both loops move together. Fixing only one would leave two classifications + /// of the same error inside one SDK, which is worse than the gap. + func testDownloadHopRetriesTransportBasecampNetworkError() async throws { + let counter = AttemptCounter() + let transport = BasecampErrorTransport( + counter: counter, failuresBeforeSuccess: 1, + error: .network(message: "Connection reset", cause: nil)) + + let client = BasecampClient( + tokenProvider: StaticTokenProvider("test-token"), + userAgent: "test-suite", + config: BasecampConfig(baseURL: "https://3.basecampapi.com", enableRetry: true), + transport: transport + ) + + let (_, response) = try await client.httpClient.performDownloadRequest( + url: "https://3.basecampapi.com/999999999/attachment.json") + + XCTAssertEqual(response.statusCode, 200) + XCTAssertEqual(counter.value, 2, "the download hop must retry a transport-thrown .network") + } + + // MARK: - The response-type guard + + /// A `Transport` that returns a non-HTTP `URLResponse` is a deterministic + /// programming error, not a connectivity blip: it must fail on the first + /// attempt, and it must still surface as the same `BasecampError.network` + /// with the same message it always did. + func testInvalidResponseTypeIsNotRetriedAndKeepsItsPublicShape() async { + let counter = AttemptCounter() + let transport = NonHTTPResponseTransport(counter: counter) + + let client = BasecampClient( + tokenProvider: StaticTokenProvider("test-token"), + userAgent: "test-suite", + config: BasecampConfig(baseURL: "https://3.basecampapi.com", enableRetry: true), + transport: transport + ) + + do { + _ = try await client.forAccount("999999999").httpClient.performRequest( + method: "GET", + url: "https://3.basecampapi.com/999999999/projects.json", + retryConfig: retryConfig + ) + XCTFail("Expected the response-type guard to fire") + } catch let error as BasecampError { + guard case .network(let message, _) = error else { + return XCTFail("Expected .network, got \(error)") + } + XCTAssertEqual(message, "Invalid response type") + } catch { + XCTFail("Expected a BasecampError, got \(error)") + } + + XCTAssertEqual(counter.value, 1, "a deterministic guard must not consume retry budget") + } + + func testInvalidResponseTypeOnDownloadHopIsNotRetried() async { + let counter = AttemptCounter() + let transport = NonHTTPResponseTransport(counter: counter) + + let client = BasecampClient( + tokenProvider: StaticTokenProvider("test-token"), + userAgent: "test-suite", + config: BasecampConfig(baseURL: "https://3.basecampapi.com", enableRetry: true), + transport: transport + ) + + do { + _ = try await client.httpClient.performDownloadRequest( + url: "https://3.basecampapi.com/999999999/attachment.json") + XCTFail("Expected the response-type guard to fire") + } catch let error as BasecampError { + guard case .network(let message, _) = error else { + return XCTFail("Expected .network, got \(error)") + } + XCTAssertEqual(message, "Invalid response type") + } catch { + XCTFail("Expected a BasecampError, got \(error)") + } + + XCTAssertEqual(counter.value, 1, "a deterministic guard must not consume retry budget") + } + + // MARK: - Control + + /// The pre-existing contract: a transport throwing its own error type + /// retries. Pinned so the fix is shown to have converged the two shapes + /// rather than swapped which one is broken. + func testForeignTransportErrorStillRetries() async throws { + let counter = AttemptCounter() + let transport = MockTransport { request in + if counter.increment() == 1 { + throw ForeignTransportError() + } + let response = HTTPURLResponse( + url: request.url!, statusCode: 200, + httpVersion: "HTTP/1.1", headerFields: [:] + )! + return (Data("{}".utf8), response) + } + + let client = makeTestClient(transport: transport, enableRetry: true) + + let (_, response) = try await client.forAccount("999999999").httpClient.performRequest( + method: "GET", + url: "https://3.basecampapi.com/999999999/projects.json", + retryConfig: retryConfig + ) + + XCTAssertEqual(response.statusCode, 200) + XCTAssertEqual(counter.value, 2) + } +} + +/// A transport that hands back a bare `URLResponse`, tripping the SDK's +/// response-type guard. +private final class NonHTTPResponseTransport: Transport, @unchecked Sendable { + private let counter: AttemptCounter + + init(counter: AttemptCounter) { + self.counter = counter + } + + private func respond(to request: URLRequest) -> (Data, URLResponse) { + counter.increment() + return (Data(), URLResponse( + url: request.url!, mimeType: nil, + expectedContentLength: 0, textEncodingName: nil)) + } + + func data(for request: URLRequest) async throws -> (Data, URLResponse) { + respond(to: request) + } + + func dataNoRedirect(for request: URLRequest) async throws -> (Data, URLResponse) { + respond(to: request) + } +} diff --git a/typescript/src/retry.ts b/typescript/src/retry.ts index ff210fa8b2..e87c10dd9c 100644 --- a/typescript/src/retry.ts +++ b/typescript/src/retry.ts @@ -35,6 +35,13 @@ export const NO_RETRY_CONFIG: RetryConfig = { const MAX_JITTER_MS = 100; +/** + * Ceiling on the backoff term (SPEC §7, "Backoff Ceiling"). Jitter is added + * after the clamp, so the longest single backoff sleep is this plus + * `MAX_JITTER_MS`. + */ +export const MAX_BACKOFF_DELAY_MS = 30_000; + /** * Lifecycle seams the retry loop emits through. The loop begins EVERY attempt * and finalizes the attempts it abandons (before the backoff sleep); the @@ -175,22 +182,52 @@ export async function executeWithRetry( } } -export function calculateBackoffDelay(config: RetryConfig, attempt: number): number { - const base = config.baseDelayMs; - let delay: number; +/** + * The backoff term, saturating at {@link MAX_BACKOFF_DELAY_MS} (SPEC §7). + * + * Shared by both of the SDK's retry loops — this one and the raw multipart + * loop in `services/base.ts` — so the ceiling cannot be honored on one path + * and not the other. + * + * The clamp is load-bearing rather than defensive. `Math.pow(2, attempt)` + * reaches `Infinity` at attempt 1024, and `setTimeout` clamps an out-of-range + * delay to **1ms**: the failure mode of an unbounded backoff in JS is not a + * long sleep, it is a tight retry loop against a server that is already + * answering 429/503. Well before that, the computed delays run to millennia. + * + * `attempt` is the 0-indexed retry count (first retry = 0), matching the SPEC + * §7 `retry_index`. + */ +export function saturatingBackoff( + baseDelayMs: number, + backoff: RetryConfig["backoff"], + attempt: number, +): number { + const base = baseDelayMs > 0 ? baseDelayMs : 0; + const index = attempt > 0 ? attempt : 0; - switch (config.backoff) { + let delay: number; + switch (backoff) { case "exponential": - delay = base * Math.pow(2, attempt); + // Bounding the exponent keeps the product finite: 2^53 is already 9e15, + // so with any base at all the ceiling is long since reached, and + // Math.min below does the rest without ever seeing Infinity or NaN. + delay = base * Math.pow(2, Math.min(index, 53)); break; case "linear": - delay = base * (attempt + 1); + delay = base * (Math.min(index, Number.MAX_SAFE_INTEGER) + 1); break; case "constant": default: delay = base; } + return Math.min(delay, MAX_BACKOFF_DELAY_MS); +} + +export function calculateBackoffDelay(config: RetryConfig, attempt: number): number { + const delay = saturatingBackoff(config.baseDelayMs, config.backoff, attempt); + // Add jitter (0-100ms) const jitter = Math.random() * MAX_JITTER_MS; return delay + jitter; diff --git a/typescript/src/services/base.ts b/typescript/src/services/base.ts index 2ab8e1f884..18ac848c2c 100644 --- a/typescript/src/services/base.ts +++ b/typescript/src/services/base.ts @@ -27,6 +27,7 @@ import { BasecampError, errorFromParsedBody, errorFromResponse } from "../errors import metadata from "../generated/metadata.js"; import { ListResult, parseTotalCount, type PaginationOptions } from "../pagination.js"; import { parseNextLink, resolveURL, isSameOrigin } from "../pagination-utils.js"; +import { saturatingBackoff } from "../retry.js"; import type { paths } from "../generated/schema.js"; import type createClient from "openapi-fetch"; @@ -208,9 +209,11 @@ export abstract class BaseService { const retryAfter = response.status === 429 ? parseInt(response.headers.get("Retry-After") ?? "", 10) * 1000 : NaN; + // The locally-computed term is bounded by SPEC §7's ceiling; the + // server-directed Retry-After is not, per the same section. const delay = !isNaN(retryAfter) && retryAfter >= 0 ? retryAfter - : (retryConfig.baseDelayMs ?? 1000) * Math.pow(2, attempt); + : saturatingBackoff(retryConfig.baseDelayMs ?? 1000, "exponential", attempt); try { const retryError = new Error(`${response.status} ${response.statusText}`); diff --git a/typescript/tests/backoff-ceiling.test.ts b/typescript/tests/backoff-ceiling.test.ts new file mode 100644 index 0000000000..4b7447920e --- /dev/null +++ b/typescript/tests/backoff-ceiling.test.ts @@ -0,0 +1,76 @@ +import { describe, it, expect } from "vitest"; +import { + calculateBackoffDelay, + saturatingBackoff, + MAX_BACKOFF_DELAY_MS, + type RetryConfig, +} from "../src/retry.js"; + +const MAX_JITTER_MS = 100; + +function config(overrides: Partial = {}): RetryConfig { + return { maxAttempts: 3, baseDelayMs: 1000, backoff: "exponential", retryOn: [429, 503], ...overrides }; +} + +/** + * SPEC §7 "Backoff Ceiling" (#577). + * + * `base * Math.pow(2, attempt)` reaches `Infinity` at attempt 1024 for a 1000ms + * base — and `setTimeout` clamps an out-of-range delay to **1ms**, so the + * failure mode is not a long sleep but a tight retry loop against a server + * already answering 429/503. Long before that, the delays are measured in + * millennia, which is its own kind of broken. + * + * The bound asserted is two-sided: a one-sided "never too long" check would + * pass on exactly the `Infinity` case that tight-loops. + */ +describe("backoff ceiling", () => { + it("saturates the exponential term instead of running away", () => { + // With a 1000ms base, attempt index 5 is the first whose unclamped term + // (32,000ms) exceeds the 30,000ms ceiling. Violations are collected rather + // than asserted one at a time so a failure names every shape at once — + // the millennia-long delays and the Infinity that setTimeout turns into 1ms. + const violations = [5, 10, 40, 100, 1023, 1024, 1025, Number.MAX_SAFE_INTEGER] + .map((attempt) => [attempt, calculateBackoffDelay(config(), attempt)] as const) + .filter(([, delay]) => delay < MAX_BACKOFF_DELAY_MS || delay > MAX_BACKOFF_DELAY_MS + MAX_JITTER_MS) + .map(([attempt, delay]) => `attempt ${attempt} -> ${delay}ms`); + + expect(violations, "every delay must land within the ceiling plus jitter").toEqual([]); + }); + + it("leaves the delays a shipped configuration actually reaches untouched", () => { + for (const [attempt, want] of [[0, 1000], [1, 2000], [2, 4000]] as const) { + const delay = calculateBackoffDelay(config(), attempt); + expect(delay).toBeGreaterThanOrEqual(want); + expect(delay).toBeLessThanOrEqual(want + MAX_JITTER_MS); + } + }); + + it("clamps linear backoff and a base delay above the ceiling", () => { + const linear = calculateBackoffDelay(config({ backoff: "linear" }), Number.MAX_SAFE_INTEGER); + expect(linear).toBeLessThanOrEqual(MAX_BACKOFF_DELAY_MS + MAX_JITTER_MS); + + // SPEC §7 requirement 3: the ceiling binds base_delay_ms itself, with no + // carve-out for the first sleep. + const constant = calculateBackoffDelay(config({ backoff: "constant", baseDelayMs: 600_000 }), 0); + expect(constant).toBeLessThanOrEqual(MAX_BACKOFF_DELAY_MS + MAX_JITTER_MS); + }); + + /** + * TypeScript has two retry loops. `retry.ts` serves the middleware-chained + * JSON path and download hop 1; `services/base.ts` runs its own for the raw + * multipart transport and computed `baseDelayMs * Math.pow(2, attempt)` + * inline. Fixing only `calculateBackoffDelay` would leave the multipart path + * overflowing, so both now go through `saturatingBackoff` — asserted here + * directly, since that shared function IS the multipart path's backoff. + */ + it("saturates the shared term both retry loops compute", () => { + for (const attempt of [5, 1023, 1024, 1025, Number.MAX_SAFE_INTEGER]) { + expect(saturatingBackoff(1000, "exponential", attempt)).toBe(MAX_BACKOFF_DELAY_MS); + } + expect(saturatingBackoff(1000, "exponential", 0)).toBe(1000); + expect(saturatingBackoff(1000, "exponential", 2)).toBe(4000); + // A zero base delay stays at zero rather than saturating. + expect(saturatingBackoff(0, "exponential", 1024)).toBe(0); + }); +}); From b5726c6555bbe784cc0ea5db690bb6b490c4ecc6 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 3 Aug 2026 00:34:44 -0700 Subject: [PATCH 2/3] Detect cancellation through the .network wrapper, and make the ceiling reachable from any base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on the #577/#567 work, one of them a regression this branch introduced. Cancellation through the wrapper (regression, Swift) Routing a transport-thrown BasecampError.network to the retry branch moved it past `catch let error as BasecampError`, where it had been terminal, and into the generic catch. isCancellation looked only at the outer error, so a Transport that normalized a cancelled request into .network(cause: CancellationError()) — the exact shape #567 exists to support — was no longer terminal: the loop fired onRetry, slept, re-authenticated and re-sent a request the caller had already cancelled. Raw URLError(.cancelled) stayed terminal, so one cancellation got two answers, which contradicts the premise of #567 that a network error is classified by meaning and not by type. isCancellation now walks the .network cause chain. The walk is bounded at 8 links rather than recursive: `cause` is caller-supplied and a cycle in it must not hang the retry loop. Red proof, against this branch's own un-fixed HTTPClient.swift: TransportNetworkErrorRetryTests.swift:348: error: -[BasecampTests.TransportNetworkErrorRetryTests testWrappedCancellationIsTerminalOnPerformRequest] : XCTAssertEqual failed: ("3") is not equal to ("1") - CancellationError: a cancelled request must not be re-sent TransportNetworkErrorRetryTests.swift:349: error: -[BasecampTests.TransportNetworkErrorRetryTests testWrappedCancellationIsTerminalOnPerformRequest] : XCTAssertEqual failed: ("2") is not equal to ("0") - CancellationError: onRetry must not fire for a cancellation TransportNetworkErrorRetryTests.swift:372: error: -[BasecampTests.TransportNetworkErrorRetryTests testWrappedCancellationIsTerminalOnTheDownloadHop] : XCTAssertEqual failed: ("3") is not equal to ("1") - URLError(.cancelled): a cancelled download must not be re-sent The download hop took 6.4s in that run: it really slept its backoff and really re-sent. Both loops are pinned, in both cause shapes, plus a control proving a non-cancellation cause still retries so the guard does not quietly reopen #567. The uniformity claim was false Go, Kotlin and Swift hard-saturate: their base delay is an integer duration, so `multiplier > MAX / base` always fires before their shift cap does. Python, Ruby and TypeScript instead capped the exponent at a fixed constant (64/64/53) and took min(base * 2**capped, CEILING). That bounds the intermediate but not the outcome — for a small enough base the capped product never reaches the ceiling and the term plateaus below it forever. Captured from this branch's own un-fixed config.py: MAX_BACKOFF_DELAY = 30.0 saturating_backoff(1e-30, 64) = 9.223372036854777e-12 saturating_backoff(1e-30, 65) = 1.8446744073709553e-11 saturating_backoff(1e-30, 66) = 1.8446744073709553e-11 saturating_backoff(1e-30, 105) = 1.8446744073709553e-11 saturating_backoff(1e-30, 1100) = 1.8446744073709553e-11 saturating_backoff(1e-30, 2147483648) = 1.8446744073709553e-11 ~18 picoseconds, forever: SPEC §7 requirement 1's tight loop, introduced by the very change meant to prevent it. Un-fixed, the term kept growing and reached a sane delay around attempt 105, so this plateau is new. It is reachable — Python validates only base_delay >= 0, and Ruby's validate! has no base_delay check at all. All three now derive the bound from the configured base: the smallest exponent e with base * 2**e >= CEILING, compared before the power is evaluated. The fixed cap survives only as a numeric-range backstop, raised to 1023 (the largest exponent whose power is a finite double) so it can never be the operative bound for a base delay expressible as a duration. SPEC §7 requirement 1 said "clamp the exponent, or compare the multiplier ... either keeps every intermediate inside the host's numeric range". That either/or is the false equivalence. It now says to compare before multiplying, and spells out why a fixed exponent cap is not a substitute. Disclosed: a hooks-lifecycle change Moving a transport-thrown .network into the generic catch also means the attempt is finalized: onRequestEnd fires with statusCode 0, where the old BasecampError catch emitted no request-end event and said so in a comment. That is intended — the error now means "the attempt failed at the transport", the same as a raw URLError, so it is reported the same way and start/end stay paired for every attempt the loop begins. It was previously undisclosed and unpinned. Against the pre-PR client the new test reports: XCTAssertEqual failed: ("[]") is not equal to ("[0, 0, 0]") - a transport failure is reported as status 0, like a raw URLError Pinned in both directions: .network gains the event, every other BasecampError still emits none. Verified, each REAL_EXIT written into its log and grepped back: make py-check 0 (878 passed) | make rb-check 0 (1087 runs, 0 failures; 147 files, no offenses) | make ts-check 0 (78 files, 1134 tests) | make swift-check 0 (325 tests executed) | make go-check 0 | make kt-check 0 | make conformance 0 (all six runners) | make check-retry-metadata-parity 0 | make check-idempotency-parity 0 Kotlin and Go were restored byte-identical to the previous commit and re-run with the caches forced off (gradle --rerun, go test -count=1) rather than quoted from a cache hit. --- SPEC.md | 18 +- python/src/basecamp/config.py | 43 +++- python/tests/test_backoff_ceiling.py | 45 ++++ ruby/lib/basecamp/config.rb | 50 +++- ruby/test/basecamp/backoff_ceiling_test.rb | 47 ++++ swift/Sources/Basecamp/HTTP/HTTPClient.swift | 32 ++- .../TransportNetworkErrorRetryTests.swift | 215 ++++++++++++++++++ typescript/src/retry.ts | 43 +++- typescript/tests/backoff-ceiling.test.ts | 58 +++++ 9 files changed, 527 insertions(+), 24 deletions(-) diff --git a/SPEC.md b/SPEC.md index aa0a386ec6..84c0a16581 100644 --- a/SPEC.md +++ b/SPEC.md @@ -662,9 +662,21 @@ each of the six SDKs demonstrated one before #577: Requirements: 1. **Saturate, never wrap, never diverge.** An implementation must not evaluate an - unbounded `2^retry_index`. Clamp the exponent, or compare the multiplier against - `MAX_BACKOFF_DELAY_MS / base_delay_ms` before multiplying — either keeps every - intermediate inside the host's numeric range. + unbounded `2^retry_index`. Compare against `MAX_BACKOFF_DELAY_MS / base_delay_ms` + *before* multiplying — either the multiplier itself, or, where the power is the + thing that overflows, the exponent against the point at which the term first + reaches the ceiling. That keeps every intermediate inside the host's numeric range + **and** lands the term on the ceiling. + + A **fixed** exponent cap followed by `min(base × 2^capped, MAX_BACKOFF_DELAY_MS)` + is not an acceptable substitute, however generous the cap looks. It bounds the + intermediate but not the *outcome*: for a base delay small enough that + `base × 2^cap < MAX_BACKOFF_DELAY_MS`, the term plateaus below the ceiling for + every subsequent attempt instead of saturating at it. At `base_delay_ms = 1e-30` + a cap of 64 pins every attempt from the 65th onward at ~1.84e-11s — which is + requirement 1's tight loop, not a fix for it. Any cap that remains must be a + pure numeric-range backstop (the largest exponent the host can represent), + provably never the operative bound for a base delay expressible as a duration. 2. **The ceiling bounds the backoff term, not the total sleep.** Jitter is added after clamping, so the longest single backoff sleep is `MAX_BACKOFF_DELAY_MS + max_jitter`. This matches Go's generated client, which has capped at `RetryConfig.MaxDelay = 30s` diff --git a/python/src/basecamp/config.py b/python/src/basecamp/config.py index f7dc2fe056..5ef6794ab9 100644 --- a/python/src/basecamp/config.py +++ b/python/src/basecamp/config.py @@ -1,5 +1,6 @@ from __future__ import annotations +import math import os from dataclasses import dataclass @@ -17,11 +18,33 @@ # plus max_jitter. MAX_BACKOFF_DELAY = 30.0 -# Largest exponent evaluated before the clamp takes over. 2**64 is 1.8e19, so -# with any base delay at all the ceiling is long since reached; the bound exists -# because Python integers are unbounded and 2 ** 10_000 is a real three-kilobyte -# integer, not an overflow. -_MAX_BACKOFF_EXPONENT = 64 +# Hard ceiling on the exponent, past which the backoff term saturates without +# being computed. It exists only to keep ``2 ** exponent`` inside the float +# range — Python integers are unbounded, so ``2 ** 10_000`` is a real +# three-kilobyte integer that raises OverflowError on conversion, not a wrap. +# +# 1023 is the largest exponent whose power is a finite float. It is deliberately +# NOT the operative bound: _saturating_exponent below derives the real one from +# the configured base, so this only binds for a base below ~1e-304 seconds, +# which is not a duration. +_MAX_BACKOFF_EXPONENT = 1023 + + +def _saturating_exponent(base_delay: float) -> int: + """Smallest exponent ``e`` with ``base_delay * 2**e >= MAX_BACKOFF_DELAY``. + + Derived from the *configured* base rather than assumed. A fixed exponent cap + plus a trailing ``min(..., MAX_BACKOFF_DELAY)`` looks equivalent and is not: + for a small enough base the capped product never reaches the ceiling, so the + delay plateaus below it forever. At ``base_delay=1e-30`` a cap of 64 pins + every attempt from 65 on at ~1.84e-11s — a tight retry loop, which is the + failure SPEC section 7's ceiling exists to prevent, not an instance of it. + """ + ratio = MAX_BACKOFF_DELAY / base_delay + if not math.isfinite(ratio): + # base_delay is denormal-small; no finite power of two gets there. + return _MAX_BACKOFF_EXPONENT + return min(math.ceil(math.log2(ratio)), _MAX_BACKOFF_EXPONENT) def saturating_backoff(base_delay: float, attempt: int) -> float: @@ -32,10 +55,18 @@ def saturating_backoff(base_delay: float, attempt: int) -> float: failure streak either raises ``OverflowError`` converting an arbitrary- precision integer to a float, or hands ``time.sleep`` a delay measured in geological time. Neither is a retry. + + The exponent is compared against the point where the term reaches the + ceiling *before* the power is evaluated, so no intermediate leaves the float + range and the term saturates AT the ceiling for every positive base — the + same contract Go, Kotlin and Swift get from comparing their multiplier + against ``MAX_BACKOFF_DELAY / base`` before multiplying. """ if base_delay <= 0: return 0.0 - exponent = min(max(attempt - 1, 0), _MAX_BACKOFF_EXPONENT) + exponent = max(attempt - 1, 0) + if exponent >= _saturating_exponent(base_delay): + return MAX_BACKOFF_DELAY return min(base_delay * (2**exponent), MAX_BACKOFF_DELAY) diff --git a/python/tests/test_backoff_ceiling.py b/python/tests/test_backoff_ceiling.py index 24961cfed7..b302a2a7b7 100644 --- a/python/tests/test_backoff_ceiling.py +++ b/python/tests/test_backoff_ceiling.py @@ -81,3 +81,48 @@ def test_saturating_backoff_edges() -> None: assert saturating_backoff(0.0, 10_000) == 0.0 # Below the ceiling the exponential term is exact. assert saturating_backoff(1.0, 3) == 4.0 + + +# Bases spanning the range Config accepts. 1e-30 is the case a fixed exponent +# cap of 64 gets wrong; the denormal-adjacent ones prove the derived bound does +# not simply move the plateau further out. +TINY_BASE_DELAYS = [1e-3, 1e-9, 1e-30, 1e-100, 1e-300, 5e-324] + + +@pytest.mark.parametrize("base_delay", TINY_BASE_DELAYS) +def test_saturating_backoff_reaches_the_ceiling_for_any_positive_base(base_delay: float) -> None: + """The term saturates AT the ceiling, whatever the base — no plateau below it. + + A fixed exponent cap plus a trailing ``min(..., MAX_BACKOFF_DELAY)`` bounds + the intermediate but not the outcome. With a cap of 64 and + ``base_delay=1e-30``, attempt 65 and every attempt after it returned + ~1.84e-11s forever: a tight retry loop against a server already answering + 429/503, which is precisely what SPEC section 7 requirement 1 forbids. + + ``Config`` accepts these bases — ``__post_init__`` validates only + ``base_delay >= 0`` — so the bound has to come from the base, not a constant. + """ + Config(base_delay=base_delay) # the configuration under test is a legal one + + for attempt in (1_100, 5_000, 2**31): + assert saturating_backoff(base_delay, attempt) == MAX_BACKOFF_DELAY, ( + f"base_delay={base_delay!r} attempt={attempt} plateaued below the ceiling" + ) + + +@pytest.mark.parametrize("base_delay", TINY_BASE_DELAYS) +def test_saturating_backoff_is_monotonic_up_to_the_ceiling(base_delay: float) -> None: + """It grows to the ceiling rather than jumping — SPEC section 7's "and then stops". + + Sampling every attempt from 1 to 1100 covers the whole exponent range any + positive double can need, so a formula that stalled anywhere in the middle + (the fixed-cap plateau) is caught wherever it stalls. + """ + previous = 0.0 + for attempt in range(1, 1_101): + delay = saturating_backoff(base_delay, attempt) + assert delay >= previous, f"base_delay={base_delay!r} went backwards at attempt {attempt}" + assert delay <= MAX_BACKOFF_DELAY, f"base_delay={base_delay!r} exceeded the ceiling at {attempt}" + previous = delay + + assert previous == MAX_BACKOFF_DELAY diff --git a/ruby/lib/basecamp/config.rb b/ruby/lib/basecamp/config.rb index 5cd9985ea4..c2f3080a65 100644 --- a/ruby/lib/basecamp/config.rb +++ b/ruby/lib/basecamp/config.rb @@ -50,11 +50,37 @@ class Config # this plus +max_jitter+. MAX_BACKOFF_DELAY = 30.0 - # Largest exponent evaluated before the clamp takes over. 2**64 is 1.8e19, - # so with any base delay at all the ceiling is long since reached; the bound - # exists because Ruby Integers are unbounded — 2**10_000 is a real - # three-kilobyte number, not an overflow. - MAX_BACKOFF_EXPONENT = 64 + # Hard ceiling on the exponent, past which the backoff term saturates + # without being computed. It exists only to keep +2**exponent+ inside the + # Float range — Ruby Integers are unbounded, so +2**10_000+ is a real + # three-kilobyte number that coerces to +Float::INFINITY+, not a wrap. + # + # 1023 is the largest exponent whose power is a finite Float. It is + # deliberately NOT the operative bound: +saturating_exponent+ derives the + # real one from the configured base, so this only binds for a base below + # ~1e-304 seconds, which is not a duration. + MAX_BACKOFF_EXPONENT = 1023 + + # Smallest exponent +e+ with base_delay * 2**e >= MAX_BACKOFF_DELAY. + # + # Derived from the *configured* base rather than assumed. A fixed exponent + # cap plus a trailing +min(..., MAX_BACKOFF_DELAY)+ looks equivalent and is + # not: for a small enough base the capped product never reaches the ceiling, + # so the delay plateaus below it forever. At +base_delay = 1e-30+ a cap of + # 64 pins every attempt from 65 on at ~1.84e-11s — a tight retry loop, which + # is the failure SPEC §7's ceiling exists to prevent, not an instance of it. + # + # @param base_delay [Float] initial backoff delay in seconds, strictly positive + # @return [Integer] the exponent at which the term reaches the ceiling + def self.saturating_exponent(base_delay) + ratio = MAX_BACKOFF_DELAY / base_delay + if ratio.finite? + [ Math.log2(ratio).ceil, MAX_BACKOFF_EXPONENT ].min + else + # base_delay is denormal-small; no finite power of two gets there. + MAX_BACKOFF_EXPONENT + end + end # Exponential backoff for a 1-based attempt, saturating at MAX_BACKOFF_DELAY. # @@ -63,6 +89,12 @@ class Config # failure streak coerces to +Float::INFINITY+ — and +sleep(Float::INFINITY)+ # never returns. A retry that never happens is not backoff. # + # The exponent is compared against the point where the term reaches the + # ceiling *before* the power is evaluated, so no intermediate leaves the + # Float range and the term saturates AT the ceiling for every positive base + # — the same contract Go, Kotlin and Swift get from comparing their + # multiplier against MAX_BACKOFF_DELAY / base before multiplying. + # # @param base_delay [Float] initial backoff delay in seconds # @param attempt [Integer] 1-based attempt number # @return [Float] the backoff term in seconds @@ -70,8 +102,12 @@ def self.saturating_backoff(base_delay, attempt) if base_delay <= 0 0.0 else - exponent = [ [ attempt - 1, 0 ].max, MAX_BACKOFF_EXPONENT ].min - [ base_delay * (2**exponent), MAX_BACKOFF_DELAY ].min.to_f + exponent = [ attempt - 1, 0 ].max + if exponent >= saturating_exponent(base_delay) + MAX_BACKOFF_DELAY + else + [ base_delay * (2**exponent), MAX_BACKOFF_DELAY ].min.to_f + end end end diff --git a/ruby/test/basecamp/backoff_ceiling_test.rb b/ruby/test/basecamp/backoff_ceiling_test.rb index 379ab086b7..4b2b983fcb 100644 --- a/ruby/test/basecamp/backoff_ceiling_test.rb +++ b/ruby/test/basecamp/backoff_ceiling_test.rb @@ -59,4 +59,51 @@ def test_saturating_backoff_edges # Below the ceiling the exponential term is exact. assert_in_delta 4.0, Basecamp::Config.saturating_backoff(1.0, 3) end + + # Bases spanning the range Config accepts. 1e-30 is the case a fixed exponent + # cap of 64 gets wrong; the denormal-adjacent ones prove the derived bound + # does not simply move the plateau further out. + TINY_BASE_DELAYS = [ 1e-3, 1e-9, 1e-30, 1e-100, 1e-300, 5e-324 ].freeze + + # A fixed exponent cap plus a trailing +min(..., MAX_BACKOFF_DELAY)+ bounds the + # intermediate but not the outcome. With a cap of 64 and +base_delay = 1e-30+, + # attempt 65 and every attempt after it returned ~1.84e-11s forever: a tight + # retry loop against a server already answering 429/503, which is precisely + # what SPEC §7 requirement 1 forbids. + # + # +validate!+ has no +base_delay+ check at all, so every one of these is a + # configuration the constructor accepts — the bound has to come from the base. + def test_saturating_backoff_reaches_the_ceiling_for_any_positive_base + violations = TINY_BASE_DELAYS.flat_map do |base_delay| + Basecamp::Config.new(base_delay: base_delay) + + [ 1_100, 5_000, 2**31 ].filter_map do |attempt| + delay = Basecamp::Config.saturating_backoff(base_delay, attempt) + "base_delay=#{base_delay} attempt=#{attempt} -> #{delay}s" unless delay == Basecamp::Config::MAX_BACKOFF_DELAY + end + end + + assert_empty violations, "the term must saturate at the ceiling, not plateau below it" + end + + # It grows to the ceiling rather than jumping — SPEC §7's "and then stops". + # Sampling every attempt from 1 to 1100 covers the whole exponent range any + # positive Float can need, so a formula that stalled anywhere in the middle + # (the fixed-cap plateau) is caught wherever it stalls. + def test_saturating_backoff_is_monotonic_up_to_the_ceiling + ceiling = Basecamp::Config::MAX_BACKOFF_DELAY + + TINY_BASE_DELAYS.each do |base_delay| + previous = 0.0 + (1..1_100).each do |attempt| + delay = Basecamp::Config.saturating_backoff(base_delay, attempt) + + assert_operator delay, :>=, previous, "base_delay=#{base_delay} went backwards at attempt #{attempt}" + assert_operator delay, :<=, ceiling, "base_delay=#{base_delay} exceeded the ceiling at attempt #{attempt}" + previous = delay + end + + assert_equal ceiling, previous, "base_delay=#{base_delay} never reached the ceiling" + end + end end diff --git a/swift/Sources/Basecamp/HTTP/HTTPClient.swift b/swift/Sources/Basecamp/HTTP/HTTPClient.swift index 27bc264d42..cab8154a1d 100644 --- a/swift/Sources/Basecamp/HTTP/HTTPClient.swift +++ b/swift/Sources/Basecamp/HTTP/HTTPClient.swift @@ -82,11 +82,39 @@ package final class HTTPClient: Sendable { /// cancelled task as `URLError(.cancelled)` — so checking only the former /// would treat a real cancelled download as a retryable network blip and /// spend the whole budget on a request the caller already abandoned. + /// + /// It also looks *through* ``BasecampError/network(message:cause:)``. A + /// `Transport` that normalizes its failures into the SDK's own taxonomy — + /// the shape #567 exists to support — reports a cancelled request as + /// `.network(cause: CancellationError())` or `.network(cause: + /// URLError(.cancelled))`. Testing only the outer type would classify one + /// cancellation two ways: terminal when raw, retried (announced, slept on, + /// re-authenticated, re-sent) when wrapped. Classifying a network error by + /// meaning rather than by type is the whole point of #567, and cancellation + /// is part of that meaning. + /// + /// The walk is bounded rather than recursive: the `cause` chain is + /// caller-supplied, and a cycle in it must not hang the retry loop. private static func isCancellation(_ error: any Error) -> Bool { - if error is CancellationError { return true } - return (error as? URLError)?.code == .cancelled + var current: (any Error)? = error + + for _ in 0..= MAX_BACKOFF_DELAY_MS`. */ +function saturatingExponent(base: number): number { + const ratio = MAX_BACKOFF_DELAY_MS / base; + // A denormal-small base makes the ratio Infinity: no finite power gets there. + if (!Number.isFinite(ratio)) return MAX_BACKOFF_EXPONENT; + return Math.min(Math.ceil(Math.log2(ratio)), MAX_BACKOFF_EXPONENT); +} + /** * The backoff term, saturating at {@link MAX_BACKOFF_DELAY_MS} (SPEC §7). * @@ -195,6 +214,14 @@ export async function executeWithRetry( * long sleep, it is a tight retry loop against a server that is already * answering 429/503. Well before that, the computed delays run to millennia. * + * The exponent is compared against the point where the term reaches the ceiling + * *before* `Math.pow` is evaluated, and that point is derived from the + * CONFIGURED base — the same contract Go, Kotlin and Swift get from comparing + * their multiplier against `MAX_BACKOFF_DELAY_MS / base` before multiplying. A + * fixed exponent cap plus a trailing `Math.min` looks equivalent and is not: + * for a small enough base the capped product never reaches the ceiling, so the + * delay plateaus below it forever instead of saturating at it. + * * `attempt` is the 0-indexed retry count (first retry = 0), matching the SPEC * §7 `retry_index`. */ @@ -205,18 +232,22 @@ export function saturatingBackoff( ): number { const base = baseDelayMs > 0 ? baseDelayMs : 0; const index = attempt > 0 ? attempt : 0; + if (base === 0) return 0; let delay: number; switch (backoff) { case "exponential": - // Bounding the exponent keeps the product finite: 2^53 is already 9e15, - // so with any base at all the ceiling is long since reached, and - // Math.min below does the rest without ever seeing Infinity or NaN. - delay = base * Math.pow(2, Math.min(index, 53)); + if (index >= saturatingExponent(base)) return MAX_BACKOFF_DELAY_MS; + delay = base * Math.pow(2, index); break; - case "linear": - delay = base * (Math.min(index, Number.MAX_SAFE_INTEGER) + 1); + case "linear": { + // Compared before multiplying, so `Infinity * 0`-shaped intermediates + // never arise. `index + 1` is finite for any finite index. + const multiplier = index + 1; + if (multiplier >= MAX_BACKOFF_DELAY_MS / base) return MAX_BACKOFF_DELAY_MS; + delay = base * multiplier; break; + } case "constant": default: delay = base; diff --git a/typescript/tests/backoff-ceiling.test.ts b/typescript/tests/backoff-ceiling.test.ts index 4b7447920e..0df62026be 100644 --- a/typescript/tests/backoff-ceiling.test.ts +++ b/typescript/tests/backoff-ceiling.test.ts @@ -73,4 +73,62 @@ describe("backoff ceiling", () => { // A zero base delay stays at zero rather than saturating. expect(saturatingBackoff(0, "exponential", 1024)).toBe(0); }); + + /** + * A fixed exponent cap plus a trailing `Math.min` bounds the intermediate but + * not the outcome. With a cap of 53 and a base of 1e-30ms, every attempt from + * the 54th on returned ~9e-15ms forever — a tight retry loop against a server + * already answering 429/503, which is what SPEC §7 requirement 1 forbids. The + * bound has to be derived from the base. + * + * Go, Kotlin and Swift never had this shape: their bases are integer + * durations, so the `MAX / base` comparison always fires before their shift + * cap does. This is what makes the six uniform rather than three-and-three. + */ + const tinyBaseDelays = [1e-3, 1e-9, 1e-30, 1e-100, 1e-300, 5e-324]; + + it("saturates at the ceiling for any positive base, never plateaus below it", () => { + const violations = tinyBaseDelays.flatMap((base) => + [1023, 1100, 5000, Number.MAX_SAFE_INTEGER] + .map((attempt) => [base, attempt, saturatingBackoff(base, "exponential", attempt)] as const) + .filter(([, , delay]) => delay !== MAX_BACKOFF_DELAY_MS) + .map(([b, attempt, delay]) => `base=${b} attempt=${attempt} -> ${delay}ms`), + ); + + expect(violations, "the term must saturate at the ceiling, not plateau below it").toEqual([]); + }); + + it("grows monotonically to the ceiling rather than stalling on the way", () => { + for (const base of tinyBaseDelays) { + let previous = 0; + for (let attempt = 0; attempt <= 1100; attempt++) { + const delay = saturatingBackoff(base, "exponential", attempt); + expect(delay, `base=${base} went backwards at attempt ${attempt}`).toBeGreaterThanOrEqual(previous); + expect(delay, `base=${base} exceeded the ceiling at attempt ${attempt}`).toBeLessThanOrEqual( + MAX_BACKOFF_DELAY_MS, + ); + previous = delay; + } + expect(previous, `base=${base} never reached the ceiling`).toBe(MAX_BACKOFF_DELAY_MS); + } + }); + + /** + * Linear backoff compares its multiplier against `MAX / base` before + * multiplying, the same as Swift's. Saturation is asserted only for the bases + * a linear term can actually reach the ceiling from: growth is `base × n`, so + * it needs `n >= 30000 / base`, and `n` is an attempt count. Below ~3.3e-12ms + * no finite attempt count gets there — an arithmetic fact about linear growth, + * not a clamp defect. The ceiling is still never exceeded, which is asserted + * for every base. + */ + it("saturates linear backoff at the ceiling wherever a linear term can reach it", () => { + for (const base of tinyBaseDelays) { + const delay = saturatingBackoff(base, "linear", Number.MAX_SAFE_INTEGER); + expect(delay).toBeLessThanOrEqual(MAX_BACKOFF_DELAY_MS); + if (Number.MAX_SAFE_INTEGER >= MAX_BACKOFF_DELAY_MS / base) { + expect(delay, `base=${base} should have saturated`).toBe(MAX_BACKOFF_DELAY_MS); + } + } + }); }); From 383a6fe1fec041420dbc768c6d86994cf51a7a17 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 3 Aug 2026 01:46:27 -0700 Subject: [PATCH 3/3] Derive the backoff exponent without overflowing its own arithmetic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 replaced the fixed exponent cap with a bound derived from the configured base, computed as MAX_BACKOFF_DELAY / base_delay. That ratio is itself infinite once base_delay drops below MAX_BACKOFF_DELAY / MAX_FLOAT (~1.67e-307s, ~1.67e-304ms), and the fixed-1023 fallback that backstopped it then failed in the mirror direction of the plateau it replaced: the term saturates EARLY. At base_delay=1e-307 the specified term is ~8.99s at attempt 1024 and ~17.98s at 1025; both returned the 30s ceiling — a sleep more than three times longer than the formula asks for, decided by a numeric backstop rather than by the ceiling. TypeScript was worse: base 1e-305 returns 30000ms where the term is ~899ms, 33x. The round-2 tests could not catch it because monotonicity and eventual saturation both hold for a formula that saturates early. The bound now comes from log2(ceiling) - log2(base), which has no cliff, and the term is scaled directly — math.ldexp in Python and Ruby, bounded repeated multiplication in TypeScript, which has no ldexp and whose Math.pow(2, e) is Infinity past 1023 even when the product is an ordinary number. The numeric backstop is therefore deleted outright rather than merely made rarer: _MAX_BACKOFF_EXPONENT, MAX_BACKOFF_EXPONENT and the TS const are gone. Red proof, new tests against this branch's own b5726c65 sources: Python E assert 30.0 == 8.988465674311579 E + where 30.0 = saturating_backoff(1e-307, 1024) Ruby Expected: 8.988465674311579 Actual: 30.0 TS AssertionError: expected 30000 to be 898.846567431158 // Object.is equality The TS saturation probe moved from attempt 1023 to 1089 — the exact saturating exponent of the smallest denormal. Probing at 1023 asserted the ceiling at an attempt where 5e-324 * 2**1023 is genuinely ~4.4e-16ms, a bound the old fallback met only by saturating early. SPEC §7 requirement 1 gains the rule this violated: the bound must be derived without overflowing its own arithmetic, and saturating early is as much a deviation as plateauing. Verified, each REAL_EXIT written into its log and grepped back: make py-check 0 (879 passed) | make rb-check 0 (1088 runs, 0 failures; 147 files, no offenses) | make ts-check 0 (78 files, 1135 tests) | make conformance 0 (all six runners) | make check-retry-metadata-parity 0 | make check-idempotency-parity 0 | make sync-spec-version-check 0 | make sync-api-version-check 0 Go, Kotlin and Swift are untouched: their base delay is an integer duration, so the multiplier comparison always fires before any exponent bound does. --- SPEC.md | 16 ++++++-- python/src/basecamp/config.py | 39 +++++++++++-------- python/tests/test_backoff_ceiling.py | 20 ++++++++++ ruby/lib/basecamp/config.rb | 41 +++++++++++--------- ruby/test/basecamp/backoff_ceiling_test.rb | 19 +++++++++ typescript/src/retry.ts | 45 +++++++++++++++------- typescript/tests/backoff-ceiling.test.ts | 30 ++++++++++++++- 7 files changed, 157 insertions(+), 53 deletions(-) diff --git a/SPEC.md b/SPEC.md index 84c0a16581..0fa5d8b7ea 100644 --- a/SPEC.md +++ b/SPEC.md @@ -674,9 +674,19 @@ Requirements: `base × 2^cap < MAX_BACKOFF_DELAY_MS`, the term plateaus below the ceiling for every subsequent attempt instead of saturating at it. At `base_delay_ms = 1e-30` a cap of 64 pins every attempt from the 65th onward at ~1.84e-11s — which is - requirement 1's tight loop, not a fix for it. Any cap that remains must be a - pure numeric-range backstop (the largest exponent the host can represent), - provably never the operative bound for a base delay expressible as a duration. + requirement 1's tight loop, not a fix for it. + + The bound must be derived from the base **without overflowing its own + arithmetic**. `MAX_BACKOFF_DELAY_MS / base_delay_ms` is itself infinite once + `base_delay_ms` drops below `MAX_BACKOFF_DELAY_MS / MAX_FLOAT`, and falling back + to a fixed exponent there fails in the mirror direction: the term saturates + **early**, returning the ceiling at an attempt whose specified value is still far + below it. Compute the crossing in the log domain (`log2(ceiling) - log2(base)`) + and scale the term directly — `ldexp`, or repeated bounded multiplication where + the host has no `ldexp` — so no fixed exponent cap is needed at all. Saturating + early is as much a deviation as plateauing: the term must track + `base × 2^retry_index` at every attempt below the ceiling and equal the ceiling + at every attempt at or above it. 2. **The ceiling bounds the backoff term, not the total sleep.** Jitter is added after clamping, so the longest single backoff sleep is `MAX_BACKOFF_DELAY_MS + max_jitter`. This matches Go's generated client, which has capped at `RetryConfig.MaxDelay = 30s` diff --git a/python/src/basecamp/config.py b/python/src/basecamp/config.py index 5ef6794ab9..61c28db482 100644 --- a/python/src/basecamp/config.py +++ b/python/src/basecamp/config.py @@ -18,17 +18,6 @@ # plus max_jitter. MAX_BACKOFF_DELAY = 30.0 -# Hard ceiling on the exponent, past which the backoff term saturates without -# being computed. It exists only to keep ``2 ** exponent`` inside the float -# range — Python integers are unbounded, so ``2 ** 10_000`` is a real -# three-kilobyte integer that raises OverflowError on conversion, not a wrap. -# -# 1023 is the largest exponent whose power is a finite float. It is deliberately -# NOT the operative bound: _saturating_exponent below derives the real one from -# the configured base, so this only binds for a base below ~1e-304 seconds, -# which is not a duration. -_MAX_BACKOFF_EXPONENT = 1023 - def _saturating_exponent(base_delay: float) -> int: """Smallest exponent ``e`` with ``base_delay * 2**e >= MAX_BACKOFF_DELAY``. @@ -39,12 +28,23 @@ def _saturating_exponent(base_delay: float) -> int: delay plateaus below it forever. At ``base_delay=1e-30`` a cap of 64 pins every attempt from 65 on at ~1.84e-11s — a tight retry loop, which is the failure SPEC section 7's ceiling exists to prevent, not an instance of it. + + Computed in the LOG domain rather than as ``MAX_BACKOFF_DELAY / base_delay``. + That ratio overflows to infinity for any base below ~1.67e-307, and falling + back to a fixed 1023 then saturates *early*: ``base_delay=1e-307`` reaches + only ~8.99s at exponent 1023, so returning the 30s ceiling there overstates + the specified term instead of tracking it. The log form has no such cliff, so + the numeric backstop is gone entirely rather than merely made rarer. """ - ratio = MAX_BACKOFF_DELAY / base_delay - if not math.isfinite(ratio): - # base_delay is denormal-small; no finite power of two gets there. - return _MAX_BACKOFF_EXPONENT - return min(math.ceil(math.log2(ratio)), _MAX_BACKOFF_EXPONENT) + # log2 is correctly rounded but the subtraction is not, so the estimate can + # land one either side of the true boundary. Both corrections are bounded and + # evaluate the term with ldexp, which scales directly and never forms 2**e. + exponent = max(math.ceil(math.log2(MAX_BACKOFF_DELAY) - math.log2(base_delay)), 0) + while exponent > 0 and math.ldexp(base_delay, exponent - 1) >= MAX_BACKOFF_DELAY: + exponent -= 1 + while math.ldexp(base_delay, exponent) < MAX_BACKOFF_DELAY: + exponent += 1 + return exponent def saturating_backoff(base_delay: float, attempt: int) -> float: @@ -61,13 +61,18 @@ def saturating_backoff(base_delay: float, attempt: int) -> float: range and the term saturates AT the ceiling for every positive base — the same contract Go, Kotlin and Swift get from comparing their multiplier against ``MAX_BACKOFF_DELAY / base`` before multiplying. + + Below that point the term is scaled with ``math.ldexp``, which computes + ``base * 2**e`` directly. ``2**e`` would be an arbitrary-precision integer + that raises OverflowError on conversion long before the *product* leaves the + float range, which is what forced the fixed cap this replaces. """ if base_delay <= 0: return 0.0 exponent = max(attempt - 1, 0) if exponent >= _saturating_exponent(base_delay): return MAX_BACKOFF_DELAY - return min(base_delay * (2**exponent), MAX_BACKOFF_DELAY) + return min(math.ldexp(base_delay, exponent), MAX_BACKOFF_DELAY) @dataclass(frozen=True) diff --git a/python/tests/test_backoff_ceiling.py b/python/tests/test_backoff_ceiling.py index b302a2a7b7..8933cbe62a 100644 --- a/python/tests/test_backoff_ceiling.py +++ b/python/tests/test_backoff_ceiling.py @@ -126,3 +126,23 @@ def test_saturating_backoff_is_monotonic_up_to_the_ceiling(base_delay: float) -> previous = delay assert previous == MAX_BACKOFF_DELAY + + +def test_saturating_backoff_tracks_the_term_for_a_denormal_adjacent_base() -> None: + """The last attempts before the ceiling are the specified term, not the ceiling. + + Monotonicity and eventual saturation both hold for a formula that saturates + EARLY, so neither catches this. ``MAX_BACKOFF_DELAY / 1e-307`` overflows to + infinity, and the fixed-1023 fallback that used to backstop it returned 30.0 + for attempt 1024 when the specified term is ~8.99s — a sleep more than three + times longer than the formula asks for, with the numeric backstop rather than + the ceiling deciding it. + """ + base_delay = 1e-307 + + # Exact: these are the products the exponential term is defined to produce. + assert saturating_backoff(base_delay, 1_024) == 8.988465674311579 + assert saturating_backoff(base_delay, 1_025) == 17.976931348623157 + # And only then does it reach the ceiling. + assert saturating_backoff(base_delay, 1_026) == MAX_BACKOFF_DELAY + assert saturating_backoff(base_delay, 2**31) == MAX_BACKOFF_DELAY diff --git a/ruby/lib/basecamp/config.rb b/ruby/lib/basecamp/config.rb index c2f3080a65..8c88cf459c 100644 --- a/ruby/lib/basecamp/config.rb +++ b/ruby/lib/basecamp/config.rb @@ -50,17 +50,6 @@ class Config # this plus +max_jitter+. MAX_BACKOFF_DELAY = 30.0 - # Hard ceiling on the exponent, past which the backoff term saturates - # without being computed. It exists only to keep +2**exponent+ inside the - # Float range — Ruby Integers are unbounded, so +2**10_000+ is a real - # three-kilobyte number that coerces to +Float::INFINITY+, not a wrap. - # - # 1023 is the largest exponent whose power is a finite Float. It is - # deliberately NOT the operative bound: +saturating_exponent+ derives the - # real one from the configured base, so this only binds for a base below - # ~1e-304 seconds, which is not a duration. - MAX_BACKOFF_EXPONENT = 1023 - # Smallest exponent +e+ with base_delay * 2**e >= MAX_BACKOFF_DELAY. # # Derived from the *configured* base rather than assumed. A fixed exponent @@ -70,16 +59,25 @@ class Config # 64 pins every attempt from 65 on at ~1.84e-11s — a tight retry loop, which # is the failure SPEC §7's ceiling exists to prevent, not an instance of it. # + # Computed in the LOG domain rather than as + # MAX_BACKOFF_DELAY / base_delay. That ratio coerces to + # +Float::INFINITY+ for any base below ~1.67e-307, and falling back to a + # fixed 1023 then saturates *early*: +base_delay = 1e-307+ reaches only + # ~8.99s at exponent 1023, so returning the 30s ceiling there overstates the + # specified term instead of tracking it. The log form has no such cliff, so + # the numeric backstop is gone entirely rather than merely made rarer. + # # @param base_delay [Float] initial backoff delay in seconds, strictly positive # @return [Integer] the exponent at which the term reaches the ceiling def self.saturating_exponent(base_delay) - ratio = MAX_BACKOFF_DELAY / base_delay - if ratio.finite? - [ Math.log2(ratio).ceil, MAX_BACKOFF_EXPONENT ].min - else - # base_delay is denormal-small; no finite power of two gets there. - MAX_BACKOFF_EXPONENT - end + # log2 is correctly rounded but the subtraction is not, so the estimate can + # land one either side of the true boundary. Both corrections are bounded + # and evaluate the term with Math.ldexp, which scales directly and never + # forms 2**e. + exponent = [ (Math.log2(MAX_BACKOFF_DELAY) - Math.log2(base_delay)).ceil, 0 ].max + exponent -= 1 while exponent > 0 && Math.ldexp(base_delay, exponent - 1) >= MAX_BACKOFF_DELAY + exponent += 1 while Math.ldexp(base_delay, exponent) < MAX_BACKOFF_DELAY + exponent end # Exponential backoff for a 1-based attempt, saturating at MAX_BACKOFF_DELAY. @@ -95,6 +93,11 @@ def self.saturating_exponent(base_delay) # — the same contract Go, Kotlin and Swift get from comparing their # multiplier against MAX_BACKOFF_DELAY / base before multiplying. # + # Below that point the term is scaled with +Math.ldexp+, which computes + # base * 2**e directly. +2**e+ would be an unbounded Integer that + # coerces to +Float::INFINITY+ long before the *product* leaves the Float + # range, which is what forced the fixed cap this replaces. + # # @param base_delay [Float] initial backoff delay in seconds # @param attempt [Integer] 1-based attempt number # @return [Float] the backoff term in seconds @@ -106,7 +109,7 @@ def self.saturating_backoff(base_delay, attempt) if exponent >= saturating_exponent(base_delay) MAX_BACKOFF_DELAY else - [ base_delay * (2**exponent), MAX_BACKOFF_DELAY ].min.to_f + [ Math.ldexp(base_delay, exponent), MAX_BACKOFF_DELAY ].min.to_f end end end diff --git a/ruby/test/basecamp/backoff_ceiling_test.rb b/ruby/test/basecamp/backoff_ceiling_test.rb index 4b2b983fcb..5efae74645 100644 --- a/ruby/test/basecamp/backoff_ceiling_test.rb +++ b/ruby/test/basecamp/backoff_ceiling_test.rb @@ -106,4 +106,23 @@ def test_saturating_backoff_is_monotonic_up_to_the_ceiling assert_equal ceiling, previous, "base_delay=#{base_delay} never reached the ceiling" end end + + # The last attempts before the ceiling are the specified term, not the ceiling. + # + # Monotonicity and eventual saturation both hold for a formula that saturates + # EARLY, so neither catches this. +MAX_BACKOFF_DELAY / 1e-307+ coerces to + # Float::INFINITY, and the fixed-1023 fallback that used to backstop it + # returned 30.0 for attempt 1024 when the specified term is ~8.99s — a sleep + # more than three times longer than the formula asks for, with the numeric + # backstop rather than the ceiling deciding it. + def test_saturating_backoff_tracks_the_term_for_a_denormal_adjacent_base + base_delay = 1e-307 + + # Exact: these are the products the exponential term is defined to produce. + assert_equal 8.988465674311579, Basecamp::Config.saturating_backoff(base_delay, 1_024) + assert_equal 17.976931348623157, Basecamp::Config.saturating_backoff(base_delay, 1_025) + # And only then does it reach the ceiling. + assert_equal Basecamp::Config::MAX_BACKOFF_DELAY, Basecamp::Config.saturating_backoff(base_delay, 1_026) + assert_equal Basecamp::Config::MAX_BACKOFF_DELAY, Basecamp::Config.saturating_backoff(base_delay, 2**31) + end end diff --git a/typescript/src/retry.ts b/typescript/src/retry.ts index 4839d4889a..87de701090 100644 --- a/typescript/src/retry.ts +++ b/typescript/src/retry.ts @@ -183,22 +183,41 @@ export async function executeWithRetry( } /** - * Hard ceiling on the exponent, past which the term saturates without being - * computed. It exists only to keep `Math.pow(2, index)` finite — 1023 is the - * largest exponent whose power is a finite double. + * `base * 2**exponent`, computed without ever forming `2**exponent`. * - * It is deliberately NOT the operative bound: {@link saturatingExponent} - * derives the real one from the configured base, so this only binds for a base - * below ~1e-304 ms, which is not a duration. + * JS has no `ldexp`, and `Math.pow(2, e)` is `Infinity` for `e > 1023` — so for + * a denormal-small base the *product* is still an ordinary number long after the + * multiplier has overflowed. Scaling in bounded steps keeps every intermediate + * finite, which is what lets the exponent bound come from the base alone. */ -const MAX_BACKOFF_EXPONENT = 1023; +function scaleByPowerOfTwo(base: number, exponent: number): number { + let result = base; + for (let remaining = exponent; remaining > 0; ) { + const step = Math.min(remaining, 1000); + result *= Math.pow(2, step); + if (!Number.isFinite(result)) return Infinity; + remaining -= step; + } + return result; +} -/** Smallest exponent `e` with `base * 2**e >= MAX_BACKOFF_DELAY_MS`. */ +/** + * Smallest exponent `e` with `base * 2**e >= MAX_BACKOFF_DELAY_MS`. + * + * Computed in the LOG domain rather than as `MAX_BACKOFF_DELAY_MS / base`. That + * ratio is `Infinity` for any base below ~1.67e-304 ms, and falling back to a + * fixed 1023 then saturates *early*: such a base reaches only a fraction of the + * ceiling at exponent 1023, so returning the ceiling there overstates the + * specified term instead of tracking it. The log form has no such cliff, so the + * numeric backstop is gone entirely rather than merely made rarer. + */ function saturatingExponent(base: number): number { - const ratio = MAX_BACKOFF_DELAY_MS / base; - // A denormal-small base makes the ratio Infinity: no finite power gets there. - if (!Number.isFinite(ratio)) return MAX_BACKOFF_EXPONENT; - return Math.min(Math.ceil(Math.log2(ratio)), MAX_BACKOFF_EXPONENT); + // log2 is not correctly rounded here, so the estimate can land one either side + // of the true boundary. Both corrections are bounded. + let exponent = Math.max(Math.ceil(Math.log2(MAX_BACKOFF_DELAY_MS) - Math.log2(base)), 0); + while (exponent > 0 && scaleByPowerOfTwo(base, exponent - 1) >= MAX_BACKOFF_DELAY_MS) exponent--; + while (scaleByPowerOfTwo(base, exponent) < MAX_BACKOFF_DELAY_MS) exponent++; + return exponent; } /** @@ -238,7 +257,7 @@ export function saturatingBackoff( switch (backoff) { case "exponential": if (index >= saturatingExponent(base)) return MAX_BACKOFF_DELAY_MS; - delay = base * Math.pow(2, index); + delay = scaleByPowerOfTwo(base, index); break; case "linear": { // Compared before multiplying, so `Infinity * 0`-shaped intermediates diff --git a/typescript/tests/backoff-ceiling.test.ts b/typescript/tests/backoff-ceiling.test.ts index 0df62026be..31d60b2e3a 100644 --- a/typescript/tests/backoff-ceiling.test.ts +++ b/typescript/tests/backoff-ceiling.test.ts @@ -87,9 +87,15 @@ describe("backoff ceiling", () => { */ const tinyBaseDelays = [1e-3, 1e-9, 1e-30, 1e-100, 1e-300, 5e-324]; + /** + * 1089 is the exact saturating exponent of the smallest denormal, and so of + * every base here. Probing at 1023 instead would assert the ceiling at an + * attempt where `5e-324 * 2**1023` is genuinely only ~4.4e-16ms — a bound the + * old fixed-1023 fallback met by saturating early, which is its own defect. + */ it("saturates at the ceiling for any positive base, never plateaus below it", () => { const violations = tinyBaseDelays.flatMap((base) => - [1023, 1100, 5000, Number.MAX_SAFE_INTEGER] + [1089, 1100, 5000, Number.MAX_SAFE_INTEGER] .map((attempt) => [base, attempt, saturatingBackoff(base, "exponential", attempt)] as const) .filter(([, , delay]) => delay !== MAX_BACKOFF_DELAY_MS) .map(([b, attempt, delay]) => `base=${b} attempt=${attempt} -> ${delay}ms`), @@ -113,6 +119,28 @@ describe("backoff ceiling", () => { } }); + /** + * The last attempts before the ceiling are the specified term, not the ceiling. + * + * Monotonicity and eventual saturation both hold for a formula that saturates + * EARLY, so neither catches this. `MAX_BACKOFF_DELAY_MS / 1e-305` is + * `Infinity`, and the fixed-1023 fallback that used to backstop it returned + * 30000 for attempt 1023 when the specified term is ~899ms — a sleep 33x + * longer than the formula asks for, with the numeric backstop rather than the + * ceiling deciding it. + */ + it("tracks the exponential term for a denormal-adjacent base", () => { + const base = 1e-305; + + // Exact: these are the products the exponential term is defined to produce. + expect(saturatingBackoff(base, "exponential", 1023)).toBe(898.846567431158); + expect(saturatingBackoff(base, "exponential", 1024)).toBe(1797.693134862316); + expect(saturatingBackoff(base, "exponential", 1028)).toBe(28763.090157797054); + // And only then does it reach the ceiling. + expect(saturatingBackoff(base, "exponential", 1029)).toBe(MAX_BACKOFF_DELAY_MS); + expect(saturatingBackoff(base, "exponential", Number.MAX_SAFE_INTEGER)).toBe(MAX_BACKOFF_DELAY_MS); + }); + /** * Linear backoff compares its multiplier against `MAX / base` before * multiplying, the same as Swift's. Saturation is asserted only for the bases