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
2026-05-13 03:39:53 -07:00
parent f50920333f
commit 0e503357ed
3 changed files with 140 additions and 24 deletions
@@ -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()
@@ -387,11 +387,15 @@ func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, r

 }
 // 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 {
@@ -408,34 +412,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
 }
@@ -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)
+	}
+}