From 6f8b6fed5c1aac63e5096a2317d95c51135ad005 Mon Sep 17 00:00:00 2001 From: jferrl Date: Thu, 17 Sep 2026 23:01:45 +0200 Subject: [PATCH] feat: return a typed RateLimitError carrying the retry hint ErrRateLimited was a bare sentinel, so a caller running its own backoff could tell that GitHub had throttled the request but not for how long. The client had already parsed Retry-After and X-RateLimit-Reset, capped the result and chosen a fallback, then discarded all of it into an error string the caller would have to re-parse. RateLimitError carries StatusCode, RetryAfter and the response message. It unwraps to ErrRateLimited, so errors.Is keeps working and the rendered message is byte-identical to before; a test pins that string, since callers log and match on it. The retry path now recovers the delay with errors.As instead of threading it through a third return value, so doCreateInstallationToken returns the usual two. RetryAfter is zero when GitHub says to retry immediately or when the reset instant has already passed, which is documented as "retry now" rather than "no hint", since the two are indistinguishable in the value alone. --- doc.go | 15 ++++++ github.go | 70 ++++++++++++++++++------ github_test.go | 144 +++++++++++++++++++++++++++++++++++++++++++++++++ llms.txt | 1 + 4 files changed, 214 insertions(+), 16 deletions(-) diff --git a/doc.go b/doc.go index c5a3005..c5f3f66 100644 --- a/doc.go +++ b/doc.go @@ -41,6 +41,21 @@ // already-expired credential. The window is tunable with WithExpirySkew // (application JWTs) and WithInstallationExpirySkew (installation tokens). // +// # Handling rate limits +// +// A throttled request returns a [RateLimitError] carrying the wait the client +// computed from GitHub's own headers, so a caller running its own backoff does +// not have to re-parse a response it never sees: +// +// var rle *githubauth.RateLimitError +// if errors.As(err, &rle) { +// time.Sleep(rle.RetryAfter) +// } +// +// It unwraps to ErrRateLimited, so errors.Is(err, ErrRateLimited) keeps working. +// A 403 permission failure is not a rate limit and matches neither, even though +// GitHub attaches rate-limit headers to it. +// // # Signing with external key stores // // NewApplicationTokenSourceFromSigner accepts any RSA-backed [crypto.Signer] diff --git a/github.go b/github.go index bd4f8ed..e22c2f7 100644 --- a/github.go +++ b/github.go @@ -36,6 +36,40 @@ const ( secondaryRateLimitBackoff = 60 * time.Second ) +// RateLimitError is the concrete error returned when GitHub throttles a +// request. It carries the wait the client computed from GitHub's own hints, so +// a caller running its own backoff does not have to re-parse the headers the +// client already read: +// +// var rle *githubauth.RateLimitError +// if errors.As(err, &rle) { +// time.Sleep(rle.RetryAfter) +// } +// +// It unwraps to ErrRateLimited, so errors.Is(err, ErrRateLimited) keeps working. +type RateLimitError struct { + // StatusCode is the HTTP status GitHub returned, 429 or 403. + StatusCode int + + // RetryAfter is how long to wait before retrying, taken from Retry-After or + // X-RateLimit-Reset and capped at maxRetrySleep. It falls back to a + // documented default when GitHub sends no usable hint. It is zero when + // GitHub says to retry immediately, or when the reset instant has already + // passed, so callers must treat zero as "retry now" rather than "no hint". + RetryAfter time.Duration + + // Message is the response body, truncated to maxErrorBodyBytes. + Message string +} + +func (e *RateLimitError) Error() string { + return fmt.Sprintf("%s: GitHub API returned status %d: %s", ErrRateLimited.Error(), e.StatusCode, e.Message) +} + +// Unwrap reports ErrRateLimited so callers can branch with errors.Is without +// knowing about this type. +func (e *RateLimitError) Unwrap() error { return ErrRateLimited } + // ErrRateLimited wraps errors returned when GitHub has throttled a request: // any HTTP 429, or a 403 that GitHub identifies as a rate limit rather than a // permission failure. A 403 counts when it carries Retry-After, reports an @@ -189,27 +223,27 @@ func (c *githubClient) createInstallationToken(ctx context.Context, installation } } - token, delay, err := c.doCreateInstallationToken(ctx, u.String(), bodyBytes) + token, err := c.doCreateInstallationToken(ctx, u.String(), bodyBytes) if err == nil { return token, nil } - if !c.retryOnThrottle || !errors.Is(err, ErrRateLimited) { + + var throttled *RateLimitError + if !c.retryOnThrottle || !errors.As(err, &throttled) { return nil, err } - if sleepErr := sleepCtx(ctx, delay); sleepErr != nil { + if sleepErr := sleepCtx(ctx, throttled.RetryAfter); sleepErr != nil { return nil, sleepErr } - token, _, err = c.doCreateInstallationToken(ctx, u.String(), bodyBytes) - return token, err + return c.doCreateInstallationToken(ctx, u.String(), bodyBytes) } -// doCreateInstallationToken performs a single POST attempt. On a throttled -// response it returns the desired retry delay in addition to the error so the -// caller can decide whether to retry. A zero delay indicates the error is not -// retryable. -func (c *githubClient) doCreateInstallationToken(ctx context.Context, reqURL string, bodyBytes []byte) (*InstallationToken, time.Duration, error) { +// doCreateInstallationToken performs a single POST attempt. A throttled +// response yields a *RateLimitError carrying the retry delay, so the caller can +// decide whether to retry without re-reading the response. +func (c *githubClient) doCreateInstallationToken(ctx context.Context, reqURL string, bodyBytes []byte) (*InstallationToken, error) { var body io.Reader if bodyBytes != nil { body = bytes.NewReader(bodyBytes) @@ -217,7 +251,7 @@ func (c *githubClient) doCreateInstallationToken(ctx context.Context, reqURL str req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, body) if err != nil { - return nil, 0, fmt.Errorf("failed to create request: %w", err) + return nil, fmt.Errorf("failed to create request: %w", err) } req.Header.Set("Accept", "application/vnd.github+json") @@ -225,7 +259,7 @@ func (c *githubClient) doCreateInstallationToken(ctx context.Context, reqURL str resp, err := c.httpClient.Do(req) if err != nil { - return nil, 0, fmt.Errorf("failed to execute request: %w", err) + return nil, fmt.Errorf("failed to execute request: %w", err) } defer func() { _ = resp.Body.Close() @@ -234,17 +268,21 @@ func (c *githubClient) doCreateInstallationToken(ctx context.Context, reqURL str if resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusCreated { var token InstallationToken if err := json.NewDecoder(resp.Body).Decode(&token); err != nil { - return nil, 0, fmt.Errorf("failed to decode response: %w", err) + return nil, fmt.Errorf("failed to decode response: %w", err) } - return &token, 0, nil + return &token, nil } bodyResp, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes)) if delay, ok := c.throttleDelay(resp, bodyResp); ok { - return nil, delay, fmt.Errorf("%w: GitHub API returned status %d: %s", ErrRateLimited, resp.StatusCode, string(bodyResp)) + return nil, &RateLimitError{ + StatusCode: resp.StatusCode, + RetryAfter: delay, + Message: string(bodyResp), + } } - return nil, 0, fmt.Errorf("GitHub API returned status %d: %s", resp.StatusCode, string(bodyResp)) + return nil, fmt.Errorf("GitHub API returned status %d: %s", resp.StatusCode, string(bodyResp)) } // throttleDelay inspects a non-2xx response and reports the retry hint GitHub diff --git a/github_test.go b/github_test.go index 46226e1..a96c56d 100644 --- a/github_test.go +++ b/github_test.go @@ -1045,3 +1045,147 @@ func c403(t *testing.T) *githubClient { t.Helper() return newGitHubClient(&http.Client{}) } + +// Test_RateLimitError_CarriesRetryHint covers the reason the type exists: a +// caller running its own backoff can read the wait the client already computed +// from GitHub's headers, instead of re-parsing the response it never sees. +func Test_RateLimitError_CarriesRetryHint(t *testing.T) { + tests := []struct { + name string + status int + headers map[string]string + body string + wantRetryAfter time.Duration + }{ + { + name: "Retry-After drives the wait", + status: http.StatusTooManyRequests, + headers: map[string]string{"Retry-After": "7"}, + body: `{"message":"Too Many Requests"}`, + wantRetryAfter: 7 * time.Second, + }, + { + name: "a bare 429 falls back to the default backoff", + status: http.StatusTooManyRequests, + headers: map[string]string{}, + body: `{"message":"Too Many Requests"}`, + wantRetryAfter: defaultThrottleBackoff, + }, + { + name: "a hintless rate-limit 403 falls back to a minute", + status: http.StatusForbidden, + headers: map[string]string{"X-RateLimit-Remaining": "0"}, + body: `{"message":"API rate limit exceeded"}`, + wantRetryAfter: secondaryRateLimitBackoff, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + for k, v := range tt.headers { + w.Header().Set(k, v) + } + w.WriteHeader(tt.status) + _, _ = w.Write([]byte(tt.body)) + })) + defer server.Close() + + c := newClientForServer(t, server) + c.retryOnThrottle = false // one attempt, so the first error surfaces + + _, err := c.createInstallationToken(context.Background(), 42, nil) + if err == nil { + t.Fatal("createInstallationToken() err = nil, want a rate-limit error") + } + + var rle *RateLimitError + if !errors.As(err, &rle) { + t.Fatalf("errors.As(*RateLimitError) = false for %T: %v", err, err) + } + if rle.StatusCode != tt.status { + t.Errorf("StatusCode = %d, want %d", rle.StatusCode, tt.status) + } + if rle.RetryAfter != tt.wantRetryAfter { + t.Errorf("RetryAfter = %v, want %v", rle.RetryAfter, tt.wantRetryAfter) + } + if rle.Message != tt.body { + t.Errorf("Message = %q, want %q", rle.Message, tt.body) + } + + // The sentinel remains the documented way to branch. + if !errors.Is(err, ErrRateLimited) { + t.Error("errors.Is(err, ErrRateLimited) = false; the sentinel contract is broken") + } + }) + } +} + +// Test_RateLimitError_MessageFormat pins the rendered message. Callers log and +// match on it, so introducing the type must not reword it. +func Test_RateLimitError_MessageFormat(t *testing.T) { + err := &RateLimitError{ + StatusCode: http.StatusForbidden, + RetryAfter: time.Second, + Message: `{"message":"API rate limit exceeded"}`, + } + + want := `github API rate limited: GitHub API returned status 403: {"message":"API rate limit exceeded"}` + if got := err.Error(); got != want { + t.Errorf("Error() =\n %q\nwant\n %q", got, want) + } + + if !errors.Is(err, ErrRateLimited) { + t.Error("errors.Is(err, ErrRateLimited) = false") + } +} + +// Test_RateLimitError_NotReturnedForTerminalFailures guards the boundary: a +// permission failure or a plain 404 must not be surfaced as rate limiting. +func Test_RateLimitError_NotReturnedForTerminalFailures(t *testing.T) { + tests := []struct { + name string + status int + headers map[string]string + body string + }{ + { + name: "permission failure with healthy budget", + status: http.StatusForbidden, + headers: map[string]string{"X-RateLimit-Remaining": "4999"}, + body: `{"message":"Resource not accessible by integration"}`, + }, + { + name: "not found", + status: http.StatusNotFound, + headers: map[string]string{}, + body: `{"message":"Not Found"}`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + for k, v := range tt.headers { + w.Header().Set(k, v) + } + w.WriteHeader(tt.status) + _, _ = w.Write([]byte(tt.body)) + })) + defer server.Close() + + _, err := newClientForServer(t, server).createInstallationToken(context.Background(), 42, nil) + if err == nil { + t.Fatal("createInstallationToken() err = nil, want an error") + } + + var rle *RateLimitError + if errors.As(err, &rle) { + t.Errorf("errors.As(*RateLimitError) = true for a terminal %d: %v", tt.status, err) + } + if errors.Is(err, ErrRateLimited) { + t.Errorf("errors.Is(err, ErrRateLimited) = true for a terminal %d", tt.status) + } + }) + } +} diff --git a/llms.txt b/llms.txt index 51d02d8..1c32da5 100644 --- a/llms.txt +++ b/llms.txt @@ -17,6 +17,7 @@ Core API (package `githubauth`): - `NewApplicationTokenSource(id, privateKeyPEM, opts...) (oauth2.TokenSource, error)` — App JWT source; `id` is a string Client ID or int64 App ID. Options: `WithApplicationTokenExpiration(d)` (>90s, max 10m; outside that range falls back to 10m), `WithExpirySkew(d)`. - `NewApplicationTokenSourceFromSigner(id, signer crypto.Signer, opts...) (oauth2.TokenSource, error)` — App JWT source backed by an external RSA signer (KMS/HSM/Vault/ssh-agent). +- `RateLimitError{StatusCode, RetryAfter, Message}` — concrete error for a throttled request; extract with `errors.As`. Unwraps to `ErrRateLimited`, so `errors.Is` keeps working. A 403 permission failure matches neither. - `NewInstallationTokenSource(installationID int64, appSource oauth2.TokenSource, opts...) oauth2.TokenSource` — exchanges the App JWT for an installation token. Options: `WithEnterpriseURL(url)` (GHES, appends /api/v3/), `WithBaseURL(url)` (verbatim; GHEC data residency or httptest), `WithHTTPClient(c)`, `WithRetryOnThrottle(bool)`, `WithInstallationExpirySkew(d)`, `WithInstallationTokenOptions(o)`, `WithContext(ctx)`. - `NewPersonalAccessTokenSource(token string) oauth2.TokenSource` — classic (`ghp_...`) or fine-grained (`github_pat_...`) PATs. - `ReuseTokenSourceWithSkew(t, src, skew) oauth2.TokenSource` — caching wrapper that refreshes `skew` before expiry (both constructors apply it with a 30s default, eliminating in-flight 401s near expiry).