Let Go's raw GET retry loop see the Retry-After it already parses

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<SearchResult>`, not `ListResult<JsonElement>` (#717)
 
 ```kotlin

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

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