From 72d22192476ba8bd762ca6c213c96e6024c39288 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 18 Aug 2026 23:42:32 -0700 Subject: [PATCH 01/16] Let Go's raw GET retry loop see the Retry-After it already parses The loop read its server-specified delay off a *retryableError that nothing in the tree ever constructed, so the branch was unreachable and every 429 fell through to the local backoff curve. The 429 arm parsed the header and spent the number on a hint string, because Error had no field to hold it. Error gains RetryAfter (seconds), populated at both 429 construction sites, and the loop sleeps it in place of the backoff when it is positive. That makes the *Error arm the only arm, so retryableError goes as dead code rather than being brought to life alongside it. Which statuses carry a value is unchanged: only the two 429 sites set the field, matching downloadURL and the generated loop. That question is #775's. Closes #795. --- SPEC.md | 2 +- go/pkg/basecamp/client.go | 24 ++- go/pkg/basecamp/client_retry_after_test.go | 238 +++++++++++++++++++++ go/pkg/basecamp/errors.go | 12 +- go/pkg/basecamp/helpers.go | 2 +- go/pkg/basecamp/http.go | 15 -- 6 files changed, 264 insertions(+), 29 deletions(-) create mode 100644 go/pkg/basecamp/client_retry_after_test.go diff --git a/SPEC.md b/SPEC.md index 995cb681b..0ff7e5cb1 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 diff --git a/go/pkg/basecamp/client.go b/go/pkg/basecamp/client.go index 7d76f23ed..5574157df 100644 --- a/go/pkg/basecamp/client.go +++ b/go/pkg/basecamp/client.go @@ -691,20 +691,20 @@ 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 ceiling, 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 +722,10 @@ 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 + // carries no ceiling by design, so an uninterruptible sleep would let a + // server pin a request the caller already abandoned open for as long as + // it liked. select { case <-ctx.Done(): return nil, ctx.Err() 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..8b684e892 --- /dev/null +++ b/go/pkg/basecamp/client_retry_after_test.go @@ -0,0 +1,238 @@ +package basecamp + +import ( + "context" + "errors" + "log/slog" + "net/http" + "net/http/httptest" + "sync" + "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. +type cancelOnRetryHooks struct { + NoopHooks + cancel context.CancelFunc +} + +func (h *cancelOnRetryHooks) OnRetry(context.Context, RequestInfo, int, error) { + h.cancel() +} + +// 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} + + start := time.Now() + _, err := client.Get(ctx, "/test.json") + // 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. + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Fatalf("Get took %v after cancellation at the retry boundary; the retry wait is not interruptible", elapsed) + } + return delays.snapshot(), err +} + +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"}, + } { + 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_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 + }{ + {"seconds", "17", 17}, + {"absent", "", 0}, + {"unparseable", "whenever", 0}, + } { + 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/errors.go b/go/pkg/basecamp/errors.go index b94932917..bb6734a0e 100644 --- a/go/pkg/basecamp/errors.go +++ b/go/pkg/basecamp/errors.go @@ -64,8 +64,15 @@ 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. + RetryAfter int + RequestID string + Cause error } // Error implements the error interface. @@ -191,6 +198,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 { From 14da26c29f75c379139213de077bdd54ffb2dd6c Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 19 Aug 2026 12:55:24 -0700 Subject: [PATCH 02/16] Clamp Retry-After to what a time.Duration can hold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 64-bit build parses `Retry-After: 9223372036854775807` cleanly, and `time.Duration(n) * time.Second` then wraps to -1s. `time.After` on a non-positive duration fires at once, so the loop spends its whole attempt budget back to back against a server that just asked it to wait — the newly honoured header turned into a tight retry loop. Reported independently by two reviewers on #796. Both doors onto Error.RetryAfter now normalize: parseRetryAfter, which feeds checkResponse, downloadURL and RequestResult, and ErrRateLimit, which is exported and takes a bare int, so it can also carry a negative value the field's own doc calls invalid. Over-range saturates rather than falling back to "absent". Falling back would compute the millisecond backoff curve and hammer the peer, which is the same tight loop by another route; saturating waits as long as the host can express, and the wait is a select on ctx.Done() so it stays abandonable. Same split the device-flow parser draws: a digit string too long to be an int is malformed and falls back, a value that parses but exceeds what we can honour is clamped. This is a representability bound, not the policy cap #793 declined — at ~292 years it rejects nothing a server could sensibly ask for. SPEC §7 already carved out exactly this for Swift's UInt64 trap; Go joins it. The generated client's own loop (client.gen.go, from go/templates/ client.tmpl) has the identical unclamped conversion and is untouched here. --- SPEC.md | 8 +- go/pkg/basecamp/client.go | 45 +++++++- go/pkg/basecamp/client_retry_after_test.go | 114 +++++++++++++++++++-- go/pkg/basecamp/errors.go | 13 +++ 4 files changed, 170 insertions(+), 10 deletions(-) diff --git a/SPEC.md b/SPEC.md index 0ff7e5cb1..d5a6ddcd4 100644 --- a/SPEC.md +++ b/SPEC.md @@ -833,7 +833,13 @@ Requirements: 4. **`Retry-After` is exempt.** It is server-directed and takes precedence per step 3h; the ceiling governs the locally-computed formula only. Implementations may still bound it against host limits — Swift clamps its seconds→nanoseconds conversion to - 86,400s because `UInt64(_:)` on an out-of-range `Double` is a trap. + 86,400s because `UInt64(_:)` on an out-of-range `Double` is a trap, and Go + saturates its seconds→`time.Duration` conversion at `math.MaxInt64 / time.Second` + (~292 years) because the product otherwise wraps negative and `time.After` on a + non-positive duration fires at once — turning a server-directed wait into a tight + retry loop. Both are *representability* bounds on the conversion, not policy caps + on what a server may ask for: an over-range value saturates rather than falling + back to the local formula, so the wait is still as long as the host can express. **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 diff --git a/go/pkg/basecamp/client.go b/go/pkg/basecamp/client.go index 5574157df..8f43b542a 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" @@ -1073,22 +1074,60 @@ func parseNextLink(linkHeader string) string { return "" } +// maxRetryAfterSeconds is the largest delta-seconds value this SDK can turn +// into a time.Duration at all: a Duration counts nanoseconds in an int64, so +// seconds × time.Second wraps past math.MaxInt64. It is a REPRESENTABILITY +// bound, 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 reason. Deciding a *policy* cap on server-directed waits is #793's, and +// this is not one: at ~292 years, nothing a server could sensibly ask for is +// affected. +const maxRetryAfterSeconds = int64(math.MaxInt64) / int64(time.Second) + +// 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. +// +// 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. This mirrors the +// device-flow parser (SPEC §16): a digit string too long to be an int is +// malformed and falls back, while a value that parses but exceeds what we can +// honour is clamped. +func clampRetryAfterSeconds(seconds int) int { + if seconds <= 0 { + return 0 + } + return int(min(int64(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. +// Returns 0 if the header is empty or cannot be parsed, and clamps a parsed +// value to what a time.Duration can hold — every caller multiplies the result +// by time.Second. 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 + 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 + 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 index 8b684e892..d579d0aa7 100644 --- a/go/pkg/basecamp/client_retry_after_test.go +++ b/go/pkg/basecamp/client_retry_after_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "log/slog" + "math" "net/http" "net/http/httptest" "sync" @@ -164,6 +165,11 @@ func TestClient_RetryAfterAbsentOrUnusableKeepsBackoff(t *testing.T) { {"zero", "0"}, {"negative", "-5"}, {"http-date in the past", "Wed, 09 Jun 2021 10:18:14 GMT"}, + // The boundary on the other side of the clamp below: a digit string + // too long to be an int at all is malformed, not over-range, so it + // falls through to the curve exactly like "sometime next week". Same + // split the device-flow parser draws (SPEC §16). + {"digits beyond int range", "99999999999999999999"}, } { t.Run(tc.name, func(t *testing.T) { delays, err := retryAfterProbe(t, rateLimited(tc.header)) @@ -183,6 +189,90 @@ func TestClient_RetryAfterAbsentOrUnusableKeepsBackoff(t *testing.T) { } } +// skipUnlessIntWiderThanDurationSeconds skips a case whose whole subject is a +// value that parses as an int but overflows a Duration of seconds. On a build +// where int is narrower than that bound (32-bit), the class is empty — such a +// header fails strconv.Atoi and is malformed, covered by the case above. +func skipUnlessIntWiderThanDurationSeconds(t *testing.T) { + t.Helper() + if int64(math.MaxInt) <= maxRetryAfterSeconds { + t.Skip("int cannot hold a value beyond the Duration-seconds bound on this platform") + } +} + +// TestClient_RetryAfterSaturatesAtDurationCeiling 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 ~292 years, +// which is precisely why nothing here may sleep it. +func TestClient_RetryAfterSaturatesAtDurationCeiling(t *testing.T) { + skipUnlessIntWiderThanDurationSeconds(t) + + delays, err := retryAfterProbe(t, rateLimited("9223372036854775807")) + + 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 "+ + "what a Duration can hold rather than falling back to the ~1ms backoff curve", delays[0], want) + } +} + +// 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 duration range", func(t *testing.T) { + skipUnlessIntWiderThanDurationSeconds(t) + + err := ErrRateLimit(math.MaxInt) + want := int(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. @@ -212,15 +302,27 @@ func TestClient_RetryAfterErrorCarriesSeconds(t *testing.T) { // typed one. func TestCheckResponse_CarriesRetryAfter(t *testing.T) { for _, tc := range []struct { - name string - header string - want int + name string + header string + want int + needsWideInt bool }{ - {"seconds", "17", 17}, - {"absent", "", 0}, - {"unparseable", "whenever", 0}, + {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 duration range", header: "9223372036854775807", want: int(maxRetryAfterSeconds), needsWideInt: true}, } { t.Run(tc.name, func(t *testing.T) { + if tc.needsWideInt { + skipUnlessIntWiderThanDurationSeconds(t) + } resp := &http.Response{StatusCode: http.StatusTooManyRequests, Header: http.Header{}} if tc.header != "" { resp.Header.Set("Retry-After", tc.header) diff --git a/go/pkg/basecamp/errors.go b/go/pkg/basecamp/errors.go index bb6734a0e..e0038e629 100644 --- a/go/pkg/basecamp/errors.go +++ b/go/pkg/basecamp/errors.go @@ -70,6 +70,10 @@ type Error struct { // 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 @@ -188,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(retryAfter) + hint := "Try again later" if retryAfter > 0 { hint = fmt.Sprintf("Try again in %d seconds", retryAfter) From ebbe93d638bbed94a52fafcfc1d20a7a1ba3e50d Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 19 Aug 2026 13:12:32 -0700 Subject: [PATCH 03/16] Keep the over-range cases compiling where int is 32 bits `int(maxRetryAfterSeconds)` is a CONSTANT conversion, so a 32-bit build rejects it where it is written rather than where it runs: pkg/basecamp/client_retry_after_test.go:265:15: constant 9223372036 overflows int pkg/basecamp/client_retry_after_test.go:320:76: constant 9223372036 overflows int The runtime skip meant to spare those cases could never fire, because the test binary did not build. Converting a variable instead defers the check to run time, where the skip has already decided. GOARCH=386 and GOARCH=arm now vet clean. --- go/pkg/basecamp/client_retry_after_test.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/go/pkg/basecamp/client_retry_after_test.go b/go/pkg/basecamp/client_retry_after_test.go index d579d0aa7..dce06994d 100644 --- a/go/pkg/basecamp/client_retry_after_test.go +++ b/go/pkg/basecamp/client_retry_after_test.go @@ -200,6 +200,17 @@ func skipUnlessIntWiderThanDurationSeconds(t *testing.T) { } } +// durationSecondsCeiling is maxRetryAfterSeconds as an int, and the assignment +// is what makes it legal: `int(maxRetryAfterSeconds)` is a CONSTANT conversion, +// and a 32-bit build rejects it where it is written — "constant 9223372036 +// overflows int" — so the test binary would not compile at all, and the skip +// above could never run to spare it. Converting a variable is checked at run +// time instead, where the skip has already fired. +func durationSecondsCeiling() int { + ceiling := maxRetryAfterSeconds + return int(ceiling) +} + // TestClient_RetryAfterSaturatesAtDurationCeiling 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 @@ -262,7 +273,7 @@ func TestErrRateLimit_NormalizesRetryAfter(t *testing.T) { skipUnlessIntWiderThanDurationSeconds(t) err := ErrRateLimit(math.MaxInt) - want := int(maxRetryAfterSeconds) + want := durationSecondsCeiling() 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) @@ -317,7 +328,7 @@ func TestCheckResponse_CarriesRetryAfter(t *testing.T) { // 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 duration range", header: "9223372036854775807", want: int(maxRetryAfterSeconds), needsWideInt: true}, + {name: "beyond duration range", header: "9223372036854775807", want: durationSecondsCeiling(), needsWideInt: true}, } { t.Run(tc.name, func(t *testing.T) { if tc.needsWideInt { From 39b2a1dd98f9a01dc38dc6e891ace36b7080371f Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 19 Aug 2026 13:18:38 -0700 Subject: [PATCH 04/16] Record the unkeyed-literal break Error.RetryAfter causes apidiff calls the field additive and is right about the API surface, but an unkeyed composite literal of a public struct is source-breaking and the tool does not model that. MIGRATING already carries the identical note for FieldErrors (#541); this follows it, with the behaviour change the raw GET loop now has. --- MIGRATING.md | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/MIGRATING.md b/MIGRATING.md index 9b64903ea..f967dca41 100644 --- a/MIGRATING.md +++ b/MIGRATING.md @@ -230,6 +230,43 @@ 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 a `time.Duration` can represent (over-range values saturate +at ~292 years rather than wrapping negative). Typed service methods, downloads +and the rate-limiter hook already honoured the header and are unchanged. + +**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, and `err.RetryAfter` on the returned +`*Error` is there if you would rather reschedule the work yourself. + ### Kotlin: `search.search` returns `ListResult`, not `ListResult` (#717) ```kotlin From 037d5341afe60fb5240a363879ead903d389b623 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 19 Aug 2026 13:30:47 -0700 Subject: [PATCH 05/16] Take the float narrowing out of the HTTP-date branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `int(time.Until(t).Seconds())` narrows a float64, and Go leaves an out-of-range float-to-int conversion implementation-defined. Where int is 32 bits, a date beyond ~68 years out — and time.Until saturates at ~292 for the year-9999 dates a server may legally send — produced a value that read as non-positive, so the header was discarded and the loop fell back to its millisecond backoff. That is the outcome the clamp exists to prevent, reached by the one path the clamp ran too late to guard. Whole seconds now come from integer division in the Duration domain, which cannot leave int64, and clampRetryAfterSeconds takes an int64 and bounds it by int's range as well as the Duration's. Cannot be executed on a 64-bit host, so the new test pins the resulting contract rather than claiming a red proof; GOARCH=386 and arm vet clean. The probe's 5s hang guard could not fire either: Get was called inline, so an uninterruptible wait never reached the elapsed check and the package timeout reported it instead — most visibly now that an over-range delay saturates at 292 years rather than wrapping negative. Get moves to a goroutine behind a select. Proved by making the retry wait a plain time.Sleep: the guard now fails in 5.00s naming the delay (2562047h47m16s) instead of hanging. SPEC said "Go saturates this conversion" where only the hand-written client does; the generated loop keeps its own unclamped copy. Says so now, pointing at #798. --- SPEC.md | 17 ++++--- go/pkg/basecamp/client.go | 25 +++++++-- go/pkg/basecamp/client_retry_after_test.go | 59 ++++++++++++++++++++-- go/pkg/basecamp/errors.go | 2 +- 4 files changed, 85 insertions(+), 18 deletions(-) diff --git a/SPEC.md b/SPEC.md index d5a6ddcd4..13553b59a 100644 --- a/SPEC.md +++ b/SPEC.md @@ -833,13 +833,16 @@ Requirements: 4. **`Retry-After` is exempt.** It is server-directed and takes precedence per step 3h; the ceiling governs the locally-computed formula only. Implementations may still bound it against host limits — Swift clamps its seconds→nanoseconds conversion to - 86,400s because `UInt64(_:)` on an out-of-range `Double` is a trap, and Go - saturates its seconds→`time.Duration` conversion at `math.MaxInt64 / time.Second` - (~292 years) because the product otherwise wraps negative and `time.After` on a - non-positive duration fires at once — turning a server-directed wait into a tight - retry loop. Both are *representability* bounds on the conversion, not policy caps - on what a server may ask for: an over-range value saturates rather than falling - back to the local formula, so the wait is still as long as the host can express. + 86,400s because `UInt64(_:)` on an out-of-range `Double` is a trap, and Go's + hand-written client saturates its seconds→`time.Duration` conversion at + `math.MaxInt64 / time.Second` (~292 years) because the product otherwise wraps + negative and `time.After` on a non-positive duration fires at once — turning a + server-directed wait into a tight retry loop. Both are *representability* bounds + on the conversion, not policy caps on what a server may ask for: an over-range + value saturates rather than falling back to the local formula, so the wait is + still as long as the host can express. Go's **generated** retry loop keeps an + independent, unclamped copy of that conversion and so still wraps; it is tracked + in #798, with the two other divergences in the same loop. **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 diff --git a/go/pkg/basecamp/client.go b/go/pkg/basecamp/client.go index 8f43b542a..37dcc62dd 100644 --- a/go/pkg/basecamp/client.go +++ b/go/pkg/basecamp/client.go @@ -1103,11 +1103,17 @@ const maxRetryAfterSeconds = int64(math.MaxInt64) / int64(time.Second) // device-flow parser (SPEC §16): a digit string too long to be an int is // malformed and falls back, while a value that parses but exceeds what we can // honour is clamped. -func clampRetryAfterSeconds(seconds int) int { +// +// It takes an int64 because the HTTP-date branch has one: seconds are derived +// from a time.Duration there, whose range exceeds int on a 32-bit build, and +// narrowing before the clamp is what the clamp exists to prevent. The ceiling +// is therefore whichever of the two host limits binds first — the Duration's +// and int's — so the returned value is always exactly representable. +func clampRetryAfterSeconds(seconds int64) int { if seconds <= 0 { return 0 } - return int(min(int64(seconds), maxRetryAfterSeconds)) + return int(min(seconds, maxRetryAfterSeconds, int64(math.MaxInt))) } // parseRetryAfter parses the Retry-After header value. @@ -1121,12 +1127,21 @@ func parseRetryAfter(header string) int { } // Try parsing as seconds (integer) if seconds, err := strconv.Atoi(header); err == nil && seconds > 0 { - return clampRetryAfterSeconds(seconds) + return clampRetryAfterSeconds(int64(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 { + // 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. + if seconds := int64(time.Until(t) / time.Second); 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 dce06994d..8b4c42c0d 100644 --- a/go/pkg/basecamp/client_retry_after_test.go +++ b/go/pkg/basecamp/client_retry_after_test.go @@ -88,17 +88,37 @@ func retryAfterProbe(t *testing.T, handler http.HandlerFunc) ([]time.Duration, e client.logger = slog.New(delays) client.hooks = &cancelOnRetryHooks{cancel: cancel} - start := time.Now() - _, err := client.Get(ctx, "/test.json") // 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. - if elapsed := time.Since(start); elapsed > 5*time.Second { - t.Fatalf("Get took %v after cancellation at the retry boundary; the retry wait is not interruptible", elapsed) + // + // 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 ~292 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 } - return delays.snapshot(), err } func rateLimited(retryAfter string) http.HandlerFunc { @@ -242,6 +262,35 @@ func TestClient_RetryAfterSaturatesAtDurationCeiling(t *testing.T) { } } +// 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) + } + if int64(seconds) > maxRetryAfterSeconds { + t.Errorf("parseRetryAfter(a year-9999 HTTP-date) = %d, want at most %d", 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) + } +} + // 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 diff --git a/go/pkg/basecamp/errors.go b/go/pkg/basecamp/errors.go index e0038e629..b8e0b788a 100644 --- a/go/pkg/basecamp/errors.go +++ b/go/pkg/basecamp/errors.go @@ -199,7 +199,7 @@ func ErrRateLimit(retryAfter int) *Error { // 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(retryAfter) + retryAfter = clampRetryAfterSeconds(int64(retryAfter)) hint := "Try again later" if retryAfter > 0 { From cb6a9f027b730b85dfce593905bc0cca6100d81e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 19 Aug 2026 13:41:05 -0700 Subject: [PATCH 06/16] Parse the delta-seconds in int64, and round the date form up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more reads of the same branch, both from review. Atoi's range is int's, so on a 32-bit build `Retry-After: 2147483648` was ErrRange, hence malformed, hence the millisecond backoff — while the same header on a 64-bit build was honoured. ParseInt into int64 makes the parse architecture-independent and leaves the ceiling where it belongs, in the clamp, which already bounds the result by what this host can hold. A digit string too long for an int64 is still malformed everywhere. The date form truncated its remainder toward zero, so a date under a second out became 0 — read by every caller as "no delay" — and one 5s out waited 4. TypeScript, Kotlin and Swift all round up, and Kotlin's source states the rule as a convention; SPEC never wrote it down. Now it does, in §6 step 2, and Go follows it. Python and Ruby still truncate: #799. SPEC and MIGRATING said the saturation bound is ~292 years, which is only true where int is 64 bits — the field is an int, so a 32-bit build saturates at ~68. Both now name both limits. --- MIGRATING.md | 8 ++++--- SPEC.md | 17 ++++++++++---- go/pkg/basecamp/client.go | 26 ++++++++++++++++++---- go/pkg/basecamp/client_retry_after_test.go | 24 ++++++++++++++++++++ 4 files changed, 64 insertions(+), 11 deletions(-) diff --git a/MIGRATING.md b/MIGRATING.md index f967dca41..517e34827 100644 --- a/MIGRATING.md +++ b/MIGRATING.md @@ -256,9 +256,11 @@ 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 a `time.Duration` can represent (over-range values saturate -at ~292 years rather than wrapping negative). Typed service methods, downloads -and the rate-limiter hook already honoured the header and are unchanged. +ceiling beyond what the host can represent: an over-range value saturates rather +than wrapping negative, at ~292 years where `int` is 64 bits and ~68 years +(`math.MaxInt`) where it is 32, since `RetryAfter` is an `int`. Do not read the +larger figure as portable. Typed service methods, downloads and the +rate-limiter hook already honoured the header and are unchanged. **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 diff --git a/SPEC.md b/SPEC.md index 13553b59a..dd1818aad 100644 --- a/SPEC.md +++ b/SPEC.md @@ -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. It does not say which response statuses a parsed value is honoured at, and the SDKs do not agree: Python honours it on any status, Go on 429 and 503, and Ruby, Kotlin, Swift and TypeScript on 429 alone. That @@ -835,9 +841,12 @@ Requirements: bound it against host limits — Swift clamps its seconds→nanoseconds conversion to 86,400s because `UInt64(_:)` on an out-of-range `Double` is a trap, and Go's hand-written client saturates its seconds→`time.Duration` conversion at - `math.MaxInt64 / time.Second` (~292 years) because the product otherwise wraps - negative and `time.After` on a non-positive duration fires at once — turning a - server-directed wait into a tight retry loop. Both are *representability* bounds + `min(math.MaxInt64 / time.Second, math.MaxInt)` because the product otherwise + wraps negative and `time.After` on a non-positive duration fires at once — + turning a server-directed wait into a tight retry loop. Which of the two host + limits binds depends on the target: ~292 years where `int` is 64 bits, ~68 years + where it is 32, since the value is surfaced as an `int`. Both are + *representability* bounds on the conversion, not policy caps on what a server may ask for: an over-range value saturates rather than falling back to the local formula, so the wait is still as long as the host can express. Go's **generated** retry loop keeps an diff --git a/go/pkg/basecamp/client.go b/go/pkg/basecamp/client.go index 37dcc62dd..2b03aaf45 100644 --- a/go/pkg/basecamp/client.go +++ b/go/pkg/basecamp/client.go @@ -1125,9 +1125,15 @@ 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 clampRetryAfterSeconds(int64(seconds)) + // Try parsing as seconds (integer). Parsed as an int64, not through Atoi: + // Atoi's range is int's, so on a 32-bit build `Retry-After: 2147483648` + // would be ErrRange → malformed → the millisecond backoff curve, while the + // same header on a 64-bit build is honoured. The clamp already bounds the + // result by what this host can hold, so the parse has no business deciding + // it. A digit string too long for an int64 is still malformed everywhere, + // which is the boundary SPEC §16's device parser draws too. + 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 { @@ -1141,7 +1147,19 @@ func parseRetryAfter(header string) int { // 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. - if seconds := int64(time.Until(t) / time.Second); seconds > 0 { + // + // 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) } } diff --git a/go/pkg/basecamp/client_retry_after_test.go b/go/pkg/basecamp/client_retry_after_test.go index 8b4c42c0d..67a0991e3 100644 --- a/go/pkg/basecamp/client_retry_after_test.go +++ b/go/pkg/basecamp/client_retry_after_test.go @@ -291,6 +291,30 @@ func TestParseRetryAfter_FarFutureHTTPDateSaturates(t *testing.T) { } } +// 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 became 0, which every caller reads +// as absent, so the request fell 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 effect is exactly one second wide, which is the whole distance between +// truncating and rounding up, so this case tolerates up to a second of +// scheduling delay between naming the date and parsing it — and no more, +// because there is no more to give. Against truncation it fails by 1. +func TestParseRetryAfter_HTTPDateRoundsUp(t *testing.T) { + // A whole-second HTTP-date 5s out leaves a sub-second remainder against + // now(), since now() is not on a second boundary: 4.xx seconds remain. + future := time.Now().Add(5 * time.Second).UTC().Format(http.TimeFormat) + + if seconds := parseRetryAfter(future); seconds != 5 { + t.Errorf("parseRetryAfter(a date 5s out) = %d, want 5 — a truncated remainder "+ + "waits less than the server asked, and under a second it rounds to 0 and is discarded", 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 From 24755a1800127eba3bbe55f380331022f66695ff Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 19 Aug 2026 13:48:00 -0700 Subject: [PATCH 07/16] Spell the clamp's bound where CodeQL can read it `int(min(seconds, maxRetryAfterSeconds, int64(math.MaxInt)))` is bounded, but go/incorrect-integer-conversion reads the guard rather than the arithmetic and could not see through the variadic min: it reported the int64-to-int conversion as unbounded, at high severity, and failed the CodeQL check. Two explicit comparisons instead. Same values, same result on both word sizes, and the bound is now legible to the query and to a reader. Removing either one still fails its own test. --- go/pkg/basecamp/client.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/go/pkg/basecamp/client.go b/go/pkg/basecamp/client.go index 2b03aaf45..68264382f 100644 --- a/go/pkg/basecamp/client.go +++ b/go/pkg/basecamp/client.go @@ -1109,11 +1109,23 @@ const maxRetryAfterSeconds = int64(math.MaxInt64) / int64(time.Second) // narrowing before the clamp is what the clamp exists to prevent. The ceiling // is therefore whichever of the two host limits binds first — the Duration's // and int's — so the returned value is always exactly representable. +// The two bounds are separate comparisons rather than one `min` of three +// values because CodeQL's go/incorrect-integer-conversion reads the guard, not +// the arithmetic: it could not see through the variadic `min` and reported the +// int64→int conversion as unbounded (a high-severity alert, and a failing +// check). Spelled this way the bound is legible to the query and to a reader, +// and the conversion is provably exact. func clampRetryAfterSeconds(seconds int64) int { if seconds <= 0 { return 0 } - return int(min(seconds, maxRetryAfterSeconds, int64(math.MaxInt))) + if seconds > maxRetryAfterSeconds { + seconds = maxRetryAfterSeconds + } + if seconds > int64(math.MaxInt) { + return math.MaxInt + } + return int(seconds) } // parseRetryAfter parses the Retry-After header value. From f8cbc8d99d4655e5000a1fadf4c999aae844e57c Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 19 Aug 2026 14:03:56 -0700 Subject: [PATCH 08/16] Saturate at one portable ceiling, and at both ends of the parse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The clamp's answer depended on GOARCH — ~292 years where int is 64 bits, ~68 where it is 32 — which two reviewers correctly read as a wart in the docs rather than a fact worth documenting. It is now 2147483647 seconds everywhere: at or below both host limits on every target, the number Kotlin already saturates this same header at, and the shared ceiling SPEC §16 names. The arch-conditional test machinery goes with it — no skip, no runtime-computed expectation, no constant-conversion hazard. That also gives CodeQL what it wanted. go/incorrect-integer-conversion reads the guard, not the arithmetic, and neither the variadic min nor the reassign-then-compare form satisfied it; a single comparison against a bound below every target's int range does, and the narrowing is exact by inspection. A positive ParseInt range error now saturates too. ParseInt reports ErrRange with the value already at math.MaxInt64, so 9223372036854775808 — one past the largest int64, and still 1*DIGIT, which is all RFC 9110 asks of delay-seconds — lands on the ceiling instead of falling off a boundary that exists only because of Go's word size. Honouring …807 and hammering the server for …808 is a cliff no reader could predict. The rounding test asserted an equality it could not hold: the wire form carries whole seconds, so the room before the answer drops by one is 1 - frac(now), arbitrarily small. It now re-measures the bound after the parse and asserts one-sidedly, which scheduling delay can only weaken toward vacuity. MIGRATING claimed downloads and the limiter were unchanged. They share parseRetryAfter, so both rounding and saturation reached them; says so now. --- MIGRATING.md | 18 +++- SPEC.md | 21 +++-- go/pkg/basecamp/client.go | 84 ++++++++++------- go/pkg/basecamp/client_retry_after_test.go | 105 ++++++++++----------- 4 files changed, 124 insertions(+), 104 deletions(-) diff --git a/MIGRATING.md b/MIGRATING.md index 517e34827..3e0766a0d 100644 --- a/MIGRATING.md +++ b/MIGRATING.md @@ -256,11 +256,19 @@ 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: an over-range value saturates rather -than wrapping negative, at ~292 years where `int` is 64 bits and ~68 years -(`math.MaxInt`) where it is 32, since `RetryAfter` is an `int`. Do not read the -larger figure as portable. Typed service methods, downloads and the -rate-limiter hook already honoured the header and are unchanged. +ceiling beyond what the host can represent: an over-range value saturates at +2147483647 seconds (~68 years) rather than wrapping negative. That figure does +not vary by architecture. + +**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 an over-range delta-seconds saturates instead of being +discarded. 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 diff --git a/SPEC.md b/SPEC.md index dd1818aad..830d3dc78 100644 --- a/SPEC.md +++ b/SPEC.md @@ -629,10 +629,14 @@ 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. +1. Attempt parse as integer. If valid and > 0 → return as seconds. A positive value too large for the SDK's integer type is **over-range, not malformed**: RFC 9110 spells `delay-seconds` as `1*DIGIT` and sets no upper bound, so it saturates at the implementation's ceiling (see §7's note 4) rather than falling through. Only input that is not `1*DIGIT` at all is malformed. 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). +Go saturates at both boundaries. TypeScript (above `Number.MAX_SAFE_INTEGER`) and Kotlin (above +`Int.MAX_VALUE`) fall through to the backoff formula instead, which retries a server that asked for +a very long wait after a few milliseconds; that divergence is tracked in #799 alongside step 2's. + 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 @@ -840,13 +844,14 @@ Requirements: the ceiling governs the locally-computed formula only. Implementations may still bound it against host limits — Swift clamps its seconds→nanoseconds conversion to 86,400s because `UInt64(_:)` on an out-of-range `Double` is a trap, and Go's - hand-written client saturates its seconds→`time.Duration` conversion at - `min(math.MaxInt64 / time.Second, math.MaxInt)` because the product otherwise - wraps negative and `time.After` on a non-positive duration fires at once — - turning a server-directed wait into a tight retry loop. Which of the two host - limits binds depends on the target: ~292 years where `int` is 64 bits, ~68 years - where it is 32, since the value is surfaced as an `int`. Both are - *representability* bounds + hand-written client saturates at 2,147,483,647s (~68 years) because + `seconds × time.Second` otherwise wraps past `math.MaxInt64` and `time.After` on + a non-positive duration fires at once — turning a server-directed wait into a + tight retry loop. Two host limits sit above Go's number (the `Duration` product, + and the `int` it surfaces the value in, which is 32 bits on a 32-bit target) and + it is at or below both, so the answer does not vary with `GOARCH`; it is also the + value Kotlin already saturates this header at, and the shared ceiling §16 names. + All are *representability* bounds on the conversion, not policy caps on what a server may ask for: an over-range value saturates rather than falling back to the local formula, so the wait is still as long as the host can express. Go's **generated** retry loop keeps an diff --git a/go/pkg/basecamp/client.go b/go/pkg/basecamp/client.go index 68264382f..fa54e940a 100644 --- a/go/pkg/basecamp/client.go +++ b/go/pkg/basecamp/client.go @@ -3,6 +3,7 @@ package basecamp import ( "context" "encoding/json" + "errors" "fmt" "io" "log/slog" @@ -1074,20 +1075,27 @@ func parseNextLink(linkHeader string) string { return "" } -// maxRetryAfterSeconds is the largest delta-seconds value this SDK can turn -// into a time.Duration at all: a Duration counts nanoseconds in an int64, so -// seconds × time.Second wraps past math.MaxInt64. It is a REPRESENTABILITY -// bound, not a policy ceiling — SPEC §7's "Retry-After is exempt" note already +// 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 reason. Deciding a *policy* cap on server-directed waits is #793's, and -// this is not one: at ~292 years, nothing a server could sensibly ask for is +// limits"), and Kotlin saturates this same header at the same number while +// 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. -const maxRetryAfterSeconds = int64(math.MaxInt64) / int64(time.Second) +// +// 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. +// 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 @@ -1099,31 +1107,20 @@ const maxRetryAfterSeconds = int64(math.MaxInt64) / int64(time.Second) // 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. This mirrors the -// device-flow parser (SPEC §16): a digit string too long to be an int is -// malformed and falls back, while a value that parses but exceeds what we can -// honour is clamped. +// absurd clamped delay is abandonable rather than a hang. // -// It takes an int64 because the HTTP-date branch has one: seconds are derived -// from a time.Duration there, whose range exceeds int on a 32-bit build, and -// narrowing before the clamp is what the clamp exists to prevent. The ceiling -// is therefore whichever of the two host limits binds first — the Duration's -// and int's — so the returned value is always exactly representable. -// The two bounds are separate comparisons rather than one `min` of three -// values because CodeQL's go/incorrect-integer-conversion reads the guard, not -// the arithmetic: it could not see through the variadic `min` and reported the -// int64→int conversion as unbounded (a high-severity alert, and a failing -// check). Spelled this way the bound is legible to the query and to a reader, -// and the conversion is provably exact. +// 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 { - seconds = maxRetryAfterSeconds - } - if seconds > int64(math.MaxInt) { - return math.MaxInt + return maxRetryAfterSeconds } return int(seconds) } @@ -1137,14 +1134,29 @@ func parseRetryAfter(header string) int { if header == "" { return 0 } - // Try parsing as seconds (integer). Parsed as an int64, not through Atoi: - // Atoi's range is int's, so on a 32-bit build `Retry-After: 2147483648` - // would be ErrRange → malformed → the millisecond backoff curve, while the - // same header on a 64-bit build is honoured. The clamp already bounds the - // result by what this host can hold, so the parse has no business deciding - // it. A digit string too long for an int64 is still malformed everywhere, - // which is the boundary SPEC §16's device parser draws too. - if seconds, err := strconv.ParseInt(header, 10, 64); err == nil && seconds > 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 POSITIVE range error saturates too, and deliberately. ParseInt reports + // ErrRange with the value already clamped to math.MaxInt64, so + // `9223372036854775808` — one past the largest int64, and still `1*DIGIT`, + // which is all RFC 9110 requires of delay-seconds — lands on the ceiling + // like every other over-range value instead of falling off a boundary that + // exists only because of Go's word size. The alternative would honour + // 9223372036854775807 and hammer the server for 9223372036854775808, one + // digit apart, which no reader could predict and no server could intend. + // Truly malformed input ("120junk", "-5", "") still falls through: ParseInt + // returns 0 with ErrSyntax, and a negative range error clamps to + // math.MinInt64, both caught by the `> 0` guard. + // + // TypeScript (`Number.isSafeInteger`) and Kotlin (`toIntOrNull`) fall back + // to the backoff curve at their own parse limits rather than saturating; + // that divergence is #799. + if seconds, err := strconv.ParseInt(header, 10, 64); seconds > 0 && + (err == nil || errors.Is(err, strconv.ErrRange)) { return clampRetryAfterSeconds(seconds) } // Try parsing as HTTP-date (e.g., "Wed, 21 Oct 2015 07:28:00 GMT") diff --git a/go/pkg/basecamp/client_retry_after_test.go b/go/pkg/basecamp/client_retry_after_test.go index 67a0991e3..7f2bafd2e 100644 --- a/go/pkg/basecamp/client_retry_after_test.go +++ b/go/pkg/basecamp/client_retry_after_test.go @@ -185,11 +185,6 @@ func TestClient_RetryAfterAbsentOrUnusableKeepsBackoff(t *testing.T) { {"zero", "0"}, {"negative", "-5"}, {"http-date in the past", "Wed, 09 Jun 2021 10:18:14 GMT"}, - // The boundary on the other side of the clamp below: a digit string - // too long to be an int at all is malformed, not over-range, so it - // falls through to the curve exactly like "sometime next week". Same - // split the device-flow parser draws (SPEC §16). - {"digits beyond int range", "99999999999999999999"}, } { t.Run(tc.name, func(t *testing.T) { delays, err := retryAfterProbe(t, rateLimited(tc.header)) @@ -209,28 +204,6 @@ func TestClient_RetryAfterAbsentOrUnusableKeepsBackoff(t *testing.T) { } } -// skipUnlessIntWiderThanDurationSeconds skips a case whose whole subject is a -// value that parses as an int but overflows a Duration of seconds. On a build -// where int is narrower than that bound (32-bit), the class is empty — such a -// header fails strconv.Atoi and is malformed, covered by the case above. -func skipUnlessIntWiderThanDurationSeconds(t *testing.T) { - t.Helper() - if int64(math.MaxInt) <= maxRetryAfterSeconds { - t.Skip("int cannot hold a value beyond the Duration-seconds bound on this platform") - } -} - -// durationSecondsCeiling is maxRetryAfterSeconds as an int, and the assignment -// is what makes it legal: `int(maxRetryAfterSeconds)` is a CONSTANT conversion, -// and a 32-bit build rejects it where it is written — "constant 9223372036 -// overflows int" — so the test binary would not compile at all, and the skip -// above could never run to spare it. Converting a variable is checked at run -// time instead, where the skip has already fired. -func durationSecondsCeiling() int { - ceiling := maxRetryAfterSeconds - return int(ceiling) -} - // TestClient_RetryAfterSaturatesAtDurationCeiling 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 @@ -239,12 +212,23 @@ func durationSecondsCeiling() int { // 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 ~292 years, +// The delay is asserted, not the elapsed time — a clamped wait is ~68 years, // which is precisely why nothing here may sleep it. +// +// Both headers saturate, and the second is the point: `…808` is one past the +// largest int64, so ParseInt reports it as out of range. Honouring `…807` and +// discarding `…808` would put a cliff between two values one digit apart, for +// no reason a server could see — RFC 9110 asks only for `1*DIGIT` (review +// follow-up, Codex). func TestClient_RetryAfterSaturatesAtDurationCeiling(t *testing.T) { - skipUnlessIntWiderThanDurationSeconds(t) + for _, header := range []string{"9223372036854775807", "9223372036854775808", "99999999999999999999"} { + t.Run(header, func(t *testing.T) { assertSaturatedRetryAfter(t, header) }) + } +} - delays, err := retryAfterProbe(t, rateLimited("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) @@ -258,7 +242,7 @@ func TestClient_RetryAfterSaturatesAtDurationCeiling(t *testing.T) { } 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 "+ - "what a Duration can hold rather than falling back to the ~1ms backoff curve", delays[0], want) + "the honoured ceiling rather than falling back to the ~1ms backoff curve", delays[0], want) } } @@ -293,25 +277,42 @@ func TestParseRetryAfter_FarFutureHTTPDateSaturates(t *testing.T) { // 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 became 0, which every caller reads -// as absent, so the request fell onto the millisecond backoff curve instead of +// 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 effect is exactly one second wide, which is the whole distance between -// truncating and rounding up, so this case tolerates up to a second of -// scheduling delay between naming the date and parsing it — and no more, -// because there is no more to give. Against truncation it fails by 1. +// 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) { - // A whole-second HTTP-date 5s out leaves a sub-second remainder against - // now(), since now() is not on a second boundary: 4.xx seconds remain. - future := time.Now().Add(5 * time.Second).UTC().Format(http.TimeFormat) - - if seconds := parseRetryAfter(future); seconds != 5 { - t.Errorf("parseRetryAfter(a date 5s out) = %d, want 5 — a truncated remainder "+ - "waits less than the server asked, and under a second it rounds to 0 and is discarded", seconds) + // 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) } } @@ -343,10 +344,8 @@ func TestErrRateLimit_NormalizesRetryAfter(t *testing.T) { } t.Run("beyond duration range", func(t *testing.T) { - skipUnlessIntWiderThanDurationSeconds(t) - err := ErrRateLimit(math.MaxInt) - want := durationSecondsCeiling() + 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) @@ -386,10 +385,9 @@ func TestClient_RetryAfterErrorCarriesSeconds(t *testing.T) { // typed one. func TestCheckResponse_CarriesRetryAfter(t *testing.T) { for _, tc := range []struct { - name string - header string - want int - needsWideInt bool + name string + header string + want int }{ {name: "seconds", header: "17", want: 17}, {name: "absent", header: "", want: 0}, @@ -401,12 +399,9 @@ func TestCheckResponse_CarriesRetryAfter(t *testing.T) { // 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 duration range", header: "9223372036854775807", want: durationSecondsCeiling(), needsWideInt: true}, + {name: "beyond duration range", header: "9223372036854775807", want: maxRetryAfterSeconds}, } { t.Run(tc.name, func(t *testing.T) { - if tc.needsWideInt { - skipUnlessIntWiderThanDurationSeconds(t) - } resp := &http.Response{StatusCode: http.StatusTooManyRequests, Header: http.Header{}} if tc.header != "" { resp.Header.Set("Retry-After", tc.header) From 4aa068d09380bce44ab86acc3078f756726eaf15 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 19 Aug 2026 14:07:22 -0700 Subject: [PATCH 09/16] =?UTF-8?q?Leave=20=C2=A77's=20host-limits=20sentenc?= =?UTF-8?q?e=20to=20#793,=20and=20carry=20the=20rule=20in=20=C2=A76?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #793 rewrites that same sentence to point at §6 rather than restate the rule, and a real content conflict exists between the two branches. Its version is the one to keep, so this drops the hunk entirely rather than handing the merge a choice. What the sentence was carrying — Go's ceiling, why 2147483647 is the portable one, and the generated loop's unclamped copy (#798) — moves into §6 beside the parsing rules it belongs with, so nothing is lost when the two land in either order. --- SPEC.md | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/SPEC.md b/SPEC.md index 830d3dc78..f80a9806b 100644 --- a/SPEC.md +++ b/SPEC.md @@ -629,10 +629,19 @@ 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. A positive value too large for the SDK's integer type is **over-range, not malformed**: RFC 9110 spells `delay-seconds` as `1*DIGIT` and sets no upper bound, so it saturates at the implementation's ceiling (see §7's note 4) rather than falling through. Only input that is not `1*DIGIT` at all is malformed. +1. Attempt parse as integer. If valid and > 0 → return as seconds. A positive value too large for the SDK's integer type is **over-range, not malformed**: RFC 9110 spells `delay-seconds` as `1*DIGIT` and sets no upper bound, so it saturates at the implementation's ceiling (below, and permitted by §7's note 4) rather than falling through. Only input that is not `1*DIGIT` at all is malformed. 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 ceiling step 1 saturates at is the implementation's own**, and §7's note 4 permits it as a +host-limit bound rather than a policy cap. Go's hand-written client uses 2,147,483,647s (~68 years): +`seconds × time.Second` wraps past `math.MaxInt64` above ~292 years, and the value is surfaced in an +`int`, which is 32 bits on a 32-bit target — 2,147,483,647 is at or below both, so the answer does +not vary with `GOARCH`, and it is both the number Kotlin already saturates this header at and the +shared ceiling §16 names. Wrapping is not a cosmetic concern: a negative duration makes a timer fire +at once, turning a server-directed wait into a tight retry loop (#795). Go's **generated** retry loop +keeps an independent, unclamped copy of this conversion and still wraps; that is #798. + Go saturates at both boundaries. TypeScript (above `Number.MAX_SAFE_INTEGER`) and Kotlin (above `Int.MAX_VALUE`) fall through to the backoff formula instead, which retries a server that asked for a very long wait after a few milliseconds; that divergence is tracked in #799 alongside step 2's. @@ -843,20 +852,7 @@ Requirements: 4. **`Retry-After` is exempt.** It is server-directed and takes precedence per step 3h; the ceiling governs the locally-computed formula only. Implementations may still bound it against host limits — Swift clamps its seconds→nanoseconds conversion to - 86,400s because `UInt64(_:)` on an out-of-range `Double` is a trap, and Go's - hand-written client saturates at 2,147,483,647s (~68 years) because - `seconds × time.Second` otherwise wraps past `math.MaxInt64` and `time.After` on - a non-positive duration fires at once — turning a server-directed wait into a - tight retry loop. Two host limits sit above Go's number (the `Duration` product, - and the `int` it surfaces the value in, which is 32 bits on a 32-bit target) and - it is at or below both, so the answer does not vary with `GOARCH`; it is also the - value Kotlin already saturates this header at, and the shared ceiling §16 names. - All are *representability* bounds - on the conversion, not policy caps on what a server may ask for: an over-range - value saturates rather than falling back to the local formula, so the wait is - still as long as the host can express. Go's **generated** retry loop keeps an - independent, unclamped copy of that conversion and so still wraps; it is tracked - in #798, with the two other divergences in the same loop. + 86,400s because `UInt64(_:)` on an out-of-range `Double` is a trap. **Reachability.** Every SDK exposes a path to a high attempt count: Kotlin's builder validates `maxRetries >= 0` with no upper bound, Go's `WithMaxRetries` only rejects From d70a8c99d8b5bd0113dabc3252990fe2c307c950 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 19 Aug 2026 14:11:51 -0700 Subject: [PATCH 10/16] Record the over-range split rather than legislating it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit §6 step 1 said an over-range delta-seconds "saturates", which #793's rewrite of §7 note 4 contradicts: it permits both a host-limit bound and refusing a value the parser's own numeric type cannot hold, forbidding only a policy cap. Two SDKs do each. A bug fix on Go's raw path is not where a six-SDK rule gets decided, so §6 now states the classification that is not in dispute — over-range is not malformed, RFC 9110 sets no upper bound — records which SDKs do which, and leaves the convergence to #799. Go still saturates: it is one of the two permitted readings, and it removes a cliff between values one digit apart. --- SPEC.md | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/SPEC.md b/SPEC.md index f80a9806b..853092ab0 100644 --- a/SPEC.md +++ b/SPEC.md @@ -629,12 +629,18 @@ 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. A positive value too large for the SDK's integer type is **over-range, not malformed**: RFC 9110 spells `delay-seconds` as `1*DIGIT` and sets no upper bound, so it saturates at the implementation's ceiling (below, and permitted by §7's note 4) rather than falling through. Only input that is not `1*DIGIT` at all is malformed. +1. Attempt parse as integer. If valid and > 0 → return as seconds. A positive value too large for the SDK's integer type is **over-range, not malformed** — RFC 9110 spells `delay-seconds` as `1*DIGIT` and sets no upper bound — and the SDKs treat that class differently today; see below. Only input that is not `1*DIGIT` at all is malformed. 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 ceiling step 1 saturates at is the implementation's own**, and §7's note 4 permits it as a -host-limit bound rather than a policy cap. Go's hand-written client uses 2,147,483,647s (~68 years): +**An over-range step 1 value either saturates at the implementation's ceiling or is rejected**, and +§7's note 4 permits both — a host-limit bound on the conversion, or refusing a value the parser's own +numeric type cannot hold. What neither may be is a *policy* cap. The two readings are observably +different (saturating waits as long as the host can express; rejecting falls through to the local +formula and retries a server that asked for a long wait after a few milliseconds), and converging +them is #799 rather than something this section decides. + +Go's hand-written client saturates, at 2,147,483,647s (~68 years): `seconds × time.Second` wraps past `math.MaxInt64` above ~292 years, and the value is surfaced in an `int`, which is 32 bits on a 32-bit target — 2,147,483,647 is at or below both, so the answer does not vary with `GOARCH`, and it is both the number Kotlin already saturates this header at and the @@ -642,9 +648,9 @@ shared ceiling §16 names. Wrapping is not a cosmetic concern: a negative durati at once, turning a server-directed wait into a tight retry loop (#795). Go's **generated** retry loop keeps an independent, unclamped copy of this conversion and still wraps; that is #798. -Go saturates at both boundaries. TypeScript (above `Number.MAX_SAFE_INTEGER`) and Kotlin (above -`Int.MAX_VALUE`) fall through to the backoff formula instead, which retries a server that asked for -a very long wait after a few milliseconds; that divergence is tracked in #799 alongside step 2's. +Go and Swift saturate; TypeScript (above `Number.MAX_SAFE_INTEGER`) and Kotlin (above +`Int.MAX_VALUE`) reject and fall through; Python and Ruby have arbitrary-precision integers and never +reach the boundary. #799 carries that table alongside step 2's. 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 — From f56784fb56413e446f77b14a7ede663cb36c27bf Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 19 Aug 2026 14:23:56 -0700 Subject: [PATCH 11/16] Require 1*DIGIT before saturating, and stop tabling six SDKs in SPEC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit strconv accepts a leading sign, so saturating range errors turned `+9223372036854775808` into a 68-year wait synthesized out of input RFC 9110's delay-seconds grammar does not admit — and `+5` had always been honoured as 5. A digits-only guard first, matching SPEC §16's device parser and the reading conformance's "partly numeric rejected (1*DIGIT)" case already asserts. Removing the guard fails the three new cases, one of them showing the 596523h wait. The over-range table in §6 was wrong in three places, all mine: Swift rejects at parse (its 86,400s clamp is on the sleep, a different thing), Kotlin saturates only in its HTTP-date branch while its integer branch rejects, and Python and Ruby do not "never reach the boundary" — they reach it downstream and raise, `float(10**400)` → OverflowError and `sleep(10**400)` → RangeError, both verified. Six SDKs split four ways with two failing after parsing is not a sentence; #799 carries the table. --- SPEC.md | 26 ++++++++---------- go/pkg/basecamp/client.go | 31 +++++++++++++++++++--- go/pkg/basecamp/client_retry_after_test.go | 7 +++++ 3 files changed, 46 insertions(+), 18 deletions(-) diff --git a/SPEC.md b/SPEC.md index 853092ab0..87a24b2b1 100644 --- a/SPEC.md +++ b/SPEC.md @@ -636,21 +636,17 @@ Given header value `value`: **An over-range step 1 value either saturates at the implementation's ceiling or is rejected**, and §7's note 4 permits both — a host-limit bound on the conversion, or refusing a value the parser's own numeric type cannot hold. What neither may be is a *policy* cap. The two readings are observably -different (saturating waits as long as the host can express; rejecting falls through to the local -formula and retries a server that asked for a long wait after a few milliseconds), and converging -them is #799 rather than something this section decides. - -Go's hand-written client saturates, at 2,147,483,647s (~68 years): -`seconds × time.Second` wraps past `math.MaxInt64` above ~292 years, and the value is surfaced in an -`int`, which is 32 bits on a 32-bit target — 2,147,483,647 is at or below both, so the answer does -not vary with `GOARCH`, and it is both the number Kotlin already saturates this header at and the -shared ceiling §16 names. Wrapping is not a cosmetic concern: a negative duration makes a timer fire -at once, turning a server-directed wait into a tight retry loop (#795). Go's **generated** retry loop -keeps an independent, unclamped copy of this conversion and still wraps; that is #798. - -Go and Swift saturate; TypeScript (above `Number.MAX_SAFE_INTEGER`) and Kotlin (above -`Int.MAX_VALUE`) reject and fall through; Python and Ruby have arbitrary-precision integers and never -reach the boundary. #799 carries that table alongside step 2's. +different: saturating waits as long as the host can express, while rejecting falls through to the +local formula and retries a server that asked for a long wait after a few milliseconds. All six SDKs +differ here, and two of them reach the boundary *after* parsing rather than during it, which no +single sentence describes honestly — #799 carries the table and the convergence. + +Go's hand-written client saturates, at 2,147,483,647s (~68 years): `seconds × time.Second` wraps past +`math.MaxInt64` above ~292 years, and the value is surfaced in an `int`, which is 32 bits on a 32-bit +target — 2,147,483,647 is at or below both, so the answer does not vary with `GOARCH`, and it is the +shared ceiling §16 already names. Wrapping is not a cosmetic concern: a negative duration makes a +timer fire at once, turning a server-directed wait into a tight retry loop (#795). Go's **generated** +retry loop keeps an independent, unclamped copy of this conversion and still wraps; that is #798. 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 — diff --git a/go/pkg/basecamp/client.go b/go/pkg/basecamp/client.go index fa54e940a..28c970dc8 100644 --- a/go/pkg/basecamp/client.go +++ b/go/pkg/basecamp/client.go @@ -1125,6 +1125,20 @@ func clampRetryAfterSeconds(seconds int64) int { 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, and clamps a parsed @@ -1155,9 +1169,20 @@ func parseRetryAfter(header string) int { // TypeScript (`Number.isSafeInteger`) and Kotlin (`toIntOrNull`) fall back // to the backoff curve at their own parse limits rather than saturating; // that divergence is #799. - if seconds, err := strconv.ParseInt(header, 10, 64); seconds > 0 && - (err == nil || errors.Is(err, strconv.ErrRange)) { - return clampRetryAfterSeconds(seconds) + // + // The digits are checked here 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 without this check + // `+9223372036854775808` would come back as a positive range error and + // saturate: a ~68-year wait synthesized out of input the grammar does not + // admit. Same digits-only test SPEC §16's device parser makes, and the + // same reading conformance's "partly numeric rejected (`1*DIGIT`)" case + // already asserts. + if isDelaySeconds(header) { + if seconds, err := strconv.ParseInt(header, 10, 64); seconds > 0 && + (err == nil || errors.Is(err, strconv.ErrRange)) { + 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 { diff --git a/go/pkg/basecamp/client_retry_after_test.go b/go/pkg/basecamp/client_retry_after_test.go index 7f2bafd2e..117f6f81c 100644 --- a/go/pkg/basecamp/client_retry_after_test.go +++ b/go/pkg/basecamp/client_retry_after_test.go @@ -185,6 +185,13 @@ func TestClient_RetryAfterAbsentOrUnusableKeepsBackoff(t *testing.T) { {"zero", "0"}, {"negative", "-5"}, {"http-date in the past", "Wed, 09 Jun 2021 10:18:14 GMT"}, + // RFC 9110's delay-seconds is `1*DIGIT`, so a sign is not a delay — + // and strconv accepts one, which would otherwise synthesize a ~68-year + // saturated wait out of `+9223372036854775808` (review follow-up, + // Codex). Both magnitudes are covered: in range and over. + {"signed, in range", "+5"}, + {"signed, over range", "+9223372036854775808"}, + {"signed negative, over range", "-9223372036854775809"}, } { t.Run(tc.name, func(t *testing.T) { delays, err := retryAfterProbe(t, rateLimited(tc.header)) From bf45888bc56a8477c50fba81e0862e31fbd3207e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 19 Aug 2026 21:46:05 -0700 Subject: [PATCH 12/16] =?UTF-8?q?Defer=20to=20SPEC's=20two-tier=20rule=20i?= =?UTF-8?q?nstead=20of=20restating=20it=20=E2=80=94=20and=20obey=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #793 decides the question my paragraph described as open. Its §6 tiers are: unrepresentable in the parser's own numeric type is MALFORMED and falls out at step 3; representable but beyond what the host can schedule SATURATES, never falls back. Merging both texts would leave SPEC carrying two live models of one question, which is the defect #793 exists to remove — so the paragraph goes rather than being reworded, and step 1 returns to its original line. The code was on the wrong side of tier 1. Accepting a positive ParseInt range error made 9223372036854775808 saturate, where the tier says a value the parser's own int64 cannot hold is malformed. #793 also states outright that Go rejects above its 64-bit integer, so that acceptance would have been false the moment the two branches met. Reverted; the two over-range digit strings move from the saturating test to the malformed table, where removing the rejection now fails both with a 596523h wait. Tier 2 is unchanged and is what this PR set out to fix: 2147483647s in clampRetryAfterSeconds. The digits-only guard stays — a sign is not 1*DIGIT — and `+5` is now the row that kills it, since the over-range signed forms are refused by the parse either way. --- MIGRATING.md | 12 ++++---- SPEC.md | 17 +---------- go/pkg/basecamp/client.go | 23 +++++++-------- go/pkg/basecamp/client_retry_after_test.go | 33 ++++++++++++---------- 4 files changed, 38 insertions(+), 47 deletions(-) diff --git a/MIGRATING.md b/MIGRATING.md index 3e0766a0d..1e2b8eb36 100644 --- a/MIGRATING.md +++ b/MIGRATING.md @@ -256,16 +256,18 @@ 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: an over-range value saturates at -2147483647 seconds (~68 years) rather than wrapping negative. That figure does -not vary by architecture. +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 malformed instead and falls through to the +backoff curve, as it always did — SPEC §6 draws that line. **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 an over-range delta-seconds saturates instead of being -discarded. The wire operations those paths perform are unchanged, and they +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). diff --git a/SPEC.md b/SPEC.md index 87a24b2b1..a2a3f66d4 100644 --- a/SPEC.md +++ b/SPEC.md @@ -629,25 +629,10 @@ 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. A positive value too large for the SDK's integer type is **over-range, not malformed** — RFC 9110 spells `delay-seconds` as `1*DIGIT` and sets no upper bound — and the SDKs treat that class differently today; see below. Only input that is not `1*DIGIT` at all is malformed. +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). -**An over-range step 1 value either saturates at the implementation's ceiling or is rejected**, and -§7's note 4 permits both — a host-limit bound on the conversion, or refusing a value the parser's own -numeric type cannot hold. What neither may be is a *policy* cap. The two readings are observably -different: saturating waits as long as the host can express, while rejecting falls through to the -local formula and retries a server that asked for a long wait after a few milliseconds. All six SDKs -differ here, and two of them reach the boundary *after* parsing rather than during it, which no -single sentence describes honestly — #799 carries the table and the convergence. - -Go's hand-written client saturates, at 2,147,483,647s (~68 years): `seconds × time.Second` wraps past -`math.MaxInt64` above ~292 years, and the value is surfaced in an `int`, which is 32 bits on a 32-bit -target — 2,147,483,647 is at or below both, so the answer does not vary with `GOARCH`, and it is the -shared ceiling §16 already names. Wrapping is not a cosmetic concern: a negative duration makes a -timer fire at once, turning a server-directed wait into a tight retry loop (#795). Go's **generated** -retry loop keeps an independent, unclamped copy of this conversion and still wraps; that is #798. - 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 diff --git a/go/pkg/basecamp/client.go b/go/pkg/basecamp/client.go index 28c970dc8..7e6589068 100644 --- a/go/pkg/basecamp/client.go +++ b/go/pkg/basecamp/client.go @@ -3,7 +3,6 @@ package basecamp import ( "context" "encoding/json" - "errors" "fmt" "io" "log/slog" @@ -1170,17 +1169,19 @@ func parseRetryAfter(header string) int { // to the backoff curve at their own parse limits rather than saturating; // that divergence is #799. // - // The digits are checked here 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 without this check - // `+9223372036854775808` would come back as a positive range error and - // saturate: a ~68-year wait synthesized out of input the grammar does not - // admit. Same digits-only test SPEC §16's device parser makes, and the - // same reading conformance's "partly numeric rejected (`1*DIGIT`)" case - // already asserts. + // A value too large for that int64 is MALFORMED and falls through to step + // 3's backoff — SPEC §6's first tier — rather than saturating. The second + // tier, saturation, is for a value the parser holds but the host cannot + // schedule, and that is clampRetryAfterSeconds' job below. The tiers are + // stated once in SPEC and deliberately not restated here. + // + // 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); seconds > 0 && - (err == nil || errors.Is(err, strconv.ErrRange)) { + 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 117f6f81c..30660f579 100644 --- a/go/pkg/basecamp/client_retry_after_test.go +++ b/go/pkg/basecamp/client_retry_after_test.go @@ -185,13 +185,19 @@ func TestClient_RetryAfterAbsentOrUnusableKeepsBackoff(t *testing.T) { {"zero", "0"}, {"negative", "-5"}, {"http-date in the past", "Wed, 09 Jun 2021 10:18:14 GMT"}, - // RFC 9110's delay-seconds is `1*DIGIT`, so a sign is not a delay — - // and strconv accepts one, which would otherwise synthesize a ~68-year - // saturated wait out of `+9223372036854775808` (review follow-up, - // Codex). Both magnitudes are covered: in range and over. - {"signed, in range", "+5"}, - {"signed, over range", "+9223372036854775808"}, - {"signed negative, over range", "-9223372036854775809"}, + // SPEC §6's first tier: too large for the parser's own int64, so + // malformed rather than over-range, and it falls through here rather + // than saturating. 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)) @@ -222,15 +228,12 @@ 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. // -// Both headers saturate, and the second is the point: `…808` is one past the -// largest int64, so ParseInt reports it as out of range. Honouring `…807` and -// discarding `…808` would put a cliff between two values one digit apart, for -// no reason a server could see — RFC 9110 asks only for `1*DIGIT` (review -// follow-up, Codex). +// This is SPEC §6's SECOND tier: a value the parser holds but the host cannot +// schedule. The first tier — a value the parser's own int64 cannot hold at all +// — is malformed and belongs in the backoff table above, which is where +// `9223372036854775808` and the 20-digit case are pinned. func TestClient_RetryAfterSaturatesAtDurationCeiling(t *testing.T) { - for _, header := range []string{"9223372036854775807", "9223372036854775808", "99999999999999999999"} { - t.Run(header, func(t *testing.T) { assertSaturatedRetryAfter(t, header) }) - } + assertSaturatedRetryAfter(t, "9223372036854775807") } func assertSaturatedRetryAfter(t *testing.T, header string) { From 6dd6702b792da419d36b01b571b4e7c90b0a7aaa Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 19 Aug 2026 23:00:40 -0700 Subject: [PATCH 13/16] Delete the justification the revert left standing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseRetryAfter carried both readings at once: fifteen lines arguing that a positive range error saturates "like every other over-range value", followed by the tier that says it is malformed. The code does the second. The first is the reading two reviewers rejected, sitting in the file as though it were current — and a comment like that gets the next change made in its direction. Three more witnesses of reverted or disproved claims went with it: - the note that TypeScript and Kotlin "fall back at their own parse limits rather than saturating", which implied Go differs from them at tier 1 when it now matches them; - maxRetryAfterSeconds citing Kotlin as saturating this header at the same number, which review disproved — Kotlin saturates only in its HTTP-date branch and its integer branch rejects. It was removed from SPEC and left here; - TestClient_RetryAfterSaturatesAtDurationCeiling and two rows named "beyond duration range", where the Duration bound is no longer what binds; the ceiling is the schedulable one. Renamed to say so, and the probe's "~292 years" figure corrected to ~68. Comments and names only. The tier-1 rejection still fails both malformed cases when removed. --- go/pkg/basecamp/client.go | 38 ++++++++-------------- go/pkg/basecamp/client_retry_after_test.go | 10 +++--- 2 files changed, 18 insertions(+), 30 deletions(-) diff --git a/go/pkg/basecamp/client.go b/go/pkg/basecamp/client.go index 7e6589068..1eaf54e86 100644 --- a/go/pkg/basecamp/client.go +++ b/go/pkg/basecamp/client.go @@ -1078,11 +1078,12 @@ func parseNextLink(linkHeader string) string { // 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 Kotlin saturates this same header at the same number while -// 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. +// 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 @@ -1153,27 +1154,14 @@ func parseRetryAfter(header string) int { // 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 POSITIVE range error saturates too, and deliberately. ParseInt reports - // ErrRange with the value already clamped to math.MaxInt64, so - // `9223372036854775808` — one past the largest int64, and still `1*DIGIT`, - // which is all RFC 9110 requires of delay-seconds — lands on the ceiling - // like every other over-range value instead of falling off a boundary that - // exists only because of Go's word size. The alternative would honour - // 9223372036854775807 and hammer the server for 9223372036854775808, one - // digit apart, which no reader could predict and no server could intend. - // Truly malformed input ("120junk", "-5", "") still falls through: ParseInt - // returns 0 with ErrSyntax, and a negative range error clamps to - // math.MinInt64, both caught by the `> 0` guard. - // - // TypeScript (`Number.isSafeInteger`) and Kotlin (`toIntOrNull`) fall back - // to the backoff curve at their own parse limits rather than saturating; - // that divergence is #799. - // // A value too large for that int64 is MALFORMED and falls through to step - // 3's backoff — SPEC §6's first tier — rather than saturating. The second - // tier, saturation, is for a value the parser holds but the host cannot - // schedule, and that is clampRetryAfterSeconds' job below. The tiers are - // stated once in SPEC and deliberately not restated here. + // 3's backoff — SPEC §6's first tier — 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. The second tier, saturation, is for a + // value the parser holds but the host cannot schedule, and that is + // clampRetryAfterSeconds' job below. The tiers are stated once in SPEC and + // deliberately not restated here. // // The digits are checked rather than left to ParseInt, which accepts a // leading `+` or `-`. RFC 9110 spells delay-seconds as `1*DIGIT` — no sign diff --git a/go/pkg/basecamp/client_retry_after_test.go b/go/pkg/basecamp/client_retry_after_test.go index 30660f579..f93fb280a 100644 --- a/go/pkg/basecamp/client_retry_after_test.go +++ b/go/pkg/basecamp/client_retry_after_test.go @@ -98,7 +98,7 @@ func retryAfterProbe(t *testing.T, handler http.HandlerFunc) ([]time.Duration, e // 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 ~292 years rather than wrapping to a negative delay. The + // 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 { @@ -217,7 +217,7 @@ func TestClient_RetryAfterAbsentOrUnusableKeepsBackoff(t *testing.T) { } } -// TestClient_RetryAfterSaturatesAtDurationCeiling is the regression test for a +// 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, @@ -232,7 +232,7 @@ func TestClient_RetryAfterAbsentOrUnusableKeepsBackoff(t *testing.T) { // schedule. The first tier — a value the parser's own int64 cannot hold at all // — is malformed and belongs in the backoff table above, which is where // `9223372036854775808` and the 20-digit case are pinned. -func TestClient_RetryAfterSaturatesAtDurationCeiling(t *testing.T) { +func TestClient_RetryAfterSaturatesAtTheHonouredCeiling(t *testing.T) { assertSaturatedRetryAfter(t, "9223372036854775807") } @@ -353,7 +353,7 @@ func TestErrRateLimit_NormalizesRetryAfter(t *testing.T) { }) } - t.Run("beyond duration range", func(t *testing.T) { + t.Run("beyond the honoured ceiling", func(t *testing.T) { err := ErrRateLimit(math.MaxInt) want := maxRetryAfterSeconds if err.RetryAfter != want { @@ -409,7 +409,7 @@ func TestCheckResponse_CarriesRetryAfter(t *testing.T) { // 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 duration range", header: "9223372036854775807", want: maxRetryAfterSeconds}, + {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{}} From 7c19452246515bf6d6c79ce501e1cc8d86afa0b1 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 20 Aug 2026 12:23:42 -0700 Subject: [PATCH 14/16] Check the context before the retry wait's select, where nothing competes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Cancellation must win over the wait" was the comment on a select that cannot promise it: when both cases are ready Go picks pseudo-randomly. The loop fires OnRetry and then waits, so a hook that cancels there, with a delay that has already elapsed, saw the timer win about half the time and sent one more request on a dead context — failing fast, but as the transport's wrapping of context.Canceled rather than ctx.Err(), and sometimes round again to "request failed after 3 attempts". A ctx.Err() check before the select closes it, and the same order goes into fetchAPIDownload, which has the identical select behind the identical OnRetry. The interleaving cannot be forced — that is what pseudo-random means — so the contract is pinned the one way it can be: with the context cancelled before the wait and a zero backoff so the timer is already ready, the loop must return ctx.Err() itself having handed the transport exactly one request, on every one of 64 runs. Against the bare select each run is a coin flip; five invocations failed five times, by run 3 at the latest, on both loops. Against the guard it cannot fail. This is also what takes the load-sensitivity out of the probe tests, whose 1ms backoff cases could lose the same flip under preemption. Three comment corrections from the same review rounds, none changing behaviour: the loop and the wait said Retry-After carries "no ceiling", which has not been true since the representability clamp landed — it carries no policy ceiling; parseRetryAfter's doc said it clamps to what a Duration can hold, where the constant is the smaller portable bound; and MIGRATING, parseRetryAfter and two test comments said "SPEC §6 draws that line" about the int64-malformed split, which §6's algorithm on its own does not — it says only to parse a positive integer. The split is Go's, it is the first tier of the rule #793 states in §6 "Retry-After Honouring", and the cross-SDK decision is #799's. Worded to be true on either side of #793 landing. The far-future HTTP-date test asserted only `<= ceiling`, which any positive value satisfies; it now asserts equality, which time.Until's saturation at the Duration maximum makes exact. --- MIGRATING.md | 8 +- go/pkg/basecamp/client.go | 53 ++++++++---- go/pkg/basecamp/client_retry_after_test.go | 97 +++++++++++++++++++--- go/pkg/basecamp/download.go | 8 ++ go/pkg/basecamp/download_test.go | 44 ++++++++++ 5 files changed, 181 insertions(+), 29 deletions(-) diff --git a/MIGRATING.md b/MIGRATING.md index 1e2b8eb36..c03a9af73 100644 --- a/MIGRATING.md +++ b/MIGRATING.md @@ -259,8 +259,12 @@ 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 malformed instead and falls through to the -backoff curve, as it always did — SPEC §6 draws that line. +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 diff --git a/go/pkg/basecamp/client.go b/go/pkg/basecamp/client.go index 1eaf54e86..dccc57485 100644 --- a/go/pkg/basecamp/client.go +++ b/go/pkg/basecamp/client.go @@ -698,9 +698,11 @@ func (c *Client) doRequestURL(ctx context.Context, method, url string, body any) } lastErr = err // A server-specified Retry-After replaces the backoff curve - // outright — no jitter, no ceiling, 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. + // 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 { @@ -724,9 +726,20 @@ func (c *Client) doRequestURL(ctx context.Context, method, url string, body any) c.hooks.OnRetry(ctx, info, attempt+1, lastErr) // Cancellation must win over the wait. A server-specified Retry-After - // carries no ceiling by design, so an uninterruptible sleep would let a - // server pin a request the caller already abandoned open for as long as - // it liked. + // 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() @@ -1142,8 +1155,9 @@ func isDelaySeconds(value string) bool { // 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 -// value to what a time.Duration can hold — every caller multiplies the result -// by time.Second. +// 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 @@ -1154,14 +1168,21 @@ func parseRetryAfter(header string) int { // 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 MALFORMED and falls through to step - // 3's backoff — SPEC §6's first tier — 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. The second tier, saturation, is for a - // value the parser holds but the host cannot schedule, and that is - // clampRetryAfterSeconds' job below. The tiers are stated once in SPEC and - // deliberately not restated here. + // 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 diff --git a/go/pkg/basecamp/client_retry_after_test.go b/go/pkg/basecamp/client_retry_after_test.go index f93fb280a..0032850c9 100644 --- a/go/pkg/basecamp/client_retry_after_test.go +++ b/go/pkg/basecamp/client_retry_after_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "sync" + "sync/atomic" "testing" "time" ) @@ -59,15 +60,25 @@ func (d *delayRecorder) snapshot() []time.Duration { // 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 + 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 @@ -185,11 +196,13 @@ func TestClient_RetryAfterAbsentOrUnusableKeepsBackoff(t *testing.T) { {"zero", "0"}, {"negative", "-5"}, {"http-date in the past", "Wed, 09 Jun 2021 10:18:14 GMT"}, - // SPEC §6's first tier: too large for the parser's own int64, so - // malformed rather than over-range, and it falls through here rather - // than saturating. 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. + // 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 @@ -228,9 +241,10 @@ 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 SPEC §6's SECOND tier: a value the parser holds but the host cannot -// schedule. The first tier — a value the parser's own int64 cannot hold at all -// — is malformed and belongs in the backoff table above, which is where +// 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") @@ -256,6 +270,61 @@ func assertSaturatedRetryAfter(t *testing.T, header string) { } } +// 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 @@ -277,8 +346,14 @@ func TestParseRetryAfter_FarFutureHTTPDateSaturates(t *testing.T) { 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) } - if int64(seconds) > maxRetryAfterSeconds { - t.Errorf("parseRetryAfter(a year-9999 HTTP-date) = %d, want at most %d", seconds, maxRetryAfterSeconds) + // 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) 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 From cf2a1e8749cf5612ab5dc3752e92ca9c6598d51e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 21 Aug 2026 00:18:23 -0700 Subject: [PATCH 15/16] Show the errors.As extraction MIGRATING implied a type assertion would do --- MIGRATING.md | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/MIGRATING.md b/MIGRATING.md index c03a9af73..1b24efeaf 100644 --- a/MIGRATING.md +++ b/MIGRATING.md @@ -280,8 +280,17 @@ copy of the parse and is untouched (#798). 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, and `err.RetryAfter` on the returned -`*Error` is there if you would rather reschedule the work yourself. +`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) From 93d1dde88e86fe121be1145f3e9d5106c7dfd27d Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 21 Aug 2026 09:17:33 -0700 Subject: [PATCH 16/16] Clear the three [PENDING #796] markers SPEC carried for this PR, now that it is what lands --- SPEC.md | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/SPEC.md b/SPEC.md index f8e2e779a..49f4605c7 100644 --- a/SPEC.md +++ b/SPEC.md @@ -860,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. @@ -883,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* @@ -1139,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