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).