diff --git a/MIGRATING.md b/MIGRATING.md index 9b64903ea..1b24efeaf 100644 --- a/MIGRATING.md +++ b/MIGRATING.md @@ -230,6 +230,68 @@ is always made. Only a negative cap now panics, with a new message: (a `recover` that inspects the message, as the conformance runner did) needs updating; code that simply never passed `0` is unaffected. +### Go: `Error` gained `RetryAfter` mid-struct, and raw GETs now sleep it (#795) + +**The compile error, if you get one:** an unkeyed composite literal +`basecamp.Error{code, msg, hint, fieldErrors, status, retryable, reqID, cause}` +no longer compiles — the field is inserted between `Retryable` and `RequestID`, +so the eight-value form is now too short. Same break `FieldErrors` caused in +#541 (below, under Go → Behavioural); the remedy is the same, and permanent: +use keyed fields. + +```go +// before +err := &basecamp.Error{basecamp.CodeRateLimit, "Rate limited", "", nil, 429, true, "", nil} +// after — and it will not break again +err := &basecamp.Error{Code: basecamp.CodeRateLimit, Message: "Rate limited", HTTPStatus: 429, Retryable: true} +``` + +`apidiff` reports this as compatible, and it is right about what it measures: +the field is additive to the *exported API surface*. Unkeyed literals are a +source-compatibility hazard the tool does not model, which is why this note +exists rather than the gate catching it. + +**The behaviour change:** `Client.Get`/`GetAll` and the raw escape hatch used to +back off on their own local curve after a 429 even when the server named a +delay, because the loop read that delay off a type nothing in the package ever +constructed. They now sleep the server's `Retry-After` — both wire forms, +delta-seconds and HTTP-date — in place of the backoff, with no jitter and no +ceiling beyond what the host can represent: a value the parser holds but the +host cannot schedule saturates at 2147483647 seconds (~68 years) rather than +wrapping negative, and that figure does not vary by architecture. A value too +large for the parser's own `int64` is treated as malformed instead and falls +through to the backoff curve, as it always did. That split is Go's: SPEC §6's +parsing algorithm says only to parse a positive integer, #793 states the +two-tier rule (unrepresentable → malformed, unschedulable → saturate) in §6 +"Retry-After Honouring", and #799 tracks the cross-SDK convergence on +over-range values, which the six SDKs still answer differently. + +**Two behaviours changed for `DownloadURL` and the rate-limiter hook as well**, +because all three paths share `parseRetryAfter`: an HTTP-date's sub-second +remainder now rounds up instead of truncating, so a date less than a second +away yields a one-second wait where it used to yield "no value" and fall onto +the backoff curve; and a delta-seconds above the schedulable ceiling saturates +instead of wrapping. The wire operations those paths perform are unchanged, and they +already honoured the header on 429 — it is what the header parses to that +moved. Typed service methods run the generated retry loop, which has its own +copy of the parse and is untouched (#798). + +**Wrong behaviour you get if you ignore it:** none, but the wait between +attempts on a throttled account can now be seconds or minutes where it used to +be milliseconds, so a caller that sized a `context` timeout against the old +backoff may now hit it. The wait observes cancellation — the loop selects on +`ctx.Done()` — so cancelling is the escape. If you would rather reschedule the +work yourself, the `*Error` carries `RetryAfter` — but the loop wraps it with +`fmt.Errorf` on exhaustion, even at a cap of one attempt, so a type assertion +on the returned error fails. Extract it with `errors.As`: + +```go +var apiErr *basecamp.Error +if errors.As(err, &apiErr) && apiErr.RetryAfter > 0 { + // reschedule after apiErr.RetryAfter seconds +} +``` + ### Kotlin: `search.search` returns `ListResult`, not `ListResult` (#717) ```kotlin diff --git a/SPEC.md b/SPEC.md index d63c23c93..49f4605c7 100644 --- a/SPEC.md +++ b/SPEC.md @@ -530,7 +530,7 @@ RECORD BasecampError extends Error END ``` -**Go divergence:** Go's `Error` struct omits `retry_after`; retry delay is tracked on `RequestResult` instead. Go also exposes a `Cause` field (the underlying error) not present in this canonical RECORD — a language-specific extension. +**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. ### Error Code Table @@ -630,9 +630,15 @@ Swift carries the slot as a fifth associated value on `.validation` plus a `fiel Given header value `value`: 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; if > 0 → return. +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). +Step 2's rounding 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. + 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. @@ -854,8 +860,8 @@ width is unreadable without it, not as a live claim: TypeScript rejects above 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 moves the hand-written parser -to `ParseInt` into `int64` `[PENDING #796]`; the generated one stays `Atoi` until #798.)* +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. @@ -877,10 +883,9 @@ every supported `GOARCH` can represent, and the same 2,147,483,647 §16 already 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`. `[PENDING #796: that is what #796 ships — `ParseInt` into `int64`, over-range -malformed, 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 — and it merges after this PR. Until it -lands, the hand-written path is unclamped and this paragraph describes the contract, not the tree.]` +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* @@ -1133,9 +1138,9 @@ Requirements: 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 in #796 `[PENDING #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 + 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 diff --git a/go/pkg/basecamp/client.go b/go/pkg/basecamp/client.go index 7d76f23ed..dccc57485 100644 --- a/go/pkg/basecamp/client.go +++ b/go/pkg/basecamp/client.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "log/slog" + "math" "math/rand" "net/http" "net/url" @@ -691,20 +692,22 @@ func (c *Client) doRequestURL(ctx context.Context, method, url string, body any) // Check for retryable error with server-specified delay var delay time.Duration - if re, ok := err.(*retryableError); ok { - lastErr = re.err - if re.retryAfter > 0 { - // Use server-specified Retry-After delay - delay = re.retryAfter - } else { - delay = c.backoffDelay(attempt) - } - } else if apiErr, ok := err.(*Error); ok { + if apiErr, ok := err.(*Error); ok { if !apiErr.Retryable { return nil, err } lastErr = err - delay = c.backoffDelay(attempt) + // 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. + if apiErr.RetryAfter > 0 { + delay = time.Duration(apiErr.RetryAfter) * time.Second + } else { + delay = c.backoffDelay(attempt) + } } else { return nil, err } @@ -722,6 +725,21 @@ func (c *Client) doRequestURL(ctx context.Context, method, url string, body any) info := RequestInfo{Method: method, URL: url, Attempt: attempt} c.hooks.OnRetry(ctx, info, attempt+1, lastErr) + // Cancellation must win over the wait. A server-specified Retry-After + // has no policy ceiling by design — only the ~68-year representability + // clamp — so an uninterruptible sleep would let a server pin a request + // the caller already abandoned open for as long as it liked. + // + // The select alone cannot promise that. When both cases are ready Go + // picks one pseudo-randomly, and OnRetry has just run: a hook that + // cancels there, with a delay that has already elapsed, would see the + // timer win and one more request go out on a dead context — failing + // fast, but as the transport's wrapping of context.Canceled rather + // than ctx.Err(). So the context is checked first, where nothing + // competes with it. + if err := ctx.Err(); err != nil { + return nil, err + } select { case <-ctx.Done(): return nil, ctx.Err() @@ -1069,22 +1087,139 @@ func parseNextLink(linkHeader string) string { return "" } +// maxRetryAfterSeconds is the largest delta-seconds value this SDK honours: +// 2147483647, ~68 years. It is a REPRESENTABILITY bound taken at the portable +// limit, not a policy ceiling — SPEC §7's "Retry-After is exempt" note already +// carves out exactly this ("implementations may still bound it against host +// limits"), and Swift clamps its own seconds→nanoseconds conversion for the +// same class of reason. Deciding a *policy* cap on server-directed waits is +// #793's, and this is not one: at ~68 years, nothing a server could sensibly +// ask for is affected. (Kotlin uses this same number, but only in its +// HTTP-date branch — its integer branch rejects instead, so it is not the +// precedent an earlier draft of this comment cited it as.) +// +// Two host limits sit above it and this is at or below both, which is why the +// answer does not depend on the word size: `seconds × time.Second` wraps past +// math.MaxInt64 above ~292 years, and RetryAfter is an `int`, which is 32 bits +// on a 32-bit target. Taking the smaller keeps one documented number for every +// platform — the same 2147483647 SPEC §16 already names as a shared cross-SDK +// ceiling — instead of an answer a reader has to compute from GOARCH. +const maxRetryAfterSeconds = math.MaxInt32 + +// clampRetryAfterSeconds normalizes a delta-seconds value to the range Error's +// RetryAfter field promises: non-negative, and small enough that the retry +// loops' `time.Duration(n) * time.Second` stays positive on every target. +// +// The failure this closes is not hypothetical arithmetic. `Retry-After: +// 9223372036854775807` parses cleanly on a 64-bit build, and the product wraps +// to -1s; `time.After` on a non-positive duration fires immediately, so a +// server-directed wait becomes a tight retry loop — the opposite of what the +// header asked for. +// +// Over-range clamps rather than falling back to "absent" because clamping is +// what honours the server: falling back would compute the millisecond backoff +// curve instead and hammer a peer that just asked for a long wait, which is the +// same tight loop by another route. The wait is a select on ctx.Done(), so an +// absurd clamped delay is abandonable rather than a hang. +// +// It takes an int64 because both callers have one — the date branch derives +// seconds from a time.Duration, and the delta-seconds branch parses into 64 +// bits so the answer cannot depend on the word size. A single comparison +// against a bound below every target's int range is also what makes the +// narrowing legible to CodeQL's go/incorrect-integer-conversion, which reads +// the guard rather than the arithmetic and flagged the previous spelling. +func clampRetryAfterSeconds(seconds int64) int { + if seconds <= 0 { + return 0 + } + if seconds > maxRetryAfterSeconds { + return maxRetryAfterSeconds + } + return int(seconds) +} + +// isDelaySeconds reports whether the value is RFC 9110's `1*DIGIT` and nothing +// else: no sign, no space, no separator, no decimal point. +func isDelaySeconds(value string) bool { + if value == "" { + return false + } + for _, r := range value { + if r < '0' || r > '9' { + return false + } + } + return true +} + // 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. +// Returns 0 if the header is empty or cannot be parsed, and clamps a parsed +// value to maxRetryAfterSeconds, the portable ceiling — every caller +// multiplies the result by time.Second, and the product must stay positive on +// every target. func parseRetryAfter(header string) int { if header == "" { return 0 } - // Try parsing as seconds (integer) - if seconds, err := strconv.Atoi(header); err == nil && seconds > 0 { - return seconds + // 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. + // + // 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. + if isDelaySeconds(header) { + if seconds, err := strconv.ParseInt(header, 10, 64); err == nil && seconds > 0 { + return clampRetryAfterSeconds(seconds) + } } // Try parsing as HTTP-date (e.g., "Wed, 21 Oct 2015 07:28:00 GMT") if t, err := http.ParseTime(header); err == nil { - seconds := int(time.Until(t).Seconds()) - if seconds > 0 { - return seconds + // Whole seconds by integer division of the Duration, NOT via + // `int(d.Seconds())`. That spelling narrows a float64 to an int, and + // Go leaves an out-of-range float→int conversion implementation- + // defined: where int is 32 bits, a date more than ~68 years out (and + // time.Until saturates at ~292 for anything further, including the + // year-9999 dates a server can legally send) produced a garbage value + // that read as non-positive, so the header was discarded and the loop + // fell back to its millisecond backoff — the one outcome this clamp + // exists to avoid. Integer division cannot leave the Duration's range, + // and clampRetryAfterSeconds bounds the result by int's. + // + // Rounded UP, matching Kotlin's `(remainingMs + 999) / 1000`, Swift's + // `.rounded(.up)` and TypeScript's `Math.ceil`, whose shared reason is + // that truncating a sub-second remainder toward zero turns the + // shortest honoured delay into "retry immediately": a date 400ms out + // became 0, was read as "no delay", and fell onto the backoff curve. + // Rounding up also never retries before the moment the server named. + // Python and Ruby still truncate; that half of the divergence is #799. + if remaining := time.Until(t); remaining > 0 { + seconds := int64(remaining / time.Second) + if remaining%time.Second != 0 { + seconds++ + } + return clampRetryAfterSeconds(seconds) } } return 0 diff --git a/go/pkg/basecamp/client_retry_after_test.go b/go/pkg/basecamp/client_retry_after_test.go new file mode 100644 index 000000000..0032850c9 --- /dev/null +++ b/go/pkg/basecamp/client_retry_after_test.go @@ -0,0 +1,504 @@ +package basecamp + +import ( + "context" + "errors" + "log/slog" + "math" + "net/http" + "net/http/httptest" + "sync" + "sync/atomic" + "testing" + "time" +) + +// The raw GET retry loop computes a delay, logs it, fires OnRetry, and only +// then sleeps. Those first three are enough to pin the delay exactly, so the +// tests below never spend one — no seam in production code, no wall clock, and +// no dependence on how long anything took. Measuring elapsed time is the test +// shape this repo is retiring (#783), and it is the wrong instrument anyway: +// the question is whether the loop chose the server's number or its own backoff +// curve, which is an equality, not a range. + +// delayRecorder reads the computed delay off the loop's own "retrying request" +// log record — a structured attribute the SDK already emits to any logger a +// caller installs. +type delayRecorder struct { + mu sync.Mutex + delays []time.Duration +} + +func (d *delayRecorder) Enabled(context.Context, slog.Level) bool { return true } +func (d *delayRecorder) WithAttrs([]slog.Attr) slog.Handler { return d } +func (d *delayRecorder) WithGroup(string) slog.Handler { return d } + +func (d *delayRecorder) Handle(_ context.Context, r slog.Record) error { + if r.Message != "retrying request" { + return nil + } + r.Attrs(func(a slog.Attr) bool { + if a.Key == "delay" { + if delay, ok := a.Value.Any().(time.Duration); ok { + d.mu.Lock() + d.delays = append(d.delays, delay) + d.mu.Unlock() + } + } + return true + }) + return nil +} + +func (d *delayRecorder) snapshot() []time.Duration { + d.mu.Lock() + defer d.mu.Unlock() + return append([]time.Duration(nil), d.delays...) +} + +// cancelOnRetryHooks cancels the request at the one instant that makes these +// tests free: the loop fires OnRetry with the delay already computed and +// logged, and the very next thing it does is wait it out. Cancelling here means +// the wait returns at once however long the delay was. +// +// It also counts the requests the transport was handed, which is how the +// cancellation-ordering test below tells "returned at the wait" from "went +// round once more on a dead context". +type cancelOnRetryHooks struct { + NoopHooks + cancel context.CancelFunc + requests atomic.Int32 +} + +func (h *cancelOnRetryHooks) OnRetry(context.Context, RequestInfo, int, error) { + h.cancel() +} + +func (h *cancelOnRetryHooks) OnRequestStart(ctx context.Context, _ RequestInfo) context.Context { + h.requests.Add(1) + return ctx +} + +// retryAfterProbe drives one GET against a handler, cancelling at the retry +// boundary, and returns the delays the loop computed plus the resulting error. +// The client's backoff curve is set to milliseconds, so any delay at or above a +// second can only have come from the Retry-After header. +func retryAfterProbe(t *testing.T, handler http.HandlerFunc) ([]time.Duration, error) { + t.Helper() + server := httptest.NewServer(handler) + defer server.Close() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + delays := &delayRecorder{} + client := NewClient(&Config{BaseURL: server.URL, CacheEnabled: false}, &StaticTokenProvider{Token: "test-token"}) + client.httpOpts.MaxRetries = 3 + client.httpOpts.BaseDelay = time.Millisecond + client.httpOpts.MaxJitter = time.Millisecond + client.logger = slog.New(delays) + client.hooks = &cancelOnRetryHooks{cancel: cancel} + + // Not the assertion — the assertions are all exact equalities below. This + // is the guard that keeps the whole file honest: if the loop's wait ever + // stopped observing cancellation, every case here would spend its full + // Retry-After instead of returning at once, and this says so in one line + // rather than hanging until the package test timeout. + // + // Get runs in a goroutine so the guard can actually fire (review + // follow-up, Copilot). Called inline, a wait that ignored cancellation + // would simply never return, and an elapsed-time check placed after it is + // unreachable — most obviously now that an over-range Retry-After + // saturates at ~68 years rather than wrapping to a negative delay. The + // abandoned goroutine outlives the test; that is the cost of reporting a + // hang instead of becoming one. + type outcome struct { + delays []time.Duration + err error + } + done := make(chan outcome, 1) + go func() { + _, err := client.Get(ctx, "/test.json") + done <- outcome{delays: delays.snapshot(), err: err} + }() + + select { + case got := <-done: + return got.delays, got.err + case <-time.After(5 * time.Second): + t.Fatalf("Get had not returned 5s after cancellation at the retry boundary; "+ + "the retry wait is not interruptible (delays computed so far: %v)", delays.snapshot()) + return nil, nil + } +} + +func rateLimited(retryAfter string) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + if retryAfter != "" { + w.Header().Set("Retry-After", retryAfter) + } + w.WriteHeader(http.StatusTooManyRequests) + } +} + +// TestClient_RetryAfterReplacesBackoff is the regression test for the raw GET +// retry loop silently discarding a server-specified Retry-After. The loop used +// to read the delay off a *retryableError that nothing in the package ever +// constructed, so the branch was unreachable and every 429 fell through to the +// backoff curve. Against that code this observes ~1ms, wants 2s, and fails. +func TestClient_RetryAfterReplacesBackoff(t *testing.T) { + delays, err := retryAfterProbe(t, rateLimited("2")) + + if !errors.Is(err, context.Canceled) { + t.Fatalf("Get returned %v, want context.Canceled", err) + } + if len(delays) != 1 { + t.Fatalf("loop computed %d delays (%v), want exactly 1", len(delays), delays) + } + if delays[0] != 2*time.Second { + t.Errorf("computed a %v retry delay, want 2s from the Retry-After header "+ + "(the backoff curve here is ~1ms, so this is the backoff, not the server's number)", delays[0]) + } +} + +// TestClient_RetryAfterHTTPDateReplacesBackoff covers the header's other wire +// form. parseRetryAfter resolves it to whole seconds relative to now, so the +// assertion is a tight window rather than an equality — but one nowhere near +// the millisecond backoff it has to be told apart from. +func TestClient_RetryAfterHTTPDateReplacesBackoff(t *testing.T) { + future := time.Now().Add(90 * time.Second).UTC().Format(http.TimeFormat) + delays, err := retryAfterProbe(t, rateLimited(future)) + + if !errors.Is(err, context.Canceled) { + t.Fatalf("Get returned %v, want context.Canceled", err) + } + if len(delays) != 1 { + t.Fatalf("loop computed %d delays (%v), want exactly 1", len(delays), delays) + } + if delays[0] < 80*time.Second || delays[0] > 90*time.Second { + t.Errorf("computed a %v retry delay for an HTTP-date 90s out, want ~90s", delays[0]) + } +} + +// TestClient_RetryAfterAbsentOrUnusableKeepsBackoff is the other half of the +// contract, and the half that stops the tests above from passing for the wrong +// reason: a "fix" that always slept a whole number of seconds would satisfy +// them. Only a header the server actually sent, and that SPEC §6 accepts, may +// displace the curve. +func TestClient_RetryAfterAbsentOrUnusableKeepsBackoff(t *testing.T) { + for _, tc := range []struct { + name string + header string + }{ + {"absent", ""}, + {"unparseable", "sometime next week"}, + {"partly numeric", "120junk"}, + {"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 + // kills that guard; the over-range signed forms would be refused by + // the parse anyway, so they would not. + {"signed", "+5"}, + } { + t.Run(tc.name, func(t *testing.T) { + delays, err := retryAfterProbe(t, rateLimited(tc.header)) + + if !errors.Is(err, context.Canceled) { + t.Fatalf("Get returned %v, want context.Canceled", err) + } + if len(delays) != 1 { + t.Fatalf("loop computed %d delays (%v), want exactly 1", len(delays), delays) + } + // backoffDelay(1) is BaseDelay + [0, MaxJitter), both 1ms here. + if delays[0] < time.Millisecond || delays[0] >= 2*time.Millisecond { + t.Errorf("computed a %v retry delay, want the backoff curve's [1ms, 2ms) — "+ + "an unusable Retry-After must not displace it", delays[0]) + } + }) + } +} + +// TestClient_RetryAfterSaturatesAtTheHonouredCeiling is the regression test for a +// server turning the retry loop into a tight loop with a syntactically valid +// header. time.Duration counts nanoseconds in an int64, so an unclamped +// `Retry-After: 9223372036854775807` multiplied by time.Second wraps to -1s, +// and time.After on a non-positive duration fires immediately: the loop would +// burn its whole attempt budget back-to-back against a peer that just asked it +// to wait. Against the unclamped code this observes -1s and fails. +// +// 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. +func TestClient_RetryAfterSaturatesAtTheHonouredCeiling(t *testing.T) { + assertSaturatedRetryAfter(t, "9223372036854775807") +} + +func assertSaturatedRetryAfter(t *testing.T, header string) { + t.Helper() + delays, err := retryAfterProbe(t, rateLimited(header)) + + if !errors.Is(err, context.Canceled) { + t.Fatalf("Get returned %v, want context.Canceled", err) + } + if len(delays) != 1 { + t.Fatalf("loop computed %d delays (%v), want exactly 1", len(delays), delays) + } + if delays[0] <= 0 { + t.Fatalf("computed a %v retry delay from an over-range Retry-After; a non-positive "+ + "delay makes time.After fire at once, so the server-directed wait becomes a tight loop", delays[0]) + } + if want := time.Duration(maxRetryAfterSeconds) * time.Second; delays[0] != want { + t.Errorf("computed a %v retry delay, want %v — an over-range Retry-After saturates at "+ + "the honoured ceiling rather than falling back to the ~1ms backoff curve", delays[0], want) + } +} + +// TestClient_RetryWaitChecksCancellationBeforeTheTimer pins the order in which +// the wait observes its two inputs. A select with both cases ready picks one +// pseudo-randomly — Go's rule, not a defect — so "cancellation wins" is not a +// property the select alone can promise. The loop fires OnRetry and then +// waits; a hook that cancels there, with a delay that has already elapsed, +// would see the timer win about half the time and send one more request on a +// context the caller had already abandoned. That request fails fast, but it is +// a request the caller cancelled, and what comes back is the transport's +// wrapping of context.Canceled rather than ctx.Err() itself (review follow-up, +// Copilot). The loop therefore checks ctx.Err() before it enters the select. +// +// Stated honestly: the interleaving cannot be forced, because pseudo-random +// means exactly that. What CAN be forced is the input — the context cancelled +// before the wait and the delay already elapsed — so the contract is asserted +// on every one of N runs: the loop returns ctx.Err() itself, having handed the +// transport nothing further. Against the bare select this fails on the first +// run the timer wins, which is all but certain over 64 (each run is a coin +// flip, so the chance of a false pass is 2^-64); against the guard it cannot +// fail, because the guard runs before there is anything to pick between. +func TestClient_RetryWaitChecksCancellationBeforeTheTimer(t *testing.T) { + server := httptest.NewServer(rateLimited("")) + defer server.Close() + + const runs = 64 + for run := range runs { + ctx, cancel := context.WithCancel(context.Background()) + hooks := &cancelOnRetryHooks{cancel: cancel} + client := NewClient(&Config{BaseURL: server.URL, CacheEnabled: false}, &StaticTokenProvider{Token: "test-token"}) + client.httpOpts.MaxRetries = 3 + // A zero backoff is the cheapest way to a timer that is already ready + // when the wait begins. It stands in for any hook that outlasts the + // computed delay — a millisecond curve and a hook that logs, say — and + // costs no wall clock. (Jitter stays at one nanosecond because a zero + // bound is not a valid argument to rand.Int63n; it contributes 0.) + client.httpOpts.BaseDelay = 0 + client.httpOpts.MaxJitter = time.Nanosecond + client.hooks = hooks + + _, err := client.Get(ctx, "/test.json") + cancel() + + // Identity, not errors.Is: the transport's error for a request sent on + // a cancelled context also satisfies errors.Is(err, context.Canceled), + // and telling the two apart is the point. + if err != context.Canceled { + t.Fatalf("run %d: Get returned %v, want ctx.Err() itself (context.Canceled) — "+ + "the loop went round again after the caller cancelled at the retry boundary", run, err) + } + if got := hooks.requests.Load(); got != 1 { + t.Fatalf("run %d: the transport was handed %d requests, want 1 — "+ + "a cancellation delivered before the wait must not be followed by another attempt", run, got) + } + } +} + +// TestParseRetryAfter_FarFutureHTTPDateSaturates covers the header's other wire +// form at the same boundary. A server may legally name a date beyond anything a +// Duration can hold — RFC 7231 puts no bound on it, and year-9999 dates are +// real — for which time.Until saturates at ~292 years. That must saturate the +// parsed delay too, not be discarded onto the millisecond backoff curve. +// +// Stated honestly (review follow-up, Codex/Copilot): the defect this guards is +// 32-bit-only and therefore cannot be executed here. `int(d.Seconds())` narrows +// a float64, and Go leaves an out-of-range float→int conversion +// implementation-defined; where int is 32 bits that produced a non-positive +// value and the header was dropped. The remedy is to remove the narrowing — +// whole seconds now come from integer division in the Duration domain — so this +// pins the resulting contract on every platform rather than proving the fix on +// the one that had the bug. +func TestParseRetryAfter_FarFutureHTTPDateSaturates(t *testing.T) { + seconds := parseRetryAfter("Fri, 31 Dec 9999 23:59:59 GMT") + + if seconds <= 0 { + t.Fatalf("parseRetryAfter(a year-9999 HTTP-date) = %d, want a positive saturated delay — "+ + "a non-positive result reads as 'no delay' and drops the server's wait onto the backoff curve", seconds) + } + // Equality, not an upper bound (review follow-up, Copilot): `<= ceiling` + // is satisfied by any positive number, including a parser that returned + // an arbitrary "safe" delay instead of saturating. time.Until saturates + // at the Duration maximum for a year-9999 date, which is far past the + // ceiling, so the saturated answer is exact and deterministic. + if seconds != maxRetryAfterSeconds { + t.Errorf("parseRetryAfter(a year-9999 HTTP-date) = %d, want exactly %d — the far-future "+ + "date must saturate at the honoured ceiling, not land somewhere below it", seconds, maxRetryAfterSeconds) + } + if delay := time.Duration(seconds) * time.Second; delay <= 0 { + t.Errorf("time.Duration(%d) * time.Second = %v, want a positive duration", seconds, delay) + } +} + +// TestParseRetryAfter_HTTPDateRoundsUp covers the other end of the same branch. +// A remainder truncated toward zero turns the shortest honoured delay into "no +// delay at all" — a date under a second out becomes 0, which every caller reads +// as absent, so the request falls onto the millisecond backoff curve instead of +// waiting. It also retries before the moment the server named. TypeScript +// (`Math.ceil`), Kotlin (`(remainingMs + 999) / 1000`) and Swift +// (`.rounded(.up)`) all round up for those two reasons; Go now joins them, and +// SPEC §6 step 2 says so. Python and Ruby still truncate (#799). +// +// The assertion is ONE-SIDED, which is what keeps it out of #783's territory +// (review follow-up, Codex). An equality against a literal cannot work here: +// the wire form carries whole seconds, so the remaining time is +// `offset - frac(now) - scheduling delay`, and the room before the answer drops +// by one is `1 - frac(now)` — arbitrarily small, entirely at the clock's +// discretion. Instead the bound is re-measured AFTER the parse, making it a +// lower bound on what the parser itself saw: scheduling delay can only weaken +// this toward vacuity, never turn it red. +func TestParseRetryAfter_HTTPDateRoundsUp(t *testing.T) { + // The next whole second is the shortest future date the wire form can + // express, and the case with the widest gap between the two roundings: + // under a second remains, which truncation reports as 0 — discarded — and + // rounding up reports as 1. + target := time.Now().Truncate(time.Second).Add(time.Second) + + seconds := parseRetryAfter(target.UTC().Format(http.TimeFormat)) + + remaining := time.Until(target) + if remaining <= 0 { + // The target second passed between formatting and measuring, so there + // is nothing left to be right or wrong about: a parser returning 0 for + // a past date is correct, and asserting anything here would be + // asserting the scheduler. + t.Skip("the target second elapsed during the test; nothing to assert") + } + if delay := time.Duration(seconds) * time.Second; delay < remaining { + t.Errorf("parseRetryAfter(a date %v away) = %ds, shorter than the wait the server named — "+ + "truncating a sub-second remainder yields 0, which reads as absent and drops the "+ + "request onto the backoff curve; any truncation retries early", remaining, seconds) + } +} + +// TestErrRateLimit_NormalizesRetryAfter covers the one door onto Error.RetryAfter +// the wire parser does not guard: an exported constructor taking a bare int. +// The field documents zero as "no delay", so a negative argument must not reach +// it — and the hint, which has always read non-positive as absent, must keep +// saying the same thing the field does. +func TestErrRateLimit_NormalizesRetryAfter(t *testing.T) { + for _, tc := range []struct { + name string + retryAfter int + want int + wantHint string + }{ + {"positive", 42, 42, "Try again in 42 seconds"}, + {"zero", 0, 0, "Try again later"}, + {"negative", -5, 0, "Try again later"}, + } { + t.Run(tc.name, func(t *testing.T) { + err := ErrRateLimit(tc.retryAfter) + if err.RetryAfter != tc.want { + t.Errorf("ErrRateLimit(%d).RetryAfter = %d, want %d", tc.retryAfter, err.RetryAfter, tc.want) + } + if err.Hint != tc.wantHint { + t.Errorf("ErrRateLimit(%d).Hint = %q, want %q", tc.retryAfter, err.Hint, tc.wantHint) + } + }) + } + + t.Run("beyond the honoured ceiling", func(t *testing.T) { + err := ErrRateLimit(math.MaxInt) + want := maxRetryAfterSeconds + if err.RetryAfter != want { + t.Errorf("ErrRateLimit(math.MaxInt).RetryAfter = %d, want %d (saturated, so the "+ + "retry loop's seconds→Duration conversion stays positive)", err.RetryAfter, want) + } + if delay := time.Duration(err.RetryAfter) * time.Second; delay <= 0 { + t.Errorf("time.Duration(RetryAfter) * time.Second = %v, want a positive duration", delay) + } + }) +} + +// TestClient_RetryAfterErrorCarriesSeconds pins the value onto the error the +// caller sees, not just onto the loop's internal delay: an application that +// gives up and reschedules the work itself needs the number too. +func TestClient_RetryAfterErrorCarriesSeconds(t *testing.T) { + server := httptest.NewServer(rateLimited("42")) + defer server.Close() + + client := NewClient(&Config{BaseURL: server.URL, CacheEnabled: false}, &StaticTokenProvider{Token: "test-token"}) + client.httpOpts.MaxRetries = 1 // one attempt, no wait, error straight back + + _, err := client.Get(context.Background(), "/test.json") + if err == nil { + t.Fatal("expected an error from a 429, got nil") + } + var apiErr *Error + if !errors.As(err, &apiErr) { + t.Fatalf("error %T is not an *Error: %v", err, err) + } + if apiErr.RetryAfter != 42 { + t.Errorf("Error.RetryAfter = %d, want 42", apiErr.RetryAfter) + } +} + +// TestCheckResponse_CarriesRetryAfter covers the other door onto *Error: every +// generated service method maps its 429 through checkResponse, so leaving the +// field unset there would make it trustworthy on the raw path and zero on the +// typed one. +func TestCheckResponse_CarriesRetryAfter(t *testing.T) { + for _, tc := range []struct { + name string + header string + want int + }{ + {name: "seconds", header: "17", want: 17}, + {name: "absent", header: "", want: 0}, + {name: "unparseable", header: "whenever", want: 0}, + // The typed path builds its *Error directly from the parsed header — + // it never passes through ErrRateLimit — so this is the case that + // holds the parser's own clamp. Without it a generated service method + // hands the caller a RetryAfter that overflows the moment anyone + // multiplies it by time.Second, which is what downloadURL, the + // resilience hook's rate-limiter block, and any caller rescheduling + // off err.RetryAfter all do. + {name: "beyond the honoured ceiling", header: "9223372036854775807", want: maxRetryAfterSeconds}, + } { + t.Run(tc.name, func(t *testing.T) { + resp := &http.Response{StatusCode: http.StatusTooManyRequests, Header: http.Header{}} + if tc.header != "" { + resp.Header.Set("Retry-After", tc.header) + } + err := checkResponse(resp, nil) + var apiErr *Error + if !errors.As(err, &apiErr) { + t.Fatalf("checkResponse returned %T, want *Error", err) + } + if apiErr.RetryAfter != tc.want { + t.Errorf("Error.RetryAfter = %d, want %d", apiErr.RetryAfter, tc.want) + } + }) + } +} diff --git a/go/pkg/basecamp/download.go b/go/pkg/basecamp/download.go index 33a3e2b72..294625be5 100644 --- a/go/pkg/basecamp/download.go +++ b/go/pkg/basecamp/download.go @@ -202,6 +202,14 @@ func (c *Client) fetchAPIDownload(ctx context.Context, rawURL string) (*Download c.hooks.OnRetry(ctx, info, attempt+1, lastErr) c.logger.Debug("retrying download request", "attempt", attempt, "maxRetries", maxAttempts, "delay", delay, "error", lastErr) + // Same order as Client.doRequestURL's wait, for the same reason: a + // select with both cases ready picks pseudo-randomly, and OnRetry has + // just run. A hook that cancels there, with the delay already elapsed, + // would otherwise see the timer win and one more request go out on a + // dead context. Check the context first, where nothing competes. + if err := ctx.Err(); err != nil { + return nil, err + } select { case <-ctx.Done(): return nil, ctx.Err() diff --git a/go/pkg/basecamp/download_test.go b/go/pkg/basecamp/download_test.go index 2f73776ca..b27be5b90 100644 --- a/go/pkg/basecamp/download_test.go +++ b/go/pkg/basecamp/download_test.go @@ -799,6 +799,50 @@ func TestDownloadURL_AuthHopRetriesOn429WithRetryAfter(t *testing.T) { } } +// 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 +// a hook cancels there and the delay has already elapsed. The reasoning — and +// why the contract can only be asserted over N runs rather than forced — is in +// that test; the assertion is the same: ctx.Err() itself, and the transport +// handed exactly one request. +func TestDownloadURL_RetryWaitChecksCancellationBeforeTheTimer(t *testing.T) { + apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTooManyRequests) + })) + defer apiServer.Close() + + const runs = 64 + for run := range runs { + ctx, cancel := context.WithCancel(context.Background()) + hooks := &cancelOnRetryHooks{cancel: cancel} + cfg := DefaultConfig() + cfg.BaseURL = apiServer.URL + client := NewClient(cfg, &StaticTokenProvider{Token: "test-token"}, + WithMaxRetries(3), + // Zero backoff, so the timer is ready before the wait begins; see + // the Client test for why that is the honest stand-in. + WithBaseDelay(0), + WithMaxJitter(time.Nanosecond), + WithTransport(http.DefaultTransport), + WithHooks(hooks), + ) + ac := client.ForAccount("12345") + + _, err := ac.DownloadURL(ctx, "https://storage.3.basecamp.com/999/blobs/abc/download/file.png") + cancel() + + if err != context.Canceled { + t.Fatalf("run %d: DownloadURL returned %v, want ctx.Err() itself (context.Canceled) — "+ + "the loop went round again after the caller cancelled at the retry boundary", run, err) + } + if got := hooks.requests.Load(); got != 1 { + t.Fatalf("run %d: the transport was handed %d requests, want 1 — "+ + "a cancellation delivered before the wait must not be followed by another attempt", run, got) + } + } +} + // flakyRoundTripper fails the first N RoundTrips with a synthetic network error, // then delegates to inner. Used to prove network-error retry without relying on // OS-level connection-reset semantics (which Go's transport sometimes retries diff --git a/go/pkg/basecamp/errors.go b/go/pkg/basecamp/errors.go index b94932917..b8e0b788a 100644 --- a/go/pkg/basecamp/errors.go +++ b/go/pkg/basecamp/errors.go @@ -64,8 +64,19 @@ type Error struct { FieldErrors map[string][]string HTTPStatus int Retryable bool - RequestID string - Cause error + // RetryAfter is the server-specified delay in seconds from a 429'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 + // curve when it is positive; callers that give up and reschedule the work + // themselves read it off the returned error. + // + // Never negative, and never large enough that `time.Duration(RetryAfter) * + // time.Second` overflows: both doors onto the field — the wire parser and + // ErrRateLimit — run it through clampRetryAfterSeconds. + RetryAfter int + RequestID string + Cause error } // Error implements the error interface. @@ -181,6 +192,15 @@ func ErrForbiddenScope() *Error { // ErrRateLimit creates a rate-limit error. func ErrRateLimit(retryAfter int) *Error { + // Normalize before either the hint or the field reads it. This is an + // exported constructor taking a bare int, so it is the one door onto + // RetryAfter that the wire parser does not guard: a negative value would + // contradict the field's documented "zero means no delay", and a value + // whose seconds→Duration conversion overflows would make the retry loop + // wait a negative delay, i.e. not wait at all. The hint already treated + // non-positive as absent; now the field agrees with it. + retryAfter = clampRetryAfterSeconds(int64(retryAfter)) + hint := "Try again later" if retryAfter > 0 { hint = fmt.Sprintf("Try again in %d seconds", retryAfter) @@ -191,6 +211,7 @@ func ErrRateLimit(retryAfter int) *Error { Hint: hint, HTTPStatus: 429, Retryable: true, + RetryAfter: retryAfter, } } diff --git a/go/pkg/basecamp/helpers.go b/go/pkg/basecamp/helpers.go index 72ffbb64c..59767ee50 100644 --- a/go/pkg/basecamp/helpers.go +++ b/go/pkg/basecamp/helpers.go @@ -68,7 +68,7 @@ func checkResponse(resp *http.Response, body []byte) error { case http.StatusNotFound: return &Error{Code: CodeNotFound, Message: msgOrDefault(serverMsg, "resource not found"), Hint: serverHint, HTTPStatus: 404, RequestID: requestID} case http.StatusTooManyRequests: - return &Error{Code: CodeRateLimit, Message: msgOrDefault(serverMsg, "rate limited - try again later"), Hint: serverHint, HTTPStatus: 429, Retryable: true, RequestID: requestID} + 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} 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 diff --git a/go/pkg/basecamp/http.go b/go/pkg/basecamp/http.go index ce2ce0536..f845ab58e 100644 --- a/go/pkg/basecamp/http.go +++ b/go/pkg/basecamp/http.go @@ -112,21 +112,6 @@ func WithTransport(t http.RoundTripper) ClientOption { } } -// retryableError wraps an error with retry metadata. -// This allows respecting Retry-After headers from 429 responses. -type retryableError struct { - err error - retryAfter time.Duration -} - -func (r *retryableError) Error() string { - return r.err.Error() -} - -func (r *retryableError) Unwrap() error { - return r.err -} - // newDefaultTransport creates an HTTP transport with sensible defaults. // It clones http.DefaultTransport to preserve proxy settings, HTTP/2, TLS config. func newDefaultTransport() http.RoundTripper {