From 2217aea2303c0d4caf6a816bc4f41f29b3d95248 Mon Sep 17 00:00:00 2001 From: the-gigi Date: Fri, 25 Apr 2025 09:55:59 -0700 Subject: [PATCH] add exponential retry logic if Github API returns 500 --- internal/github/github.go | 85 +++++++++- internal/github/github_test.go | 295 +++++++++++++++++++++++++++++++++ version.txt | 2 +- 3 files changed, 379 insertions(+), 3 deletions(-) create mode 100644 internal/github/github_test.go diff --git a/internal/github/github.go b/internal/github/github.go index 53de468..6a49211 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -2,6 +2,9 @@ package github import ( "context" + "errors" + "fmt" + "time" "github.com/google/go-github/v38/github" "golang.org/x/oauth2" @@ -27,6 +30,8 @@ type Client interface { type client struct { ghc *github.Client + maxRetries int + retryDelay time.Duration } func NewClient(ctx context.Context, token string) Client { @@ -36,13 +41,89 @@ func NewClient(ctx context.Context, token string) Client { AccessToken: token, }, ))), + maxRetries: 5, + retryDelay: 1 * time.Second, } } func (c *client) GetCombinedStatus(ctx context.Context, owner, repo, ref string, opts *ListOptions) (*CombinedStatus, *Response, error) { - return c.ghc.Repositories.GetCombinedStatus(ctx, owner, repo, ref, opts) + var statusResp *CombinedStatus + var resp *Response + var err error + + for attempt := 0; attempt < c.maxRetries; attempt++ { + statusResp, resp, err = c.ghc.Repositories.GetCombinedStatus(ctx, owner, repo, ref, opts) + if err == nil { + return statusResp, resp, nil + } + + // Check if context is canceled or deadline exceeded before retrying + if ctx.Err() != nil { + return nil, resp, fmt.Errorf("context error while getting combined status: %w", ctx.Err()) + } + + // Don't retry on deadline exceeded errors + if errors.Is(err, context.DeadlineExceeded) { + return nil, resp, err + } + + // Only retry on 5xx server errors + if resp != nil && (resp.StatusCode < 500 || resp.StatusCode > 599) { + return statusResp, resp, err + } + + // Wait with exponential backoff before retrying + if attempt < c.maxRetries-1 { + backoffDuration := c.retryDelay * time.Duration(1< 599) { + return checksResp, resp, err + } + + // Wait with exponential backoff before retrying + if attempt < c.maxRetries-1 { + backoffDuration := c.retryDelay * time.Duration(1<= 400 { + resp := &Response{ + Response: &http.Response{ + StatusCode: code, + }, + } + return nil, resp, errors.New("API error") + } + } + + // Default success response + return &CombinedStatus{ + TotalCount: github.Int(1), + Statuses: []*RepoStatus{ + { + Context: github.String("test"), + State: github.String("success"), + }, + }, + }, &Response{ + Response: &http.Response{ + StatusCode: 200, + }, + }, nil +} + +func (m *MockClient) ListCheckRunsForRef(ctx context.Context, owner, repo, ref string, opts *ListCheckRunsOptions) (*ListCheckRunsResults, *Response, error) { + // Count this call + m.ListCheckRunsForRefCalls++ + + if m.ShouldTimeout { + return nil, nil, context.DeadlineExceeded + } + + // Determine the response based on the current call index + callIndex := m.ListCheckRunsForRefCalls - 1 + if callIndex < len(m.StatusCodes) { + code := m.StatusCodes[callIndex] + if code >= 400 { + resp := &Response{ + Response: &http.Response{ + StatusCode: code, + }, + } + return nil, resp, errors.New("API error") + } + } + + // Default success response + return &ListCheckRunsResults{ + Total: github.Int(1), + CheckRuns: []*CheckRun{ + { + Name: github.String("test"), + Status: github.String("completed"), + Conclusion: github.String("success"), + }, + }, + }, &Response{ + Response: &http.Response{ + StatusCode: 200, + }, + }, nil +} + +func TestGetCombinedStatusRetry(t *testing.T) { + tests := map[string]struct { + statusCodes []int + expectedCalls int + shouldSucceed bool + shouldTimeout bool + }{ + "success on first try": { + statusCodes: []int{200}, + expectedCalls: 1, + shouldSucceed: true, + }, + "retry once then succeed": { + statusCodes: []int{500, 200}, + expectedCalls: 2, + shouldSucceed: true, + }, + "retry twice then succeed": { + statusCodes: []int{500, 500, 200}, + expectedCalls: 3, + shouldSucceed: true, + }, + "fail after max retries": { + statusCodes: []int{500, 500, 500, 500, 500, 500}, + expectedCalls: 5, // maxRetries + shouldSucceed: false, + }, + "don't retry on 4xx errors": { + statusCodes: []int{404}, + expectedCalls: 1, + shouldSucceed: false, + }, + "don't retry on rate limits": { + statusCodes: []int{403}, + expectedCalls: 1, + shouldSucceed: false, + }, + "timeout during retry": { + shouldTimeout: true, + expectedCalls: 1, + shouldSucceed: false, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + mockClient := &MockClient{ + StatusCodes: tc.statusCodes, + ShouldTimeout: tc.shouldTimeout, + } + + ctx := context.Background() + var result *CombinedStatus + var resp *Response + var err error + + // Use modified version of the retry code for testing + for attempt := 0; attempt < 5; attempt++ { + result, resp, err = mockClient.GetCombinedStatus(ctx, "owner", "repo", "ref", &ListOptions{}) + if err == nil { + break + } + + // For timeout error, don't retry + if errors.Is(err, context.DeadlineExceeded) { + break + } + + // Only retry on 5xx server errors + if resp != nil && (resp.StatusCode < 500 || resp.StatusCode > 599) { + break + } + + // Don't actually sleep in tests, just continue to next attempt + if attempt == 4 { + break + } + } + + // Verify results + if tc.shouldSucceed { + if err != nil { + t.Errorf("Expected success but got error: %v", err) + } + if result == nil { + t.Error("Expected result but got nil") + } + } else { + if err == nil { + t.Error("Expected error but got success") + } + } + + if mockClient.GetCombinedStatusCalls != tc.expectedCalls { + t.Errorf("Expected %d API calls, got %d", tc.expectedCalls, mockClient.GetCombinedStatusCalls) + } + }) + } +} + +func TestListCheckRunsForRefRetry(t *testing.T) { + tests := map[string]struct { + statusCodes []int + expectedCalls int + shouldSucceed bool + shouldTimeout bool + }{ + "success on first try": { + statusCodes: []int{200}, + expectedCalls: 1, + shouldSucceed: true, + }, + "retry once then succeed": { + statusCodes: []int{500, 200}, + expectedCalls: 2, + shouldSucceed: true, + }, + "retry twice then succeed": { + statusCodes: []int{500, 500, 200}, + expectedCalls: 3, + shouldSucceed: true, + }, + "fail after max retries": { + statusCodes: []int{500, 500, 500, 500, 500, 500}, + expectedCalls: 5, // maxRetries + shouldSucceed: false, + }, + "don't retry on 4xx errors": { + statusCodes: []int{404}, + expectedCalls: 1, + shouldSucceed: false, + }, + "don't retry on rate limits": { + statusCodes: []int{403}, + expectedCalls: 1, + shouldSucceed: false, + }, + "timeout during retry": { + shouldTimeout: true, + expectedCalls: 1, + shouldSucceed: false, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + mockClient := &MockClient{ + StatusCodes: tc.statusCodes, + ShouldTimeout: tc.shouldTimeout, + } + + ctx := context.Background() + var result *ListCheckRunsResults + var resp *Response + var err error + + // Use modified version of the retry code for testing + for attempt := 0; attempt < 5; attempt++ { + result, resp, err = mockClient.ListCheckRunsForRef(ctx, "owner", "repo", "ref", &ListCheckRunsOptions{}) + if err == nil { + break + } + + // For timeout error, don't retry + if errors.Is(err, context.DeadlineExceeded) { + break + } + + // Only retry on 5xx server errors + if resp != nil && (resp.StatusCode < 500 || resp.StatusCode > 599) { + break + } + + // Don't actually sleep in tests, just continue to next attempt + if attempt == 4 { + break + } + } + + // Verify results + if tc.shouldSucceed { + if err != nil { + t.Errorf("Expected success but got error: %v", err) + } + if result == nil { + t.Error("Expected result but got nil") + } + } else { + if err == nil { + t.Error("Expected error but got success") + } + } + + if mockClient.ListCheckRunsForRefCalls != tc.expectedCalls { + t.Errorf("Expected %d API calls, got %d", tc.expectedCalls, mockClient.ListCheckRunsForRefCalls) + } + }) + } +} \ No newline at end of file diff --git a/version.txt b/version.txt index 24e56e0..06043b8 100644 --- a/version.txt +++ b/version.txt @@ -1 +1 @@ -v1.2.1 \ No newline at end of file +v1.2.2 \ No newline at end of file