diff --git a/internal/gitprovider/github/client.go b/internal/gitprovider/github/client.go index 4fe0419c..4b43520d 100644 --- a/internal/gitprovider/github/client.go +++ b/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. diff --git a/internal/gitprovider/github/http.go b/internal/gitprovider/github/http.go index 878be122..4ce2d630 100644 --- a/internal/gitprovider/github/http.go +++ b/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 { diff --git a/internal/gitprovider/github/http_retry_test.go b/internal/gitprovider/github/http_retry_test.go new file mode 100644 index 00000000..dae90308 --- /dev/null +++ b/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) + } +}