From 94cf894c49e0e8851a484f491fba30202edb72f2 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 03:39:53 -0700 Subject: [PATCH] fix: address review feedback on PR #106 - Add 429 rate-limit retry logic to doJSONRequest (matching doRequest behavior) so write operations (PostReview, DismissReview) properly retry when rate-limited by GitHub - Remove redundant explicit case for ReviewEventComment in translateReviewEvent (default already handles it) - Add ordering comment on --gitea-url alias registration explaining the dependency on registration-before-parse evaluation order - Add tests for doJSONRequest retry/exhaust behavior --- cmd/review-bot/main.go | 4 ++ github/client.go | 107 ++++++++++++++++++++++++++++++++--------- github/client_test.go | 53 ++++++++++++++++++++ github/reviews.go | 2 - 4 files changed, 140 insertions(+), 26 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 5f79fb2..4e0487b 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -91,6 +91,10 @@ func main() { // NOTE: If a user passes both --vcs-url and --gitea-url, the last one on // the command line takes effect (standard flag package behavior). This is // acceptable since --gitea-url is deprecated and both serve the same purpose. + // + // ORDERING: This must remain AFTER vcsURL's flag.String declaration and BEFORE + // flag.Parse(). The *vcsURL dereference captures the env-var-resolved default + // at registration time; moving flag.Parse() above this line would break it. flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead") flag.Parse() diff --git a/github/client.go b/github/client.go index 1f17308..345f06c 100644 --- a/github/client.go +++ b/github/client.go @@ -346,11 +346,15 @@ func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) { } // doJSONRequest performs an HTTP request with a JSON body and returns the response body. -// It handles HTTPS validation, authentication, and response reading. +// It handles HTTPS validation, authentication, response reading, and retries on HTTP 429 +// rate limit responses (matching the retry behavior of doRequest). // This is a general-purpose helper used by any method that needs to send JSON payloads // (e.g. PostReview, DismissReview). func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload any) ([]byte, error) { - const maxErrorBodyBytes = 4 * 1024 + const ( + maxErrorBodyBytes = 4 * 1024 + maxRetryAfter = 120 * time.Second + ) jsonBody, err := json.Marshal(payload) if err != nil { @@ -367,34 +371,89 @@ func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, paylo } } - req, err := http.NewRequestWithContext(ctx, method, reqURL, bytes.NewReader(jsonBody)) - if err != nil { - return nil, fmt.Errorf("create request: %w", err) + defaultBackoff := []time.Duration{1 * time.Second, 2 * time.Second} + var backoff []time.Duration + if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 { + backoff = make([]time.Duration, len(c.retryBackoff)) + copy(backoff, c.retryBackoff) + } else { + backoff = make([]time.Duration, len(defaultBackoff)) + copy(backoff, defaultBackoff) } - if c.token != "" { - req.Header.Set("Authorization", "Bearer "+c.token) - } - req.Header.Set("User-Agent", userAgent) - req.Header.Set("Accept", "application/vnd.github+json") - req.Header.Set("Content-Type", "application/json") - resp, err := c.httpClient.Do(req) - if err != nil { - return nil, fmt.Errorf("do request: %w", err) - } - defer resp.Body.Close() + var lastErr error + for attempt := 0; attempt < maxRetryAttempts; attempt++ { + if attempt > 0 { + var delay time.Duration + if attempt-1 < len(backoff) { + delay = backoff[attempt-1] + } + if delay > 0 { + timer := time.NewTimer(delay) + select { + case <-timer.C: + timer.Stop() + case <-ctx.Done(): + timer.Stop() + return nil, ctx.Err() + } + } + } - if resp.StatusCode >= 200 && resp.StatusCode < 300 { - body, err := io.ReadAll(io.LimitReader(resp.Body, int64(maxResponseBytes)+1)) + req, err := http.NewRequestWithContext(ctx, method, reqURL, bytes.NewReader(jsonBody)) if err != nil { - return nil, fmt.Errorf("read response body: %w", err) + return nil, fmt.Errorf("create request: %w", err) } - if len(body) > maxResponseBytes { - return nil, fmt.Errorf("response body exceeded %d bytes", maxResponseBytes) + if c.token != "" { + req.Header.Set("Authorization", "Bearer "+c.token) } - return body, nil + req.Header.Set("User-Agent", userAgent) + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("Content-Type", "application/json") + + resp, err := c.httpClient.Do(req) + if err != nil { + return nil, fmt.Errorf("do request: %w", err) + } + + respStatus := resp.StatusCode + retryAfterHeader := resp.Header.Get("Retry-After") + + body, done, handleErr := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes) + if done { + return body, handleErr + } + lastErr = handleErr + + // Retry on 429 rate limit + if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 { + if ra := retryAfterHeader; ra != "" { + if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 { + delay := time.Duration(seconds) * time.Second + if delay > maxRetryAfter { + delay = maxRetryAfter + } + if attempt < len(backoff) { + backoff[attempt] = delay + } + } else if retryAt, err := http.ParseTime(ra); err == nil { + delay := time.Until(retryAt) + if delay < 0 { + delay = 0 + } + if delay > maxRetryAfter { + delay = maxRetryAfter + } + if attempt < len(backoff) { + backoff[attempt] = delay + } + } + } + continue + } + + return nil, lastErr } - errBody, _ := io.ReadAll(io.LimitReader(resp.Body, int64(maxErrorBodyBytes))) - return nil, &APIError{StatusCode: resp.StatusCode, Body: string(errBody)} + return nil, lastErr } diff --git a/github/client_test.go b/github/client_test.go index ab6f4d1..7701e92 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -2,6 +2,7 @@ package github import ( "context" + "errors" "net/http" "net/http/httptest" "net/url" @@ -592,3 +593,55 @@ func TestSetRetryBackoff_RejectsInvalidLength(t *testing.T) { t.Fatalf("unexpected error for valid backoff: %v", err) } } + +func TestDoJSONRequest_429Retry(t *testing.T) { + attempts := 0 + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts++ + if attempts < 3 { + w.WriteHeader(429) + w.Write([]byte(`{"message":"rate limit exceeded"}`)) + return + } + w.WriteHeader(200) + w.Write([]byte(`{"id":1}`)) + })) + defer ts.Close() + + c := NewClient("token", ts.URL, AllowInsecureHTTP()) + c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) + + body, err := c.doJSONRequest(context.Background(), http.MethodPost, ts.URL+"/test", map[string]string{"key": "val"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if attempts != 3 { + t.Errorf("expected 3 attempts, got %d", attempts) + } + if string(body) != `{"id":1}` { + t.Errorf("unexpected body: %s", body) + } +} + +func TestDoJSONRequest_429ExhaustsRetries(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(429) + w.Write([]byte(`{"message":"rate limit"}`)) + })) + defer ts.Close() + + c := NewClient("token", ts.URL, AllowInsecureHTTP()) + c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) + + _, err := c.doJSONRequest(context.Background(), http.MethodPost, ts.URL+"/test", map[string]string{"key": "val"}) + if err == nil { + t.Fatal("expected error after exhausting retries") + } + var apiErr *APIError + if !errors.As(err, &apiErr) { + t.Fatalf("expected APIError, got %T: %v", err, err) + } + if apiErr.StatusCode != 429 { + t.Errorf("expected 429, got %d", apiErr.StatusCode) + } +} diff --git a/github/reviews.go b/github/reviews.go index 4cf2290..b6aa3b4 100644 --- a/github/reviews.go +++ b/github/reviews.go @@ -61,8 +61,6 @@ func translateReviewEvent(event vcs.ReviewEvent) string { return "APPROVE" case vcs.ReviewEventRequestChanges: return "REQUEST_CHANGES" - case vcs.ReviewEventComment: - return "COMMENT" default: return "COMMENT" }