fix(github): retry reads that fail with a transient upstream error

internal/gitprovider/github/client.go

@@ -56,9 +56,11 @@ type Client struct {
 	host       string
 	token      string
 	httpClient *http.Client
-	baseURL    *url.URL
-	graphQLURL *url.URL
-	appAuth    *githubAppAuth
+	// readRetryDelays overrides defaultReadRetryDelays; tests shorten it.
+	readRetryDelays []time.Duration
+	baseURL         *url.URL
+	graphQLURL      *url.URL
+	appAuth         *githubAppAuth
 }
 
 // NewFromGitConfig builds a GitHub client and credential from config plus a token store.

internal/gitprovider/github/http.go

@@ -12,6 +12,7 @@ import (
 	"path"
 	"strconv"
 	"strings"
+	"time"
 
 	"github.com/open-cli-collective/codereview-cli/internal/gitprovider"
 )
@@ -30,30 +31,63 @@ func (c *Client) doREST(ctx context.Context, op gitprovider.Operation, method, e
 	return c.doRESTWithToken(ctx, op, method, endpoint, token, accept, out)
 }
 
+// defaultReadRetryDelays spaces the retries of a GET that failed with a
+// retryable upstream error. GitHub answers an occasional read with a 5xx that
+// the same request succeeds on moments later, and a review makes many reads,
+// so without a retry one transient failure aborts the whole run.
+var defaultReadRetryDelays = []time.Duration{time.Second, 2 * time.Second}
+
 func (c *Client) doRESTWithToken(ctx context.Context, op gitprovider.Operation, method, endpoint, token, accept string, out any) ([]byte, http.Header, error) {
+	delays := c.readRetryDelays
+	if delays == nil {
+		delays = defaultReadRetryDelays
+	}
+	for attempt := 0; ; attempt++ {
+		body, header, status, err := c.doRESTOnce(ctx, op, method, endpoint, token, accept, out)
+		if err == nil || method != http.MethodGet || attempt >= len(delays) || !isTransientReadFailure(err, status) {
+			return body, header, err
+		}
+		timer := time.NewTimer(delays[attempt])
+		select {
+		case <-ctx.Done():
+			timer.Stop()
+			return body, header, err
+		case <-timer.C:
+		}
+	}
+}
+
+// isTransientReadFailure reports whether a read is worth repeating. A 429 is
+// retryable for the caller but not within seconds, since GitHub's rate-limit
+// window is longer than the retry delays.
+func isTransientReadFailure(err error, status int) bool {
+	return errors.Is(err, gitprovider.ErrRetryable) && status != http.StatusTooManyRequests
+}
+
+func (c *Client) doRESTOnce(ctx context.Context, op gitprovider.Operation, method, endpoint, token, accept string, out any) ([]byte, http.Header, int, error) {
 	req, err := http.NewRequestWithContext(ctx, method, endpoint, nil)
 	if err != nil {
-		return nil, nil, err
+		return nil, nil, 0, err
 	}
 	setHeaders(req, token, accept)
 	resp, err := c.httpClient.Do(req)
 	if err != nil {
-		return nil, nil, mapTransportError(op, err)
+		return nil, nil, 0, mapTransportError(op, err)
 	}
 	defer resp.Body.Close()
 	body, readErr := io.ReadAll(resp.Body)
 	if readErr != nil {
-		return nil, nil, gitprovider.WrapError(gitprovider.ErrRetryable, op, readErr)
+		return nil, nil, resp.StatusCode, gitprovider.WrapError(gitprovider.ErrRetryable, op, readErr)
 	}
 	if resp.StatusCode < 200 || resp.StatusCode > 299 {
-		return nil, resp.Header, mapHTTPStatus(op, resp.StatusCode, body)
+		return nil, resp.Header, resp.StatusCode, mapHTTPStatus(op, resp.StatusCode, body)
 	}
 	if out != nil {
 		if err := json.Unmarshal(body, out); err != nil {
-			return nil, resp.Header, fmt.Errorf("%w: decode GitHub response: %w", ErrValidation, err)
+			return nil, resp.Header, resp.StatusCode, fmt.Errorf("%w: decode GitHub response: %w", ErrValidation, err)
 		}
 	}
-	return body, resp.Header, nil
+	return body, resp.Header, resp.StatusCode, nil
 }
 
 func (c *Client) doRESTJSON(ctx context.Context, op gitprovider.Operation, method, endpoint string, in any, out any) error {

internal/gitprovider/github/http_retry_test.go

@@ -0,0 +1,113 @@
+package github
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"github.com/open-cli-collective/codereview-cli/internal/gitprovider"
+)
+
+// statusSequenceServer answers each request with the next status in statuses,
+// repeating the last one, and a file body on 200.
+func statusSequenceServer(t *testing.T, statuses ...int) (*httptest.Server, *atomic.Int32) {
+	t.Helper()
+	var requests atomic.Int32
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		n := int(requests.Add(1))
+		status := statuses[min(n, len(statuses))-1]
+		w.WriteHeader(status)
+		if status == http.StatusOK {
+			_, _ = w.Write([]byte("package main\n"))
+		}
+	}))
+	t.Cleanup(server.Close)
+	return server, &requests
+}
+
+func retryTestClient(t *testing.T, server *httptest.Server) *Client {
+	t.Helper()
+	client := mustClient(t, Options{Token: "token", BaseURL: server.URL, GraphQLURL: server.URL + "/graphql"})
+	client.readRetryDelays = []time.Duration{time.Millisecond, time.Millisecond}
+	return client
+}
+
+func TestGetFileAtRefRetriesTransientServerError(t *testing.T) {
+	server, requests := statusSequenceServer(t, http.StatusInternalServerError, http.StatusGatewayTimeout, http.StatusOK)
+	client := retryTestClient(t, server)
+
+	contents, err := client.GetFileAtRef(context.Background(), testPRRef(), "head-sha", "dir/file.go")
+	if err != nil {
+		t.Fatalf("GetFileAtRef: %v", err)
+	}
+	if string(contents) != "package main\n" {
+		t.Fatalf("contents = %q", contents)
+	}
+	if got := requests.Load(); got != 3 {
+		t.Fatalf("requests = %d, want 3", got)
+	}
+}
+
+func TestGetFileAtRefGivesUpAfterRetries(t *testing.T) {
+	server, requests := statusSequenceServer(t, http.StatusBadGateway)
+	client := retryTestClient(t, server)
+
+	_, err := client.GetFileAtRef(context.Background(), testPRRef(), "head-sha", "dir/file.go")
+	if !errors.Is(err, gitprovider.ErrRetryable) {
+		t.Fatalf("err = %v, want ErrRetryable", err)
+	}
+	if got := requests.Load(); got != 3 {
+		t.Fatalf("requests = %d, want 3 (one try and two retries)", got)
+	}
+}
+
+func TestGetFileAtRefDoesNotRetryNonTransientStatuses(t *testing.T) {
+	for _, status := range []int{http.StatusNotFound, http.StatusTooManyRequests, http.StatusUnauthorized} {
+		server, requests := statusSequenceServer(t, status, http.StatusOK)
+		client := retryTestClient(t, server)
+
+		if _, err := client.GetFileAtRef(context.Background(), testPRRef(), "head-sha", "dir/file.go"); err == nil {
+			t.Fatalf("status %d: err = nil, want an error", status)
+		}
+		if got := requests.Load(); got != 1 {
+			t.Fatalf("status %d: requests = %d, want 1", status, got)
+		}
+	}
+}
+
+func TestRESTWriteIsNotRetried(t *testing.T) {
+	server, requests := statusSequenceServer(t, http.StatusInternalServerError, http.StatusOK)
+	client := retryTestClient(t, server)
+
+	_, _, err := client.doREST(context.Background(), gitprovider.OperationGetFileAtRef, http.MethodPatch, server.URL+"/user/repository_invitations/1", acceptJSON, nil)
+	if !errors.Is(err, gitprovider.ErrRetryable) {
+		t.Fatalf("err = %v, want ErrRetryable", err)
+	}
+	if got := requests.Load(); got != 1 {
+		t.Fatalf("requests = %d, want 1", got)
+	}
+}
+
+func TestReadRetryStopsWhenContextIsCanceled(t *testing.T) {
+	server, requests := statusSequenceServer(t, http.StatusServiceUnavailable)
+	client := mustClient(t, Options{Token: "token", BaseURL: server.URL, GraphQLURL: server.URL + "/graphql"})
+	client.readRetryDelays = []time.Duration{time.Hour}
+	ctx, cancel := context.WithCancel(context.Background())
+	time.AfterFunc(10*time.Millisecond, cancel)
+
+	start := time.Now()
+	_, err := client.GetFileAtRef(ctx, testPRRef(), "head-sha", "dir/file.go")
+	if !errors.Is(err, gitprovider.ErrRetryable) {
+		t.Fatalf("err = %v, want the last ErrRetryable", err)
+	}
+	if elapsed := time.Since(start); elapsed > 5*time.Second {
+		t.Fatalf("elapsed = %s, want the wait to end with the context", elapsed)
+	}
+	if got := requests.Load(); got != 1 {
+		t.Fatalf("requests = %d, want 1", got)
+	}
+}