diff --git a/github/client.go b/github/client.go index be72d19..dc10c7a 100644 --- a/github/client.go +++ b/github/client.go @@ -21,6 +21,10 @@ const ( // maxResponseBytes limits successful response body reads to 10 MiB. maxResponseBytes = 10 * 1024 * 1024 + + // maxRetryAttempts is the number of times doRequest will attempt a request. + // The retry backoff slice must have length maxRetryAttempts-1. + maxRetryAttempts = 3 ) // APIError represents an HTTP error response from the GitHub API. @@ -178,30 +182,33 @@ func (c *Client) SetHTTPClient(hc *http.Client) { // SetRetryBackoff configures the retry backoff durations for testing. // It must be called before any goroutines issue requests. +// The slice must have exactly maxRetryAttempts-1 entries (one delay per retry gap). // In production the default {1s, 2s} applies. -func (c *Client) SetRetryBackoff(d []time.Duration) { +func (c *Client) SetRetryBackoff(d []time.Duration) error { + if len(d) != maxRetryAttempts-1 { + return fmt.Errorf("github: backoff length %d does not match maxRetryAttempts-1 (%d)", len(d), maxRetryAttempts-1) + } c.retryBackoff = d + return nil } // doRequest performs an HTTP request with retry on 429 rate limit responses. // It respects the Retry-After header when present (capped at maxRetryAfter). // Transport errors (network failures, context cancellation) are not retried. func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) { - const maxAttempts = 3 const maxRetryAfter = 120 * time.Second // backoff holds per-attempt delays: backoff[i] is the delay before attempt i+1. - // Length must be maxAttempts-1 (one entry per retry gap). Panic early on misconfiguration - // so a maxAttempts change without a matching backoff update is caught in tests, not production. + // Length must be maxRetryAttempts-1 (one entry per retry gap). + // SetRetryBackoff validates at configuration time; the default is always valid. + defaultBackoff := []time.Duration{1 * time.Second, 2 * time.Second} var backoff []time.Duration - if c.retryBackoff != nil { + if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 { backoff = make([]time.Duration, len(c.retryBackoff)) copy(backoff, c.retryBackoff) } else { - backoff = []time.Duration{1 * time.Second, 2 * time.Second} - } - if len(backoff) != maxAttempts-1 { - panic(fmt.Sprintf("github: backoff length %d does not match maxAttempts-1 (%d)", len(backoff), maxAttempts-1)) + backoff = make([]time.Duration, len(defaultBackoff)) + copy(backoff, defaultBackoff) } // maxErrorBodyBytes limits how much of an error response body is stored. @@ -221,7 +228,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st } var lastErr error - for attempt := 0; attempt < maxAttempts; attempt++ { + for attempt := 0; attempt < maxRetryAttempts; attempt++ { if attempt > 0 { var delay time.Duration if attempt-1 < len(backoff) { @@ -272,7 +279,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st lastErr = err // Retry on 429 rate limit - if respStatus == http.StatusTooManyRequests && attempt < maxAttempts-1 { + if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 { // Check for Retry-After header and override backoff if present. // Supports both integer seconds (common) and HTTP-date format (RFC 7231). if ra := retryAfterHeader; ra != "" { @@ -319,7 +326,7 @@ func (c *Client) handleResponse(resp *http.Response, maxRespBytes int, maxErrByt return nil, true, fmt.Errorf("read response body: %w", err) } if len(body) > maxRespBytes { - return nil, true, fmt.Errorf("response body exceeded %d bytes (truncated)", maxRespBytes) + return nil, true, fmt.Errorf("response body exceeded %d bytes", maxRespBytes) } return body, true, nil } diff --git a/github/client_test.go b/github/client_test.go index a8ccc06..ab6f4d1 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -83,7 +83,9 @@ func TestDoRequest_429Retry(t *testing.T) { c := NewClient("token", srv.URL, AllowInsecureHTTP()) c.SetHTTPClient(srv.Client()) - c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond}) + if err := c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond}); err != nil { + t.Fatalf("SetRetryBackoff: %v", err) + } body, err := c.doGet(context.Background(), srv.URL+"/test") if err != nil { @@ -108,7 +110,9 @@ func TestDoRequest_429ExhaustsRetries(t *testing.T) { c := NewClient("token", srv.URL, AllowInsecureHTTP()) c.SetHTTPClient(srv.Client()) - c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) + if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil { + t.Fatalf("SetRetryBackoff: %v", err) + } _, err := c.doGet(context.Background(), srv.URL+"/test") if err == nil { @@ -218,7 +222,9 @@ func TestDoRequest_429RetryAfterHeader(t *testing.T) { c := NewClient("token", srv.URL, AllowInsecureHTTP()) c.SetHTTPClient(srv.Client()) // Use short backoff; Retry-After should override - c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) + if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil { + t.Fatalf("SetRetryBackoff: %v", err) + } start := time.Now() body, err := c.doGet(context.Background(), srv.URL+"/test") @@ -259,7 +265,9 @@ func TestDoRequest_RetryAfterDoesNotMutateBackoff(t *testing.T) { c := NewClient("token", srv.URL, AllowInsecureHTTP()) c.SetHTTPClient(srv.Client()) - c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) + if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil { + t.Fatalf("SetRetryBackoff: %v", err) + } _, err := c.doGet(context.Background(), srv.URL+"/test") if err != nil { @@ -297,7 +305,9 @@ func TestDoRequest_429RetryAfterHTTPDate(t *testing.T) { c := NewClient("token", srv.URL, AllowInsecureHTTP()) c.SetHTTPClient(srv.Client()) - c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) + if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil { + t.Fatalf("SetRetryBackoff: %v", err) + } start := time.Now() body, err := c.doGet(context.Background(), srv.URL+"/test") @@ -338,7 +348,9 @@ func TestDoRequest_429RetryAfterHTTPDateInPast(t *testing.T) { c := NewClient("token", srv.URL, AllowInsecureHTTP()) c.SetHTTPClient(srv.Client()) - c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second}) + if err := c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second}); err != nil { + t.Fatalf("SetRetryBackoff: %v", err) + } start := time.Now() _, err := c.doGet(context.Background(), srv.URL+"/test") @@ -554,3 +566,29 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) { t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)") } } + + +func TestSetRetryBackoff_RejectsInvalidLength(t *testing.T) { + c := NewClient("token", "https://api.github.com") + + // Too short + err := c.SetRetryBackoff([]time.Duration{1 * time.Second}) + if err == nil { + t.Fatal("expected error for backoff length 1") + } + if !strings.Contains(err.Error(), "backoff length 1") { + t.Errorf("unexpected error message: %v", err) + } + + // Too long + err = c.SetRetryBackoff([]time.Duration{1 * time.Second, 2 * time.Second, 3 * time.Second}) + if err == nil { + t.Fatal("expected error for backoff length 3") + } + + // Correct length succeeds + err = c.SetRetryBackoff([]time.Duration{1 * time.Second, 2 * time.Second}) + if err != nil { + t.Fatalf("unexpected error for valid backoff: %v", err) + } +}