diff --git a/SPEC.md b/SPEC.md index b12573fda4..726aeda806 100644 --- a/SPEC.md +++ b/SPEC.md @@ -528,7 +528,7 @@ RECORD BasecampError extends Error hint : String? -- optional user-friendly resolution guidance http_status : Integer? -- HTTP status code that caused the error retryable : Boolean -- whether the operation can be retried - retry_after : Integer? -- seconds to wait before retrying (from Retry-After header) + retry_after : Integer? -- seconds to wait before retrying (from Retry-After header, at any status) request_id : String? -- X-Request-Id from response headers field_errors : Map? -- structured 400/422 field messages confirmation_people : TemplateLibraryConfirmationPerson[]? -- people requiring template-copy access confirmation @@ -536,7 +536,7 @@ RECORD BasecampError extends Error END ``` -**Go divergence:** Go exposes a `Cause` field (the underlying error) not present in this canonical RECORD — a language-specific extension. `retry_after` is no longer a divergence: Go's `Error` carries it, populated at both 429 construction sites, and the raw GET retry loop sleeps it in place of the backoff curve. `RequestResult.retry_after` is unchanged and remains the hook-facing copy rather than the only one. +**Go divergence:** Go exposes a `Cause` field (the underlying error) not present in this canonical RECORD — a language-specific extension. `retry_after` is no longer a divergence: Go's `Error` carries it, populated at every status the header parses at, and the raw GET retry loop sleeps it in place of the backoff curve. `RequestResult.retry_after` is unchanged and remains the hook-facing copy rather than the only one. ### Error Code Table @@ -580,6 +580,19 @@ Step 11 must precede the 5xx catch-all. A 507 is a *server* status carrying a *c In all cases, extract `request_id` from `X-Request-Id` response header if present. `[conformance]` +In all cases, `retry_after` is `parseRetryAfter(headers)` — populated at **every** status the header +parses at, not only in step 4. One parse feeds both the retry loop's sleep and the error's field, so +an exhausted 503 that was slept on for the value the origin named surfaces that value to the caller, +and so does the error §7 step 3i hands to `on_retry`. Step 4 spells it out only because 429 is where +the `hint` is derived from it. `[CONFLICT: Go, TypeScript, Ruby, Python and Rust populate it at every +status THIS ALGORITHM MAPS — the shared mapper each retry loop and operation path returns through; a +status mapped by hand elsewhere carries it only where that site parses the header itself. Ruby's two +download hops do (#855); Go's hop-2 errors and its hop-1 500, Python's download errors on both the sync +and async paths, and TypeScript's hop-2 errors build their `api_error` without the header, so a +`Retry-After` on those responses is dropped — owed by the sweep (#857), where routing them through this +mapper or parsing the header at the site is the change. Kotlin's `Api` and Swift's `.api` carry no slot +for it yet — adding one is a source-breaking change to Swift's enum, tracked in #775.]` + ### Statusless `api_error` for a malformed 2xx body `[manual]` The mapping above is keyed on an HTTP status, because it maps *failed* responses. A composite (§18) can also fail on a **successful** one: the transport returned 2xx, and the body is malformed in a way that makes the composite's next step unsafe — a writable field of the wrong type, or a required field absent, on a read the composite is about to echo back into a full-replace write. @@ -668,24 +681,84 @@ The typed list is available only when every person entry satisfies that shape. A different or malformed `people` member remains a canonical validation error, so unrelated validation bodies retain the shared error taxonomy. -### Retry-After Parsing Algorithm +### Retry-After Parsing Algorithm `[conformance]` + +Given header value `value`, after the transport has stripped the optional whitespace RFC 9110 +permits around any field value: -Given header value `value`: +1. If `value` is RFC 9110's `delay-seconds` — `1*DIGIT`, nothing else — read it as a whole number of + seconds. If that is `0` → step 3. If it exceeds `MAX_RETRY_AFTER_SECONDS` → return + `MAX_RETRY_AFTER_SECONDS`. Otherwise → return it. +2. If `value` is an HTTP-date (RFC 7231 §7.1.1.1) → compute `date - now()` in seconds, **rounding a + sub-second remainder up**. If that is not positive → step 3. If it exceeds + `MAX_RETRY_AFTER_SECONDS` → return `MAX_RETRY_AFTER_SECONDS`. Otherwise → return it. +3. → `undefined`: no server-directed delay. The caller falls through to the backoff formula. -1. Attempt parse as integer. If valid and > 0 → return as seconds. -2. Attempt parse as HTTP-date (RFC 7231, e.g., `Wed, 09 Jun 2021 10:18:14 GMT`). If valid → compute `max(0, date - now())` in seconds, **rounding a sub-second remainder up**; if > 0 → return. -3. → `undefined` (fall through to backoff formula). +The same algorithm as a table, because the seven SDKs each inherited a different answer from whatever +standard-library parse they reached for, and the rows are where that showed: -Step 2's rounding is up, not truncating, for two reasons: a positive remainder must never round to +| `value` | Result | Why | +|---|---|---| +| absent, or empty | `undefined` | nothing was said | +| `120` | 120 | step 1 | +| `0` | `undefined` | a zero wait is not an instruction to retry at once; it is no instruction, and the backoff curve applies | +| `-5`, `+5`, `1.5`, `120junk` | `undefined` | not `1*DIGIT`. A sign, a fraction, a trailing character — any of them makes the whole value malformed. Parsers that delegate to a stdlib integer parse tend to accept `+5` as 5; the grammar does not | +| `2147483648`, `99999999999999999999` | `2147483647` | step 1 saturates. The value is over-range, not malformed: `1*DIGIT` has no upper bound, so no digit string is refused for its length, and a parse that reads it as "no delay" would replace a request for a very long wait with the millisecond backoff curve | +| `Wed, 09 Jun 2021 10:18:14 GMT` (past) | `undefined` | step 2, non-positive | +| IMF-fixdate 2.4 s ahead | 3 | step 2, rounded up | +| IMF-fixdate centuries ahead | `2147483647` | step 2 saturates, the same ceiling | +| `Sunday, 06-Nov-94 08:49:37 GMT` (RFC 850), `Sun Nov 6 08:49:37 1994` (asctime) | MAY parse as step 2; otherwise `undefined` | see "Date forms" | +| `2021-06-09T10:18:14Z`, `2099`, `Jan 1 2099` | `undefined` | not an HTTP-date. A permissive date parser is how a malformed header once bought a 73-year delay (#781) | + +**`MAX_RETRY_AFTER_SECONDS` is 2,147,483,647** (Appendix A), and it is a representability bound +pinned once for all seven SDKs rather than a policy cap: the largest value the narrowest integer any +of the seven carries a `retry_after` in can hold — Go's `int` on its 32-bit targets, Kotlin's `Int` — +and the same number §16 already names as the shared token-lifetime ceiling. It replaces the two-tier +rule an earlier revision stated here, under which a digit string wider than the parser's own integer +type was *malformed* and one inside it *saturated*. That tier boundary was the parser's word size, +which is exactly the kind of inherited-not-chosen answer this table exists to remove: the same header +was honoured on a 64-bit build and dropped on a 32-bit one. One ceiling, compared against the digit +string before any conversion can overflow, gives every host the same answer. Saturation sits **in the +parser**, so the error's `retry_after` reads the saturated value; that is the one deliberate exception +to the "never in the parser" rule in the next section, and it is accepted because a value beyond the +ceiling is one the SDK could not have reported faithfully in that field anyway. + +**Rounding** (step 2) is up, not truncating, for two reasons: a positive remainder must never round to zero, because zero is read as "no usable value" and drops the request onto the local backoff curve — the opposite of what the header said; and rounding down retries up to a second *before* the moment -the server named, which is the one thing a date is unambiguous about. TypeScript, Kotlin, Swift and -Go round up. Python and Ruby still truncate, tracked in #799. +the server named, which is the one thing a date is unambiguous about. The difference is exactly one +second wide, so a test that asserts the parsed value against a literal is flaky by construction — the +room before the answer drops by one is `1 - frac(now)`; pin it one-sidedly (the parsed delay is never +shorter than the time actually remaining) or against a frozen clock. + +**Date forms.** RFC 7231 §7.1.1.1 obliges a sender to emit IMF-fixdate (`Sun, 06 Nov 1994 08:49:37 +GMT`) and a recipient to also accept the obsolete RFC 850 and asctime forms. This contract requires the +first and permits the other two: **an SDK MUST accept IMF-fixdate, MAY accept RFC 850 and asctime, and +MUST NOT accept a shape that is not an HTTP-date at all.** The obsolete forms are permitted rather than +required because no conformant origin can send them, BC5 does not, and requiring them costs the SDKs +whose standard library lacks them a hand-rolled two-digit-year pivot for a header they will never see — +whereas the cost of not accepting one is a ~1 s local backoff in place of the server's interval, which +nothing hangs on. The last clause is the load-bearing one: it is the shape gate that keeps an ISO-8601 +timestamp or a bare year from being read as a date. An SDK whose stdlib parser takes the obsolete forms +keeps them; an SDK whose parser takes a wider family of HTTP-date-shaped variants (Kotlin's ktor list) +is inside the MAY, because every member is an HTTP-date shape. What this rules out is the accidental +state that preceded it — two permissive by inheritance, four strict by inheritance, and the contract +silent on which. This algorithm defines **parsing** only — how a header value becomes a number of seconds. Which statuses honour the result, and what bounds the sleep it buys, is the next section; do not read a status set into the steps above. +`[CONFLICT: the table is the contract; the SDKs converge on it in two steps. The status gate, the +rounding rule and the added-jitter defect are converged; the ceiling row is implemented in Go +(both parsers) and in Rust, and owed by the other five — each still refuses or raises above its own integer width — +and the sign row is owed by TypeScript, Ruby, Python, Kotlin and Swift. The two that delegate to a +stdlib integer parse owe more than the sign, since they inherit everything that parse admits: Python's +`int()` takes `_` digit separators and surrounding whitespace, so `1_000` is honoured as a thousand-second +wait rather than falling through to backoff; Ruby's `Integer()` without a base takes `_` and the `0x`, +`0b` and `0o` prefixes, and reads a leading zero as octal, so `010` sleeps 8 seconds and `09` is malformed. +What each owes is the whole `1*DIGIT` gate. Per-parser inventory and call sites in #799 and #775.]` + ### Retry-After Honouring `[CONFLICT]` **A parsed `Retry-After` is honoured at any status a retry is already going to happen at.** There is @@ -800,22 +873,22 @@ jitter term is part of the locally computed formula, where it exists to decorrel the same delay independently; a delay the origin named is already the origin's choice, so adding to it makes the client wait longer than it was told for no benefit. An implementation MAY have a **host limit** — a timer that cannot schedule the value, a conversion that would trap or wrap — and where a -parsed value meets one it MUST bound the value there: saturate, per the second representability tier -below, never let the conversion trap or wrap, and never fall back to the local backoff. A bound of +parsed value meets one it MUST bound the value there: saturate, never let the conversion trap or +wrap, and never fall back to the local backoff. A bound of that kind belongs at the sleep, so wherever the error carries `retry_after` the caller reads what the server said, never the clamped copy. TypeScript's `Math.min(seconds × 1000, MAX_TIMEOUT_MS)`, applied in both of its retry loops as the delay is computed, is the worked example: the parser's result stays the public `retryAfter`, and only what reaches the timer is clamped. -That is a guarantee about the field's *integrity*, not its *presence*. The Status Mapping Algorithm -above populates `retry_after` in its 429 arm only, so today an exhausted 503 that was slept on for -the value the origin named surfaces no `retry_after` to the caller, and neither does the error §7 step -3i hands to `on_retry`. Whether the mapping grows the field at every status a declared retry set -carries is part of the status convergence in #775, not decided here — for the SDKs whose delay loop -reads the value off the constructed error, it is the same change as the status gate, because one -parse feeds both. +That is a guarantee about the field's *integrity*, and the Status Mapping Algorithm above now settles +its *presence* too: `retry_after` is populated at every status the header parses at, so an exhausted +503 that was slept on for the value the origin named surfaces that value, and so does the error §7 +step 3i hands to `on_retry`. For the SDKs whose delay loop reads the value off the constructed error +it is literally the same change as the status gate, because one parse feeds both. The one exception +to integrity is the ceiling: a value above `MAX_RETRY_AFTER_SECONDS` saturates in the parser, so the +field reads the ceiling rather than the origin's number (Parsing Algorithm above). -Read that paragraph narrowly: it says what may not be done *to* the value — not capped, not summed +Read the paragraph before last narrowly: it says what may not be done *to* the value — not capped, not summed with a jitter term. It does not say what the value is combined *with*, which is the next paragraph's subject and is not the same question. `max(interval, retryAfter)` neither caps the value nor adds to it. @@ -866,74 +939,32 @@ was going to be retried anyway; only the composition differs, which is the whole The five rows do not conflict with each other — they are five loops, not five answers to one question — and this paragraph converges nothing: each section keeps the behaviour it already has, and -what changes is that it now states it on purpose rather than by omission. One *implementation* -divergence sits on this axis and is already recorded below: the generated Go client sleeps -`retryDelay + rand(0, 100ms)` on the §7 row, which is the added jitter the paragraph above forbids, -and it is tracked with the rest in #775. +what changes is that it now states it on purpose rather than by omission. The one *implementation* +divergence that sat on this axis — the generated Go client sleeping `retryDelay + rand(0, 100ms)` on +the §7 row — is closed: `go/templates/client.tmpl` now waits a server-directed delay exactly and adds +its jitter to the local curve alone. -**Representability is not a policy cap, and it is exempt from the "never in the parser" rule.** A +**Representability is not a policy cap, and its one bound is fixed by the Parsing Algorithm.** A policy cap answers "how long *should* a caller wait"; representability answers "can this host express -the value at all", and where the answer is no there is nothing to preserve. Two tiers, in the shape -§16's device-flow parser already settled — its second tier bounds against a domain ceiling (the -remaining device-code lifetime) rather than a host one, but the fall-back-versus-clamp split is the -same and is adopted here: - -- **Unrepresentable in the parser's own numeric type → malformed.** It falls out at step 3 of the - Parsing Algorithm to the local backoff, exactly like a fractional or non-positive value. Nothing is - surfaced on the error, because nothing was parsed. -- **Representable by the parser but beyond what the host can schedule → saturate**, never fall back. - Falling back would replace a server's request for a long wait with the millisecond backoff curve - and hammer a peer that just asked to be left alone — the same tight loop by another route. Where - the overflowing conversion is the one *feeding* the parser's own output type, the saturation may - sit in the parser, and the caller then reads the saturated value; that is accepted, and it is the - one carve-out from the paragraph above. - -The host whose limit binds is the one the build runs on, and an SDK that ships to more than one -build target MAY pin the second-tier ceiling at the smallest value every supported target can -represent and schedule, so the delay honoured does not vary by build. That is still a -representability bound, not a policy cap: it is derived from a host limit — the narrowest one the -SDK ships to — and answers "can every host express this value" rather than "how long should a caller -wait". It clamps nothing the narrowest build could have honoured, and what it costs the wider builds -is only the waits the narrowest one could never have scheduled. - -The width itself is deliberately not fixed here, because it is a property of the host. *(As-of -observation, verified against `wt/lane-spec` @ `fc5645dfe` — kept because the decision not to fix a -width is unreadable without it, not as a live claim: TypeScript rejects above -`Number.MAX_SAFE_INTEGER`, Kotlin above `Int.MAX_VALUE` on the delta-seconds form — its date form -computes in `Long` and saturates to `Int.MAX_VALUE` instead, which is the parser-output carve-out -above rather than a rejection — Swift above its 64-bit `Int`, and Go above native `int`: both its -hand-written and its generated parser are `strconv.Atoi` at that revision, so the width is 32 bits on -the 32-bit targets this repository keeps viable and 64 elsewhere. #796 has since moved the -hand-written parser to `ParseInt` into `int64`; the generated one stays `Atoi` until #798.)* -Four hosts reject cleanly at thresholds differing by nine orders of magnitude without any of them -misbehaving, which is the evidence that the width does not need fixing — a `Retry-After` naming a wait -longer than the host can count is not a delay any caller is worse off for missing. - -`[CONFLICT: Ruby and Python have no such width, and that is a defect rather than a third position — -but it is the **second** tier they owe, not the first. `Integer` and `int` are arbitrary-precision, so -the parse cannot fail on magnitude and the first tier has nothing to fire on; the failure is one layer -down, at the scheduler, where the sleep raises rather than saturating. Reading it as the first tier -would oblige an implementer to invent a parser limit this section otherwise forbids. Both owe -saturation at whatever their own sleep can schedule, applied at the sleep — which for Python is two -different ceilings for the sync and async clients, since the binding host limit differs. Exact -ceilings, exception types and call sites in #775.]` - -Go is the worked example of the two tiers meeting in one parser, and the two numbers govern different -questions: a digit string too large for the parser's own `int64` is **malformed** and falls through to -the backoff (first tier), while a value it holds but the host cannot schedule **saturates** (second -tier) at the pinned portable ceiling the tier permits — `math.MaxInt32` seconds, the smallest value -every supported `GOARCH` can represent, and the same 2,147,483,647 §16 already names as a shared -cross-SDK ceiling. Pinning matters because two host limits sit above a Go `Retry-After` — native -`int`, which the public `Error.RetryAfter` field is and which is 32 bits wide on the 32-bit targets -this repository keeps viable, and `time.Duration` — and a ceiling derived from only the larger would -change with `GOARCH`. That is what #796 ships: `ParseInt` into `int64`, over-range malformed, and a -clamp at `math.MaxInt32` inside the shared hand-written `parseRetryAfter` that the raw retry loop, -the download path and the hook result all read. - -The identical unclamped conversion in `go/pkg/generated/client.gen.go`, and an `Atoi` there whose -range error is discarded into a rate-limit hint, are **not yet fixed anywhere**. Their fix *belongs* -in `go/templates/client.tmpl` — the generated file is emitted from it and must never be edited -directly — and #798 is where the work is tracked, not where it has landed. +the value at all", and where the answer is no there is nothing to preserve. The Parsing Algorithm +above pins that bound once for every SDK — `MAX_RETRY_AFTER_SECONDS`, applied in the parser to both +the delta-seconds and the HTTP-date form — so the honoured wait no longer depends on which integer +type a standard-library parse happened to use. Below the ceiling, a host that cannot *schedule* a +value it can hold MUST still saturate at its own limit — at the sleep, never by falling back to the +local backoff, which would replace a server's request for a long wait with the millisecond curve and +hammer a peer that just asked to be left alone. TypeScript's `setTimeout` bound of 2,147,483,647 ms +is the worked example: the parser's result stays the public `retryAfter`, and only what reaches the +timer is clamped. Every other host can schedule the full ceiling — 2,147,483,647 s is inside `sleep`, +`time.sleep`, `float`, `Duration`, coroutine `delay` and `Task.sleep`'s nanosecond `UInt64` alike — so +no second bound is needed elsewhere, and none is permitted: Swift's 86,400 s clamp is a policy cap by +its own comment ("no SDK retry is worth sleeping longer"), five orders of magnitude below the trap it +cites, and is recorded as a conflict below. + +`[CONFLICT: Ruby and Python parse arbitrary-precision integers and raise at the scheduler above their +own ceilings (`RangeError` from `sleep`, `OverflowError` from `float`); TypeScript, Kotlin and Swift +refuse above their integer width instead of saturating. All five owe the parser ceiling — the exact +sites are in the Parsing Algorithm's conflict note. Go implements it in both parsers as of #796 and +the template change that closed #798; Rust shipped with it (#859).]` **The exemption is conditioned on the escape, not on a number.** There is deliberately no policy cap; in its place, **an honoured `Retry-After` delay MUST be awaited through the platform's cancellation @@ -951,31 +982,22 @@ changes their mind still waits it out, and bounding the worst case that way is a another name. Which satisfying shape each language picks carries a caller-facing API dimension and is not settled here. -`[CONFLICT: the cost of this position is not uniform — four of the SDK sleep paths give the caller no -handle at all today, and all four already carry the exposure independently of this decision: each -honours Retry-After on its 429 path now, so the un-abandonable server-directed sleep predates the -status rule, and widening the status set widens it rather than introducing it. Per-path inventory -and remedies in #775.]` +`[CONFLICT: four of the SDK sleep paths give the caller no handle today — TypeScript's multipart +upload and `DownloadURL` hop 1, Ruby, and Python's sync client — and all four carried the exposure +before the status rule, since each honoured Retry-After on its 429 path already; the status +convergence widened it rather than introducing it. Per-path inventory and remedies in #775.]` Strictly, none of those four is *uninterruptible*: a signal on the main thread, or `Thread#raise` from another, will break any of them. What they lack is a cancellation handle the caller can **hold**, and the requirement above is about the handle, not about whether the platform can ever intervene. -`[CONFLICT: five of the seven SDKs gate honouring on a narrower status set than this section -prescribes, in three different shapes, and two of this section's other clauses are also divergent — -one policy cap and one added jitter term. Converging is a behaviour change across five SDKs. The -per-SDK inventory is deliberately NOT restated here: it states current behaviour, the convergence -work below changes the very rows it would state, and no gate can catch it going stale. It lives in -#775, verified as of that issue's dated comment.]` - -Which **date forms** step 2 accepts diverges on a second axis, and the inventory is over *parsers* -rather than one per SDK — Go has two and they do not agree, the generated one -accepting no date form at all, so **every** HTTP-date falls through to local backoff on **every** -generated Go wire operation. That matters to the contract in one way only, and it is the reason this -sentence stays: convergence scoped by SDK name would fix the hand-written parser and leave the -generated surface untouched, so the fix site is `go/templates/client.tmpl`, never a file under -`go/pkg/generated/`. `[CONFLICT: per-parser inventory in #775; the template change rides with #798, -which owns the other two `Retry-After` defects at those same two lines.]` +**Where the seven stand against this section**, stated as which clause is still open rather than as a +per-SDK inventory, which changes as work lands and belongs in #775: the status gate is converged in +all seven loops (§7 and §14 hop 1 alike, `retry.json` and `downloads.json` pin it); the added-jitter +term is gone; `retry_after` is on the error at every status except in Kotlin and Swift (Status +Mapping Algorithm); the parser ceiling and the sign row are held by Go's two parsers and Rust's and +owed by the other five (Parsing Algorithm); the policy cap is Swift's alone; and the cancellation +handle is owed by the four paths above. --- @@ -1180,17 +1202,16 @@ Requirements: 4. **`Retry-After` is exempt.** It is server-directed and takes precedence per step 3h, at every status that step reaches (§6 "Retry-After Honouring"); the ceiling governs the locally-computed formula only, and step 2's jitter is part of that formula rather - than an addend on a server-directed delay. Implementations may still bound it against - **host limits** — a timer that cannot schedule the value, such as TypeScript's clamp to - the 2,147,483,647ms `setTimeout` accepts, or a conversion that would trap or wrap, such - as the seconds→`time.Duration` saturation Go takes (#796) — and may reject outright a - value the parser's own numeric type cannot hold. §6 "Retry-After Honouring" governs - which of those belongs at the sleep and which may sit in the parser. A **policy** cap is a - different thing and is not permitted: Swift's 86,400s clamp is one (the `UInt64` - nanosecond trap it cites sits five orders of magnitude higher), and §6 records it as a - conflict alongside the status divergence. The exemption is not unconditional: §6 - requires the honoured delay to be awaited through the caller's cancellation primitive, - and that requirement — not a number — is what stands in for a policy cap here. + than an addend on a server-directed delay. The one bound on it is + `MAX_RETRY_AFTER_SECONDS`, applied in the parser (§6 "Retry-After Parsing Algorithm"), + plus whatever a host's timer cannot schedule below that — TypeScript's clamp to the + 2,147,483,647 ms `setTimeout` accepts — applied at the sleep. A **policy** cap is a + different thing and is not permitted: Swift's 86,400 s clamp is one (the `UInt64` + nanosecond trap it cites sits five orders of magnitude higher, and the parser ceiling + already keeps the product inside `UInt64`), and §6 records it as a conflict. The + exemption is not unconditional: §6 requires the honoured delay to be awaited through + the caller's cancellation primitive, and that requirement — not a number — is what + stands in for a policy cap here. **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 @@ -1829,7 +1850,7 @@ END The authenticated first hop retries on **network errors plus {429, 502, 503, 504}** — never 500. The set is declared here rather than inherited from anywhere else, and it matches neither of the two sets an SDK already has to hand: it is broader than the per-operation `retry_on` in `behavior-model.json` (`{429, 503}` for all `262` operations but `UpdateProjectClientAccess`, and never governing `DownloadURL` because it has no entry there), and narrower than the error taxonomy's "all 5xx retryable" flag, which would sweep in the 500 this policy deliberately excludes. It is the gateway-error set Go's hand-written `singleRequest` already uses for GETs. Backoff is exponential from a 1-second base with jitter; `Retry-After` is honoured at **every status in that set**, not at 429 alone. The second hop is exempt: no retry, no auth. -That last clause changed with §6's "Retry-After Honouring", and the reason it changed is the reason this set is declared here at all: honouring is derived from retry eligibility, so a loop that declares its own eligibility set inherits the honouring rule over that set rather than over §7's. A 502, 503 or 504 on hop 1 carrying `Retry-After` therefore waits what the origin named, exactly as a 429 does. `[CONFLICT: most download loops honour it on 429 alone today and owe convergence; one SDK already conforms. Per-SDK state and call sites in #775 — not restated here, because this is exactly the row that convergence changes. For conformance: the existing downloads.json case covering the 429 path stays valid; the other three statuses need cases of their own.]` The honoured value is subject to §6's other two clauses on this path as well: nothing is added to it, and it must be awaited through a cancellation handle the caller holds, which not every download path yet gives them (#775). +That last clause changed with §6's "Retry-After Honouring", and the reason it changed is the reason this set is declared here at all: honouring is derived from retry eligibility, so a loop that declares its own eligibility set inherits the honouring rule over that set rather than over §7's. A 502, 503 or 504 on hop 1 carrying `Retry-After` therefore waits what the origin named, exactly as a 429 does — `downloads.json` pins all four statuses. The honoured value is subject to §6's other two clauses on this path as well: nothing is added to it, and it must be awaited through a cancellation handle the caller holds, which not every download path yet gives them (#775). **Composition (§6 "Composition is per-loop"): a valid `Retry-After` REPLACES this hop's exponential-plus-jitter delay**, the same answer §7's loop gives and for the same reason — the wait is pacing a retry of exactly the request the origin just answered. It is stated here rather than inherited: §6 supplies no default, so a loop that declares its own retry set (as this one does) declares its own composition too. @@ -2725,6 +2746,22 @@ Test cases conform to `conformance/schema.json`. Each test specifies: - `mockResponses` — sequence of mock responses the test server returns - `assertions` — behavioral assertions to verify +A fixture is a static JSON literal, and the harness has no clock of its own — which is enough for +every assertion but one. §6's Retry-After Parsing Algorithm honours an HTTP-date only when +`date - now()` is positive, so a literal date is either already past (and pins only the fall-through) +or far enough ahead to make a compliant SDK sleep for years. A response header value MAY therefore +carry the token **`{{httpdate+Ns}}`** (`N` = one to nine digits, so every runner resolves it in exact +integer arithmetic and every date formatter stays in range), which every runner resolves **at the moment +it serves that response** to the IMF-fixdate of `floor(now) + N + 1` seconds — the first whole +second strictly more than `N` seconds after the second the response is served in. A compliant parser +sees a remainder in `(N - latency, N + 1]` and, rounding up, computes at least `N` whole seconds for +any serve-to-parse latency under one second, so the fixture asserts `delayBetweenRequests` with +`min: N × 1000`; a parser that drops the date form waits the ~1 s local backoff instead and fails. +The token is deliberately relative and near: an absolute far-future date is not merely slow, it is +differently behaved per host (one saturates, one clamps, four run long), so a fixture naming one would +assert six different things. An unrecognised `{{…}}` token is a runner error, never served literally. +Each runner's resolver is unit-tested the way `checkDelayGaps` is. + ### Assertion Types Enumerated from `conformance/schema.json` — the table below is gated against @@ -4067,7 +4104,7 @@ consumption) are recorded in Appendix F with their compensating tier-3 tests. All magic numbers in one place, derived from shipping SDK code (not `rubric-audit.json`). -Only `API_VERSION` is gated (``, checked by `make doc-constants-check`). The other 14 pre-§23 rows are hand-maintained: 13 were read against their cited sources on 2026-08-03 — all 13 matched — and `MAX_BACKOFF_DELAY` joined with #592 under that PR's own six-SDK verification (the sentence previously said 13 rows while the table carried 14). The `EVENT_FEED_*` block below them is different in kind and marked so: those rows are contract-first — their source is §23's normative text, connector code ships in later PRs, and the two server-owned values are provisional until bc3's merge-time gate; when the connector lands, they join the read-against-source discipline. They are not gated because each is asserted of several SDKs at once in a different spelling per language (Go `1 * time.Second`, Python `1.0`, Ruby `1.0`, Kotlin `30.seconds`, Swift `1_000`), so a checker would need a per-row, per-language extraction rule rather than the one-value-one-source substitution the marker convention is built on. The name in the table is the concept, not a symbol to grep: `MAX_ERROR_MESSAGE_LENGTH` is `MaxErrorMessageBytes` in Go and `MAX_ERROR_MESSAGE_BYTES` in Ruby, and `TOKEN_REFRESH_BUFFER` is the literal `300` in `creds.ExpiresAt-300` (`go/pkg/basecamp/auth.go`) rather than a named constant at all. If one of these starts moving, gate that row rather than the appendix. +Only `API_VERSION` is gated (``, checked by `make doc-constants-check`). The other 15 pre-§23 rows are hand-maintained: 13 were read against their cited sources on 2026-08-03 — all 13 matched — `MAX_BACKOFF_DELAY` joined with #592 under that PR's own six-SDK verification (the sentence previously said 13 rows while the table carried 14), and `MAX_RETRY_AFTER_SECONDS` joined with the Retry-After convergence, verified in Go's two parsers and Rust's and owed by the other five (§6). The `EVENT_FEED_*` block below them is different in kind and marked so: those rows are contract-first — their source is §23's normative text, connector code ships in later PRs, and the two server-owned values are provisional until bc3's merge-time gate; when the connector lands, they join the read-against-source discipline. They are not gated because each is asserted of several SDKs at once in a different spelling per language (Go `1 * time.Second`, Python `1.0`, Ruby `1.0`, Kotlin `30.seconds`, Swift `1_000`), so a checker would need a per-row, per-language extraction rule rather than the one-value-one-source substitution the marker convention is built on. The name in the table is the concept, not a symbol to grep: `MAX_ERROR_MESSAGE_LENGTH` is `MaxErrorMessageBytes` in Go and `MAX_ERROR_MESSAGE_BYTES` in Ruby, and `TOKEN_REFRESH_BUFFER` is the literal `300` in `creds.ExpiresAt-300` (`go/pkg/basecamp/auth.go`) rather than a named constant at all. If one of these starts moving, gate that row rather than the appendix. | Constant | Value | Unit | Source | |----------|-------|------|--------| @@ -4081,6 +4118,7 @@ Only `API_VERSION` is gated (``, checked by `make doc-const | `DEFAULT_BASE_DELAY` | 1000 | milliseconds | All seven SDKs | | `DEFAULT_MAX_JITTER` | 100 | milliseconds | All seven SDKs | | `MAX_BACKOFF_DELAY` | 30,000 (30s) | milliseconds | All seven SDKs; ceiling on the §7 backoff term, jitter added on top. Was Go's generated `RetryConfig.MaxDelay` before #577 generalized it | +| `MAX_RETRY_AFTER_SECONDS` | 2,147,483,647 | seconds | §6 Retry-After Parsing Algorithm — the value a parsed `Retry-After` saturates at, in both wire forms; a representability bound (the narrowest `retry_after` integer any SDK ships, and §16's shared ceiling), not a policy cap. `go/pkg/basecamp/client.go` (`maxRetryAfterSeconds`), `go/templates/client.tmpl`, `rust/basecamp-sdk/src/error.rs` (`MAX_RETRY_AFTER_SECONDS`); owed by the other five (§6) | | `DEFAULT_MAX_PAGES` | 10,000 | — | All seven SDKs | | `MAX_CACHE_ENTRIES` | 1000 | entries | `typescript/src/client.ts` | | `MAX_TOKEN_HASH_ENTRIES` | 100 | entries | `typescript/src/client.ts` | @@ -4204,6 +4242,8 @@ what `make doc-constants-check` asserts — not a case-by-case index. | `retry.json` | Retry-After HTTP-date in the past falls through to backoff | §6, §7 | | `retry.json` | Retry-After of 0, and a negative value, rejected | §6, §7 | | `retry.json` | Partly numeric Retry-After rejected (`1*DIGIT`) | §6, §7 | +| `retry.json` | GET retries on 503 with Retry-After (honoured at every declared status) | §6, §7 | +| `retry.json` | Retry-After HTTP-date in the future is honoured (`{{httpdate+Ns}}`) | §6, §7, §19 | | `security.json` | Cross-origin Link rejected | §8, §9 | | `security.json` | HTTPS enforced (non-localhost) | §9 | | `security.json` | HTTP allowed for localhost | §9 | @@ -4230,6 +4270,7 @@ what `make doc-constants-check` asserts — not a case-by-case index. | `downloads.json` | DownloadURL retries hop 1 on a network error | §14, §7 | | `downloads.json` | DownloadURL does not retry hop 1 on 500 | §14, §7 | | `downloads.json` | DownloadURL honors Retry-After on 429 at the auth'd first hop | §14, §7 | +| `downloads.json` | DownloadURL honors Retry-After on 502, 503 and 504 at the auth'd first hop | §14, §6 | | `downloads.json` | DownloadURL surfaces redirect with no Location | §14 | | `downloads.json` | DownloadURL refuses a redirect on the signed second hop | §14 | | `network-retry.json` | Network error on a non-idempotent POST is not retried | §7 (Gate 2) | diff --git a/conformance/runner/go/header_tokens.go b/conformance/runner/go/header_tokens.go new file mode 100644 index 0000000000..ebf8b57aeb --- /dev/null +++ b/conformance/runner/go/header_tokens.go @@ -0,0 +1,49 @@ +package main + +import ( + "fmt" + "net/http" + "regexp" + "strconv" + "time" +) + +var ( + headerToken = regexp.MustCompile(`^\{\{(.*)\}\}$`) + httpdateToken = regexp.MustCompile(`^httpdate\+([0-9]{1,9})s$`) +) + +// resolveHeaderValue substitutes the one token a fixture header value may +// carry, `{{httpdate+Ns}}`, at the moment the response is served (SPEC §19, +// conformance/schema.json). Every other value passes through untouched. +// +// The token resolves to the IMF-fixdate of floor(now) + N + 1 seconds: the +// first whole second strictly more than N seconds after the second the +// response is served in. A compliant SPEC §6 parser sees a remainder in +// (N - latency, N + 1] and, rounding up, computes at least N whole seconds, so +// the fixture pairs it with a `delayBetweenRequests` floor of N × 1000 ms. It +// exists because a static fixture has no clock: a literal past date pins only +// the fall-through, and a far-future one is differently behaved per host. +// +// N is one to nine digits, so the arithmetic is exact everywhere and every +// runner's date formatter stays in range; a longer N is an unrecognised token. +// +// An unrecognised `{{…}}` is an error rather than a literal: a typo'd token +// served verbatim would be an unparseable header, which the SDK answers with +// its ordinary backoff — the exact outcome the case exists to distinguish from. +func resolveHeaderValue(value string, now time.Time) (string, error) { + token := headerToken.FindStringSubmatch(value) + if token == nil { + return value, nil + } + inner := httpdateToken.FindStringSubmatch(token[1]) + if inner == nil { + return "", fmt.Errorf("unrecognised header token %q: only {{httpdate+Ns}} is defined (conformance/schema.json)", value) + } + n, err := strconv.ParseInt(inner[1], 10, 64) + if err != nil { + return "", fmt.Errorf("header token %q: %w", value, err) + } + at := time.Unix(now.Unix()+n+1, 0).UTC() + return at.Format(http.TimeFormat), nil +} diff --git a/conformance/runner/go/header_tokens_test.go b/conformance/runner/go/header_tokens_test.go new file mode 100644 index 0000000000..150ca57b69 --- /dev/null +++ b/conformance/runner/go/header_tokens_test.go @@ -0,0 +1,50 @@ +package main + +import ( + "strings" + "testing" + "time" +) + +// A quarter-second into 10:18:14, so the floor and the round-up land on +// different seconds and a resolver that rounded would show it. +var tokenNow = time.Date(2021, time.June, 9, 10, 18, 14, 250_000_000, time.UTC) + +func TestResolveHeaderValue_PassesPlainValuesThrough(t *testing.T) { + for _, v := range []string{"", "2", "Wed, 09 Jun 2021 10:18:14 GMT", "application/json", "{not a token}"} { + got, err := resolveHeaderValue(v, tokenNow) + if err != nil || got != v { + t.Errorf("resolveHeaderValue(%q) = %q, %v; want the value unchanged", v, got, err) + } + } +} + +func TestResolveHeaderValue_ResolvesHttpdateToTheWholeSecondPastN(t *testing.T) { + cases := map[string]string{ + "{{httpdate+2s}}": "Wed, 09 Jun 2021 10:18:17 GMT", + "{{httpdate+0s}}": "Wed, 09 Jun 2021 10:18:15 GMT", + "{{httpdate+10s}}": "Wed, 09 Jun 2021 10:18:25 GMT", + } + for token, want := range cases { + got, err := resolveHeaderValue(token, tokenNow) + if err != nil { + t.Fatalf("resolveHeaderValue(%q): %v", token, err) + } + if got != want { + t.Errorf("resolveHeaderValue(%q) = %q, want %q (floor(now) + N + 1, IMF-fixdate)", token, got, want) + } + } +} + +func TestResolveHeaderValue_RejectsAnUnknownToken(t *testing.T) { + for _, v := range []string{"{{httpdate}}", "{{httpdate+2}}", "{{httpdate-2s}}", "{{now}}", "{{}}", "{{httpdate+1000000000s}}"} { + got, err := resolveHeaderValue(v, tokenNow) + if err == nil { + t.Errorf("resolveHeaderValue(%q) = %q, want an error — an unknown token must never be served literally", v, got) + continue + } + if !strings.Contains(err.Error(), v) { + t.Errorf("error for %q does not name the token: %v", v, err) + } + } +} diff --git a/conformance/runner/go/main.go b/conformance/runner/go/main.go index 737f0124ad..06c7eb31a8 100644 --- a/conformance/runner/go/main.go +++ b/conformance/runner/go/main.go @@ -405,9 +405,16 @@ func runTest(tc TestCase) TestResult { // WithResponse parsing requires it for JSON body detection). w.Header().Set("Content-Type", "application/json") - // Set response headers (may override Content-Type) + // Set response headers (may override Content-Type). Resolved at serve + // time: a `{{httpdate+Ns}}` value is relative to NOW, not to when the + // fixture was loaded. for k, v := range resp.Headers { - w.Header().Set(k, v) + resolved, err := resolveHeaderValue(v, time.Now()) + if err != nil { + fmt.Fprintf(os.Stderr, "fixture error: %v\n", err) + os.Exit(1) + } + w.Header().Set(k, resolved) } w.WriteHeader(resp.Status) diff --git a/conformance/runner/python/runner.py b/conformance/runner/python/runner.py index 6824dc7036..40ad9ebc64 100644 --- a/conformance/runner/python/runner.py +++ b/conformance/runner/python/runner.py @@ -7,10 +7,12 @@ from __future__ import annotations import json +import math import os import re import sys import time +from email.utils import formatdate from dataclasses import dataclass, field from pathlib import Path from typing import Any @@ -461,6 +463,39 @@ def _summarize_upcoming(envelope: dict) -> dict: return summary +_HEADER_TOKEN = re.compile(r"^\{\{(.*)\}\}$") +_HTTPDATE_TOKEN = re.compile(r"^httpdate\+([0-9]{1,9})s$") + + +def resolve_header_value(value: str, now: float) -> str: + """Substitute the one token a fixture header value may carry, `{{httpdate+Ns}}`. + + Resolved at the moment the response is served (SPEC section 19, + conformance/schema.json) to the IMF-fixdate of floor(now) + N + 1 seconds: + the first whole second strictly more than N seconds after the second the + response is served in. A compliant SPEC section 6 parser sees a remainder in + (N - latency, N + 1] and, rounding up, computes at least N whole seconds, so + the fixture pairs it with a `delayBetweenRequests` floor of N * 1000 ms. It + exists because a static fixture has no clock: a literal past date pins only + the fall-through, and a far-future one is differently behaved per host. + + N is one to nine digits, so the arithmetic is exact everywhere and every + runner's date formatter stays in range; a longer N is an unrecognised token. + + An unrecognised `{{...}}` is an error rather than a literal: a typo'd token + served verbatim would be an unparseable header, which the SDK answers with + its ordinary backoff -- the exact outcome the case exists to distinguish from. + Every other value passes through untouched. + """ + token = _HEADER_TOKEN.match(value) + if token is None: + return value + inner = _HTTPDATE_TOKEN.match(token.group(1)) + if inner is None: + raise ValueError(f"unrecognised header token {value!r}: only {{{{httpdate+Ns}}}} is defined (conformance/schema.json)") + return formatdate(math.floor(now) + int(inner.group(1)) + 1, usegmt=True) + + def _normalize_body(body: Any, status: int | None) -> Any: """Normalize a mock response body for SDK compatibility. @@ -1170,7 +1205,9 @@ def side_effect(request: httpx.Request) -> httpx.Response: raise httpx.ConnectError("simulated network error") body = json.dumps(_normalize_body(r["body"], r.get("status"))).encode() if r.get("body") is not None else b"" headers = {"Content-Type": "application/json"} - headers.update(r.get("headers", {})) + # Resolved at serve time: a `{{httpdate+Ns}}` value is + # relative to NOW, not to when the fixture was loaded. + headers.update({k: resolve_header_value(v, time.time()) for k, v in r.get("headers", {}).items()}) return httpx.Response(r["status"], content=body, headers=headers) elif paginates: return httpx.Response(200, content=b"[]", headers={"Content-Type": "application/json"}) diff --git a/conformance/runner/python/test_header_tokens.py b/conformance/runner/python/test_header_tokens.py new file mode 100644 index 0000000000..09eb572ae6 --- /dev/null +++ b/conformance/runner/python/test_header_tokens.py @@ -0,0 +1,40 @@ +"""The `{{httpdate+Ns}}` header token (SPEC section 19, conformance/schema.json). + +Run: `uv run pytest test_header_tokens.py` + +A static fixture has no clock, so the positive half of SPEC section 6's +HTTP-date branch was unpinnable until this token (#780). These cases pin the +resolver's arithmetic against a frozen instant so the fixture's one-sided +timing floor rests on a deterministic contract. +""" +from __future__ import annotations + +import pytest + +from runner import resolve_header_value + +# A quarter-second into 10:18:14 UTC, so floor and round-up differ. +NOW = 1623233894.25 + + +@pytest.mark.parametrize("value", ["", "2", "Wed, 09 Jun 2021 10:18:14 GMT", "application/json", "{not a token}"]) +def test_plain_values_pass_through(value: str) -> None: + assert resolve_header_value(value, NOW) == value + + +@pytest.mark.parametrize( + "token,expected", + [ + ("{{httpdate+2s}}", "Wed, 09 Jun 2021 10:18:17 GMT"), + ("{{httpdate+0s}}", "Wed, 09 Jun 2021 10:18:15 GMT"), + ("{{httpdate+10s}}", "Wed, 09 Jun 2021 10:18:25 GMT"), + ], +) +def test_httpdate_resolves_to_the_whole_second_past_n(token: str, expected: str) -> None: + assert resolve_header_value(token, NOW) == expected + + +@pytest.mark.parametrize("value", ["{{httpdate}}", "{{httpdate+2}}", "{{httpdate-2s}}", "{{now}}", "{{}}", "{{httpdate+1000000000s}}"]) +def test_unknown_tokens_are_errors_not_literals(value: str) -> None: + with pytest.raises(ValueError, match="unrecognised header token"): + resolve_header_value(value, NOW) diff --git a/conformance/runner/ruby/header_tokens_test.rb b/conformance/runner/ruby/header_tokens_test.rb new file mode 100644 index 0000000000..69af354a19 --- /dev/null +++ b/conformance/runner/ruby/header_tokens_test.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +# The `{{httpdate+Ns}}` header token (SPEC §19, conformance/schema.json). +# +# A static fixture has no clock, so the positive half of SPEC §6's HTTP-date +# branch was unpinnable until this token (#780). These cases pin the resolver's +# arithmetic against a frozen instant so the fixture's one-sided timing floor +# rests on a deterministic contract. Ruby is the runner that had to move its +# header merge into the serve block for the token to see the right `now`. +# +# Run: `bundle exec ruby header_tokens_test.rb` + +require "minitest/autorun" +require_relative "runner" + +class HeaderTokensTest < Minitest::Test + # A quarter-second into 10:18:14 UTC, so floor and round-up differ. + NOW = Time.at(1_623_233_894.25).utc + + def test_plain_values_pass_through + [ "", "2", "Wed, 09 Jun 2021 10:18:14 GMT", "application/json", "{not a token}" ].each do |value| + assert_equal value, HeaderTokens.resolve(value, NOW) + end + end + + def test_httpdate_resolves_to_the_whole_second_past_n + assert_equal "Wed, 09 Jun 2021 10:18:17 GMT", HeaderTokens.resolve("{{httpdate+2s}}", NOW) + assert_equal "Wed, 09 Jun 2021 10:18:15 GMT", HeaderTokens.resolve("{{httpdate+0s}}", NOW) + assert_equal "Wed, 09 Jun 2021 10:18:25 GMT", HeaderTokens.resolve("{{httpdate+10s}}", NOW) + end + + def test_unknown_tokens_are_errors_not_literals + [ "{{httpdate}}", "{{httpdate+2}}", "{{httpdate-2s}}", "{{now}}", "{{}}", "{{httpdate+1000000000s}}" ].each do |value| + error = assert_raises(ArgumentError) { HeaderTokens.resolve(value, NOW) } + assert_includes error.message, value + end + end +end diff --git a/conformance/runner/ruby/runner.rb b/conformance/runner/ruby/runner.rb index d19671b00d..8222c0b18a 100644 --- a/conformance/runner/ruby/runner.rb +++ b/conformance/runner/ruby/runner.rb @@ -12,6 +12,7 @@ require "json" require "set" require "fileutils" +require "time" WebMock.enable! WebMock.disable_net_connect! @@ -235,6 +236,41 @@ def self.check(dispatch_failed) # The delayBetweenRequests assertion contract, kept apart from the runner so # its bounds branches are unit-testable (delay_gaps_test.rb). +# The one token a fixture header value may carry, `{{httpdate+Ns}}` (SPEC §19, +# conformance/schema.json), resolved at the moment the response is served to +# the IMF-fixdate of floor(now) + N + 1 seconds: the first whole second strictly +# more than N seconds after the second the response is served in. A compliant +# SPEC §6 parser sees a remainder in (N - latency, N + 1] and, rounding up, +# computes at least N whole seconds, so the fixture pairs it with a +# `delayBetweenRequests` floor of N × 1000 ms. It exists because a static +# fixture has no clock: a literal past date pins only the fall-through, and a +# far-future one is differently behaved per host. +# +# N is one to nine digits, so the arithmetic is exact everywhere and every +# runner's date formatter stays in range; a longer N is an unrecognised token. +# +# An unrecognised `{{…}}` is an error rather than a literal: a typo'd token +# served verbatim would be an unparseable header, which the SDK answers with its +# ordinary backoff — the exact outcome the case exists to distinguish from. +# Every other value passes through untouched. +module HeaderTokens + TOKEN = /\A\{\{(.*)\}\}\z/ + HTTPDATE = /\Ahttpdate\+(\d{1,9})s\z/ + + def self.resolve(value, now) + token = TOKEN.match(value) + return value unless token + + inner = HTTPDATE.match(token[1]) + unless inner + raise ArgumentError, + "unrecognised header token #{value.inspect}: only {{httpdate+Ns}} is defined (conformance/schema.json)" + end + + Time.at(now.to_i + inner[1].to_i + 1).utc.httpdate + end +end + module DelayGaps # Validates one assertion against the recorded inter-request gaps, returning # nil when it holds and a failure message otherwise. @@ -1259,7 +1295,11 @@ def setup_mock_responses # unlike a blanket stub .to_raise. raise Faraday::ConnectionFailed, "simulated network error" if resp[:network_error] - resp + # Header values are resolved HERE, inside the to_return block, and not + # when the queue was built above: a `{{httpdate+Ns}}` token is relative + # to the moment the response is served, and the queue is built eagerly + # before any request arrives. + resp.merge(headers: resp[:headers].transform_values { |v| HeaderTokens.resolve(v, Time.now) }) elsif paginates # Beyond defined responses for paginated ops: empty 200 terminates pagination call_count += 1 diff --git a/conformance/runner/swift/Sources/ConformanceRunner/ScriptedTransport.swift b/conformance/runner/swift/Sources/ConformanceRunner/ScriptedTransport.swift index ca691cc126..834010a252 100644 --- a/conformance/runner/swift/Sources/ConformanceRunner/ScriptedTransport.swift +++ b/conformance/runner/swift/Sources/ConformanceRunner/ScriptedTransport.swift @@ -1,4 +1,5 @@ import Basecamp +import ConformanceSupport import Foundation /// One outbound request captured by the scripted transport. @@ -120,8 +121,10 @@ final class ScriptedTransport: Transport, @unchecked Sendable { } var headerFields = ["Content-Type": "application/json"] + // Resolved at serve time: a `{{httpdate+Ns}}` value is relative to + // NOW, not to when the fixture was loaded. for (key, value) in mock.allHeaders { - headerFields[key] = value + headerFields[key] = try resolveHeaderValue(value, now: Date()) } let body: Data diff --git a/conformance/runner/swift/Sources/ConformanceSupport/HeaderTokens.swift b/conformance/runner/swift/Sources/ConformanceSupport/HeaderTokens.swift new file mode 100644 index 0000000000..f1363a89e1 --- /dev/null +++ b/conformance/runner/swift/Sources/ConformanceSupport/HeaderTokens.swift @@ -0,0 +1,43 @@ +import Foundation + +/// A `{{…}}` header value the runner does not define. Surfaced as an error +/// rather than served literally: a typo'd token on the wire would be an +/// unparseable header, which the SDK answers with its ordinary backoff — the +/// exact outcome the case exists to distinguish from. +public struct UnrecognisedHeaderToken: Error, CustomStringConvertible, Sendable { + public let value: String + public var description: String { + "unrecognised header token \"\(value)\": only {{httpdate+Ns}} is defined (conformance/schema.json)" + } +} + +/// Substitutes the one token a fixture header value may carry, +/// `{{httpdate+Ns}}` (SPEC §19, conformance/schema.json), at the moment the +/// response is served. Every other value passes through untouched. +/// +/// The token resolves to the IMF-fixdate of floor(now) + N + 1 seconds: the +/// first whole second strictly more than N seconds after the second the +/// response is served in. A compliant SPEC §6 parser sees a remainder in +/// (N − latency, N + 1] and, rounding up, computes at least N whole seconds, so +/// the fixture pairs it with a `delayBetweenRequests` floor of N × 1000 ms. It +/// exists because a static fixture has no clock: a literal past date pins only +/// the fall-through, and a far-future one is differently behaved per host. +/// +/// N is one to nine digits, so the arithmetic is exact everywhere and every +/// runner's date formatter stays in range; a longer N is an unrecognised token. +public func resolveHeaderValue(_ value: String, now: Date) throws -> String { + guard value.hasPrefix("{{"), value.hasSuffix("}}"), value.count >= 4 else { return value } + let inner = value.dropFirst(2).dropLast(2) + let prefix = "httpdate+" + guard inner.hasPrefix(prefix), inner.hasSuffix("s") else { throw UnrecognisedHeaderToken(value: value) } + let digits = inner.dropFirst(prefix.count).dropLast() + guard !digits.isEmpty, digits.count <= 9, digits.allSatisfy({ $0.isASCII && $0.isNumber }), let n = Int(digits) else { + throw UnrecognisedHeaderToken(value: value) + } + let seconds = floor(now.timeIntervalSince1970) + Double(n) + 1 + let formatter = DateFormatter() + formatter.locale = Locale(identifier: "en_US_POSIX") + formatter.timeZone = TimeZone(identifier: "GMT") + formatter.dateFormat = "EEE, dd MMM yyyy HH:mm:ss 'GMT'" + return formatter.string(from: Date(timeIntervalSince1970: seconds)) +} diff --git a/conformance/runner/swift/Tests/ConformanceSupportTests/HeaderTokensTests.swift b/conformance/runner/swift/Tests/ConformanceSupportTests/HeaderTokensTests.swift new file mode 100644 index 0000000000..d559308a43 --- /dev/null +++ b/conformance/runner/swift/Tests/ConformanceSupportTests/HeaderTokensTests.swift @@ -0,0 +1,34 @@ +import XCTest + +@testable import ConformanceSupport + +/// The `{{httpdate+Ns}}` header token (SPEC §19, conformance/schema.json). +/// +/// A static fixture has no clock, so the positive half of SPEC §6's HTTP-date +/// branch was unpinnable until this token (#780). These cases pin the +/// resolver's arithmetic against a frozen instant so the fixture's one-sided +/// timing floor rests on a deterministic contract. +final class HeaderTokensTests: XCTestCase { + /// A quarter-second into 10:18:14 UTC, so floor and round-up differ. + private let now = Date(timeIntervalSince1970: 1_623_233_894.25) + + func testPlainValuesPassThrough() throws { + for value in ["", "2", "Wed, 09 Jun 2021 10:18:14 GMT", "application/json", "{not a token}"] { + XCTAssertEqual(try resolveHeaderValue(value, now: now), value) + } + } + + func testHttpdateResolvesToTheWholeSecondPastN() throws { + XCTAssertEqual(try resolveHeaderValue("{{httpdate+2s}}", now: now), "Wed, 09 Jun 2021 10:18:17 GMT") + XCTAssertEqual(try resolveHeaderValue("{{httpdate+0s}}", now: now), "Wed, 09 Jun 2021 10:18:15 GMT") + XCTAssertEqual(try resolveHeaderValue("{{httpdate+10s}}", now: now), "Wed, 09 Jun 2021 10:18:25 GMT") + } + + func testUnknownTokensAreErrorsNotLiterals() { + for value in ["{{httpdate}}", "{{httpdate+2}}", "{{httpdate-2s}}", "{{now}}", "{{}}", "{{httpdate+1000000000s}}"] { + XCTAssertThrowsError(try resolveHeaderValue(value, now: now), value) { error in + XCTAssertTrue("\(error)".contains(value), "error for \(value) does not name the token: \(error)") + } + } + } +} diff --git a/conformance/runner/typescript/header-tokens.test.ts b/conformance/runner/typescript/header-tokens.test.ts new file mode 100644 index 0000000000..5437449ca7 --- /dev/null +++ b/conformance/runner/typescript/header-tokens.test.ts @@ -0,0 +1,33 @@ +/** + * The `{{httpdate+Ns}}` header token (SPEC §19, conformance/schema.json). + * + * A static fixture has no clock, so the positive half of SPEC §6's HTTP-date + * branch was unpinnable until this token (#780). These cases pin the resolver's + * arithmetic against a frozen instant so the fixture's one-sided timing floor + * rests on a deterministic contract. + */ +import { describe, it, expect } from "vitest"; +import { resolveHeaderValue } from "./header-tokens.js"; + +// A quarter-second into 10:18:14 UTC, so floor and round-up differ. +const NOW_MS = 1_623_233_894_250; + +describe("resolveHeaderValue", () => { + it("passes plain values through", () => { + for (const value of ["", "2", "Wed, 09 Jun 2021 10:18:14 GMT", "application/json", "{not a token}"]) { + expect(resolveHeaderValue(value, NOW_MS)).toBe(value); + } + }); + + it("resolves httpdate to the whole second past N", () => { + expect(resolveHeaderValue("{{httpdate+2s}}", NOW_MS)).toBe("Wed, 09 Jun 2021 10:18:17 GMT"); + expect(resolveHeaderValue("{{httpdate+0s}}", NOW_MS)).toBe("Wed, 09 Jun 2021 10:18:15 GMT"); + expect(resolveHeaderValue("{{httpdate+10s}}", NOW_MS)).toBe("Wed, 09 Jun 2021 10:18:25 GMT"); + }); + + it("throws on an unknown token rather than serving it literally", () => { + for (const value of ["{{httpdate}}", "{{httpdate+2}}", "{{httpdate-2s}}", "{{now}}", "{{}}", "{{httpdate+1000000000s}}"]) { + expect(() => resolveHeaderValue(value, NOW_MS)).toThrow(value); + } + }); +}); diff --git a/conformance/runner/typescript/header-tokens.ts b/conformance/runner/typescript/header-tokens.ts new file mode 100644 index 0000000000..739d5063a7 --- /dev/null +++ b/conformance/runner/typescript/header-tokens.ts @@ -0,0 +1,40 @@ +/** + * The one token a fixture header value may carry, `{{httpdate+Ns}}` (SPEC §19, + * conformance/schema.json), kept apart from the runner so its arithmetic is + * unit-testable against a frozen instant (header-tokens.test.ts). + */ + +const HEADER_TOKEN = /^\{\{(.*)\}\}$/; +const HTTPDATE_TOKEN = /^httpdate\+(\d{1,9})s$/; + +/** + * Resolves `{{httpdate+Ns}}` at the moment the response is served to the + * IMF-fixdate of floor(now) + N + 1 seconds: the first whole second strictly + * more than N seconds after the second the response is served in. A compliant + * SPEC §6 parser sees a remainder in (N − latency, N + 1] and, rounding up, + * computes at least N whole seconds, so the fixture pairs it with a + * `delayBetweenRequests` floor of N × 1000 ms. It exists because a static + * fixture has no clock: a literal past date pins only the fall-through, and a + * far-future one is differently behaved per host. + * + * N is one to nine digits, so the arithmetic is exact everywhere and every + * runner's date formatter stays in range; a longer N is an unrecognised token. + * + * An unrecognised `{{…}}` throws rather than passing through: a typo'd token + * served verbatim would be an unparseable header, which the SDK answers with + * its ordinary backoff — the exact outcome the case exists to distinguish + * from. Every other value passes through untouched. + */ +export function resolveHeaderValue(value: string, nowMs: number): string { + const token = HEADER_TOKEN.exec(value); + if (token === null) return value; + const inner = HTTPDATE_TOKEN.exec(token[1]!); + if (inner === null) { + throw new Error( + `unrecognised header token ${JSON.stringify(value)}: only {{httpdate+Ns}} is defined (conformance/schema.json)`, + ); + } + const seconds = Math.floor(nowMs / 1000) + Number(inner[1]) + 1; + // Date#toUTCString is specified as the IMF-fixdate shape (ECMA-262 §21.4.4.43). + return new Date(seconds * 1000).toUTCString(); +} diff --git a/conformance/runner/typescript/runner.test.ts b/conformance/runner/typescript/runner.test.ts index 092680d459..e2419f1eff 100644 --- a/conformance/runner/typescript/runner.test.ts +++ b/conformance/runner/typescript/runner.test.ts @@ -30,6 +30,7 @@ import { writeExecutionManifest, } from "./case-census.js"; import { checkDelayGaps } from "./delay-gaps.js"; +import { resolveHeaderValue } from "./header-tokens.js"; import { errorRaisedFailure } from "./error-raised.js"; import { checkRequestCount, requestCountApplies } from "./request-count.js"; @@ -1272,8 +1273,10 @@ function installMockHandlers(tc: TestCase): { "Content-Type": "application/json", }; if (mock.headers) { + // Resolved at serve time: a `{{httpdate+Ns}}` value is relative to NOW, + // not to when the fixture was loaded. for (const [k, v] of Object.entries(mock.headers)) { - headers[k] = v; + headers[k] = resolveHeaderValue(v, Date.now()); } } diff --git a/conformance/schema.json b/conformance/schema.json index 3881ce275d..deaca41ccf 100644 --- a/conformance/schema.json +++ b/conformance/schema.json @@ -94,7 +94,7 @@ }, "headers": { "type": "object", - "description": "Response headers", + "description": "Response headers. A value may be the token `{{httpdate+Ns}}` (N = one to nine digits, so every runner resolves it in exact integer arithmetic and every date formatter is inside its range), which the runner resolves AT THE MOMENT IT SERVES THE RESPONSE to the IMF-fixdate of floor(now) + N + 1 seconds — the first whole second strictly more than N seconds after the second the response is served in. It exists for SPEC §6's positive HTTP-date branch, which a literal date cannot pin: a past date only exercises the fall-through, and a far-future one makes a compliant SDK sleep for years. A compliant parser (rounding up) computes at least N whole seconds for any serve-to-parse latency under a second, so pair it with `delayBetweenRequests` `min: N*1000`. Any other `{{...}}` token is a runner error, never served literally.", "additionalProperties": { "type": "string" } diff --git a/conformance/tests/downloads.json b/conformance/tests/downloads.json index e33707b1bd..bf47c38a58 100644 --- a/conformance/tests/downloads.json +++ b/conformance/tests/downloads.json @@ -99,18 +99,18 @@ }, { "name": "DownloadURL honors Retry-After on 429 at the auth'd first hop", - "description": "429 Too Many Requests with Retry-After: 1 pauses for at least one second before retry. Retry succeeds with 302 → 200 body.", + "description": "429 Too Many Requests with Retry-After: 2 waits what the origin named before retrying. The 2000ms floor sits above the longest first backoff (1000ms base + 100ms jitter), so a loop that ignored the header and slept its own curve fails here; the earlier `Retry-After: 1` with a 1000ms floor could not tell the two apart. Retry succeeds with 302 → 200 body.", "operation": "DownloadURL", "method": "GET", "path": "/999999999/blobs/abcd1234/download/logo.png", "mockResponses": [ - {"status": 429, "headers": {"Retry-After": "1"}}, + {"status": 429, "headers": {"Retry-After": "2"}}, {"status": 302, "headers": {"Location": "/signed/logo.png"}}, {"status": 200, "headers": {"Content-Type": "image/png"}, "body": "pixels"} ], "assertions": [ {"type": "requestCount", "expected": 3}, - {"type": "delayBetweenRequests", "min": 1000, "index": 0}, + {"type": "delayBetweenRequests", "min": 2000, "index": 0}, {"type": "noError"}, {"type": "headerPresent", "path": "Authorization", "index": 0}, {"type": "headerAbsent", "path": "Authorization", "index": -1}, @@ -118,6 +118,69 @@ ], "tags": ["download", "retry", "429", "retry-after"] }, + { + "name": "DownloadURL honors Retry-After on 502 at the auth'd first hop", + "description": "SPEC §14 \"Hop-1 Retry\" declares {429, 502, 503, 504} and honours Retry-After at every status in that set, not at 429 alone (SPEC §6 \"Retry-After Honouring\": honouring is derived from retry eligibility, so a loop that declares its own set inherits the rule over that set). Until this case existed five of six download loops honoured the header on 429 only, so a 502 carrying `Retry-After: 2` backed off ~1s instead. The 2000ms floor sits above the longest first backoff (1000ms base + 100ms jitter), as the 429 sibling's does. Retry succeeds with 302 → 200 body.", + "operation": "DownloadURL", + "method": "GET", + "path": "/999999999/blobs/abcd1234/download/logo.png", + "mockResponses": [ + {"status": 502, "headers": {"Retry-After": "2"}}, + {"status": 302, "headers": {"Location": "/signed/logo.png"}}, + {"status": 200, "headers": {"Content-Type": "image/png"}, "body": "pixels"} + ], + "assertions": [ + {"type": "requestCount", "expected": 3}, + {"type": "delayBetweenRequests", "min": 2000, "index": 0}, + {"type": "noError"}, + {"type": "headerPresent", "path": "Authorization", "index": 0}, + {"type": "headerAbsent", "path": "Authorization", "index": -1}, + {"type": "requestPath", "expected": "/signed/logo.png", "index": -1} + ], + "tags": ["download", "retry", "502", "retry-after", "retry-after-honoured"] + }, + { + "name": "DownloadURL honors Retry-After on 503 at the auth'd first hop", + "description": "SPEC §14 \"Hop-1 Retry\" declares {429, 502, 503, 504} and honours Retry-After at every status in that set, not at 429 alone (SPEC §6 \"Retry-After Honouring\": honouring is derived from retry eligibility, so a loop that declares its own set inherits the rule over that set). Until this case existed five of six download loops honoured the header on 429 only, so a 503 carrying `Retry-After: 2` backed off ~1s instead. The 2000ms floor sits above the longest first backoff (1000ms base + 100ms jitter), as the 429 sibling's does. Retry succeeds with 302 → 200 body.", + "operation": "DownloadURL", + "method": "GET", + "path": "/999999999/blobs/abcd1234/download/logo.png", + "mockResponses": [ + {"status": 503, "headers": {"Retry-After": "2"}}, + {"status": 302, "headers": {"Location": "/signed/logo.png"}}, + {"status": 200, "headers": {"Content-Type": "image/png"}, "body": "pixels"} + ], + "assertions": [ + {"type": "requestCount", "expected": 3}, + {"type": "delayBetweenRequests", "min": 2000, "index": 0}, + {"type": "noError"}, + {"type": "headerPresent", "path": "Authorization", "index": 0}, + {"type": "headerAbsent", "path": "Authorization", "index": -1}, + {"type": "requestPath", "expected": "/signed/logo.png", "index": -1} + ], + "tags": ["download", "retry", "503", "retry-after", "retry-after-honoured"] + }, + { + "name": "DownloadURL honors Retry-After on 504 at the auth'd first hop", + "description": "SPEC §14 \"Hop-1 Retry\" declares {429, 502, 503, 504} and honours Retry-After at every status in that set, not at 429 alone (SPEC §6 \"Retry-After Honouring\": honouring is derived from retry eligibility, so a loop that declares its own set inherits the rule over that set). Until this case existed five of six download loops honoured the header on 429 only, so a 504 carrying `Retry-After: 2` backed off ~1s instead. The 2000ms floor sits above the longest first backoff (1000ms base + 100ms jitter), as the 429 sibling's does. Retry succeeds with 302 → 200 body.", + "operation": "DownloadURL", + "method": "GET", + "path": "/999999999/blobs/abcd1234/download/logo.png", + "mockResponses": [ + {"status": 504, "headers": {"Retry-After": "2"}}, + {"status": 302, "headers": {"Location": "/signed/logo.png"}}, + {"status": 200, "headers": {"Content-Type": "image/png"}, "body": "pixels"} + ], + "assertions": [ + {"type": "requestCount", "expected": 3}, + {"type": "delayBetweenRequests", "min": 2000, "index": 0}, + {"type": "noError"}, + {"type": "headerPresent", "path": "Authorization", "index": 0}, + {"type": "headerAbsent", "path": "Authorization", "index": -1}, + {"type": "requestPath", "expected": "/signed/logo.png", "index": -1} + ], + "tags": ["download", "retry", "504", "retry-after", "retry-after-honoured"] + }, { "name": "DownloadURL surfaces redirect with no Location", "description": "A 3xx response without a Location header is a protocol error, not a retryable failure. SDK surfaces an error after one request.", diff --git a/conformance/tests/retry.json b/conformance/tests/retry.json index fcf9284999..ae8b57d1a5 100644 --- a/conformance/tests/retry.json +++ b/conformance/tests/retry.json @@ -36,6 +36,24 @@ ], "tags": ["retry", "429", "rate-limit"] }, + { + "name": "GET operation retries on 503 with Retry-After", + "description": "SPEC §6 \"Retry-After Honouring\": a parsed Retry-After is honoured at every status a retry is already going to happen at, and 503 is in every operation's declared retryOn. RFC 9110 §10.2.3 gives 503 the one case with explicit Retry-After semantics (how long the service expects to be unavailable), so this is the canonical use of the header — and until this case existed it was the one five of six SDKs ignored: Kotlin, Swift, TypeScript and Ruby gated the sleep on `status == 429`, and the generated Go client did the same, so a 503 carrying `Retry-After: 120` backed off ~1s instead. The 429 sibling above could not see that, because 429 was the one status every loop already honoured. `requestCount` cannot see it either — a loop that ignores the header still retries and still succeeds — so `delayBetweenRequests` is the whole assertion, and its floor (2000ms) sits above the longest first backoff (1000ms base + 100ms jitter).", + "operation": "GetProject", + "method": "GET", + "path": "/projects/{projectId}", + "pathParams": {"projectId": 12345}, + "mockResponses": [ + {"status": 503, "headers": {"Retry-After": "2"}}, + {"status": 200, "body": {"id": 12345, "name": "Test Project", "status": "active", "created_at": "2025-01-01T00:00:00Z", "updated_at": "2025-01-01T00:00:00Z", "url": "https://3.basecampapi.com/999/projects/12345.json", "app_url": "https://3.basecamp.com/999/projects/12345"}} + ], + "assertions": [ + {"type": "requestCount", "expected": 2}, + {"type": "delayBetweenRequests", "min": 2000}, + {"type": "noError"} + ], + "tags": ["retry", "503", "retry-after-honoured"] + }, { "name": "POST operation does NOT retry (not idempotent)", "description": "Verifies that POST operations do NOT retry since they are not idempotent", @@ -70,7 +88,7 @@ }, { "name": "Retry-After HTTP-date in the past falls through to backoff", - "description": "SPEC §6 step 2 honours an HTTP-date only when `date - now()` is positive. A past date yields a non-positive value, so the algorithm must fall through to step 3 — the backoff formula — and the pin is `delayBetweenRequests`, not the retry itself.\n\nThis replaces a case that asserted only `requestCount: 2` and `noError` against the same past date. Those two assertions are satisfied identically by a parser that never looks at dates at all: it fails the integer parse, backs off, and retries. That is why Kotlin's missing HTTP-date branch (#564) survived a green conformance suite for the SDK's entire life — measured against un-fixed Kotlin and un-fixed TypeScript, the old case passed in ~1.0s, which is precisely the backoff it was meant to be distinguishing the date from. The delay pin gives the case something to fail on: a parser that returns the negative difference, or clamps it to 0 and honours that, retries with no wait — the shape TypeScript's `retry.ts` had before #564.\n\nWhat this case still cannot see is the POSITIVE half of step 2, which needs a date a few seconds ahead of the run's own clock. A fixture is a static JSON literal and the harness has no clock: a date far enough ahead to stay future-dated would make a compliant SDK sleep for the rest of the decade — Retry-After is exempt from SPEC §7's backoff ceiling, so the only bound left is each platform's own timer, and those bounds are absurd rather than helpful here (TypeScript's is ~24.85 days, the largest delay a 32-bit millisecond timer can serve) — and a date near enough to assert a small delay against is past-dated within seconds of being written. The positive branch is therefore pinned by per-SDK unit tests — Kotlin's `parseRetryAfterParsesFutureHttpDate`, TypeScript's `errors.test.ts` — and #780 tracks giving the harness a clock so this fixture can cover it.", + "description": "SPEC §6 step 2 honours an HTTP-date only when `date - now()` is positive. A past date yields a non-positive value, so the algorithm must fall through to step 3 — the backoff formula — and the pin is `delayBetweenRequests`, not the retry itself.\n\nThis replaces a case that asserted only `requestCount: 2` and `noError` against the same past date. Those two assertions are satisfied identically by a parser that never looks at dates at all: it fails the integer parse, backs off, and retries. That is why Kotlin's missing HTTP-date branch (#564) survived a green conformance suite for the SDK's entire life — measured against un-fixed Kotlin and un-fixed TypeScript, the old case passed in ~1.0s, which is precisely the backoff it was meant to be distinguishing the date from. The delay pin gives the case something to fail on: a parser that returns the negative difference, or clamps it to 0 and honours that, retries with no wait — the shape TypeScript's `retry.ts` had before #564.\n\nWhat this case still cannot see is the POSITIVE half of step 2, which needs a date a few seconds ahead of the run's own clock. A fixture is a static JSON literal and the harness has no clock: a date far enough ahead to stay future-dated would make a compliant SDK sleep for the rest of the decade — Retry-After is exempt from SPEC §7's backoff ceiling, so the only bound left is each platform's own timer, and those bounds are absurd rather than helpful here (TypeScript's is ~24.85 days, the largest delay a 32-bit millisecond timer can serve) — and a date near enough to assert a small delay against is past-dated within seconds of being written. The positive branch is pinned by the sibling case below, which serves a near-future date through the `{{httpdate+Ns}}` token (#780) — and by per-SDK unit tests against a frozen clock, where the rounding rule is pinned deterministically.", "operation": "GetProject", "method": "GET", "path": "/projects/{projectId}", @@ -86,6 +104,24 @@ ], "tags": ["retry", "429", "retry-after-date"] }, + { + "name": "Retry-After HTTP-date in the future is honoured", + "description": "The positive half of SPEC §6 step 2, which the past-date case above says it cannot see: a fixture is a static literal and the harness had no clock, so until #780 the only date this file could serve was one already past, and that half of the branch was pinned by per-SDK unit tests alone — which is how Kotlin's missing date branch (#564) survived a green suite for the SDK's whole life. The header value is the `{{httpdate+2s}}` token (conformance/schema.json, SPEC §19), which every runner resolves as it serves the response to the IMF-fixdate of floor(now) + 3 seconds. A compliant parser sees a remainder in (2 − latency, 3] and, rounding up per step 2, computes at least 2 whole seconds — so the 2000ms floor holds for any serve-to-parse latency under a second — while a parser that drops the date form falls onto the ~1s backoff curve and fails. The token is deliberately near rather than an absolute far-future date: past the ceiling one host saturates, one clamps and four run long, so a far date would assert six different things.\n\nThe floor is one-sided by design (SPEC §6 \"Rounding\"): the true remainder is a fraction of a second under 3, so an equality assertion would be flaky by exactly the width of the rounding rule. A truncating parser computes 2 here on most runs and 1 only when the serve and the parse straddle a second boundary — that regression is pinned deterministically by the per-SDK unit tests against a frozen clock, not by this case.", + "operation": "GetProject", + "method": "GET", + "path": "/projects/{projectId}", + "pathParams": {"projectId": 12345}, + "mockResponses": [ + {"status": 429, "headers": {"Retry-After": "{{httpdate+2s}}"}}, + {"status": 200, "body": {"id": 12345, "name": "Test Project", "status": "active", "created_at": "2025-01-01T00:00:00Z", "updated_at": "2025-01-01T00:00:00Z", "url": "https://3.basecampapi.com/999/projects/12345.json", "app_url": "https://3.basecamp.com/999/projects/12345"}} + ], + "assertions": [ + {"type": "requestCount", "expected": 2}, + {"type": "delayBetweenRequests", "min": 2000}, + {"type": "noError"} + ], + "tags": ["retry", "429", "retry-after-date", "retry-after-honoured"] + }, { "name": "Retry-After of 0 is rejected and falls through to backoff", "description": "`0` is a well-formed `delay-seconds`, and SPEC §6 step 1 deliberately declines to honour it: the integer branch returns a value only when it is > 0. `0` is not an HTTP-date either, so step 3 applies and the retry waits the ordinary backoff rather than firing immediately.\n\nRed against TypeScript before #564: `retry.ts` read the header with `parseInt` and no `> 0` guard, so `Retry-After: 0` became a 0 ms sleep and the backoff collapsed entirely. `requestCount` cannot see that — a collapsed backoff still retries, and still succeeds — so `delayBetweenRequests` is the whole assertion here. The second TypeScript copy of the same algorithm, on the multipart upload path in `services/base.ts`, guarded with `>= 0` and so admitted zero explicitly; conformance reaches only the JSON client path, so that copy is pinned by a TypeScript unit test instead.", diff --git a/go/pkg/basecamp/client.go b/go/pkg/basecamp/client.go index 6e203614b4..6ea4167bca 100644 --- a/go/pkg/basecamp/client.go +++ b/go/pkg/basecamp/client.go @@ -186,7 +186,7 @@ func WithAuthStrategy(strategy AuthStrategy) ClientOption { // - Retries failed GET requests with exponential backoff // - Does NOT retry POST/PUT/DELETE on 429/5xx (to avoid duplicating data) // - Retries mutations once after successful 401 token refresh -// - Respects Retry-After headers on 429 responses +// - Respects Retry-After headers at every retried status // - Follows pagination via Link headers // // Configuration options: @@ -701,9 +701,9 @@ func (c *Client) doRequestURL(ctx context.Context, method, url string, body any) // A server-specified Retry-After replaces the backoff curve // outright — no jitter, no policy ceiling (only the // representability clamp parseRetryAfter already applied), same - // idiom as downloadURL. Only the 429 arm of singleRequest sets it - // today; widening the set of statuses that carry one is #775's - // call, not this loop's. + // idiom as downloadURL. Every retryable arm of singleRequest sets + // it, so the header governs the wait at 503 as it does at 429 + // (SPEC §6 "Retry-After Honouring"). if apiErr.RetryAfter > 0 { delay = time.Duration(apiErr.RetryAfter) * time.Second } else { @@ -880,17 +880,17 @@ func (c *Client) singleRequest(ctx context.Context, method, url string, body any } } } - return nil, ErrAuth("Authentication failed").withRequestID(requestID) + return nil, ErrAuth("Authentication failed").withRequestID(requestID).withRetryAfter(resp.Header.Get("Retry-After")) case http.StatusForbidden: // 403 // Check if this might be a scope issue if method != "GET" { - return nil, ErrForbiddenScope().withRequestID(requestID) + return nil, ErrForbiddenScope().withRequestID(requestID).withRetryAfter(resp.Header.Get("Retry-After")) } - return nil, ErrForbidden("Access denied").withRequestID(requestID) + return nil, ErrForbidden("Access denied").withRequestID(requestID).withRetryAfter(resp.Header.Get("Retry-After")) case http.StatusNotFound: // 404 - return nil, ErrNotFound("Resource", url).withRequestID(requestID) + return nil, ErrNotFound("Resource", url).withRequestID(requestID).withRetryAfter(resp.Header.Get("Retry-After")) case http.StatusBadRequest, http.StatusUnprocessableEntity: // 400, 422 // The generated service layer maps these through checkResponse; the raw @@ -898,7 +898,7 @@ func (c *Client) singleRequest(ctx context.Context, method, url string, body any // api_error with the field-keyed detail dropped. respBody, _ := limitedReadAll(resp.Body, MaxErrorBodyBytes) serverMsg, serverHint, fieldErrors := parseErrorBody(respBody) - return nil, validationErrorFromBody(serverMsg, serverHint, fieldErrors, resp.StatusCode, requestID, respBody) + return nil, validationErrorFromBody(serverMsg, serverHint, fieldErrors, resp.StatusCode, requestID, parseRetryAfter(resp.Header.Get("Retry-After")), respBody) case http.StatusInsufficientStorage: // 507 // Same reason the 400/422 arm above exists: the generated service layer @@ -914,17 +914,21 @@ func (c *Client) singleRequest(ctx context.Context, method, url string, body any Hint: serverHint, HTTPStatus: 507, Retryable: false, - }).withRequestID(requestID) + }).withRequestID(requestID).withRetryAfter(resp.Header.Get("Retry-After")) case http.StatusInternalServerError: // 500 - return nil, ErrAPI(500, "Server error (500)").withRequestID(requestID) + return nil, ErrAPI(500, "Server error (500)").withRequestID(requestID).withRetryAfter(resp.Header.Get("Retry-After")) case http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout: // 502, 503, 504 + // RetryAfter is carried at every status (SPEC §6 "HTTP Status Mapping + // Algorithm"): the retry loop above reads it off this error, so this + // is also what makes a 503's Retry-After govern the sleep. return nil, (&Error{ Code: CodeAPI, Message: fmt.Sprintf("Gateway error (%d)", resp.StatusCode), HTTPStatus: resp.StatusCode, Retryable: true, + RetryAfter: parseRetryAfter(resp.Header.Get("Retry-After")), }).withRequestID(requestID) default: @@ -940,7 +944,7 @@ func (c *Client) singleRequest(ctx context.Context, method, url string, body any Message: msgOrDefault(serverMsg, fmt.Sprintf("Request failed (HTTP %d)", resp.StatusCode)), Hint: serverHint, HTTPStatus: resp.StatusCode, - }).withRequestID(requestID) + }).withRequestID(requestID).withRetryAfter(resp.Header.Get("Retry-After")) } } @@ -1153,6 +1157,23 @@ func isDelaySeconds(value string) bool { return true } +// maxRetryAfterDigits is the width of maxRetryAfterSeconds in decimal; a +// digit string longer than it (leading zeros aside) is over the ceiling +// without needing to be converted. +const maxRetryAfterDigits = len("2147483647") + +// exceedsRetryAfterCeiling reports whether a `1*DIGIT` value is above +// maxRetryAfterSeconds, by width and then by value, so the answer never +// depends on an integer conversion that could itself overflow. +func exceedsRetryAfterCeiling(digits string) bool { + digits = strings.TrimLeft(digits, "0") + if len(digits) > maxRetryAfterDigits { + return true + } + seconds, err := strconv.ParseInt(digits, 10, 64) + return err == nil && seconds > maxRetryAfterSeconds +} + // parseRetryAfter parses the Retry-After header value. // It handles both seconds (integer) and HTTP-date formats. // Returns 0 if the header is empty or cannot be parsed, and clamps a parsed @@ -1163,34 +1184,25 @@ func parseRetryAfter(header string) int { if header == "" { return 0 } - // Try parsing as seconds (integer). Parsed as an int64 rather than through - // Atoi, whose range is int's: `Retry-After: 2147483648` would otherwise be - // ErrRange, hence malformed, hence the millisecond backoff on a 32-bit - // build while the same header is honoured on a 64-bit one. Deciding the - // ceiling is the clamp's job, not the parse's. - // - // A value too large for that int64 is treated as MALFORMED and falls - // through to step 3's backoff rather than saturating. So is any other - // unparseable input: ParseInt returns 0 with ErrSyntax, and a negative - // range error clamps to math.MinInt64, both caught by the `> 0` guard - // alongside the err check. Saturation is reserved for a value the parser - // holds but the host cannot schedule, and that is clampRetryAfterSeconds' - // job below. + // Delta-seconds. The digits are checked rather than left to ParseInt, + // which accepts a leading `+` or `-`: RFC 9110 spells delay-seconds as + // `1*DIGIT` — no sign — so `+5` is not a delay at all, and ParseInt would + // otherwise honour it as 5. Same digits-only test SPEC §16's device parser + // makes, and the same reading conformance's "partly numeric rejected + // (`1*DIGIT`)" case asserts. // - // That split is Go's, not something §6's parsing algorithm mandates on - // its own — the algorithm says only "parse a positive integer". It is the - // two-tier rule #793 states in SPEC §6 "Retry-After Honouring" - // (unrepresentable in the parser's own type → malformed; representable but - // unschedulable → saturate); the cross-SDK convergence on over-range - // values, which the SDKs still answer differently, is #799's. The rule is - // deliberately not restated here; #793 is where it is argued. - // - // The digits are checked rather than left to ParseInt, which accepts a - // leading `+` or `-`. RFC 9110 spells delay-seconds as `1*DIGIT` — no sign - // — so `+5` is not a delay at all, and ParseInt would otherwise honour it - // as 5. Same digits-only test SPEC §16's device parser makes, and the same - // reading conformance's "partly numeric rejected (`1*DIGIT`)" case asserts. + // `1*DIGIT` has no upper bound, so no digit string is malformed for its + // length: a value above maxRetryAfterSeconds SATURATES there (SPEC §6 + // "Retry-After Parsing Algorithm"), whether or not it fits an int64. An + // earlier revision treated a string too wide for the parser's own int64 as + // malformed and fell through to the backoff, which made the honoured wait + // depend on the parser's word size — and reading "wait a very long time" + // as "no delay" hammers a peer that just asked to be left alone. The + // width test comes before ParseInt so no conversion can overflow. if isDelaySeconds(header) { + if exceedsRetryAfterCeiling(header) { + return maxRetryAfterSeconds + } if seconds, err := strconv.ParseInt(header, 10, 64); err == nil && seconds > 0 { return clampRetryAfterSeconds(seconds) } diff --git a/go/pkg/basecamp/client_retry_after_test.go b/go/pkg/basecamp/client_retry_after_test.go index 0032850c9a..5b2dbb19ab 100644 --- a/go/pkg/basecamp/client_retry_after_test.go +++ b/go/pkg/basecamp/client_retry_after_test.go @@ -196,15 +196,6 @@ func TestClient_RetryAfterAbsentOrUnusableKeepsBackoff(t *testing.T) { {"zero", "0"}, {"negative", "-5"}, {"http-date in the past", "Wed, 09 Jun 2021 10:18:14 GMT"}, - // Too large for the parser's own int64, so Go treats it as malformed - // rather than over-range, and it falls through here rather than - // saturating — the first of the two tiers #793 states in SPEC §6 - // "Retry-After Honouring"; the cross-SDK convergence is #799. One past - // the largest int64 and a 20-digit value are the same case; both are - // pinned so the boundary cannot drift into the saturating table by - // accident. - {"one past the largest int64", "9223372036854775808"}, - {"digits beyond int64 range", "99999999999999999999"}, // RFC 9110's delay-seconds is `1*DIGIT`, so a sign is not a delay, and // strconv accepts one — without the digits-only guard ParseInt would // honour this as 5 (review follow-up, Codex). This row is the one that @@ -241,13 +232,47 @@ func TestClient_RetryAfterAbsentOrUnusableKeepsBackoff(t *testing.T) { // The delay is asserted, not the elapsed time — a clamped wait is ~68 years, // which is precisely why nothing here may sleep it. // -// This is the SECOND of the two tiers #793 states in SPEC §6 "Retry-After -// Honouring": a value the parser holds but the host cannot schedule. The first -// — a value the parser's own int64 cannot hold at all — Go treats as malformed, -// and it belongs in the backoff table above, which is where -// `9223372036854775808` and the 20-digit case are pinned. +// SPEC §6 "Retry-After Parsing Algorithm" has ONE ceiling, MAX_RETRY_AFTER_SECONDS, +// and `1*DIGIT` has no upper bound, so no digit string is malformed for its +// width: one past the largest int64 and a 20-digit value saturate exactly as a +// value the parser can hold does. An earlier revision treated those two as +// malformed and fell through to the backoff curve, which made the honoured +// wait depend on the parser's word size. func TestClient_RetryAfterSaturatesAtTheHonouredCeiling(t *testing.T) { - assertSaturatedRetryAfter(t, "9223372036854775807") + for _, tc := range []struct{ name, header string }{ + {"largest int64", "9223372036854775807"}, + {"one past the largest int64", "9223372036854775808"}, + {"digits beyond int64 range", "99999999999999999999"}, + {"one past the ceiling", "2147483648"}, + {"leading zeros past the ceiling", "0002147483648"}, + } { + t.Run(tc.name, func(t *testing.T) { assertSaturatedRetryAfter(t, tc.header) }) + } +} + +func serviceUnavailable(retryAfter string) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + if retryAfter != "" { + w.Header().Set("Retry-After", retryAfter) + } + w.WriteHeader(http.StatusServiceUnavailable) + } +} + +// TestClient_RetryAfterHonouredAt503 pins SPEC §6 "Retry-After Honouring" on +// the raw GET loop: the header governs the wait at every status the loop +// retries, and 503 is the one RFC 9110 gives explicit Retry-After semantics. +// Before singleRequest's gateway arm carried RetryAfter, this observed the +// ~1ms backoff curve and failed. +func TestClient_RetryAfterHonouredAt503(t *testing.T) { + delays, err := retryAfterProbe(t, serviceUnavailable("2")) + + if !errors.Is(err, context.Canceled) { + t.Fatalf("Get returned %v, want context.Canceled", err) + } + if len(delays) != 1 || delays[0] != 2*time.Second { + t.Errorf("loop computed %v, want exactly [2s] — a 503's Retry-After replaces the backoff curve", delays) + } } func assertSaturatedRetryAfter(t *testing.T, header string) { diff --git a/go/pkg/basecamp/download.go b/go/pkg/basecamp/download.go index 52cbbdaa68..0793dcab33 100644 --- a/go/pkg/basecamp/download.go +++ b/go/pkg/basecamp/download.go @@ -114,7 +114,7 @@ func (ac *AccountClient) DownloadURL(ctx context.Context, rawURL string) (result // The authenticated hop is wrapped in the SDK-standard GET retry loop. // Retry scope matches Client.singleRequest's @retryable set: network errors // and 429/502/503/504 responses are retried up to MaxRetries with exponential -// backoff, honoring Retry-After on 429. Non-retried statuses (including 500) +// backoff, honoring Retry-After at every status in that set. Non-retried statuses (including 500) // are surfaced via the dispatch switch — 500 is mapped to a non-retryable // Error that mirrors singleRequest's ErrAPI(500, ...); other statuses go // through checkResponse. Retries stop once the response enters 2xx/3xx @@ -212,8 +212,15 @@ func (c *Client) fetchAPIDownload(ctx context.Context, rawURL string) (*Download _, _ = io.Copy(io.Discard, io.LimitReader(r.Body, MaxErrorBodyBytes)) _ = r.Body.Close() lastErr = checkResponse(r, bodyForErr) - if r.StatusCode == http.StatusTooManyRequests { - retryAfter = parseRetryAfter(r.Header.Get("Retry-After")) + // Honoured at every status in the hop-1 set, not at 429 alone + // (SPEC §14 "Hop-1 Retry"), and read off the mapped error rather + // than parsed again: one parse feeds both the sleep and the + // error's field (SPEC §6), so the OnRetry hook sees the delay the + // loop takes. A second parse of an HTTP-date could cross a + // whole-second boundary and report one second less. + var mapped *Error + if errors.As(lastErr, &mapped) { + retryAfter = mapped.RetryAfter } default: resp = r diff --git a/go/pkg/basecamp/download_test.go b/go/pkg/basecamp/download_test.go index edaed16c78..4cd6082891 100644 --- a/go/pkg/basecamp/download_test.go +++ b/go/pkg/basecamp/download_test.go @@ -858,6 +858,79 @@ func TestDownloadURL_AuthHopRetriesOn429WithRetryAfter(t *testing.T) { } } +// retryAfterHooks records the error each OnRetry call carries. +type retryAfterHooks struct { + NoopHooks + errs []error +} + +func (h *retryAfterHooks) OnRetry(_ context.Context, _ RequestInfo, _ int, err error) { + h.errs = append(h.errs, err) +} + +// TestDownloadURL_AuthHopSleepsTheRetryAfterItMaps pins SPEC §6's one-parse +// rule on the download loop: the value checkResponse mapped onto the error is +// the value the loop sleeps, and the OnRetry hook sees that same error. The +// loop used to parse the header a second time for its own sleep, which agreed +// with the mapped value for delay-seconds and could disagree by one for an +// HTTP-date read across a whole-second boundary — a case no test can make +// happen on demand, so this asserts the path (one value, on the error the +// hook receives, at a newly covered 503) rather than the boundary itself. +func TestDownloadURL_AuthHopSleepsTheRetryAfterItMaps(t *testing.T) { + var attempts atomic.Int32 + s3Server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("ok")) + })) + defer s3Server.Close() + apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if attempts.Add(1) == 1 { + w.Header().Set("Retry-After", "1") + w.WriteHeader(http.StatusServiceUnavailable) + return + } + w.Header().Set("Location", s3Server.URL+"/bucket/file.pdf") + w.WriteHeader(http.StatusFound) + })) + defer apiServer.Close() + + hooks := &retryAfterHooks{} + cfg := DefaultConfig() + cfg.BaseURL = apiServer.URL + client := NewClient(cfg, &StaticTokenProvider{Token: "test-token"}, + WithMaxRetries(3), + WithBaseDelay(10*time.Millisecond), + WithMaxJitter(time.Millisecond), + WithTransport(http.DefaultTransport), + WithHooks(hooks), + ) + ac := client.ForAccount("12345") + + start := time.Now() + result, err := ac.DownloadURL(context.Background(), + "https://storage.3.basecamp.com/999/blobs/abc/download/doc.pdf") + elapsed := time.Since(start) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer result.Body.Close() + + if len(hooks.errs) != 1 { + t.Fatalf("OnRetry called %d times, want 1", len(hooks.errs)) + } + var mapped *Error + if !errors.As(hooks.errs[0], &mapped) { + t.Fatalf("OnRetry received %T, want *Error", hooks.errs[0]) + } + if mapped.HTTPStatus != 503 || mapped.RetryAfter != 1 { + t.Errorf("OnRetry error = status %d retryAfter %d, want 503 with the mapped Retry-After of 1", + mapped.HTTPStatus, mapped.RetryAfter) + } + if elapsed < time.Second { + t.Errorf("slept %v, want at least the 1s the mapped error carries", elapsed) + } +} + // TestDownloadURL_RetryWaitChecksCancellationBeforeTheTimer is the download // loop's copy of TestClient_RetryWaitChecksCancellationBeforeTheTimer: the same // select, the same fire-OnRetry-then-wait order, and so the same coin flip when diff --git a/go/pkg/basecamp/errors.go b/go/pkg/basecamp/errors.go index 37799efa2d..cb53ff45dc 100644 --- a/go/pkg/basecamp/errors.go +++ b/go/pkg/basecamp/errors.go @@ -64,10 +64,10 @@ type Error struct { FieldErrors map[string][]string HTTPStatus int Retryable bool - // RetryAfter is the server-specified delay in seconds from a 429's + // RetryAfter is the server-specified delay in seconds from the response's // Retry-After header, resolved from either wire form (delta-seconds or - // HTTP-date). Zero when the server named no delay, which is every status - // but 429 today. The GET retry loop sleeps this instead of its backoff + // HTTP-date) and carried at every status (SPEC §6). Zero when the server + // named no delay. The GET retry loop sleeps this instead of its backoff // curve when it is positive; callers that give up and reschedule the work // themselves read it off the returned error. // @@ -119,6 +119,21 @@ func (e *Error) withRequestID(requestID string) *Error { return &errCopy } +// withRetryAfter returns a copy carrying the delay a Retry-After header names, +// parsed per SPEC §6. Every status-mapped error carries it (SPEC §6 "HTTP +// Status Mapping Algorithm"), not only the ones a retry loop reads it off, so +// a caller rescheduling the work themselves sees what the origin said whatever +// the status. A header that names no delay leaves the error untouched. +func (e *Error) withRetryAfter(header string) *Error { + retryAfter := parseRetryAfter(header) + if e == nil || retryAfter == 0 { + return e + } + errCopy := *e + errCopy.RetryAfter = retryAfter + return &errCopy +} + // ExitCode returns the appropriate exit code for this error. func (e *Error) ExitCode() int { return ExitCodeFor(e.Code) diff --git a/go/pkg/basecamp/helpers.go b/go/pkg/basecamp/helpers.go index 627c2d635b..f3616ceec0 100644 --- a/go/pkg/basecamp/helpers.go +++ b/go/pkg/basecamp/helpers.go @@ -63,29 +63,33 @@ func checkResponse(resp *http.Response, body []byte) error { requestID := resp.Header.Get(requestIDHeader) serverMsg, serverHint, fieldErrors := parseErrorBody(body) + // Parsed once and carried on every arm (SPEC §6 "HTTP Status Mapping + // Algorithm"): the retry loops read it off a 429 or 503, and a caller + // rescheduling the work themselves reads it off whatever came back. + retryAfter := parseRetryAfter(resp.Header.Get("Retry-After")) switch resp.StatusCode { case http.StatusBadRequest, http.StatusUnprocessableEntity: - return validationErrorFromBody(serverMsg, serverHint, fieldErrors, resp.StatusCode, requestID, body) + return validationErrorFromBody(serverMsg, serverHint, fieldErrors, resp.StatusCode, requestID, retryAfter, body) case http.StatusUnauthorized: - return &Error{Code: CodeAuth, Message: msgOrDefault(serverMsg, "authentication required"), Hint: serverHint, HTTPStatus: 401, RequestID: requestID} + return &Error{Code: CodeAuth, Message: msgOrDefault(serverMsg, "authentication required"), Hint: serverHint, HTTPStatus: 401, RetryAfter: retryAfter, RequestID: requestID} case http.StatusForbidden: - return &Error{Code: CodeForbidden, Message: msgOrDefault(serverMsg, "access denied"), Hint: serverHint, HTTPStatus: 403, RequestID: requestID} + return &Error{Code: CodeForbidden, Message: msgOrDefault(serverMsg, "access denied"), Hint: serverHint, HTTPStatus: 403, RetryAfter: retryAfter, RequestID: requestID} case http.StatusNotFound: - return &Error{Code: CodeNotFound, Message: msgOrDefault(serverMsg, "resource not found"), Hint: serverHint, HTTPStatus: 404, RequestID: requestID} + return &Error{Code: CodeNotFound, Message: msgOrDefault(serverMsg, "resource not found"), Hint: serverHint, HTTPStatus: 404, RetryAfter: retryAfter, RequestID: requestID} case http.StatusTooManyRequests: - return &Error{Code: CodeRateLimit, Message: msgOrDefault(serverMsg, "rate limited - try again later"), Hint: serverHint, HTTPStatus: 429, Retryable: true, RetryAfter: parseRetryAfter(resp.Header.Get("Retry-After")), RequestID: requestID} + return &Error{Code: CodeRateLimit, Message: msgOrDefault(serverMsg, "rate limited - try again later"), Hint: serverHint, HTTPStatus: 429, Retryable: true, RetryAfter: retryAfter, RequestID: requestID} case http.StatusInsufficientStorage: // A 5xx status carrying a client fact: the account is out of storage, or // at its webhook ceiling. Retrying cannot satisfy it, so this must be // decided before the 5xx catch-all below. - return &Error{Code: CodeLimitExceeded, Message: msgOrDefault(serverMsg, "account limit reached"), Hint: serverHint, HTTPStatus: 507, Retryable: false, RequestID: requestID} + return &Error{Code: CodeLimitExceeded, Message: msgOrDefault(serverMsg, "account limit reached"), Hint: serverHint, HTTPStatus: 507, Retryable: false, RetryAfter: retryAfter, RequestID: requestID} default: retryable := resp.StatusCode >= 500 && resp.StatusCode < 600 // SPEC §6 step 5: the fixed code-bearing phrase, never resp.Status — // the wire reason phrase does not exist under HTTP/2 and a platform's // table is empty for an unregistered code. - return &Error{Code: CodeAPI, Message: msgOrDefault(serverMsg, fmt.Sprintf("Request failed (HTTP %d)", resp.StatusCode)), Hint: serverHint, HTTPStatus: resp.StatusCode, Retryable: retryable, RequestID: requestID} + return &Error{Code: CodeAPI, Message: msgOrDefault(serverMsg, fmt.Sprintf("Request failed (HTTP %d)", resp.StatusCode)), Hint: serverHint, HTTPStatus: resp.StatusCode, Retryable: retryable, RetryAfter: retryAfter, RequestID: requestID} } } @@ -336,8 +340,9 @@ func validationError(serverMsg, serverHint string, fieldErrors map[string][]stri } } -func validationErrorFromBody(serverMsg, serverHint string, fieldErrors map[string][]string, status int, requestID string, body []byte) error { +func validationErrorFromBody(serverMsg, serverHint string, fieldErrors map[string][]string, status int, requestID string, retryAfter int, body []byte) error { validation := validationError(serverMsg, serverHint, fieldErrors, status, requestID) + validation.RetryAfter = retryAfter if status != http.StatusUnprocessableEntity { return validation } diff --git a/go/pkg/generated/client.gen.go b/go/pkg/generated/client.gen.go index ff166183f3..7674e69f6c 100644 --- a/go/pkg/generated/client.gen.go +++ b/go/pkg/generated/client.gen.go @@ -5710,6 +5710,80 @@ func isRetryableStatus(statusCode int, operationId string) bool { return false } +// maxRetryAfterSeconds is SPEC §6's MAX_RETRY_AFTER_SECONDS: the value a parsed +// Retry-After saturates at, in both wire forms. A representability bound (the +// narrowest `retry_after` integer any SDK ships — this package's own `int` on a +// 32-bit target — and the same 2147483647 §16 names as the shared ceiling), +// not a policy cap. Spelled as a literal rather than math.MaxInt32 so the +// generated file needs no extra import and the number reads as the contract. +const maxRetryAfterSeconds = 2147483647 + +// parseRetryAfter reads a Retry-After header per SPEC §6 "Retry-After Parsing +// Algorithm": RFC 9110 delay-seconds (`1*DIGIT`, honoured when > 0) or an RFC +// 7231 HTTP-date (the seconds remaining, rounded up, honoured when > 0), either +// saturated at maxRetryAfterSeconds. Returns 0 for an absent, malformed, zero or +// past value, which every caller reads as "no server-directed delay". +// +// This is the generated package's copy of go/pkg/basecamp's parseRetryAfter; +// the two cannot share code in that direction because this package is the +// dependency. Before this helper the loop below did a bare strconv.Atoi: no +// HTTP-date at all (every date fell through to the backoff curve on every +// generated operation), an unchecked range error, and a seconds×time.Second +// product that wrapped negative for `Retry-After: 9223372036854775807` — an +// already-expired timer, so the loop burned its whole attempt budget back to +// back against an origin that had asked it to wait (#798). +func parseRetryAfter(header string) int { + if header == "" { + return 0 + } + if isDelaySeconds(header) { + digits := strings.TrimLeft(header, "0") + // Width first, so the answer never depends on a conversion that can + // itself overflow; then value. + if len(digits) > len("2147483647") { + return maxRetryAfterSeconds + } + seconds, err := strconv.ParseInt(digits, 10, 64) + if err != nil || seconds <= 0 { + return 0 + } + if seconds > maxRetryAfterSeconds { + return maxRetryAfterSeconds + } + return int(seconds) + } + if t, err := http.ParseTime(header); err == nil { + remaining := time.Until(t) + if remaining <= 0 { + return 0 + } + seconds := int64(remaining / time.Second) + if remaining%time.Second != 0 { + seconds++ + } + if seconds > maxRetryAfterSeconds { + return maxRetryAfterSeconds + } + return int(seconds) + } + return 0 +} + +// isDelaySeconds reports whether the value is RFC 9110's `1*DIGIT` and nothing +// else: no sign, no space, no separator, no decimal point. strconv would accept +// a leading `+` or `-`, and `+5` is not a delay. +func isDelaySeconds(value string) bool { + if value == "" { + return false + } + for _, r := range value { + if r < '0' || r > '9' { + return false + } + } + return true +} + // captureReplayBody inspects the finalized first-attempt request and returns a // closure that reproduces its body on later attempts, plus the ContentLength to // restore each attempt. When retriable is false the body cannot be safely @@ -5987,20 +6061,25 @@ func (c *Client) doWithRetry(ctx context.Context, buildRequest func() (*http.Req // Close body before retry _ = resp.Body.Close() - // For 429 responses, respect Retry-After header if present - retryDelay := delay - if resp.StatusCode == http.StatusTooManyRequests { - if retryAfter := resp.Header.Get("Retry-After"); retryAfter != "" { - if seconds, err := strconv.Atoi(retryAfter); err == nil && seconds > 0 { - retryDelay = time.Duration(seconds) * time.Second - } - } + // A Retry-After the origin sent replaces the backoff at EVERY status + // this branch reaches — the status already passed isRetryableStatus, + // and SPEC §6 "Retry-After Honouring" derives honouring from retry + // eligibility rather than from a status list (a 429-only gate here + // left a 503 carrying `Retry-After: 120` backing off ~1s). It is + // waited EXACTLY: the jitter term decorrelates delays this client + // chose, and a delay the origin named is already the origin's choice, + // so it is added to the local curve alone (SPEC §7 "Backoff Formula"). + var wait time.Duration + if seconds := parseRetryAfter(resp.Header.Get("Retry-After")); seconds > 0 { + wait = time.Duration(seconds) * time.Second + } else { + wait = delay + time.Duration(rand.Int63n(int64(100*time.Millisecond))) } select { case <-ctx.Done(): return nil, ctx.Err() - case <-time.After(retryDelay + time.Duration(rand.Int63n(int64(100*time.Millisecond)))): + case <-time.After(wait): } delay = time.Duration(float64(delay) * c.RetryConfig.Multiplier) if delay > c.RetryConfig.MaxDelay { @@ -26320,11 +26399,7 @@ func ParseHTTPError(resp *http.Response) *APIError { case http.StatusNotFound: return NewNotFoundError("Resource", resp.Request.URL.Path) case http.StatusTooManyRequests: - retryAfter := 0 - if ra := resp.Header.Get("Retry-After"); ra != "" { - retryAfter, _ = strconv.Atoi(ra) - } - return NewRateLimitError(retryAfter) + return NewRateLimitError(parseRetryAfter(resp.Header.Get("Retry-After"))) case http.StatusInternalServerError, http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout: return &APIError{ diff --git a/go/pkg/generated/retry_after_test.go b/go/pkg/generated/retry_after_test.go new file mode 100644 index 0000000000..dbc5866f82 --- /dev/null +++ b/go/pkg/generated/retry_after_test.go @@ -0,0 +1,121 @@ +package generated_test + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/basecamp/basecamp-sdk/go/pkg/generated" +) + +// The generated client's retry loop, driven through its public surface the +// way auth_transport_test.go drives the transport: nothing here names an +// unexported symbol, so a regeneration cannot leave it testing a shape the +// template no longer emits. Before #855 the loop did a bare strconv.Atoi +// behind a 429 gate (#798): no HTTP-date form, a 503's header ignored, and a +// seconds×time.Second product that wrapped negative for the largest int64 — +// an already-expired timer, so a typed operation burned its whole attempt +// budget back to back against an origin that had asked it to wait. + +// retryAfterClient answers every request with the given handler and retries +// on a millisecond curve, so any delay at or above a second can only have +// come from the Retry-After header. +func retryAfterClient(t *testing.T, handler http.HandlerFunc) *generated.Client { + t.Helper() + server := httptest.NewServer(handler) + t.Cleanup(server.Close) + client, err := generated.NewClient(server.URL, generated.WithRetryConfig(generated.RetryConfig{ + MaxRetries: 3, + BaseDelay: time.Millisecond, + MaxDelay: time.Millisecond, + Multiplier: 2, + })) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + return client +} + +func serviceUnavailableThenOK(retryAfter string, attempts *atomic.Int32) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + if attempts.Add(1) == 1 { + w.Header().Set("Retry-After", retryAfter) + w.WriteHeader(http.StatusServiceUnavailable) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{}`)) + } +} + +// A 503's Retry-After governs the wait (SPEC §6 "Retry-After Honouring"), in +// both wire forms, and is waited exactly rather than as a floor under the +// jittered curve — the elapsed floor is the header's own value. +func TestGeneratedClient_HonoursRetryAfterAt503(t *testing.T) { + for _, tc := range []struct { + name string + header func() string + wantAtLeast time.Duration + }{ + {"delta-seconds", func() string { return "2" }, 2 * time.Second}, + // Minted inside the subtest, not before the table: the delta-seconds + // case spends two seconds, and a date minted before it would be a + // second nearer by the time this case reads it. A whole-second date, + // because the wire form carries whole seconds: the remainder at parse + // time is in (2s, 3s] and rounds up to 3. + {"http-date", func() string { + return time.Now().Truncate(time.Second).Add(3 * time.Second).UTC().Format(http.TimeFormat) + }, 2 * time.Second}, + } { + t.Run(tc.name, func(t *testing.T) { + var attempts atomic.Int32 + client := retryAfterClient(t, serviceUnavailableThenOK(tc.header(), &attempts)) + + start := time.Now() + resp, err := client.GetProject(context.Background(), "999", 1) + elapsed := time.Since(start) + + if err != nil { + t.Fatalf("GetProject: %v", err) + } + _ = resp.Body.Close() + if got := attempts.Load(); got != 2 { + t.Errorf("made %d requests, want 2", got) + } + if elapsed < tc.wantAtLeast { + t.Errorf("retried after %v, want at least %v — the 503's Retry-After must replace the millisecond curve", elapsed, tc.wantAtLeast) + } + }) + } +} + +// The regression for the wrapped conversion. Against the old Atoi loop the +// largest int64 became a -1s timer that fired at once, and the whole attempt +// budget went out inside the deadline; against the saturating parser the loop +// is still waiting when the deadline lands, having sent exactly one request. +func TestGeneratedClient_OverRangeRetryAfterSaturatesRatherThanWrapping(t *testing.T) { + for _, header := range []string{"9223372036854775807", "99999999999999999999", "2147483648"} { + t.Run(header, func(t *testing.T) { + var attempts atomic.Int32 + client := retryAfterClient(t, serviceUnavailableThenOK(header, &attempts)) + + ctx, cancel := context.WithTimeout(context.Background(), 300*time.Millisecond) + defer cancel() + resp, err := client.GetProject(ctx, "999", 1) + if resp != nil { + _ = resp.Body.Close() + } + + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("GetProject returned %v, want context.DeadlineExceeded — the loop must still be waiting out the (saturated) header when the deadline lands", err) + } + if got := attempts.Load(); got != 1 { + t.Errorf("made %d requests inside the deadline, want exactly 1 — a wrapped delay retries at once", got) + } + }) + } +} diff --git a/go/templates/client.tmpl b/go/templates/client.tmpl index 7e3aab89b1..75802c44a1 100644 --- a/go/templates/client.tmpl +++ b/go/templates/client.tmpl @@ -172,6 +172,80 @@ func isRetryableStatus(statusCode int, operationId string) bool { return false } +// maxRetryAfterSeconds is SPEC §6's MAX_RETRY_AFTER_SECONDS: the value a parsed +// Retry-After saturates at, in both wire forms. A representability bound (the +// narrowest `retry_after` integer any SDK ships — this package's own `int` on a +// 32-bit target — and the same 2147483647 §16 names as the shared ceiling), +// not a policy cap. Spelled as a literal rather than math.MaxInt32 so the +// generated file needs no extra import and the number reads as the contract. +const maxRetryAfterSeconds = 2147483647 + +// parseRetryAfter reads a Retry-After header per SPEC §6 "Retry-After Parsing +// Algorithm": RFC 9110 delay-seconds (`1*DIGIT`, honoured when > 0) or an RFC +// 7231 HTTP-date (the seconds remaining, rounded up, honoured when > 0), either +// saturated at maxRetryAfterSeconds. Returns 0 for an absent, malformed, zero or +// past value, which every caller reads as "no server-directed delay". +// +// This is the generated package's copy of go/pkg/basecamp's parseRetryAfter; +// the two cannot share code in that direction because this package is the +// dependency. Before this helper the loop below did a bare strconv.Atoi: no +// HTTP-date at all (every date fell through to the backoff curve on every +// generated operation), an unchecked range error, and a seconds×time.Second +// product that wrapped negative for `Retry-After: 9223372036854775807` — an +// already-expired timer, so the loop burned its whole attempt budget back to +// back against an origin that had asked it to wait (#798). +func parseRetryAfter(header string) int { + if header == "" { + return 0 + } + if isDelaySeconds(header) { + digits := strings.TrimLeft(header, "0") + // Width first, so the answer never depends on a conversion that can + // itself overflow; then value. + if len(digits) > len("2147483647") { + return maxRetryAfterSeconds + } + seconds, err := strconv.ParseInt(digits, 10, 64) + if err != nil || seconds <= 0 { + return 0 + } + if seconds > maxRetryAfterSeconds { + return maxRetryAfterSeconds + } + return int(seconds) + } + if t, err := http.ParseTime(header); err == nil { + remaining := time.Until(t) + if remaining <= 0 { + return 0 + } + seconds := int64(remaining / time.Second) + if remaining%time.Second != 0 { + seconds++ + } + if seconds > maxRetryAfterSeconds { + return maxRetryAfterSeconds + } + return int(seconds) + } + return 0 +} + +// isDelaySeconds reports whether the value is RFC 9110's `1*DIGIT` and nothing +// else: no sign, no space, no separator, no decimal point. strconv would accept +// a leading `+` or `-`, and `+5` is not a delay. +func isDelaySeconds(value string) bool { + if value == "" { + return false + } + for _, r := range value { + if r < '0' || r > '9' { + return false + } + } + return true +} + // captureReplayBody inspects the finalized first-attempt request and returns a // closure that reproduces its body on later attempts, plus the ContentLength to // restore each attempt. When retriable is false the body cannot be safely @@ -449,20 +523,25 @@ func (c *{{ $clientTypeName }}) doWithRetry(ctx context.Context, buildRequest fu // Close body before retry _ = resp.Body.Close() - // For 429 responses, respect Retry-After header if present - retryDelay := delay - if resp.StatusCode == http.StatusTooManyRequests { - if retryAfter := resp.Header.Get("Retry-After"); retryAfter != "" { - if seconds, err := strconv.Atoi(retryAfter); err == nil && seconds > 0 { - retryDelay = time.Duration(seconds) * time.Second - } - } + // A Retry-After the origin sent replaces the backoff at EVERY status + // this branch reaches — the status already passed isRetryableStatus, + // and SPEC §6 "Retry-After Honouring" derives honouring from retry + // eligibility rather than from a status list (a 429-only gate here + // left a 503 carrying `Retry-After: 120` backing off ~1s). It is + // waited EXACTLY: the jitter term decorrelates delays this client + // chose, and a delay the origin named is already the origin's choice, + // so it is added to the local curve alone (SPEC §7 "Backoff Formula"). + var wait time.Duration + if seconds := parseRetryAfter(resp.Header.Get("Retry-After")); seconds > 0 { + wait = time.Duration(seconds) * time.Second + } else { + wait = delay + time.Duration(rand.Int63n(int64(100*time.Millisecond))) } select { case <-ctx.Done(): return nil, ctx.Err() - case <-time.After(retryDelay + time.Duration(rand.Int63n(int64(100*time.Millisecond)))): + case <-time.After(wait): } delay = time.Duration(float64(delay) * c.RetryConfig.Multiplier) if delay > c.RetryConfig.MaxDelay { @@ -1406,11 +1485,7 @@ func ParseHTTPError(resp *http.Response) *APIError { case http.StatusNotFound: return NewNotFoundError("Resource", resp.Request.URL.Path) case http.StatusTooManyRequests: - retryAfter := 0 - if ra := resp.Header.Get("Retry-After"); ra != "" { - retryAfter, _ = strconv.Atoi(ra) - } - return NewRateLimitError(retryAfter) + return NewRateLimitError(parseRetryAfter(resp.Header.Get("Retry-After"))) case http.StatusInternalServerError, http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout: return &APIError{ diff --git a/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/HeaderTokens.kt b/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/HeaderTokens.kt new file mode 100644 index 0000000000..cb97c68ce7 --- /dev/null +++ b/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/HeaderTokens.kt @@ -0,0 +1,37 @@ +package com.basecamp.sdk.conformance + +import io.ktor.http.toHttpDate +import io.ktor.util.date.GMTDate + +private val HEADER_TOKEN = Regex("""^\{\{(.*)\}\}$""") +private val HTTPDATE_TOKEN = Regex("""^httpdate\+(\d{1,9})s$""") + +/** + * Substitutes the one token a fixture header value may carry, `{{httpdate+Ns}}` + * (SPEC §19, conformance/schema.json), at the moment the response is served. + * Every other value passes through untouched. + * + * The token resolves to the IMF-fixdate of floor(now) + N + 1 seconds: the + * first whole second strictly more than N seconds after the second the response + * is served in. A compliant SPEC §6 parser sees a remainder in (N − latency, + * N + 1] and, rounding up, computes at least N whole seconds, so the fixture + * pairs it with a `delayBetweenRequests` floor of N × 1000 ms. It exists + * because a static fixture has no clock: a literal past date pins only the + * fall-through, and a far-future one is differently behaved per host. + * + * N is one to nine digits, so the arithmetic is exact everywhere and every + * runner's date formatter stays in range; a longer N is an unrecognised token. + * + * An unrecognised `{{…}}` throws rather than passing through: a typo'd token + * served verbatim would be an unparseable header, which the SDK answers with + * its ordinary backoff — the exact outcome the case exists to distinguish from. + */ +fun resolveHeaderValue(value: String, nowMs: Long): String { + val token = HEADER_TOKEN.matchEntire(value) ?: return value + val inner = HTTPDATE_TOKEN.matchEntire(token.groupValues[1]) + ?: throw IllegalArgumentException( + "unrecognised header token \"$value\": only {{httpdate+Ns}} is defined (conformance/schema.json)", + ) + val seconds = Math.floorDiv(nowMs, 1000L) + inner.groupValues[1].toLong() + 1 + return GMTDate(seconds * 1000).toHttpDate() +} diff --git a/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt b/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt index 1189a48280..b129c6044b 100644 --- a/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt +++ b/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt @@ -490,8 +490,10 @@ private fun runTest(tc: TestCase): TestResult { val responseHeaders = HeadersBuilder().apply { append(HttpHeaders.ContentType, ContentType.Application.Json.toString()) + // Resolved at serve time: a `{{httpdate+Ns}}` value is relative + // to NOW, not to when the fixture was loaded. for ((key, value) in mockResp.headers) { - append(key, value) + append(key, resolveHeaderValue(value, System.currentTimeMillis())) } } diff --git a/kotlin/conformance/src/test/kotlin/com/basecamp/sdk/conformance/HeaderTokensTest.kt b/kotlin/conformance/src/test/kotlin/com/basecamp/sdk/conformance/HeaderTokensTest.kt new file mode 100644 index 0000000000..6cfc5da2ad --- /dev/null +++ b/kotlin/conformance/src/test/kotlin/com/basecamp/sdk/conformance/HeaderTokensTest.kt @@ -0,0 +1,41 @@ +package com.basecamp.sdk.conformance + +import kotlin.test.Test +import kotlin.test.assertContains +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith + +/** + * The `{{httpdate+Ns}}` header token (SPEC §19, conformance/schema.json). + * + * A static fixture has no clock, so the positive half of SPEC §6's HTTP-date + * branch was unpinnable until this token (#780). These cases pin the resolver's + * arithmetic against a frozen instant so the fixture's one-sided timing floor + * rests on a deterministic contract. + */ +class HeaderTokensTest { + /** A quarter-second into 10:18:14 UTC, so floor and round-up differ. */ + private val nowMs = 1_623_233_894_250L + + @Test + fun `plain values pass through`() { + for (value in listOf("", "2", "Wed, 09 Jun 2021 10:18:14 GMT", "application/json", "{not a token}")) { + assertEquals(value, resolveHeaderValue(value, nowMs)) + } + } + + @Test + fun `httpdate resolves to the whole second past N`() { + assertEquals("Wed, 09 Jun 2021 10:18:17 GMT", resolveHeaderValue("{{httpdate+2s}}", nowMs)) + assertEquals("Wed, 09 Jun 2021 10:18:15 GMT", resolveHeaderValue("{{httpdate+0s}}", nowMs)) + assertEquals("Wed, 09 Jun 2021 10:18:25 GMT", resolveHeaderValue("{{httpdate+10s}}", nowMs)) + } + + @Test + fun `unknown tokens are errors not literals`() { + for (value in listOf("{{httpdate}}", "{{httpdate+2}}", "{{httpdate-2s}}", "{{now}}", "{{}}", "{{httpdate+1000000000s}}")) { + val error = assertFailsWith { resolveHeaderValue(value, nowMs) } + assertContains(error.message ?: "", value) + } + } +} diff --git a/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt b/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt index a7d14e2ce1..e06e3d4e03 100644 --- a/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt +++ b/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt @@ -92,7 +92,7 @@ fun filenameFromURL(rawURL: String): String { * * The first hop retries under the SPEC §14 policy — network errors plus * {429, 502, 503, 504}, never 500 — with exponential backoff (Retry-After - * honored on 429) under the public maxRetries total-attempt cap coerced to at + * honored at every status in the set) under the public maxRetries total-attempt cap coerced to at * least one; `enableRetry = false` collapses it to exactly one attempt. The * second hop is exempt: no retry, no auth. * @@ -263,7 +263,7 @@ suspend fun AccountClient.downloadURL(rawURL: String): DownloadResult { /** * Runs the download's authenticated hop 1 under the SPEC §14 retry policy: * network errors plus [DOWNLOAD_RETRY_ON] — never 500 — retried with - * exponential backoff (Retry-After honored on 429) while attempts remain. + * exponential backoff (Retry-After honored at every status in the set) while attempts remain. * DownloadURL is deliberately absent from the behavior model, so the policy * lives here rather than being looked up by operation. * @@ -365,8 +365,10 @@ private suspend fun AccountClient.downloadHop1( return response } + // Honoured at every status in DOWNLOAD_RETRY_ON, not at 429 alone + // (SPEC §14 "Hop-1 Retry"). val retryAfter = parseRetryAfter(response.headers["Retry-After"]) - val delayMs = if (status == 429 && retryAfter != null) { + val delayMs = if (retryAfter != null) { retryAfter.toLong() * 1000 } else { BasecampHttpClient.calculateBackoffDelay(baseDelayMs, attempt) 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 94ff39ab7f..b59024b43f 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 @@ -210,8 +210,13 @@ internal class BasecampHttpClient( } if (shouldRetry && attempt < maxAttempts) { + // A Retry-After replaces the backoff at EVERY status this branch + // reaches — it already passed the declared retryOn gate, and SPEC §6 + // "Retry-After Honouring" derives honouring from retry eligibility, + // not from a status list. A `status == 429` gate here left a 503 + // carrying `Retry-After: 120` backing off ~1s. val retryAfter = parseRetryAfter(response.headers["Retry-After"]) - val delayMs = if (status == 429 && retryAfter != null) { + val delayMs = if (retryAfter != null) { retryAfter.toLong() * 1000 } else { calculateBackoffDelay(baseDelayMs, attempt) 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 798dad5673..5a719cb2e5 100644 --- a/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/RetryTest.kt +++ b/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/RetryTest.kt @@ -538,6 +538,46 @@ class RetryTest { client.close() } + /** + * SPEC §6 "Retry-After Honouring": the header governs the wait at every + * status in the declared retry set. A `status == 429` gate left a 503 + * carrying `Retry-After: 2` on the ~1s backoff curve (#775). + */ + @Test + fun retryAfterHonouredAt503() = runTest { + var requestCount = 0 + val requestTimestamps = mutableListOf() + val engine = MockEngine { _ -> + requestCount++ + requestTimestamps.add(testScheduler.currentTime) + if (requestCount == 1) { + respond( + content = "", + status = HttpStatusCode.ServiceUnavailable, + headers = headersOf("Retry-After", "2"), + ) + } else { + respondOk("""{"id": 1}""") + } + } + + val client = testBasecampClient { + accessToken("test-token") + baseUrl = "http://localhost:3000" + this.engine = engine + } + + val account = client.forAccount("12345") + val url = "${client.config.baseUrl}/12345/projects.json" + val response = account.httpClient.requestWithRetry(HttpMethod.Get, url) + + assertEquals(200, response.status.value) + assertEquals(2, requestCount) + val elapsed = requestTimestamps[1] - requestTimestamps[0] + assertTrue(elapsed >= 2000, "Expected delay >= 2000ms from Retry-After: 2 on a 503, got $elapsed") + client.close() + } + @Test fun enableRetryFalseDisablesRetry() = runTest { var requestCount = 0 diff --git a/python/src/basecamp/errors.py b/python/src/basecamp/errors.py index 2ef4e424dd..262f42af74 100644 --- a/python/src/basecamp/errors.py +++ b/python/src/basecamp/errors.py @@ -1,7 +1,8 @@ from __future__ import annotations import json -from datetime import UTC +import math +from datetime import UTC, datetime from enum import IntEnum, StrEnum from typing import Any @@ -415,7 +416,12 @@ def error_from_response(status: int, body: str | bytes | None, headers: dict[str return err -def _parse_retry_after(value: str | None) -> int | None: +def _parse_retry_after(value: str | None, *, now: datetime | None = None) -> int | None: + """SPEC section 6 "Retry-After Parsing Algorithm". + + ``now`` is a seam for tests: the HTTP-date branch is one second wide at its + boundary, so its rounding is only pinnable against a frozen clock. + """ if not value: return None try: @@ -424,12 +430,15 @@ def _parse_retry_after(value: str | None) -> int | None: except ValueError: pass # Try HTTP-date - from datetime import datetime from email.utils import parsedate_to_datetime try: date = parsedate_to_datetime(value) - diff = int((date - datetime.now(UTC)).total_seconds()) + # Rounded UP (SPEC section 6 step 2): truncating a sub-second remainder + # toward zero turned a date 400ms out into 0, which reads as "no usable + # value" and drops onto the backoff curve, and retried up to a second + # before the moment the server named. + diff = math.ceil((date - (now or datetime.now(UTC))).total_seconds()) return diff if diff > 0 else None except (ValueError, TypeError): pass diff --git a/python/tests/test_errors.py b/python/tests/test_errors.py index 286b346268..003cc638ea 100644 --- a/python/tests/test_errors.py +++ b/python/tests/test_errors.py @@ -261,6 +261,22 @@ def test_http_date_in_past_returns_none(self): value = format_datetime(past) assert _parse_retry_after(value) is None + def test_http_date_sub_second_remainder_rounds_up(self): + # SPEC section 6 step 2: 2.75s out is 3 seconds, never 2. Truncation + # retried up to a second before the moment the server named, and turned + # a remainder under a second into 0 -- read as "no usable value". + now = datetime(2021, 6, 9, 10, 18, 14, 250_000, tzinfo=UTC) + assert _parse_retry_after("Wed, 09 Jun 2021 10:18:17 GMT", now=now) == 3 + assert _parse_retry_after("Wed, 09 Jun 2021 10:18:15 GMT", now=now) == 1 + assert _parse_retry_after("Wed, 09 Jun 2021 10:18:14 GMT", now=now) is None + + def test_retry_after_carried_on_every_status(self): + # SPEC section 6 "HTTP Status Mapping Algorithm": one parse feeds both + # the retry loop's sleep and the error's field, at 503 as at 429. + err = error_from_response(503, None, {"Retry-After": "7"}) + assert isinstance(err, ApiError) + assert err.retry_after == 7 + class TestBareFieldMap: """SPEC section 6 step 2. diff --git a/python/tests/test_http.py b/python/tests/test_http.py index 01050aef07..3c3e5d274e 100644 --- a/python/tests/test_http.py +++ b/python/tests/test_http.py @@ -236,6 +236,23 @@ def test_retry_after_header_respected(self): assert resp.status_code == 200 mock_sleep.assert_called_once_with(1.0) + @respx.mock + def test_retry_after_honoured_on_503(self): + # SPEC section 6 "Retry-After Honouring": the header governs the wait at + # every status in the declared retry set, not at 429 alone. + from unittest.mock import patch + + route = respx.get("https://3.basecampapi.com/test") + route.side_effect = [ + httpx.Response(503, headers={"Retry-After": "2"}), + httpx.Response(200, json={"ok": True}), + ] + client = make_client(max_retries=3) + with patch("time.sleep") as mock_sleep: + resp = client.get("/test") + assert resp.status_code == 200 + mock_sleep.assert_called_once_with(2.0) + class TestHeaders: @respx.mock diff --git a/ruby/README.md b/ruby/README.md index dfd18df88b..85fff6cf17 100644 --- a/ruby/README.md +++ b/ruby/README.md @@ -448,7 +448,7 @@ Only plain GET requests retry — automatically, with exponential backoff. Mutat - **Which errors**: A GET issued through a generated service carries its operation ID and is **governed** — status retries are gated on the statuses that operation declares, which is `[429, 503]` for every operation in the current metadata. A governed GET does **not** retry 500. Only the handful of GETs that carry no operation ID (`get_absolute`, OAuth discovery) fall back to the error taxonomy, where 429, 500, 502, 503, 504, and any other 5xx all retry. `NetworkError` (connection failures, including DNS and connect-phase timeouts) retries on both paths, since it has no status to gate on. Read timeouts are the exception: Faraday surfaces them as a status-less `ApiError` with `retryable? == false`, so a GET that times out mid-response fails on the first attempt. 400, 401, 403, 404, and 422 never retry. - **`max_retries`**: Total request attempts for GET requests, including the initial request — the default `3` means one initial attempt plus two retries. `max_retries: 0` is floored to a single attempt rather than sending zero requests. - **Backoff**: Exponential with jitter — `base_delay * 2^(attempt - 1) + rand * max_jitter` — uncapped, bounded in practice by the attempt budget. -- **Rate limits**: A 429's `Retry-After` header overrides the calculated backoff. Only 429 carries it: 5xx and network errors always use the exponential backoff. +- **`Retry-After`**: A `Retry-After` header overrides the calculated backoff at every status the loop is already retrying — a 429, or a retryable 5xx — and `retry_after` is carried on the error at every status, not only 429. Network errors have no header and always use the exponential backoff. - **401 responses**: With a refresh-capable token provider, the SDK refreshes the token and replays the request **once** — for all methods, including mutations — outside the `max_retries` budget. A second 401 is surfaced. The raw upload path has no 401 replay. - **Per-operation metadata**: Every GET a generated service issues passes its canonical operation ID, so essentially all SDK reads are **governed**: attempts are bounded by `min(config.max_retries, operation max)` and status retries are gated on the operation's declared `retry_on`. The GETs that carry no operation ID — `get_absolute` and the Launchpad authorization fetch it backs — are **ungoverned** and ride the classification-based loop bounded by `config.max_retries` alone. The declared `base_delay_ms` and `backoff` are inert in Ruby either way: the backoff is always the client's. (OAuth discovery uses its own single-attempt transport.) - **`retryable?`**: Unlike SDKs where the error classification is only a hint for your own code, in Ruby an error's `retryable?` (and `retry_after`) is exactly what the transport acts on for GET requests. diff --git a/ruby/lib/basecamp.rb b/ruby/lib/basecamp.rb index a9729184e9..fc29cf982c 100644 --- a/ruby/lib/basecamp.rb +++ b/ruby/lib/basecamp.rb @@ -124,7 +124,7 @@ def self.error_from_response(status, body = nil, retry_after: nil) server_message = parse_error_message(body) message = server_message || "Request failed" - case status + err = case status when 400, 422 field_errors = parse_field_errors(body) message = Security.truncate(compose_validation_message(server_message, field_errors) || "Request failed") @@ -149,12 +149,19 @@ def self.error_from_response(status, body = nil, retry_after: nil) # transient server failure, and no retry can satisfy it. LimitExceededError.new(Security.truncate(message), hint: hint) when 500 - ApiError.new("Server error (500)", http_status: 500, retryable: true, hint: hint) + ApiError.new("Server error (500)", http_status: 500, retryable: true, hint: hint, retry_after: retry_after) when 502, 503, 504 - ApiError.new("Gateway error (#{status})", http_status: status, retryable: true, hint: hint) + ApiError.new("Gateway error (#{status})", http_status: status, retryable: true, hint: hint, retry_after: retry_after) else - ApiError.from_status(status, server_message, hint: hint) + ApiError.from_status(status, server_message, hint: hint, retry_after: retry_after) end + + # Every status carries the parsed Retry-After (SPEC §6 "HTTP Status Mapping + # Algorithm"), including the arms whose error classes take no such + # argument — the same back-fill Http#handle_error applies, so this public + # mapper and the private one answer alike. + err.instance_variable_set(:@retry_after, retry_after) if retry_after && err.retry_after.nil? + err end # Extracts a filename from the last path segment of a URL. diff --git a/ruby/lib/basecamp/api_error.rb b/ruby/lib/basecamp/api_error.rb index 6cfd1d9396..bd29182bbc 100644 --- a/ruby/lib/basecamp/api_error.rb +++ b/ruby/lib/basecamp/api_error.rb @@ -3,13 +3,14 @@ module Basecamp # Raised for generic API errors. class ApiError < Error - def initialize(message, http_status: nil, hint: nil, retryable: false, cause: nil) + def initialize(message, http_status: nil, hint: nil, retryable: false, retry_after: nil, cause: nil) super( code: ErrorCode::API, message: message, hint: hint, http_status: http_status, retryable: retryable, + retry_after: retry_after, cause: cause ) end @@ -18,11 +19,12 @@ def initialize(message, http_status: nil, hint: nil, retryable: false, cause: ni # @param status [Integer] HTTP status code # @param message [String, nil] optional error message # @param hint [String, nil] optional hint (SPEC section 6 step 3) + # @param retry_after [Integer, nil] seconds from a parsed Retry-After header # @return [ApiError] - def self.from_status(status, message = nil, hint: nil) + def self.from_status(status, message = nil, hint: nil, retry_after: nil) message ||= "Request failed (HTTP #{status})" retryable = status >= 500 && status < 600 - new(message, http_status: status, hint: hint, retryable: retryable) + new(message, http_status: status, hint: hint, retryable: retryable, retry_after: retry_after) end end end diff --git a/ruby/lib/basecamp/client.rb b/ruby/lib/basecamp/client.rb index d261e68cdf..2a16ba98b2 100644 --- a/ruby/lib/basecamp/client.rb +++ b/ruby/lib/basecamp/client.rb @@ -311,8 +311,12 @@ def download_url(raw_url) else # This shouldn't happen because Faraday's raise_error middleware - # handles 4xx/5xx, but handle it defensively - raise Basecamp.error_from_response(response.status, response.body) + # handles 4xx/5xx, but handle it defensively — carrying the parsed + # Retry-After as every other mapping does (SPEC §6). + raise Basecamp.error_from_response( + response.status, response.body, + retry_after: http.parse_retry_after_header(response.headers["Retry-After"] || response.headers["retry-after"]) + ) end rescue => e duration = ((Process.clock_gettime(Process::CLOCK_MONOTONIC) - start) * 1000).round @@ -674,14 +678,22 @@ def fetch_signed_download(url) raise NetworkError.new("Download failed"), cause: nil end + # Hop 2 is never retried, but its error still carries the parsed + # Retry-After like every other mapped status (SPEC §6): a caller + # rescheduling the download themselves reads it off whatever came back. + retry_after = http.parse_retry_after_header(response["Retry-After"]) + # The exact set hop 1 dispatches on, not Net::HTTPRedirection — that # class also covers 304, which is a cache answer, not a redirect. if [ 301, 302, 303, 307, 308 ].include?(response.code.to_i) - raise ApiError.new("redirect #{response.code} on the signed download hop is not followed", http_status: response.code.to_i) + raise ApiError.new( + "redirect #{response.code} on the signed download hop is not followed", + http_status: response.code.to_i, retry_after: retry_after + ) end unless response.is_a?(Net::HTTPSuccess) - raise ApiError.new("download failed with status #{response.code}", http_status: response.code.to_i) + raise ApiError.new("download failed with status #{response.code}", http_status: response.code.to_i, retry_after: retry_after) end response diff --git a/ruby/lib/basecamp/http.rb b/ruby/lib/basecamp/http.rb index 778f2aab66..d75acc1512 100644 --- a/ruby/lib/basecamp/http.rb +++ b/ruby/lib/basecamp/http.rb @@ -230,6 +230,16 @@ def paginate_wrapped(path, key:, params: {}, operation: nil, max_items: nil) wrapper.merge(key => events) end + # The parsed Retry-After for a response some other path is mapping itself + # (the download hops in Client), so its error carries the same value the + # retry loop reads here — populated at every status (SPEC §6). + # + # @param value [String, nil] the raw Retry-After header value + # @return [Integer, nil] seconds, or nil when absent or malformed + def parse_retry_after_header(value) + parse_retry_after(value) + end + private # Shared paginator core behind paginate/paginate_key/paginate_wrapped. @@ -698,15 +708,24 @@ def handle_error(error, refresh_on_401: true) message = Security.truncate(Basecamp.parse_error_message(body) || "Account limit reached") Basecamp::LimitExceededError.new(message, hint: hint) when 500 - Basecamp::ApiError.new("Server error (500)", http_status: 500, retryable: true, hint: hint) + Basecamp::ApiError.new("Server error (500)", http_status: 500, retryable: true, hint: hint, retry_after: retry_after) when 502, 503, 504 - Basecamp::ApiError.new("Gateway error (#{status})", http_status: status, retryable: true, hint: hint) + # retry_after rides along here as it does on the 429 arm (SPEC §6 + # "HTTP Status Mapping Algorithm"): request_with_retry reads the delay + # off the error, so this is also what makes a 503's Retry-After govern + # the sleep rather than the backoff curve. + Basecamp::ApiError.new( + "Gateway error (#{status})", http_status: status, retryable: true, hint: hint, retry_after: retry_after + ) else message = Security.truncate(Basecamp.parse_error_message(body) || "Request failed (HTTP #{status})") - Basecamp::ApiError.from_status(status || 0, message, hint: hint) + Basecamp::ApiError.from_status(status || 0, message, hint: hint, retry_after: retry_after) end err.instance_variable_set(:@request_id, request_id) if request_id + # Every status carries a parsed Retry-After (SPEC §6), including the arms + # whose error classes take no such argument. + err.instance_variable_set(:@retry_after, retry_after) if retry_after && err.retry_after.nil? err end @@ -757,17 +776,20 @@ def calculate_delay(attempt, server_retry_after) base + jitter end - def parse_retry_after(value) + def parse_retry_after(value, now: Time.now) return nil if value.nil? || value.empty? # Try parsing as seconds (integer) seconds = Integer(value, exception: false) return seconds if seconds&.positive? - # Try parsing as HTTP-date + # Try parsing as HTTP-date. Rounded UP (SPEC §6 step 2): truncating a + # sub-second remainder toward zero turned a date 400ms out into 0, which + # reads as "no usable value" and drops onto the backoff curve, and + # retried up to a second before the moment the server named. begin date = Time.httpdate(value) - diff = (date - Time.now).to_i + diff = (date - now).ceil return diff if diff.positive? rescue ArgumentError # Not a valid HTTP-date diff --git a/ruby/test/basecamp/download_test.rb b/ruby/test/basecamp/download_test.rb index e439253890..590031348b 100644 --- a/ruby/test/basecamp/download_test.rb +++ b/ruby/test/basecamp/download_test.rb @@ -292,6 +292,41 @@ def test_download_url_hop2_304_is_a_failure_not_a_refused_redirect assert_match(/download failed with status 304/, error.message) end + # SPEC §6: retry_after is populated at every status the header parses at. + # Hop 2 maps its own response rather than going through Http#handle_error, + # so it has to carry the header itself. + def test_download_url_hop2_failure_carries_retry_after + stub_request(:get, "#{base_url}/12345/attachments/abc/download/file.txt") + .with(headers: { "Authorization" => "Bearer #{access_token}" }) + .to_return(status: 302, headers: { "Location" => "https://s3.amazonaws.com/bucket/file" }) + + stub_request(:get, "https://s3.amazonaws.com/bucket/file") + .to_return(status: 503, headers: { "Retry-After" => "7" }) + + error = assert_raises(Basecamp::ApiError) do + @account.download_url("https://3.basecampapi.com/12345/attachments/abc/download/file.txt") + end + + assert_equal 503, error.http_status + assert_equal 7, error.retry_after + end + + def test_download_url_hop2_refused_redirect_carries_retry_after + stub_request(:get, "#{base_url}/12345/attachments/abc/download/file.txt") + .with(headers: { "Authorization" => "Bearer #{access_token}" }) + .to_return(status: 302, headers: { "Location" => "https://s3.amazonaws.com/bucket/file" }) + + stub_request(:get, "https://s3.amazonaws.com/bucket/file") + .to_return(status: 307, headers: { "Location" => "https://elsewhere.example.com/final/file", "Retry-After" => "3" }) + + error = assert_raises(Basecamp::ApiError) do + @account.download_url("https://3.basecampapi.com/12345/attachments/abc/download/file.txt") + end + + assert_equal 307, error.http_status + assert_equal 3, error.retry_after + end + # -- Auth header tests -- def test_download_url_auth_on_api_not_on_s3 diff --git a/ruby/test/basecamp/errors_test.rb b/ruby/test/basecamp/errors_test.rb index 7b6388dbd9..12279a3f29 100644 --- a/ruby/test/basecamp/errors_test.rb +++ b/ruby/test/basecamp/errors_test.rb @@ -348,6 +348,16 @@ def test_error_from_response_429 assert_equal 60, error.retry_after end + def test_error_from_response_carries_retry_after_at_every_status + # SPEC §6: retry_after is populated at every status the header parses at, + # not only at 429 — the public mapper must answer as Http#handle_error does. + [ 400, 401, 403, 404, 422, 429, 500, 502, 503, 504, 507, 418 ].each do |status| + error = Basecamp.error_from_response(status, nil, retry_after: 7) + + assert_equal 7, error.retry_after, "status #{status} dropped retry_after" + end + end + def test_error_from_response_500 error = Basecamp.error_from_response(500, nil) diff --git a/ruby/test/basecamp/http_extended_test.rb b/ruby/test/basecamp/http_extended_test.rb index 66fe5abafa..8477a9093b 100644 --- a/ruby/test/basecamp/http_extended_test.rb +++ b/ruby/test/basecamp/http_extended_test.rb @@ -57,6 +57,12 @@ def test_503_service_unavailable_is_retryable end def test_503_with_retry_after_header + # SPEC §6 "Retry-After Honouring": the header governs the wait at every + # status in the declared retry set, not at 429 alone. Before the gateway + # arm carried retry_after this slept the 0.01s backoff curve instead. + delays = [] + @http.define_singleton_method(:sleep) { |delay| delays << delay } + stub_request(:get, "https://3.basecampapi.com/test.json") .to_return(status: 503, body: "{}", headers: { "Retry-After" => "5" }) .then.to_return(status: 200, body: '{"ok": true}') @@ -64,6 +70,30 @@ def test_503_with_retry_after_header response = @http.get("/test.json") assert_equal 200, response.status + assert_equal [ 5 ], delays + end + + def test_503_error_carries_retry_after + @http.define_singleton_method(:sleep) { |_delay| } + + stub_request(:get, "https://3.basecampapi.com/test.json") + .to_return(status: 503, body: "{}", headers: { "Retry-After" => "7" }) + + error = assert_raises(Basecamp::ApiError) { @http.get("/test.json") } + + assert_equal 503, error.http_status + assert_equal 7, error.retry_after + end + + def test_parse_retry_after_rounds_a_sub_second_remainder_up + # SPEC §6 step 2: 2.75s out is 3 seconds, never 2. Truncation retried up + # to a second before the moment the server named, and turned a remainder + # under a second into 0 — read as "no usable value". + now = Time.utc(2021, 6, 9, 10, 18, 14.25) + + assert_equal 3, @http.send(:parse_retry_after, "Wed, 09 Jun 2021 10:18:17 GMT", now: now) + assert_equal 1, @http.send(:parse_retry_after, "Wed, 09 Jun 2021 10:18:15 GMT", now: now) + assert_nil @http.send(:parse_retry_after, "Wed, 09 Jun 2021 10:18:14 GMT", now: now) end def test_502_bad_gateway_is_retryable diff --git a/swift/Sources/Basecamp/Download.swift b/swift/Sources/Basecamp/Download.swift index 1a216579d3..1bcec26ce9 100644 --- a/swift/Sources/Basecamp/Download.swift +++ b/swift/Sources/Basecamp/Download.swift @@ -49,7 +49,7 @@ extension AccountClient { /// /// The first hop retries under the SPEC §14 policy — network errors plus /// {429, 502, 503, 504}, never 500 — with exponential backoff, honoring - /// `Retry-After` on 429, over a fixed three attempts when + /// `Retry-After` at every status in that set, over a fixed three attempts when /// ``BasecampConfig/enableRetry`` is true and exactly one when it is false. /// Every attempt is authenticated. The second hop is exempt: no retry, and /// no credentials on the signed URL. diff --git a/swift/Sources/Basecamp/HTTP/HTTPClient.swift b/swift/Sources/Basecamp/HTTP/HTTPClient.swift index 4f6202965c..5a5d0b88d2 100644 --- a/swift/Sources/Basecamp/HTTP/HTTPClient.swift +++ b/swift/Sources/Basecamp/HTTP/HTTPClient.swift @@ -33,7 +33,7 @@ package final class HTTPClient: Sendable { /// 500, which stays aligned with the main GET loop's declared-set /// discipline rather than the error taxonomy's broader "all 5xx retryable" /// flag. Backoff is exponential from ``defaultBaseDelayMs``, honoring - /// `Retry-After` on 429. The signed second hop is exempt: no retry, no auth. + /// `Retry-After` at every status in that set. The signed second hop is exempt: no retry, no auth. private static let downloadMaxAttempts = 3 private static let downloadRetryOn: Set = [429, 502, 503, 504] @@ -352,8 +352,7 @@ package final class HTTPClient: Sendable { attempt: attempt, baseDelayMs: effectiveConfig.baseDelayMs, backoff: effectiveConfig.backoff, - retryAfterHeader: httpResponse.value(forHTTPHeaderField: "Retry-After"), - statusCode: statusCode + retryAfterHeader: httpResponse.value(forHTTPHeaderField: "Retry-After") ) let error = BasecampError.fromHTTPResponse( status: statusCode, data: data, @@ -400,8 +399,7 @@ package final class HTTPClient: Sendable { attempt: attempt, baseDelayMs: effectiveConfig.baseDelayMs, backoff: effectiveConfig.backoff, - retryAfterHeader: nil, - statusCode: nil + retryAfterHeader: nil ) directive = .retry(error: error, delaySeconds: delaySeconds) } else { @@ -540,8 +538,7 @@ package final class HTTPClient: Sendable { attempt: attempt, baseDelayMs: Self.defaultBaseDelayMs, backoff: .exponential, - retryAfterHeader: httpResponse.value(forHTTPHeaderField: "Retry-After"), - statusCode: statusCode + retryAfterHeader: httpResponse.value(forHTTPHeaderField: "Retry-After") ) let error = BasecampError.fromHTTPResponse( status: statusCode, data: data, @@ -580,8 +577,7 @@ package final class HTTPClient: Sendable { attempt: attempt, baseDelayMs: Self.defaultBaseDelayMs, backoff: .exponential, - retryAfterHeader: nil, - statusCode: nil + retryAfterHeader: nil ) // SPEC §9: the transport error renders the hop-1 URL (and // any signed query smuggled into it), so onRetry receives @@ -678,11 +674,14 @@ package final class HTTPClient: Sendable { attempt: Int, baseDelayMs: UInt64, backoff: RetryBackoff, - retryAfterHeader: String?, - statusCode: Int? + retryAfterHeader: String? ) -> TimeInterval { - // For 429, respect Retry-After header - if statusCode == 429, let retryAfter = BasecampError.parseRetryAfter(retryAfterHeader) { + // A Retry-After replaces the backoff at EVERY status this is reached + // for — the caller already passed the declared retry set, and SPEC §6 + // "Retry-After Honouring" derives honouring from retry eligibility, not + // from a status list. A `statusCode == 429` gate here left a 503 + // carrying `Retry-After: 120` backing off ~1s. + if let retryAfter = BasecampError.parseRetryAfter(retryAfterHeader) { return TimeInterval(retryAfter) } diff --git a/swift/Tests/BasecampTests/RetryTests.swift b/swift/Tests/BasecampTests/RetryTests.swift index 608ed4ef8f..e1f84753f7 100644 --- a/swift/Tests/BasecampTests/RetryTests.swift +++ b/swift/Tests/BasecampTests/RetryTests.swift @@ -295,6 +295,49 @@ final class RetryTests: XCTestCase { XCTAssertLessThan(elapsed, 5.0, "Retry-After header should override base delay") } + /// SPEC §6 "Retry-After Honouring": the header governs the wait at every + /// status in the declared retry set. A `statusCode == 429` gate left a 503 + /// carrying `Retry-After: 1` on the backoff curve (#775) — here a 10s base, + /// so honouring the header is what keeps this under the 5s ceiling. + func testRetryAfterHonouredAt503() async throws { + let counter = Counter() + let transport = MockTransport { request in + let count = counter.increment() + if count == 1 { + let response = HTTPURLResponse( + url: request.url!, statusCode: 503, + httpVersion: "HTTP/1.1", headerFields: ["Retry-After": "1"] + )! + return (Data(), response) + } else { + 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 account = client.forAccount("999999999") + + let start = CFAbsoluteTimeGetCurrent() + let (_, response) = try await account.httpClient.performRequest( + method: "GET", + url: "https://3.basecampapi.com/999999999/projects.json", + retryConfig: RetryConfig( + maxAttempts: 2, baseDelayMs: 10_000, + backoff: .constant, retryOn: [503] + ) + ) + let elapsed = CFAbsoluteTimeGetCurrent() - start + + XCTAssertEqual(response.statusCode, 200) + XCTAssertEqual(counter.value, 2) + XCTAssertGreaterThanOrEqual(elapsed, 1.0, "Retry-After: 1 on a 503 must be waited") + XCTAssertLessThan(elapsed, 5.0, "Retry-After: 1 on a 503 must replace the 10s base delay") + } + // MARK: - Network Error Triggers Retry func testNetworkErrorTriggersRetry() async throws { diff --git a/typescript/src/download.ts b/typescript/src/download.ts index 30f2014532..742e6c80c6 100644 --- a/typescript/src/download.ts +++ b/typescript/src/download.ts @@ -12,7 +12,7 @@ import { /** * The fixed hop-1 retry policy (SPEC §14): three total attempts when retry is * enabled, retrying network errors plus {429, 502, 503, 504} — never 500 — - * with exponential backoff, honoring Retry-After on 429. DownloadURL is + * with exponential backoff, honoring Retry-After at every status in that set. DownloadURL is * deliberately absent from behavior-model.json, so the policy is passed to * the retry primitive directly rather than looked up by operation. There is * no public knob for the attempt count. diff --git a/typescript/src/errors.ts b/typescript/src/errors.ts index 5e7ca02c02..264f19e45c 100644 --- a/typescript/src/errors.ts +++ b/typescript/src/errors.ts @@ -339,10 +339,15 @@ export async function errorFromResponse( export function errorFromParsedBody( response: Response, body: unknown, - requestId?: string + requestId?: string, + // The parsed Retry-After, when the caller already holds it: a retry loop + // hands over the value that governs the sleep it is about to take, so the + // error §7 step 3i gives on_retry carries that number and not a second + // parse of an HTTP-date, which can round to one second less across a + // whole-second boundary. Absent, the header is parsed here. + retryAfter: number | undefined = parseRetryAfter(response.headers.get("Retry-After")) ): BasecampError { const httpStatus = response.status; - const retryAfter = parseRetryAfter(response.headers.get("Retry-After")); // Try to extract error message from the parsed body. The fallback is the // fixed code-bearing phrase (SPEC §6 step 5), never response.statusText — @@ -384,11 +389,11 @@ export function errorFromParsedBody( switch (httpStatus) { case 401: - return new BasecampError("auth_required", message, { httpStatus, hint, requestId }); + return new BasecampError("auth_required", message, { httpStatus, hint, requestId, retryAfter }); case 403: - return new BasecampError("forbidden", message, { httpStatus, hint, requestId }); + return new BasecampError("forbidden", message, { httpStatus, hint, requestId, retryAfter }); case 404: - return new BasecampError("not_found", message, { httpStatus, hint, requestId }); + return new BasecampError("not_found", message, { httpStatus, hint, requestId, retryAfter }); case 429: return new BasecampError("rate_limit", message, { httpStatus, @@ -398,7 +403,7 @@ export function errorFromParsedBody( requestId, }); case 400: - return new BasecampError("validation", message, { httpStatus, hint, requestId, fieldErrors }); + return new BasecampError("validation", message, { httpStatus, hint, requestId, fieldErrors, retryAfter }); case 422: if (confirmationPeople) { return new PeopleConfirmationRequiredError(message, confirmationPeople, { @@ -406,9 +411,10 @@ export function errorFromParsedBody( hint, requestId, fieldErrors, + retryAfter, }); } - return new BasecampError("validation", message, { httpStatus, hint, requestId, fieldErrors }); + return new BasecampError("validation", message, { httpStatus, hint, requestId, fieldErrors, retryAfter }); case 507: // A 5xx status carrying a client fact: the account is out of storage, or // at its webhook ceiling. Retrying cannot satisfy it, so this must be @@ -418,15 +424,20 @@ export function errorFromParsedBody( retryable: false, hint, requestId, + retryAfter, }); default: // 5xx errors are retryable const retryable = httpStatus >= 500 && httpStatus < 600; + // retryAfter rides along at every status (SPEC §6 "HTTP Status Mapping + // Algorithm"): one parse feeds both the retry loop's sleep and this + // field, so an exhausted 503 reports the wait the origin named. return new BasecampError("api_error", message, { httpStatus, retryable, hint, requestId, + retryAfter, }); } } diff --git a/typescript/src/retry.ts b/typescript/src/retry.ts index 86e6ef028c..5f5ac2244d 100644 --- a/typescript/src/retry.ts +++ b/typescript/src/retry.ts @@ -8,7 +8,7 @@ */ // errors.ts imports nothing, so this edge introduces no cycle. -import { parseRetryAfter } from "./errors.js"; +import { errorFromParsedBody, parseRetryAfter } from "./errors.js"; /** * Retry configuration matching x-basecamp-retry extension schema. @@ -114,7 +114,8 @@ export class TerminalRetryError extends Error { * * `config.maxAttempts` is a total attempt count — the caller passes the * effective budget (e.g. 1 when retry is disabled). Status retry is gated on - * the declared `retryOn` set; 429 honors Retry-After. Transport errors retry + * the declared `retryOn` set, honoring Retry-After at every status in it. + * Transport errors retry * on the same budget, except aborts, which are terminal no matter what the * budget says: a caller cancellation must not re-send, and a request-timeout * budget is shared by every attempt and backoff — once it fires, a retry @@ -179,22 +180,33 @@ export async function executeWithRetry( return response; } - // For 429, respect Retry-After; otherwise back off. The header goes through - // errors.ts's parseRetryAfter — the single SPEC §6 implementation — rather - // than a local parseInt: 0, a negative value and an unparseable one all - // come back undefined and fall through to backoff, where the local copy - // this replaced turned them into a zero or negative sleep. - const retryAfterSeconds = - response.status === 429 - ? parseRetryAfter(response.headers.get("Retry-After")) - : undefined; + // A Retry-After the origin sent replaces the backoff at EVERY status this + // branch reaches — the status already passed the declared retryOn gate, + // and SPEC §6 "Retry-After Honouring" derives honouring from retry + // eligibility rather than from a status list (a 429-only gate here left a + // 503 carrying `Retry-After: 120` backing off ~1s). The header goes + // through errors.ts's parseRetryAfter — the single SPEC §6 implementation + // — rather than a local parseInt: 0, a negative value and an unparseable + // one all come back undefined and fall through to backoff, where the local + // copy this replaced turned them into a zero or negative sleep. + const retryAfterSeconds = parseRetryAfter(response.headers.get("Retry-After")); const delay = retryAfterSeconds !== undefined ? timerSafeDelayMs(retryAfterSeconds) : calculateBackoffDelay(config, attempt - 1); - const statusError = new Error( - `HTTP ${response.status}: ${response.statusText || "Request failed"}`, + // SPEC §7 step 3i: the error handed to onRetry is the status-mapped + // BasecampError, so a hook sees the same httpStatus and retryAfter the + // terminal error would carry — the parsed value that governs this very + // sleep, handed over rather than parsed again, so an HTTP-date crossing + // a second boundary between the two cannot make the hook's number + // differ from the wait. Built from the status and headers alone: the + // body is being discarded below, and the mapper needs none of it. + const statusError = errorFromParsedBody( + response, + null, + response.headers.get("X-Request-Id") ?? undefined, + retryAfterSeconds, ); // End the failed attempt before sleeping, so a slow backoff cannot leave diff --git a/typescript/src/services/base.ts b/typescript/src/services/base.ts index 345514d019..7171880430 100644 --- a/typescript/src/services/base.ts +++ b/typescript/src/services/base.ts @@ -286,14 +286,14 @@ export abstract class BaseService { // Drain response body before retry to free resources and enable connection reuse response.body?.cancel(); - // Backoff before retry. The header goes through errors.ts's - // parseRetryAfter — the single SPEC §6 implementation — rather than a - // local parseInt: the copy this replaced had no HTTP-date branch and - // guarded with `>= 0`, so it honoured `Retry-After: 0` as a - // zero-millisecond delay and retried with no wait at all. - const retryAfterSeconds = response.status === 429 - ? parseRetryAfter(response.headers.get("Retry-After")) - : undefined; + // Backoff before retry. A Retry-After replaces the curve at every + // status this branch reaches (SPEC §6 "Retry-After Honouring"), and + // the header goes through errors.ts's parseRetryAfter — the single + // SPEC §6 implementation — rather than a local parseInt: the copy this + // replaced had no HTTP-date branch and guarded with `>= 0`, so it + // honoured `Retry-After: 0` as a zero-millisecond delay and retried + // with no wait at all. + const retryAfterSeconds = parseRetryAfter(response.headers.get("Retry-After")); // The locally-computed term is bounded by SPEC §7's ceiling; the // server-directed Retry-After is not, per the same section. const delay = retryAfterSeconds !== undefined @@ -301,7 +301,15 @@ export abstract class BaseService { : saturatingBackoff(retryConfig.baseDelayMs ?? 1000, "exponential", attempt); try { - const retryError = new Error(`${response.status} ${response.statusText}`); + // SPEC §7 step 3i: the status-mapped error, carrying the parsed + // retryAfter that governs this sleep — the same value, not a + // second parse — rather than a bare Error. + const retryError = errorFromParsedBody( + response, + null, + response.headers.get("X-Request-Id") ?? undefined, + retryAfterSeconds, + ); // SPEC section 7: RequestInfo.attempt is the attempt that just failed // (1-based), while the standalone argument is the UPCOMING attempt. this.hooks?.onRetry?.( diff --git a/typescript/tests/errors.test.ts b/typescript/tests/errors.test.ts index ea764dbb7d..c976057b00 100644 --- a/typescript/tests/errors.test.ts +++ b/typescript/tests/errors.test.ts @@ -877,3 +877,18 @@ describe("row-keyed error bodies (SPEC §6 step 1b)", () => { expect(error.fieldErrors).toBeUndefined(); }); }); + +describe("retryAfter at every status (SPEC §6 HTTP Status Mapping Algorithm)", () => { + it("carries a parsed Retry-After on a 503 api_error, not only on 429", () => { + const response = new Response(null, { status: 503, headers: { "Retry-After": "7" } }); + const error = errorFromParsedBody(response, { error: "Service Unavailable" }); + + expect(error.code).toBe("api_error"); + expect(error.retryAfter).toBe(7); + }); + + it("leaves retryAfter undefined when the header is absent", () => { + const error = errorFromParsedBody(new Response(null, { status: 503 }), null); + expect(error.retryAfter).toBeUndefined(); + }); +}); diff --git a/typescript/tests/retry-after.test.ts b/typescript/tests/retry-after.test.ts index dfc9b0ad22..753baeb056 100644 --- a/typescript/tests/retry-after.test.ts +++ b/typescript/tests/retry-after.test.ts @@ -280,10 +280,28 @@ describe("the shared retry loop honours the parsed value", () => { expect(timerSafeDelayMs(error.retryAfter!)).toBeLessThanOrEqual(2_147_483_647); }); - it("ignores Retry-After on a status that is not 429", async () => { - // Which statuses honour the header is divergent across the six SDKs and is - // tracked in #775; this pins TypeScript's current position so a parsing - // change cannot move it by accident. + /** + * SPEC §7 step 3i: the error on_retry receives carries the retryAfter that + * governs the sleep. The loop parses the header once for the delay and + * hands that value to the mapper; a mapper that parsed the header a second + * time could round an HTTP-date to one second less across a whole-second + * boundary, and the hook would then report a shorter wait than the loop + * takes. + */ + it("carries the caller's parsed retryAfter instead of parsing the header again", () => { + const response = new Response(null, { + status: 503, + headers: { "Retry-After": "3" }, + }); + expect(errorFromParsedBody(response, null, undefined, 7).retryAfter).toBe(7); + // Absent, the header is parsed here, as before. + expect(errorFromParsedBody(response, null).retryAfter).toBe(3); + }); + + it("honours Retry-After on 503, not only on 429", async () => { + // SPEC §6 "Retry-After Honouring": the header governs the wait at every + // status in the declared retryOn set. A `status === 429` ternary here left + // a 503 carrying `Retry-After: 120` on the ~1s backoff curve (#775). const controller = new AbortController(); let chosen = Number.NaN; const emit: RetryEmit = { @@ -304,7 +322,6 @@ describe("the shared retry loop honours the parsed value", () => { ), ).rejects.toThrow("delay captured"); - expect(chosen).toBeGreaterThanOrEqual(BACKOFF_MIN_MS); - expect(chosen).toBeLessThanOrEqual(BACKOFF_MAX_MS); + expect(chosen).toBe(120_000); }); });