From d6ce34778ccc34c56691a7921ae704357e65c647 Mon Sep 17 00:00:00 2001 From: Fabio Bonelli Date: Thu, 24 Sep 2026 15:09:44 +0200 Subject: [PATCH] fix: bound rate limit waits and let a context cancel them A server answering with Retry-After: 86400 stalled a validation for a day per retry. Waits the server asks for now stop at 30 seconds, and Retry-After also accepts an HTTP date as RFC 9110 allows. See italia/httpclient-lib-go#13. Co-authored-by: Peak Twilight <77903714+peaktwilight@users.noreply.github.com> --- internal/httpclient/httpclient.go | 122 ++++++++++++++++++++++--- internal/httpclient/httpclient_test.go | 89 +++++++++++++++++- internal/httpclient/retry_test.go | 119 ++++++++++++++++++++++++ 3 files changed, 315 insertions(+), 15 deletions(-) create mode 100644 internal/httpclient/retry_test.go diff --git a/internal/httpclient/httpclient.go b/internal/httpclient/httpclient.go index c68d1bb..022aba6 100644 --- a/internal/httpclient/httpclient.go +++ b/internal/httpclient/httpclient.go @@ -20,15 +20,23 @@ type Doer interface { // Client sends GET requests and retries the ones the server rate limits. type Client struct { doer Doer - sleep func(time.Duration) + sleep func(context.Context, time.Duration) error now func() time.Time } +const ( + headerRetryAfter = "Retry-After" + headerRateReset = "X-Ratelimit-Reset" + headerRateRemaining = "X-Ratelimit-Remaining" +) + var ( ErrNotFound = errors.New("not found") ErrForbidden = errors.New("forbidden") ErrRateLimited = errors.New("rate limited") ErrUnexpectedStatus = errors.New("unexpected status") + + errInvalidHeader = errors.New("invalid header") ) // maxAttempts caps the retries when the server says how long to wait, so @@ -40,6 +48,10 @@ const maxAttempts = 8 // rate limit, and double each time: seven minutes in total. const maxBlindAttempts = 4 +// maxRetryWait caps a wait the server asks for, so a Retry-After of a day +// or a far away X-Ratelimit-Reset does not stall a validation. +const maxRetryWait = 30 * time.Second + // New returns a Client that sends its requests through doer. When doer is // nil it uses a default client with a one minute timeout. func New(doer Doer) *Client { @@ -47,16 +59,23 @@ func New(doer Doer) *Client { doer = &http.Client{Timeout: 60 * time.Second} } - return &Client{doer: doer, sleep: time.Sleep, now: time.Now} + return &Client{doer: doer, sleep: sleepWithContext, now: time.Now} } // Get returns the body at url. On a 429, or on a 403 that is a rate limit, // it waits and tries again: for as long as Retry-After says, until the // time in X-Ratelimit-Reset, or with an exponential backoff from one -// minute when the server gave no time. +// minute when the server gave no time. A wait the server asks for is +// capped at 30 seconds. func (c *Client) Get(url string, headers map[string]string) ([]byte, error) { + return c.GetWithContext(context.Background(), url, headers) +} + +// GetWithContext is Get with a context. The request and the waits between +// retries stop when ctx is cancelled or reaches its deadline. +func (c *Client) GetWithContext(ctx context.Context, url string, headers map[string]string) ([]byte, error) { for attempt := 1; ; attempt++ { - resp, err := c.do(url, headers) + resp, err := c.do(ctx, url, headers) if err != nil { return nil, err } @@ -78,12 +97,14 @@ func (c *Client) Get(url string, headers map[string]string) ([]byte, error) { return nil, fmt.Errorf("%w after %d attempts: %s", ErrRateLimited, attempt, resp.Status) } - c.sleep(wait) + if err := c.sleep(ctx, wait); err != nil { + return nil, err + } } } -func (c *Client) do(url string, headers map[string]string) (*http.Response, error) { - req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, url, nil) +func (c *Client) do(ctx context.Context, url string, headers map[string]string) (*http.Response, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { return nil, fmt.Errorf("building the request: %w", err) } @@ -106,13 +127,13 @@ func (c *Client) do(url string, headers map[string]string) (*http.Response, erro // X-Ratelimit-Reset with no quota left, as GitHub answers when the API // quota is used up. Any other 403 is a denial. func (c *Client) retryAfter(resp *http.Response) (time.Duration, bool, bool) { - retryAfter := resp.Header.Get("Retry-After") - reset := resp.Header.Get("X-Ratelimit-Reset") + retryAfter := resp.Header.Get(headerRetryAfter) + reset := resp.Header.Get(headerRateReset) switch resp.StatusCode { case http.StatusTooManyRequests: case http.StatusForbidden: - quotaUsedUp := reset != "" && resp.Header.Get("X-Ratelimit-Remaining") == "0" + quotaUsedUp := reset != "" && resp.Header.Get(headerRateRemaining) == "0" if retryAfter == "" && !quotaUsedUp { return 0, false, false } @@ -120,17 +141,90 @@ func (c *Client) retryAfter(resp *http.Response) (time.Duration, bool, bool) { return 0, false, false } - if secs, err := strconv.Atoi(retryAfter); err == nil { - return time.Duration(secs) * time.Second, true, true + now := c.now() + + if wait, err := parseRetryAfter(retryAfter, now); err == nil { + return wait, true, true } - if epoch, err := strconv.ParseInt(reset, 10, 64); err == nil { - return max(time.Unix(epoch, 0).Sub(c.now()), 0), true, true + if wait, err := parseRateLimitReset(reset, now); err == nil { + return wait, true, true } return 0, false, true } +// parseRetryAfter reads Retry-After as delay-seconds or as an HTTP date +// (RFC 9110, section 10.2.3). ParseUint takes digits only, as the grammar +// does: no sign, so "-5" and "+5" are invalid. A number too large for +// uint64 is still valid and means the longest wait. +func parseRetryAfter(value string, now time.Time) (time.Duration, error) { + seconds, err := strconv.ParseUint(value, 10, 64) + if err == nil || errors.Is(err, strconv.ErrRange) { + if seconds >= uint64(maxRetryWait/time.Second) { + return maxRetryWait, nil + } + + return time.Duration(seconds) * time.Second, nil + } + + retryAt, err := http.ParseTime(value) + if err != nil { + return 0, fmt.Errorf("%w %s value %q", errInvalidHeader, headerRetryAfter, value) + } + + if !retryAt.After(now) { + return 0, nil + } + + return capRetryWait(retryAt.Sub(now)), nil +} + +// parseRateLimitReset returns the time left until the X-Ratelimit-Reset +// epoch, in seconds. +func parseRateLimitReset(value string, now time.Time) (time.Duration, error) { + reset, err := strconv.ParseUint(value, 10, 64) + if err != nil && !errors.Is(err, strconv.ErrRange) { + return 0, fmt.Errorf("%w %s value %q", errInvalidHeader, headerRateReset, value) + } + + nowSeconds := uint64(max(now.Unix(), 0)) + if reset <= nowSeconds { + return 0, nil + } + + waitSeconds := reset - nowSeconds + if waitSeconds >= uint64(maxRetryWait/time.Second) { + return maxRetryWait, nil + } + + return time.Duration(waitSeconds) * time.Second, nil +} + +func capRetryWait(wait time.Duration) time.Duration { + if wait > maxRetryWait { + return maxRetryWait + } + + return wait +} + +func sleepWithContext(ctx context.Context, wait time.Duration) error { + if wait <= 0 { + return nil + } + + timer := time.NewTimer(wait) + defer timer.Stop() + + select { + case <-ctx.Done(): + return fmt.Errorf("waiting to retry: %w", ctx.Err()) + case <-timer.C: + return nil + } +} + func read(resp *http.Response) ([]byte, error) { defer func() { _ = resp.Body.Close() }() diff --git a/internal/httpclient/httpclient_test.go b/internal/httpclient/httpclient_test.go index 5bb05cd..c229d8c 100644 --- a/internal/httpclient/httpclient_test.go +++ b/internal/httpclient/httpclient_test.go @@ -1,6 +1,7 @@ package httpclient import ( + "context" "errors" "net/http" "net/http/httptest" @@ -46,7 +47,11 @@ func newClient(t *testing.T, steps ...step) (*Client, string, *[]time.Duration) t.Cleanup(srv.Close) c := New(srv.Client()) - c.sleep = func(d time.Duration) { waits = append(waits, d) } + c.sleep = func(_ context.Context, d time.Duration) error { + waits = append(waits, d) + + return nil + } c.now = func() time.Time { return time.Unix(1000, 0) } return c, srv.URL + "/file.txt", &waits @@ -274,3 +279,85 @@ func TestGetGivesUpSoonerWithoutAWaitFromTheServer(t *testing.T) { t.Errorf("waits = %v, want %v", *waits, want) } } + +func TestGetCapsTheWaitTheServerAsksFor(t *testing.T) { + c, url, waits := newClient(t, + step{status: http.StatusTooManyRequests, retryAfter: "86400"}, + step{status: http.StatusForbidden, rateReset: "90000", remaining: "0"}, + ) + + if _, err := c.Get(url, nil); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if want := []time.Duration{maxRetryWait, maxRetryWait}; !slices.Equal(*waits, want) { + t.Errorf("waits = %v, want %v", *waits, want) + } +} + +func TestGetHonorsARetryAfterDate(t *testing.T) { + date := time.Unix(1007, 0).UTC().Format(http.TimeFormat) + c, url, waits := newClient(t, step{status: http.StatusTooManyRequests, retryAfter: date}) + + if _, err := c.Get(url, nil); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if want := []time.Duration{7 * time.Second}; !slices.Equal(*waits, want) { + t.Errorf("waits = %v, want %v", *waits, want) + } +} + +func TestGetIgnoresANegativeRetryAfter(t *testing.T) { + c, url, waits := newClient(t, step{status: http.StatusTooManyRequests, retryAfter: "-5"}) + + if _, err := c.Get(url, nil); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if want := []time.Duration{time.Minute}; !slices.Equal(*waits, want) { + t.Errorf("waits = %v, want %v", *waits, want) + } +} + +// TestGetWithContextAlreadyCancelled verifies that a pre-cancelled context returns immediately. +func TestGetWithContextAlreadyCancelled(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("body")) + })) + defer ts.Close() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + client := New(nil) + _, err := client.GetWithContext(ctx, ts.URL, nil) + if err == nil { + t.Fatal("expected error for cancelled context, got nil") + } +} + +// TestGetWithContextCancelInterruptsBackoff verifies that an explicit +// context cancellation does interrupt a Retry-After sleep. +func TestGetWithContextCancelInterruptsBackoff(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "30") + w.WriteHeader(http.StatusTooManyRequests) + })) + defer ts.Close() + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + time.Sleep(200 * time.Millisecond) + cancel() + }() + + start := time.Now() + client := New(nil) + _, err := client.GetWithContext(ctx, ts.URL, nil) + elapsed := time.Since(start) + + if err == nil { + t.Fatal("expected error for cancelled context, got nil") + } + if elapsed >= 5*time.Second { + t.Errorf("cancel did not interrupt backoff: elapsed %v", elapsed) + } +} diff --git a/internal/httpclient/retry_test.go b/internal/httpclient/retry_test.go new file mode 100644 index 0000000..f679b3c --- /dev/null +++ b/internal/httpclient/retry_test.go @@ -0,0 +1,119 @@ +package httpclient + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "strconv" + "testing" + "time" +) + +func TestParseRetryAfter(t *testing.T) { + now := time.Date(2026, time.August, 20, 12, 0, 0, 0, time.UTC) + + tests := []struct { + name string + value string + want time.Duration + ok bool + }{ + {name: "seconds", value: "5", want: 5 * time.Second, ok: true}, + {name: "http date", value: now.Add(5 * time.Second).Format(http.TimeFormat), want: 5 * time.Second, ok: true}, + {name: "http date in the past", value: now.Add(-5 * time.Second).Format(http.TimeFormat), want: 0, ok: true}, + {name: "capped", value: "86400", want: maxRetryWait, ok: true}, + {name: "capped http date", value: now.Add(24 * time.Hour).Format(http.TimeFormat), want: maxRetryWait, ok: true}, + {name: "overflow", value: "99999999999999999999999", want: maxRetryWait, ok: true}, + {name: "negative", value: "-1", ok: false}, + {name: "plus sign", value: "+5", ok: false}, + {name: "invalid", value: "invalid", ok: false}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got, err := parseRetryAfter(test.value, now) + if (err == nil) != test.ok { + t.Fatalf("parseRetryAfter(%q) error = %v, want success = %v", test.value, err, test.ok) + } + if got != test.want { + t.Fatalf("parseRetryAfter(%q) = %s, want %s", test.value, got, test.want) + } + }) + } +} + +func TestParseRateLimitReset(t *testing.T) { + now := time.Date(2026, time.August, 20, 12, 0, 0, 0, time.UTC) + + tests := []struct { + name string + value string + want time.Duration + ok bool + }{ + {name: "future", value: strconv.FormatInt(now.Add(5*time.Second).Unix(), 10), want: 5 * time.Second, ok: true}, + {name: "capped", value: strconv.FormatInt(now.Add(24*time.Hour).Unix(), 10), want: maxRetryWait, ok: true}, + {name: "overflow", value: "99999999999999999999999", want: maxRetryWait, ok: true}, + {name: "past", value: strconv.FormatInt(now.Add(-time.Second).Unix(), 10), want: 0, ok: true}, + {name: "negative", value: "-1", ok: false}, + {name: "invalid", value: "invalid", ok: false}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got, err := parseRateLimitReset(test.value, now) + if (err == nil) != test.ok { + t.Fatalf("parseRateLimitReset(%q) error = %v, want success = %v", test.value, err, test.ok) + } + if got != test.want { + t.Fatalf("parseRateLimitReset(%q) = %s, want %s", test.value, got, test.want) + } + }) + } +} + +func TestGetWithContextDeadlineInterruptsRetryAfter(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "86400") + w.WriteHeader(http.StatusTooManyRequests) + })) + defer server.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + start := time.Now() + _, err := New(nil).GetWithContext(ctx, server.URL, nil) + elapsed := time.Since(start) + + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("GetWithContext() error = %v, want context deadline exceeded", err) + } + if elapsed > 500*time.Millisecond { + t.Fatalf("GetWithContext() took %s, want less than 500ms", elapsed) + } +} + +func TestGetWithContextCancellationInterruptsRetryAfter(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "86400") + w.WriteHeader(http.StatusTooManyRequests) + })) + defer server.Close() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + time.AfterFunc(50*time.Millisecond, cancel) + + start := time.Now() + _, err := New(nil).GetWithContext(ctx, server.URL, nil) + elapsed := time.Since(start) + + if !errors.Is(err, context.Canceled) { + t.Fatalf("GetWithContext() error = %v, want context canceled", err) + } + if elapsed > 500*time.Millisecond { + t.Fatalf("GetWithContext() took %s, want less than 500ms", elapsed) + } +}