diff --git a/SPEC.md b/SPEC.md index d33d2a77e4..0fa5d8b7ea 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,84 @@ 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`. 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. + + 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` + 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 +1973,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..61c28db482 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 @@ -12,6 +13,67 @@ 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 + + +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. + + 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. + """ + # 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: + """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. + + 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. + + 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(math.ldexp(base_delay, 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..8933cbe62a --- /dev/null +++ b/python/tests/test_backoff_ceiling.py @@ -0,0 +1,148 @@ +"""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 + + +# 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 + + +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 673983e996..8c88cf459c 100644 --- a/ruby/lib/basecamp/config.rb +++ b/ruby/lib/basecamp/config.rb @@ -45,6 +45,75 @@ 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 + + # 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. + # + # 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) + # 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. + # + # 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. + # + # 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. + # + # 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 + def self.saturating_backoff(base_delay, attempt) + if base_delay <= 0 + 0.0 + else + exponent = [ attempt - 1, 0 ].max + if exponent >= saturating_exponent(base_delay) + MAX_BACKOFF_DELAY + else + [ Math.ldexp(base_delay, exponent), MAX_BACKOFF_DELAY ].min.to_f + end + 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..5efae74645 --- /dev/null +++ b/ruby/test/basecamp/backoff_ceiling_test.rb @@ -0,0 +1,128 @@ +# 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 + + # 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 + + # 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/swift/Sources/Basecamp/HTTP/HTTPClient.swift b/swift/Sources/Basecamp/HTTP/HTTPClient.swift index 5660f44494..cab8154a1d 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,17 +37,84 @@ 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 /// 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.. 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..56597670f9 --- /dev/null +++ b/swift/Tests/BasecampTests/TransportNetworkErrorRetryTests.swift @@ -0,0 +1,547 @@ +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) + } + + // MARK: - Cancellation through the wrapper + + /// The two shapes a `Transport` normalizing into the SDK's taxonomy produces + /// when the caller cancels. Both are cancellation; neither is a blip. + private static let wrappedCancellations: [(name: String, error: BasecampError)] = [ + ("CancellationError", .network(message: "Cancelled", cause: CancellationError())), + ("URLError(.cancelled)", .network(message: "Cancelled", cause: URLError(.cancelled))), + ] + + /// Cancellation stays terminal when it arrives wrapped in `.network`. + /// + /// Routing a transport-thrown `.network` to the retry branch (#567) put it + /// past `catch let error as BasecampError`, where it used to be terminal, + /// and into the generic catch — where `isCancellation` saw only the outer + /// `BasecampError`. The retry loop then announced, slept, re-authenticated + /// and **re-sent a request the caller had cancelled**, while the identical + /// raw `CancellationError` stayed terminal. Classifying a network error by + /// meaning rather than by type is the whole premise of #567; it has to hold + /// for cancellation too, or the same error gets two answers. + func testWrappedCancellationIsTerminalOnPerformRequest() async { + for (name, error) in Self.wrappedCancellations { + let counter = AttemptCounter() + let hooks = RequestLifecycleSpy() + 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), + hooks: hooks, + 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, "\(name): a cancelled request must not be re-sent") + XCTAssertEqual(hooks.retries.count, 0, "\(name): onRetry must not fire for a cancellation") + } + } + + /// Both loops carry the same classification, so both are pinned. + func testWrappedCancellationIsTerminalOnTheDownloadHop() async { + for (name, error) in Self.wrappedCancellations { + let counter = AttemptCounter() + let hooks = RequestLifecycleSpy() + 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), + hooks: hooks, + transport: transport + ) + + _ = try? await client.httpClient.performDownloadRequest( + url: "https://3.basecampapi.com/999999999/attachment.json") + + XCTAssertEqual(counter.value, 1, "\(name): a cancelled download must not be re-sent") + XCTAssertEqual(hooks.retries.count, 0, "\(name): onRetry must not fire for a cancellation") + } + } + + /// The cancellation surfaces as the transport's own error, not a re-wrap. + func testWrappedCancellationSurfacesTheTransportsError() async { + let counter = AttemptCounter() + let transport = BasecampErrorTransport( + counter: counter, failuresBeforeSuccess: .max, + error: .network(message: "Cancelled", cause: CancellationError())) + + 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 cancellation error") + } catch let error as BasecampError { + XCTAssertEqual(error.message, "Cancelled") + } catch { + XCTFail("Expected a BasecampError, got \(error)") + } + } + + /// A `.network` whose cause is an ordinary failure is NOT cancellation and + /// must still retry — otherwise the guard above would close #567 back up. + func testWrappedNonCancellationCauseStillRetries() async throws { + let counter = AttemptCounter() + let transport = BasecampErrorTransport( + counter: counter, failuresBeforeSuccess: 1, + error: .network(message: "Connection reset", cause: URLError(.networkConnectionLost))) + + 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 non-cancellation cause must still reach the retry branch") + } + + // MARK: - Request-end lifecycle + + /// #567 moves a transport-thrown `.network` from the `BasecampError` catch — + /// which emitted **no** request-end event and said so in a comment — into the + /// generic catch, which finalizes the attempt with `statusCode: 0`. That is + /// a deliberate consequence, not an accident: the error now means "the + /// attempt failed at the transport", the same as a raw `URLError`, so it is + /// reported the same way and `onRequestStart`/`onRequestEnd` stay paired for + /// every attempt the loop begins. + /// + /// Pinned in both directions, because the split is the whole point: the + /// `.network` path gains the event, and every other `BasecampError` — the + /// transport's own final verdict — still emits none. + func testTransportNetworkErrorFinalizesEachAttempt() async { + let counter = AttemptCounter() + let hooks = RequestLifecycleSpy() + 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), + hooks: hooks, + transport: transport + ) + + _ = try? await client.forAccount("999999999").httpClient.performRequest( + method: "GET", + url: "https://3.basecampapi.com/999999999/projects.json", + retryConfig: retryConfig + ) + + XCTAssertEqual(counter.value, 3, "should have spent the whole retry budget") + XCTAssertEqual(hooks.requestStarts.count, 3) + XCTAssertEqual(hooks.requestEnds.count, 3, "every attempt the loop begins is finalized") + XCTAssertEqual(hooks.requestEnds.map(\.statusCode), [0, 0, 0], + "a transport failure is reported as status 0, like a raw URLError") + } + + /// The other side of the split: a non-`.network` `BasecampError` is terminal + /// on sight and still emits no request-end event. + func testNonNetworkBasecampErrorEmitsNoRequestEnd() async { + let counter = AttemptCounter() + let hooks = RequestLifecycleSpy() + let transport = BasecampErrorTransport( + counter: counter, failuresBeforeSuccess: .max, + error: .auth(message: "nope", hint: nil, requestId: nil)) + + let client = BasecampClient( + tokenProvider: StaticTokenProvider("test-token"), + userAgent: "test-suite", + config: BasecampConfig(baseURL: "https://3.basecampapi.com", enableRetry: true), + hooks: hooks, + transport: transport + ) + + _ = try? await client.forAccount("999999999").httpClient.performRequest( + method: "GET", + url: "https://3.basecampapi.com/999999999/projects.json", + retryConfig: retryConfig + ) + + XCTAssertEqual(hooks.requestStarts.count, 1) + XCTAssertEqual(hooks.requestEnds.count, 0, "the transport's own verdict emits no request-end") + } +} + +/// Records the request lifecycle so the #567 classification split can be +/// asserted on events, not just on attempt counts. +private final class RequestLifecycleSpy: BasecampHooks, @unchecked Sendable { + private let lock = NSLock() + private var _requestStarts: [RequestInfo] = [] + private var _requestEnds: [RequestResult] = [] + private var _retries: [Int] = [] + + var requestStarts: [RequestInfo] { lock.withLock { _requestStarts } } + var requestEnds: [RequestResult] { lock.withLock { _requestEnds } } + var retries: [Int] { lock.withLock { _retries } } + + func onRequestStart(_ info: RequestInfo) { + lock.withLock { _requestStarts.append(info) } + } + + func onRequestEnd(_ info: RequestInfo, result: RequestResult) { + lock.withLock { _requestEnds.append(result) } + } + + func onRetry(_ info: RequestInfo, attempt: Int, error: any Error, delaySeconds: TimeInterval) { + lock.withLock { _retries.append(attempt) } + } +} + +/// 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..87de701090 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,102 @@ export async function executeWithRetry( } } -export function calculateBackoffDelay(config: RetryConfig, attempt: number): number { - const base = config.baseDelayMs; - let delay: number; +/** + * `base * 2**exponent`, computed without ever forming `2**exponent`. + * + * 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. + */ +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`. + * + * 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 { + // 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; +} + +/** + * 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. + * + * 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`. + */ +export function saturatingBackoff( + baseDelayMs: number, + backoff: RetryConfig["backoff"], + attempt: number, +): number { + const base = baseDelayMs > 0 ? baseDelayMs : 0; + const index = attempt > 0 ? attempt : 0; + if (base === 0) return 0; - switch (config.backoff) { + let delay: number; + switch (backoff) { case "exponential": - delay = base * Math.pow(2, attempt); + if (index >= saturatingExponent(base)) return MAX_BACKOFF_DELAY_MS; + delay = scaleByPowerOfTwo(base, index); break; - case "linear": - delay = base * (attempt + 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; } + 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..31d60b2e3a --- /dev/null +++ b/typescript/tests/backoff-ceiling.test.ts @@ -0,0 +1,162 @@ +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); + }); + + /** + * 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]; + + /** + * 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) => + [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`), + ); + + 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); + } + }); + + /** + * 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 + * 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); + } + } + }); +});