From 4ebe623f46b7ca1bb285f631ed1034abea6678a4 Mon Sep 17 00:00:00 2001 From: Ladislav Prskavec Date: Fri, 30 Jan 2026 11:12:56 +0100 Subject: [PATCH] Handle HTTP 429 status code for rate limiting GitHub API can return either 403 or 429 for rate limiting, but the library was only checking for 403. This caused 429 responses to be treated as generic ErrorResponse instead of RateLimitError or AbuseRateLimitError, breaking retry logic and rate limit handling. Changes: - CheckResponse now detects 429 with X-RateLimit-Remaining: 0 as RateLimitError (primary rate limit) - CheckResponse now detects 429 with secondary rate limit documentation_url as AbuseRateLimitError Reference: https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api --- github/github.go | 9 ++++-- github/github_test.go | 71 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/github/github.go b/github/github.go index a806e7b8030..6e4f138961e 100644 --- a/github/github.go +++ b/github/github.go @@ -1451,13 +1451,18 @@ func CheckResponse(r *http.Response) error { switch { case r.StatusCode == http.StatusUnauthorized && strings.HasPrefix(r.Header.Get(headerOTP), "required"): return (*TwoFactorAuthError)(errorResponse) - case r.StatusCode == http.StatusForbidden && r.Header.Get(headerRateRemaining) == "0": + // Primary rate limit exceeded: GitHub returns 403 or 429 with X-RateLimit-Remaining: 0 + // See: https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api + case (r.StatusCode == http.StatusForbidden || r.StatusCode == http.StatusTooManyRequests) && + r.Header.Get(headerRateRemaining) == "0": return &RateLimitError{ Rate: parseRate(r), Response: errorResponse.Response, Message: errorResponse.Message, } - case r.StatusCode == http.StatusForbidden && + // Secondary rate limit exceeded: GitHub returns 403 or 429 with specific documentation_url + // See: https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#about-secondary-rate-limits + case (r.StatusCode == http.StatusForbidden || r.StatusCode == http.StatusTooManyRequests) && (strings.HasSuffix(errorResponse.DocumentationURL, "#abuse-rate-limits") || strings.HasSuffix(errorResponse.DocumentationURL, "secondary-rate-limits")): abuseRateLimitError := &AbuseRateLimitError{ diff --git a/github/github_test.go b/github/github_test.go index c6f6941a65b..b95abc4d7b9 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -2182,6 +2182,77 @@ func TestCheckResponse_AbuseRateLimit(t *testing.T) { } } +// TestCheckResponse_RateLimit_TooManyRequests tests that HTTP 429 with +// X-RateLimit-Remaining: 0 is correctly detected as RateLimitError. +// GitHub API can return either 403 or 429 for rate limiting. +// See: https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api +func TestCheckResponse_RateLimit_TooManyRequests(t *testing.T) { + t.Parallel() + res := &http.Response{ + Request: &http.Request{}, + StatusCode: http.StatusTooManyRequests, + Header: http.Header{}, + Body: io.NopCloser(strings.NewReader(`{"message":"m", + "documentation_url": "url"}`)), + } + res.Header.Set(headerRateLimit, "60") + res.Header.Set(headerRateRemaining, "0") + res.Header.Set(headerRateUsed, "60") + res.Header.Set(headerRateReset, "243424") + res.Header.Set(headerRateResource, "core") + + var err *RateLimitError + errors.As(CheckResponse(res), &err) + + if err == nil { + t.Error("Expected error response.") + } + + want := &RateLimitError{ + Rate: parseRate(res), + Response: res, + Message: "m", + } + if !errors.Is(err, want) { + t.Errorf("Error = %#v, want %#v", err, want) + } +} + +// TestCheckResponse_AbuseRateLimit_TooManyRequests tests that HTTP 429 with +// secondary rate limit documentation_url is correctly detected as AbuseRateLimitError. +// GitHub API can return either 403 or 429 for secondary rate limits. +// See: https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#about-secondary-rate-limits +func TestCheckResponse_AbuseRateLimit_TooManyRequests(t *testing.T) { + t.Parallel() + res := &http.Response{ + Request: &http.Request{}, + StatusCode: http.StatusTooManyRequests, + Header: http.Header{}, + Body: io.NopCloser(strings.NewReader(`{"message":"m", + "documentation_url": "https://docs.github.com/rest/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits"}`)), + } + res.Header.Set(headerRetryAfter, "60") + + var err *AbuseRateLimitError + errors.As(CheckResponse(res), &err) + + if err == nil { + t.Fatal("Expected error response.") + } + + if err.Response != res { + t.Errorf("Response = %v, want %v", err.Response, res) + } + if err.Message != "m" { + t.Errorf("Message = %q, want %q", err.Message, "m") + } + if err.RetryAfter == nil { + t.Error("Expected RetryAfter to be set") + } else if *err.RetryAfter != 60*time.Second { + t.Errorf("RetryAfter = %v, want %v", *err.RetryAfter, 60*time.Second) + } +} + func TestCheckResponse_RedirectionError(t *testing.T) { t.Parallel() urlStr := "/foo/bar"