diff --git a/CLAUDE.md b/CLAUDE.md deleted file mode 100644 index 32b39573f1..0000000000 --- a/CLAUDE.md +++ /dev/null @@ -1,3 +0,0 @@ -# CLAUDE.md - -Project rules and instructions live in [AGENTS.md](AGENTS.md). Read that file now — it is the single source of truth for all agent-facing guidance in this repo. diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index b110b55c3d..5900e95556 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -145,7 +145,7 @@ func (c *LiveClient) do(ctx context.Context, method, path string, body any) (*ht retryAfter := resp.Header.Get("Retry-After") if attempt == maxRetries-1 { - msg := fmt.Sprintf("rate limited after %d retries on %s %s (last delay: %s", maxRetries, method, path, delay) + msg := fmt.Sprintf("retryable error after %d attempts on %s %s (last delay: %s", maxRetries, method, path, delay) if retryAfter != "" { msg += fmt.Sprintf(", Retry-After: %s", retryAfter) } @@ -167,11 +167,17 @@ func (c *LiveClient) do(ctx context.Context, method, path string, body any) (*ht // GitHub uses 429 for primary rate limits and 403 for secondary rate limits. // Secondary rate limits may include a Retry-After header, or may only be // identifiable by the response body containing "secondary rate limit". +// Server errors (500, 502, 503, 504) are also retried as transient failures. func isRetryable(resp *http.Response) (bool, []byte) { if resp.StatusCode == http.StatusTooManyRequests { io.Copy(io.Discard, resp.Body) return true, nil } + // Transient server errors. + if resp.StatusCode >= 500 && resp.StatusCode <= 504 { + io.Copy(io.Discard, resp.Body) + return true, nil + } if resp.StatusCode == http.StatusForbidden { if resp.Header.Get("Retry-After") != "" { io.Copy(io.Discard, resp.Body) @@ -466,7 +472,7 @@ func (c *LiveClient) CreateFileOnBranch(ctx context.Context, owner, repo, branch func (c *LiveClient) CreateOrUpdateFile(ctx context.Context, owner, repo, path, message string, content []byte) error { apiPath := fmt.Sprintf("/repos/%s/%s/contents/%s", owner, repo, path) - return c.retryOnTransient(ctx, path, func() error { + return c.retryOnRepoRace(ctx, path, func() error { // Try to get existing file for its SHA. existingResp, err := c.do(ctx, http.MethodGet, apiPath, nil) if err != nil { @@ -505,7 +511,7 @@ func (c *LiveClient) CreateOrUpdateFile(ctx context.Context, owner, repo, path, func (c *LiveClient) CreateOrUpdateFileOnBranch(ctx context.Context, owner, repo, branch, path, message string, content []byte) error { apiPath := fmt.Sprintf("/repos/%s/%s/contents/%s", owner, repo, path) - return c.retryOnTransient(ctx, path, func() error { + return c.retryOnRepoRace(ctx, path, func() error { // Try to get existing file on the branch for its SHA. existingResp, err := c.do(ctx, http.MethodGet, apiPath+"?ref="+branch, nil) if err != nil { @@ -540,10 +546,9 @@ func (c *LiveClient) CreateOrUpdateFileOnBranch(ctx context.Context, owner, repo } // putFileWithRetry wraps a single PUT to the Contents API with retry on -// transient errors (404 from async repo init, 409 from branch ref races, -// 502/503/504 from server-side infrastructure issues). +// repo race conditions (404 from async repo init, 409 from branch ref races). func (c *LiveClient) putFileWithRetry(ctx context.Context, apiPath string, payload map[string]any, path string) error { - return c.retryOnTransient(ctx, path, func() error { + return c.retryOnRepoRace(ctx, path, func() error { resp, err := c.put(ctx, apiPath, payload) if err != nil { return fmt.Errorf("create file %s: %w", path, err) @@ -553,12 +558,13 @@ func (c *LiveClient) putFileWithRetry(ctx context.Context, apiPath string, paylo }) } -// retryOnTransient retries an operation that may fail with transient HTTP -// errors. It handles 404 (async repo initialization), 409 (branch ref update -// races), and server-side 5xx errors (502, 503, 504) that indicate transient -// GitHub infrastructure issues. It uses linear backoff (2s between attempts) -// and up to 5 attempts (~10s total). -func (c *LiveClient) retryOnTransient(ctx context.Context, label string, fn func() error) error { +// retryOnRepoRace retries an operation that may fail due to GitHub +// repository initialization races. It handles 404 (async repo/branch +// creation where the ref is not yet materialized) and 409 (branch ref +// update conflicts). Server-side 5xx errors are handled at a lower level +// by do(). It uses linear backoff (2s between attempts) and up to 5 +// attempts (~10s total). +func (c *LiveClient) retryOnRepoRace(ctx context.Context, label string, fn func() error) error { const attempts = 5 const delay = 2 * time.Second @@ -590,16 +596,13 @@ func (c *LiveClient) retryOnTransient(ctx context.Context, label string, fn func } // isTransientStatus returns true for HTTP status codes that indicate a -// transient error worth retrying: 404 (async repo init), 409 (branch ref -// conflict), and server-side 500, 502, 503, 504 (GitHub infrastructure errors). +// repo/branch race condition worth retrying: 404 (async repo init) and +// 409 (branch ref conflict). Server-side 5xx errors are retried at a +// lower level by do(). func isTransientStatus(code int) bool { switch code { case http.StatusNotFound, - http.StatusConflict, - http.StatusInternalServerError, - http.StatusBadGateway, - http.StatusServiceUnavailable, - http.StatusGatewayTimeout: + http.StatusConflict: return true default: return false @@ -646,10 +649,10 @@ func (c *LiveClient) CommitFilesToBranch(ctx context.Context, owner, repo, branc // the Git Trees/Blobs/Commits API. func (c *LiveClient) commitFilesTo(ctx context.Context, owner, repo, branch, message string, files []forge.TreeFile) (bool, error) { // 1. Get current commit SHA from the branch ref. - // Wrapped in retryOnTransient for freshly-created repos/branches where + // Wrapped in retryOnRepoRace for freshly-created repos/branches where // the ref may not be materialized yet (async auto_init). var commitSHA string - if err := c.retryOnTransient(ctx, "get branch ref", func() error { + if err := c.retryOnRepoRace(ctx, "get branch ref", func() error { refResp, refErr := c.get(ctx, fmt.Sprintf("/repos/%s/%s/git/ref/heads/%s", owner, repo, branch)) if refErr != nil { return fmt.Errorf("get branch ref: %w", refErr) @@ -958,7 +961,7 @@ func (c *LiveClient) listDirContents(ctx context.Context, owner, repo, path, ref func (c *LiveClient) DeleteFile(ctx context.Context, owner, repo, path, message string) error { apiPath := fmt.Sprintf("/repos/%s/%s/contents/%s", owner, repo, path) - return c.retryOnTransient(ctx, path, func() error { + return c.retryOnRepoRace(ctx, path, func() error { // GET the file to obtain its SHA. existingResp, err := c.do(ctx, http.MethodGet, apiPath, nil) if err != nil { diff --git a/internal/forge/github/github_test.go b/internal/forge/github/github_test.go index 242fb9b5a3..1377562937 100644 --- a/internal/forge/github/github_test.go +++ b/internal/forge/github/github_test.go @@ -1288,27 +1288,24 @@ func TestListOrgRepos_Pagination(t *testing.T) { } func TestCreateOrUpdateFile_RetriesOn504(t *testing.T) { + // 5xx is now retried at the do() level, so the PUT is retried + // internally without re-running the GET. callNum := 0 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { callNum++ switch { case callNum == 1: - // First GET for existing file — return 404 (file doesn't exist) + // GET for existing file — return 404 (file doesn't exist) assert.Equal(t, "GET", r.Method) w.WriteHeader(http.StatusNotFound) json.NewEncoder(w).Encode(map[string]any{"message": "Not Found"}) case callNum == 2: - // First PUT — return 504 Gateway Timeout + // PUT — return 504 Gateway Timeout (do() will retry) assert.Equal(t, "PUT", r.Method) w.WriteHeader(http.StatusGatewayTimeout) json.NewEncoder(w).Encode(map[string]any{"message": "Gateway Timeout"}) case callNum == 3: - // Retry: GET for existing file — return 404 - assert.Equal(t, "GET", r.Method) - w.WriteHeader(http.StatusNotFound) - json.NewEncoder(w).Encode(map[string]any{"message": "Not Found"}) - case callNum == 4: - // Retry: PUT — succeeds + // do() retry: PUT — succeeds assert.Equal(t, "PUT", r.Method) w.WriteHeader(http.StatusCreated) json.NewEncoder(w).Encode(map[string]any{}) @@ -1321,10 +1318,12 @@ func TestCreateOrUpdateFile_RetriesOn504(t *testing.T) { client := newTestClient(t, srv) err := client.CreateOrUpdateFile(context.Background(), "owner", "repo", "test.txt", "add file", []byte("content")) require.NoError(t, err) - assert.Equal(t, 4, callNum, "expected exactly 4 calls (GET+PUT fail, GET+PUT succeed)") + assert.Equal(t, 3, callNum, "expected exactly 3 calls (GET, PUT fail, PUT retry succeed)") } func TestCreateOrUpdateFile_RetriesOnAll5xxCodes(t *testing.T) { + // 5xx is retried at the do() level. The PUT fails once, do() retries, + // and succeeds — without re-running the GET. for _, statusCode := range []int{ http.StatusBadGateway, http.StatusServiceUnavailable, @@ -1340,15 +1339,11 @@ func TestCreateOrUpdateFile_RetriesOnAll5xxCodes(t *testing.T) { w.WriteHeader(http.StatusNotFound) json.NewEncoder(w).Encode(map[string]any{"message": "Not Found"}) case callNum == 2: - // PUT — return 5xx + // PUT — return 5xx (do() will retry) w.WriteHeader(statusCode) json.NewEncoder(w).Encode(map[string]any{"message": http.StatusText(statusCode)}) case callNum == 3: - // Retry GET — 404 - w.WriteHeader(http.StatusNotFound) - json.NewEncoder(w).Encode(map[string]any{"message": "Not Found"}) - case callNum == 4: - // Retry PUT — succeeds + // do() retry: PUT — succeeds w.WriteHeader(http.StatusCreated) json.NewEncoder(w).Encode(map[string]any{}) } @@ -1358,7 +1353,7 @@ func TestCreateOrUpdateFile_RetriesOnAll5xxCodes(t *testing.T) { client := newTestClient(t, srv) err := client.CreateOrUpdateFile(context.Background(), "owner", "repo", "test.txt", "add", []byte("data")) require.NoError(t, err) - assert.GreaterOrEqual(t, callNum, 4, "should have retried after %d", statusCode) + assert.Equal(t, 3, callNum, "expected 3 calls (GET, PUT fail, PUT retry succeed) for %d", statusCode) }) } } @@ -1389,6 +1384,9 @@ func TestCreateOrUpdateFile_NoRetryOnNon5xx(t *testing.T) { } func TestCreateOrUpdateFile_MaxRetriesExceeded(t *testing.T) { + // 5xx errors are retried at the do() level, not retryOnRepoRace. + // With a persistent 504 on PUT, do() exhausts its 3 attempts and + // returns immediately — retryOnRepoRace does not retry 5xx. callNum := 0 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { callNum++ @@ -1407,21 +1405,55 @@ func TestCreateOrUpdateFile_MaxRetriesExceeded(t *testing.T) { client := newTestClient(t, srv) err := client.CreateOrUpdateFile(context.Background(), "owner", "repo", "test.txt", "add", []byte("data")) require.Error(t, err) - assert.Contains(t, err.Error(), "after 5 attempts") + assert.Contains(t, err.Error(), "retryable error after 3 attempts") } func TestIsTransientStatus(t *testing.T) { - transient := []int{404, 409, 500, 502, 503, 504} + // After moving 5xx retry to isRetryable in do(), isTransientStatus + // only covers race-condition statuses (404 async repo init, 409 ref conflict). + transient := []int{404, 409} for _, code := range transient { assert.True(t, isTransientStatus(code), "expected %d to be transient", code) } - nonTransient := []int{200, 201, 400, 401, 403, 422} + nonTransient := []int{200, 201, 400, 401, 403, 422, 500, 502, 503, 504} for _, code := range nonTransient { assert.False(t, isTransientStatus(code), "expected %d to not be transient", code) } } +func TestIsRetryable_ServerErrors(t *testing.T) { + for _, code := range []int{500, 502, 503, 504} { + resp := &http.Response{ + StatusCode: code, + Body: http.NoBody, + } + retryable, _ := isRetryable(resp) + assert.True(t, retryable, "expected %d to be retryable", code) + } +} + +func TestDo_RetriesOnServerError(t *testing.T) { + attempt := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempt++ + if attempt == 1 { + w.WriteHeader(http.StatusBadGateway) + fmt.Fprintln(w, `{"message":"Bad Gateway"}`) + return + } + w.WriteHeader(http.StatusOK) + fmt.Fprintln(w, `{"ok":true}`) + })) + defer srv.Close() + + client := newTestClient(t, srv) + resp, err := client.get(context.Background(), "/test") + require.NoError(t, err) + resp.Body.Close() + assert.Equal(t, 2, attempt, "expected exactly 2 attempts (1 retry)") +} + func TestBlobSHA(t *testing.T) { // printf "blob 5\0hello" | sha1sum got := blobSHA([]byte("hello")) diff --git a/qf-tests/GH-24/README.md b/qf-tests/GH-24/README.md new file mode 100644 index 0000000000..5ba6317113 --- /dev/null +++ b/qf-tests/GH-24/README.md @@ -0,0 +1,7 @@ +# QualityFlow Tests — GH-24 + +Generated by the QualityFlow pipeline. + +| Directory | Count | Framework | +|-----------|-------|-----------| +| `go/` | 8 files | Go | diff --git a/qf-tests/GH-24/go/do_retry_5xx_test.go b/qf-tests/GH-24/go/do_retry_5xx_test.go new file mode 100644 index 0000000000..abc3eae21d --- /dev/null +++ b/qf-tests/GH-24/go/do_retry_5xx_test.go @@ -0,0 +1,153 @@ +package github + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +/* +do() 5xx Retry Behavior Tests + +STP Reference: outputs/stp/GH-24/GH-24_test_plan.md +STD Reference: outputs/std/GH-24/GH-24_test_description.yaml +Jira: GH-24 +*/ + +// TestDoRetries502AndSucceeds validates that do() retries a 502 Bad Gateway +// and succeeds when the subsequent attempt returns 200. +// Covers: TS-GH-24-008 +func TestDoRetries502AndSucceeds(t *testing.T) { + // [test_id:TS-GH-24-008] Verify do() retries and succeeds after transient 502 + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := callCount.Add(1) + if n == 1 { + w.WriteHeader(http.StatusBadGateway) + return + } + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, `{"sha": "abc123"}`) + })) + defer srv.Close() + + client := newTestClient(t, srv) + resp, err := client.do(context.Background(), http.MethodGet, "/repos/org/repo/pulls/1", nil) + require.NoError(t, err) + require.NotNil(t, resp) + resp.Body.Close() + assert.Equal(t, int32(2), callCount.Load(), "expected exactly 2 HTTP calls (1 fail + 1 success)") +} + +// TestDoRetries503AndSucceeds validates that do() retries after receiving 503 +// Service Unavailable and succeeds on a subsequent attempt returning 200. +// Covers: TS-GH-24-009 +func TestDoRetries503AndSucceeds(t *testing.T) { + // [test_id:TS-GH-24-009] Verify do() retries and succeeds after transient 503 + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := callCount.Add(1) + if n == 1 { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, `{"ok": true}`) + })) + defer srv.Close() + + client := newTestClient(t, srv) + resp, err := client.do(context.Background(), http.MethodGet, "/repos/org/repo/contents/file", nil) + require.NoError(t, err) + require.NotNil(t, resp) + resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, int32(2), callCount.Load(), "expected exactly 2 HTTP calls") +} + +// TestDoExhaustsRetriesOnPersistent500 validates that do() exhausts all retry +// attempts when the server persistently returns 500 and returns an error. +// Covers: TS-GH-24-010 +func TestDoExhaustsRetriesOnPersistent500(t *testing.T) { + // [test_id:TS-GH-24-010] Verify do() exhausts retries and returns error after persistent 500 + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + client := newTestClient(t, srv) + _, err := client.do(context.Background(), http.MethodGet, "/repos/org/repo/pulls/1", nil) + require.Error(t, err) + // maxRetries=3, so exactly 3 attempts + assert.Equal(t, int32(maxRetries), callCount.Load(), "expected maxRetries total calls") +} + +// TestDoRespectsContextCancellation validates that do() stops retrying and +// returns a context error when the context is cancelled during backoff. +// Covers: TS-GH-24-012 +func TestDoRespectsContextCancellation(t *testing.T) { + // [test_id:TS-GH-24-012] Verify do() respects context cancellation during retry backoff + var callCount atomic.Int32 + ctx, cancel := context.WithCancel(context.Background()) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := callCount.Add(1) + w.WriteHeader(http.StatusBadGateway) + // Cancel context after first request so do() is cancelled during backoff + if n == 1 { + cancel() + } + })) + defer srv.Close() + defer cancel() // idempotent + + client := newTestClient(t, srv) + _, err := client.do(ctx, http.MethodGet, "/repos/org/repo/contents/file.txt", nil) + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled, "expected context.Canceled error") + // Should have made 1 request, then been cancelled during backoff + assert.LessOrEqual(t, callCount.Load(), int32(2), + "should not make many requests after cancellation") +} + +// TestDoBackoffHonorsRetryAfterFor429 validates that the backoff delay for +// rate-limited requests (429) respects the Retry-After header value. +// Covers: TS-GH-24-034 +func TestDoBackoffHonorsRetryAfterFor429(t *testing.T) { + // [test_id:TS-GH-24-034] Verify rate limit backoff timing uses Retry-After + var callCount atomic.Int32 + var timestamps []time.Time + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + timestamps = append(timestamps, time.Now()) + n := callCount.Add(1) + if n == 1 { + w.Header().Set("Retry-After", "1") + w.WriteHeader(http.StatusTooManyRequests) + return + } + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, `{}`) + })) + defer srv.Close() + + client := newTestClient(t, srv) + resp, err := client.do(context.Background(), http.MethodGet, "/repos/org/repo/pulls/1", nil) + require.NoError(t, err) + resp.Body.Close() + + require.Len(t, timestamps, 2, "expected exactly 2 requests") + elapsed := timestamps[1].Sub(timestamps[0]) + // Retry-After: 1 means at least 1 second backoff + assert.GreaterOrEqual(t, elapsed, 900*time.Millisecond, + "backoff should respect Retry-After header (>= ~1s)") +} diff --git a/qf-tests/GH-24/go/double_retry_test.go b/qf-tests/GH-24/go/double_retry_test.go new file mode 100644 index 0000000000..46ecc896d8 --- /dev/null +++ b/qf-tests/GH-24/go/double_retry_test.go @@ -0,0 +1,128 @@ +package github + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +/* +Double-Retry Prevention Tests + +STP Reference: outputs/stp/GH-24/GH-24_test_plan.md +STD Reference: outputs/std/GH-24/GH-24_test_description.yaml +Jira: GH-24 +*/ + +// TestCreateOrUpdateFileNoDoubleRetry504 validates that CreateOrUpdateFile +// with a 504 on PUT results in exactly 3 HTTP calls: GET(200) + PUT(504) + +// PUT(200). The retry happens only at do() level, not retryOnRepoRace. +// Covers: TS-GH-24-013 +func TestCreateOrUpdateFileNoDoubleRetry504(t *testing.T) { + // [test_id:TS-GH-24-013] Verify CreateOrUpdateFile with 504 retries only at do() level + var callCount atomic.Int32 + var putCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + if r.Method == http.MethodGet { + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{"sha": "abc123"}) + return + } + n := putCount.Add(1) + if n == 1 { + w.WriteHeader(http.StatusGatewayTimeout) + return + } + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{}) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.CreateOrUpdateFile(context.Background(), "org", "repo", "file.txt", "update file", []byte("content")) + require.NoError(t, err) + assert.Equal(t, int32(3), callCount.Load(), "expected GET + PUT(504) + PUT(200) = 3 calls") +} + +// TestCreateOrUpdateFileSingleLayerRetryAll5xx validates that CreateOrUpdateFile +// handles all 5xx status codes (500-504) with retries occurring only at the +// do() level, not duplicated by retryOnRepoRace. +// Covers: TS-GH-24-014 +func TestCreateOrUpdateFileSingleLayerRetryAll5xx(t *testing.T) { + tests := []struct { + name string + statusCode int + }{ + // [test_id:TS-GH-24-014] Verify single-layer retry for all 5xx codes + {"500 Internal Server Error", http.StatusInternalServerError}, + {"501 Not Implemented", 501}, + {"502 Bad Gateway", http.StatusBadGateway}, + {"503 Service Unavailable", http.StatusServiceUnavailable}, + {"504 Gateway Timeout", http.StatusGatewayTimeout}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var callCount atomic.Int32 + var putCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + if r.Method == http.MethodGet { + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{"sha": "abc123"}) + return + } + n := putCount.Add(1) + if n == 1 { + w.WriteHeader(tc.statusCode) + return + } + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{}) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.CreateOrUpdateFile(context.Background(), "org", "repo", "file.txt", "update", []byte("content")) + require.NoError(t, err) + assert.Equal(t, int32(3), callCount.Load(), "expected 3 calls for %d", tc.statusCode) + }) + } +} + +// TestPersistent5xxExhaustsDoRetryOnly validates that when do() exhausts all +// retries on a persistent 5xx error, retryOnRepoRace does not attempt +// additional retries. The error propagates directly to the caller. +// Covers: TS-GH-24-015 +func TestPersistent5xxExhaustsDoRetryOnly(t *testing.T) { + // [test_id:TS-GH-24-015] Verify retryOnRepoRace does not re-invoke on do()-exhausted 5xx + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + if r.Method == http.MethodGet { + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{"sha": "abc123"}) + return + } + w.WriteHeader(http.StatusServiceUnavailable) + fmt.Fprint(w, `{"message": "Service Unavailable"}`) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.CreateOrUpdateFile(context.Background(), "org", "repo", "file.txt", "update", []byte("content")) + require.Error(t, err) + assert.Contains(t, err.Error(), "retryable error after 3 attempts") + // 1 GET + maxRetries PUT attempts = 1 + 3 = 4 total calls + // If retryOnRepoRace added retries, total would be much higher + assert.Equal(t, int32(1+maxRetries), callCount.Load(), + "expected 1 GET + maxRetries PUTs (no retryOnRepoRace multiplier)") +} diff --git a/qf-tests/GH-24/go/error_messages_test.go b/qf-tests/GH-24/go/error_messages_test.go new file mode 100644 index 0000000000..60c43cd757 --- /dev/null +++ b/qf-tests/GH-24/go/error_messages_test.go @@ -0,0 +1,82 @@ +package github + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +/* +Retry Exhaustion Error Message Tests + +STP Reference: outputs/stp/GH-24/GH-24_test_plan.md +STD Reference: outputs/std/GH-24/GH-24_test_description.yaml +Jira: GH-24 +*/ + +// TestRetryExhaustionErrorContainsRetryableError validates that the error +// message contains "retryable error" and does NOT contain the old +// "rate limited" text. +// Covers: TS-GH-24-011, TS-GH-24-024 +func TestRetryExhaustionErrorContainsRetryableError(t *testing.T) { + // [test_id:TS-GH-24-011] Verify error message reads 'retryable error after N attempts' + // [test_id:TS-GH-24-024] Verify error message contains 'retryable error' (not 'rate limited') + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + client := newTestClient(t, srv) + _, err := client.do(context.Background(), http.MethodGet, "/repos/org/repo/pulls/1", nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "retryable error", + "error message should contain 'retryable error'") + assert.True(t, !strings.Contains(err.Error(), "rate limited"), + "error message should NOT contain old 'rate limited' text") + assert.Contains(t, err.Error(), "attempts", + "error message should include attempt count") +} + +// TestRetryExhaustionErrorContainsMethodAndPath validates that the error +// message includes the HTTP method and request path for debugging. +// Covers: TS-GH-24-025 +func TestRetryExhaustionErrorContainsMethodAndPath(t *testing.T) { + // [test_id:TS-GH-24-025] Verify error includes method, path, and delay information + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + client := newTestClient(t, srv) + _, err := client.do(context.Background(), http.MethodGet, "/repos/org/repo/contents/file", nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "GET", + "error message should include HTTP method") + assert.Contains(t, err.Error(), "/repos/org/repo/contents/file", + "error message should include request path") + assert.Contains(t, err.Error(), "delay", + "error message should include delay information") +} + +// TestRetryExhaustionErrorIncludesRetryAfter validates that when a 5xx +// response includes a Retry-After header, the error message incorporates it. +// Covers: TS-GH-24-026 +func TestRetryExhaustionErrorIncludesRetryAfter(t *testing.T) { + // [test_id:TS-GH-24-026] Verify error includes Retry-After header value when present + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Retry-After", "1") + w.WriteHeader(http.StatusServiceUnavailable) + })) + defer srv.Close() + + client := newTestClient(t, srv) + _, err := client.do(context.Background(), http.MethodGet, "/repos/org/repo/contents/file", nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "Retry-After", + "error message should include Retry-After header information") +} diff --git a/qf-tests/GH-24/go/file_operations_test.go b/qf-tests/GH-24/go/file_operations_test.go new file mode 100644 index 0000000000..d753d7c26f --- /dev/null +++ b/qf-tests/GH-24/go/file_operations_test.go @@ -0,0 +1,189 @@ +package github + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +/* +File Operations Integration Tests + +STP Reference: outputs/stp/GH-24/GH-24_test_plan.md +STD Reference: outputs/std/GH-24/GH-24_test_description.yaml +Jira: GH-24 +*/ + +// TestCreateOrUpdateFileSucceedsFirstAttempt validates the happy path where +// CreateOrUpdateFile succeeds on the first attempt without any retries. +// Covers: TS-GH-24-027 +func TestCreateOrUpdateFileSucceedsFirstAttempt(t *testing.T) { + // [test_id:TS-GH-24-027] Verify CreateOrUpdateFile succeeds on first attempt + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + switch r.Method { + case http.MethodGet: + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{"sha": "abc123"}) + case http.MethodPut: + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{}) + } + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.CreateOrUpdateFile(context.Background(), "org", "repo", "file.txt", "create file", []byte("hello")) + require.NoError(t, err) + assert.Equal(t, int32(2), callCount.Load(), "expected exactly 2 requests (1 GET + 1 PUT)") +} + +// TestCreateOrUpdateFileOnBranchRetries404 validates that +// CreateOrUpdateFileOnBranch retries when encountering a 404 (repo/branch not +// yet initialized) via the retryOnRepoRace wrapper. The 404 on PUT triggers +// an APIError which retryOnRepoRace catches and retries the whole operation. +// Covers: TS-GH-24-028 +func TestCreateOrUpdateFileOnBranchRetries404(t *testing.T) { + // [test_id:TS-GH-24-028] Verify CreateOrUpdateFileOnBranch retries on 404 via retryOnRepoRace + var totalCalls atomic.Int32 + var putCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + totalCalls.Add(1) + switch r.Method { + case http.MethodGet: + // GET always returns 404 (file not found) — this is fine, + // CreateOrUpdateFileOnBranch treats it as "create new file". + w.WriteHeader(http.StatusNotFound) + fmt.Fprint(w, `{"message": "Not Found"}`) + case http.MethodPut: + n := putCount.Add(1) + if n == 1 { + // First PUT returns 404 (branch not ready yet) + w.WriteHeader(http.StatusNotFound) + fmt.Fprint(w, `{"message": "Not Found"}`) + return + } + // Second PUT succeeds + w.WriteHeader(http.StatusCreated) + json.NewEncoder(w).Encode(map[string]any{}) + } + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.CreateOrUpdateFileOnBranch(context.Background(), "org", "repo", "feature-branch", "file.txt", "update", []byte("content")) + require.NoError(t, err) + assert.Greater(t, putCount.Load(), int32(1), + "expected retryOnRepoRace to retry after PUT returned 404") +} + +// TestDeleteFileRetries409 validates that DeleteFile retries when encountering +// a 409 Conflict (branch ref conflict during concurrent operations) via +// retryOnRepoRace. +// Covers: TS-GH-24-029 +func TestDeleteFileRetries409(t *testing.T) { + // [test_id:TS-GH-24-029] Verify DeleteFile retries on 409 via retryOnRepoRace + var callCount atomic.Int32 + var deleteCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + switch r.Method { + case http.MethodGet: + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{"sha": "abc123"}) + case http.MethodDelete: + n := deleteCount.Add(1) + if n == 1 { + // First DELETE returns 409 (branch ref conflict) + w.WriteHeader(http.StatusConflict) + fmt.Fprint(w, `{"message": "Conflict"}`) + return + } + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{}) + } + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.DeleteFile(context.Background(), "org", "repo", "file.txt", "delete file") + require.NoError(t, err) + assert.Greater(t, deleteCount.Load(), int32(1), + "expected DeleteFile to retry on 409") +} + +// TestPutFileWithRetryNonTransientPassthrough validates that putFileWithRetry +// does not retry on non-transient errors like 400 or 422, passing them +// through immediately to the caller. +// Covers: TS-GH-24-030 +func TestPutFileWithRetryNonTransientPassthrough(t *testing.T) { + // [test_id:TS-GH-24-030] Verify putFileWithRetry passes through non-transient errors + tests := []struct { + name string + statusCode int + }{ + {"422 Unprocessable Entity", http.StatusUnprocessableEntity}, + {"400 Bad Request", http.StatusBadRequest}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + w.WriteHeader(tc.statusCode) + fmt.Fprintf(w, `{"message": "Error %d"}`, tc.statusCode) + })) + defer srv.Close() + + client := newTestClient(t, srv) + payload := map[string]any{ + "message": "test", + "content": "dGVzdA==", + } + err := client.putFileWithRetry(context.Background(), "/repos/org/repo/contents/file.txt", payload, "file.txt") + require.Error(t, err) + // Only 1 call to do() (no retry for non-transient) + assert.Equal(t, int32(1), callCount.Load(), + "expected no retry for non-transient %d error", tc.statusCode) + }) + } +} + +// TestCreateOrUpdateFileRetriesPUTNotGET validates that when PUT fails with +// 5xx at do()-level, only the PUT is retried, not the entire GET+PUT sequence. +func TestCreateOrUpdateFileRetriesPUTNotGET(t *testing.T) { + var getCount atomic.Int32 + var putCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet: + getCount.Add(1) + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{"sha": "abc123"}) + case http.MethodPut: + n := putCount.Add(1) + if n == 1 { + w.WriteHeader(http.StatusBadGateway) + return + } + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{}) + } + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.CreateOrUpdateFile(context.Background(), "org", "repo", "file.txt", "update", []byte("content")) + require.NoError(t, err) + assert.Equal(t, int32(1), getCount.Load(), "GET should be called exactly once") + assert.Equal(t, int32(2), putCount.Load(), "PUT should be called twice (1 fail + 1 retry)") +} diff --git a/qf-tests/GH-24/go/isretryable_5xx_test.go b/qf-tests/GH-24/go/isretryable_5xx_test.go new file mode 100644 index 0000000000..342de5fcea --- /dev/null +++ b/qf-tests/GH-24/go/isretryable_5xx_test.go @@ -0,0 +1,104 @@ +package github + +import ( + "io" + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +/* +isRetryable 5xx Status Code Tests + +STP Reference: outputs/stp/GH-24/GH-24_test_plan.md +STD Reference: outputs/std/GH-24/GH-24_test_description.yaml +Jira: GH-24 +*/ + +// TestIsRetryableReturnsTrue5xx validates that isRetryable returns true for +// all 5xx status codes in the retryable range (500-504). +// Covers: TS-GH-24-001 through TS-GH-24-005 +func TestIsRetryableReturnsTrue5xx(t *testing.T) { + tests := []struct { + name string + statusCode int + testID string + }{ + // [test_id:TS-GH-24-001] Verify isRetryable returns true for HTTP 500 + {"500 Internal Server Error", http.StatusInternalServerError, "TS-GH-24-001"}, + // [test_id:TS-GH-24-005] Verify isRetryable returns true for HTTP 501 + {"501 Not Implemented", 501, "TS-GH-24-005"}, + // [test_id:TS-GH-24-002] Verify isRetryable returns true for HTTP 502 + {"502 Bad Gateway", http.StatusBadGateway, "TS-GH-24-002"}, + // [test_id:TS-GH-24-003] Verify isRetryable returns true for HTTP 503 + {"503 Service Unavailable", http.StatusServiceUnavailable, "TS-GH-24-003"}, + // [test_id:TS-GH-24-004] Verify isRetryable returns true for HTTP 504 + {"504 Gateway Timeout", http.StatusGatewayTimeout, "TS-GH-24-004"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + resp := &http.Response{ + StatusCode: tc.statusCode, + Body: http.NoBody, + } + result, _ := isRetryable(resp) + assert.True(t, result, "[%s] expected isRetryable to return true for %d", tc.testID, tc.statusCode) + }) + } +} + +// TestIsRetryableDrainsBodyOn5xx verifies that isRetryable drains the response +// body when it encounters a 5xx status code. This prevents connection leaks +// from undrained response bodies. +// Covers: TS-GH-24-006 +func TestIsRetryableDrainsBodyOn5xx(t *testing.T) { + // [test_id:TS-GH-24-006] Verify isRetryable drains response body on 5xx + bodyContent := "test-body-data-that-should-be-drained" + body := io.NopCloser(strings.NewReader(bodyContent)) + + resp := &http.Response{ + StatusCode: http.StatusBadGateway, + Body: body, + } + + retryable, _ := isRetryable(resp) + require.True(t, retryable, "expected isRetryable to return true for 502") + + // After isRetryable, the body should be fully drained. + buf := make([]byte, 1) + n, err := resp.Body.Read(buf) + assert.Equal(t, 0, n, "expected 0 bytes remaining after body drain") + assert.Equal(t, io.EOF, err, "expected io.EOF after body drain") +} + +// TestIsRetryableReturnsFalseNonRetryable validates that isRetryable returns +// false for HTTP status codes that should NOT trigger retries. +// Covers: TS-GH-24-007 +func TestIsRetryableReturnsFalseNonRetryable(t *testing.T) { + tests := []struct { + name string + statusCode int + }{ + // [test_id:TS-GH-24-007] Verify isRetryable returns false for 400, 401, 404, 422 + {"400 Bad Request", http.StatusBadRequest}, + {"401 Unauthorized", http.StatusUnauthorized}, + {"404 Not Found", http.StatusNotFound}, + {"422 Unprocessable Entity", http.StatusUnprocessableEntity}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + resp := &http.Response{ + StatusCode: tc.statusCode, + Header: http.Header{}, + Body: io.NopCloser(strings.NewReader("")), + } + result, _ := isRetryable(resp) + assert.False(t, result, "expected isRetryable to return false for %d", tc.statusCode) + }) + } +} diff --git a/qf-tests/GH-24/go/non_retryable_test.go b/qf-tests/GH-24/go/non_retryable_test.go new file mode 100644 index 0000000000..3f83c19bf8 --- /dev/null +++ b/qf-tests/GH-24/go/non_retryable_test.go @@ -0,0 +1,80 @@ +package github + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +/* +Non-Retryable HTTP Error Tests + +STP Reference: outputs/stp/GH-24/GH-24_test_plan.md +STD Reference: outputs/std/GH-24/GH-24_test_description.yaml +Jira: GH-24 + +Note: do() returns (*http.Response, nil) for non-retryable status codes. +The status code error is surfaced by higher-level methods (get/put/post) +via checkStatus(). These tests verify that no retry occurs by checking +callCount == 1. +*/ + +// TestNonRetryableStatusCodesReturnImmediately validates that do() returns +// the response without retrying for non-retryable client error codes +// (400, 401, 404, 422). +// Covers: TS-GH-24-007 +func TestNonRetryableStatusCodesReturnImmediately(t *testing.T) { + tests := []struct { + name string + statusCode int + body string + }{ + // [test_id:TS-GH-24-007] Verify isRetryable returns false for 400, 401, 404, 422 + {"400 Bad Request", http.StatusBadRequest, `{"message": "Bad Request"}`}, + {"401 Unauthorized", http.StatusUnauthorized, `{"message": "Bad credentials"}`}, + {"404 Not Found", http.StatusNotFound, `{"message": "Not Found"}`}, + {"422 Unprocessable Entity", http.StatusUnprocessableEntity, `{"message": "Validation Failed"}`}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + w.WriteHeader(tc.statusCode) + fmt.Fprint(w, tc.body) + })) + defer srv.Close() + + client := newTestClient(t, srv) + resp, err := client.do(context.Background(), http.MethodGet, "/repos/org/repo/pulls/1", nil) + require.NoError(t, err, "do() should not return error for non-retryable status codes") + require.NotNil(t, resp) + resp.Body.Close() + assert.Equal(t, tc.statusCode, resp.StatusCode) + assert.Equal(t, int32(1), callCount.Load(), "expected no retry for %d", tc.statusCode) + }) + } +} + +// TestNonRetryableResponseBodyPreserved validates that the response body +// is preserved and accessible for non-retryable status codes. +func TestNonRetryableResponseBodyPreserved(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnprocessableEntity) + fmt.Fprint(w, `{"message": "Validation Failed", "errors": [{"resource": "PullRequest", "field": "title", "code": "missing_field"}]}`) + })) + defer srv.Close() + + client := newTestClient(t, srv) + _, err := client.get(context.Background(), "/repos/org/repo/pulls") + require.Error(t, err) + assert.Contains(t, err.Error(), "Validation Failed") +} diff --git a/qf-tests/GH-24/go/ratelimit_retry_test.go b/qf-tests/GH-24/go/ratelimit_retry_test.go new file mode 100644 index 0000000000..34890db1c3 --- /dev/null +++ b/qf-tests/GH-24/go/ratelimit_retry_test.go @@ -0,0 +1,72 @@ +package github + +import ( + "io" + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +/* +Rate Limit Retry Tests (429, 403 Secondary) + +STP Reference: outputs/stp/GH-24/GH-24_test_plan.md +STD Reference: outputs/std/GH-24/GH-24_test_description.yaml +Jira: GH-24 +*/ + +// TestIsRetryable429TooManyRequests validates that isRetryable returns true +// for 429 Too Many Requests (GitHub primary rate limit). +// Covers: TS-GH-24-031 +func TestIsRetryable429TooManyRequests(t *testing.T) { + // [test_id:TS-GH-24-031] Verify isRetryable still returns true for 429 + resp := &http.Response{ + StatusCode: http.StatusTooManyRequests, + Body: http.NoBody, + } + result, _ := isRetryable(resp) + assert.True(t, result, "expected isRetryable to return true for 429") +} + +// TestIsRetryable403WithRetryAfterHeader validates that isRetryable returns +// true for 403 with Retry-After header (GitHub secondary rate limit signal). +// Covers: TS-GH-24-032 +func TestIsRetryable403WithRetryAfterHeader(t *testing.T) { + // [test_id:TS-GH-24-032] Verify isRetryable returns true for 403 with Retry-After header + resp := &http.Response{ + StatusCode: http.StatusForbidden, + Header: http.Header{"Retry-After": {"60"}}, + Body: http.NoBody, + } + result, _ := isRetryable(resp) + assert.True(t, result, "expected isRetryable to return true for 403 with Retry-After") +} + +// TestIsRetryable403WithoutRetryAfterNotRetried validates that isRetryable +// returns false for 403 without any rate limit indicators. +func TestIsRetryable403WithoutRetryAfterNotRetried(t *testing.T) { + resp := &http.Response{ + StatusCode: http.StatusForbidden, + Header: http.Header{}, + Body: io.NopCloser(strings.NewReader("Resource not accessible by personal access token")), + } + result, _ := isRetryable(resp) + assert.False(t, result, "expected isRetryable to return false for 403 without rate limit") +} + +// TestIsRetryable403SecondaryRateLimitInBody validates that isRetryable detects +// GitHub's secondary rate limit signal in the response body for 403 responses +// without a Retry-After header. +// Covers: TS-GH-24-033 +func TestIsRetryable403SecondaryRateLimitInBody(t *testing.T) { + // [test_id:TS-GH-24-033] Verify isRetryable detects secondary rate limit in body + resp := &http.Response{ + StatusCode: http.StatusForbidden, + Header: http.Header{}, + Body: io.NopCloser(strings.NewReader(`{"message":"You have exceeded a secondary rate limit"}`)), + } + result, _ := isRetryable(resp) + assert.True(t, result, "expected isRetryable to return true for 403 with secondary rate limit in body") +} diff --git a/qf-tests/GH-24/go/transient_status_test.go b/qf-tests/GH-24/go/transient_status_test.go new file mode 100644 index 0000000000..8d7173158f --- /dev/null +++ b/qf-tests/GH-24/go/transient_status_test.go @@ -0,0 +1,215 @@ +package github + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +/* +isTransientStatus and retryOnRepoRace Tests + +STP Reference: outputs/stp/GH-24/GH-24_test_plan.md +STD Reference: outputs/std/GH-24/GH-24_test_description.yaml +Jira: GH-24 +*/ + +// TestIsTransientStatusTrue404And409 validates that isTransientStatus returns +// true for 404 (async repo init) and 409 (branch ref conflict). +// Covers: TS-GH-24-021, TS-GH-24-022 +func TestIsTransientStatusTrue404And409(t *testing.T) { + tests := []struct { + name string + statusCode int + testID string + }{ + // [test_id:TS-GH-24-021] Verify isTransientStatus returns true for 404 + {"404 Not Found", http.StatusNotFound, "TS-GH-24-021"}, + // [test_id:TS-GH-24-022] Verify isTransientStatus returns true for 409 + {"409 Conflict", http.StatusConflict, "TS-GH-24-022"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result := isTransientStatus(tc.statusCode) + assert.True(t, result, "[%s] expected isTransientStatus to return true for %d", + tc.testID, tc.statusCode) + }) + } +} + +// TestIsTransientStatusFalse5xx validates that isTransientStatus returns false +// for all 5xx status codes, since 5xx retries are now handled by do(). +// Covers: TS-GH-24-023 +func TestIsTransientStatusFalse5xx(t *testing.T) { + tests := []struct { + name string + statusCode int + }{ + // [test_id:TS-GH-24-023] Verify isTransientStatus returns false for 500-504 + {"500 Internal Server Error", http.StatusInternalServerError}, + {"502 Bad Gateway", http.StatusBadGateway}, + {"503 Service Unavailable", http.StatusServiceUnavailable}, + {"504 Gateway Timeout", http.StatusGatewayTimeout}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result := isTransientStatus(tc.statusCode) + assert.False(t, result, "expected isTransientStatus to return false for %d", tc.statusCode) + }) + } +} + +// TestRetryOnRepoRaceRetries404 validates that retryOnRepoRace retries when +// it encounters a 404 (async repo initialization race condition). +// Covers: TS-GH-24-016 +func TestRetryOnRepoRaceRetries404(t *testing.T) { + // [test_id:TS-GH-24-016] Verify retryOnRepoRace retries on 404 + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := callCount.Add(1) + if n <= 1 { + w.WriteHeader(http.StatusNotFound) + fmt.Fprint(w, `{"message": "Not Found"}`) + return + } + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{"sha": "abc123"}) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.retryOnRepoRace(context.Background(), "test-404", func() error { + resp, err := client.do(context.Background(), http.MethodGet, "/repos/org/repo/contents/file.txt", nil) + if err != nil { + return err + } + if err := checkStatus(resp, http.StatusOK); err != nil { + return err + } + resp.Body.Close() + return nil + }) + require.NoError(t, err) + assert.Greater(t, callCount.Load(), int32(1), "expected retryOnRepoRace to retry on 404") +} + +// TestRetryOnRepoRaceRetries409 validates that retryOnRepoRace retries when +// it encounters a 409 Conflict (branch ref conflict). +// Covers: TS-GH-24-017 +func TestRetryOnRepoRaceRetries409(t *testing.T) { + // [test_id:TS-GH-24-017] Verify retryOnRepoRace retries on 409 + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := callCount.Add(1) + if n <= 1 { + w.WriteHeader(http.StatusConflict) + fmt.Fprint(w, `{"message": "Conflict"}`) + return + } + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{"sha": "abc123"}) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.retryOnRepoRace(context.Background(), "test-409", func() error { + resp, err := client.do(context.Background(), http.MethodGet, "/repos/org/repo/contents/file.txt", nil) + if err != nil { + return err + } + if err := checkStatus(resp, http.StatusOK); err != nil { + return err + } + resp.Body.Close() + return nil + }) + require.NoError(t, err) + assert.Greater(t, callCount.Load(), int32(1), "expected retryOnRepoRace to retry on 409") +} + +// TestRetryOnRepoRaceDoesNotRetry500 validates that retryOnRepoRace does NOT +// retry when do() returns a 500 error (after do() has exhausted its retries). +// Covers: TS-GH-24-018 +func TestRetryOnRepoRaceDoesNotRetry500(t *testing.T) { + // [test_id:TS-GH-24-018] Verify retryOnRepoRace does not retry on 500 + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.retryOnRepoRace(context.Background(), "test-500", func() error { + _, innerErr := client.do(context.Background(), http.MethodGet, "/repos/org/repo/contents/file.txt", nil) + return innerErr + }) + require.Error(t, err) + // do() makes maxRetries=3 calls. retryOnRepoRace should NOT add more. + assert.Equal(t, int32(maxRetries), callCount.Load(), + "expected only do()-level retries (maxRetries calls), not retryOnRepoRace retries") +} + +// TestRetryOnRepoRaceDoesNotRetry502 validates that retryOnRepoRace does NOT +// retry when do() returns a 502 error. +// Covers: TS-GH-24-019 +func TestRetryOnRepoRaceDoesNotRetry502(t *testing.T) { + // [test_id:TS-GH-24-019] Verify retryOnRepoRace does not retry on 502 + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + w.WriteHeader(http.StatusBadGateway) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.retryOnRepoRace(context.Background(), "test-502", func() error { + _, innerErr := client.do(context.Background(), http.MethodGet, "/repos/org/repo/contents/file.txt", nil) + return innerErr + }) + require.Error(t, err) + assert.Equal(t, int32(maxRetries), callCount.Load(), + "expected only do()-level retries, not retryOnRepoRace retries for 502") +} + +// TestRetryOnRepoRaceExhaustsAndReturnsWrappedError validates that +// retryOnRepoRace exhausts its retry attempts when the underlying operation +// persistently returns 404, and returns a wrapped error with context. +// Covers: TS-GH-24-020 +func TestRetryOnRepoRaceExhaustsAndReturnsWrappedError(t *testing.T) { + // [test_id:TS-GH-24-020] Verify retryOnRepoRace exhausts attempts and returns wrapped error + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + w.WriteHeader(http.StatusNotFound) + fmt.Fprint(w, `{"message": "Not Found"}`) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.retryOnRepoRace(context.Background(), "test-exhaust", func() error { + resp, innerErr := client.do(context.Background(), http.MethodGet, "/repos/org/repo/contents/file.txt", nil) + if innerErr != nil { + return innerErr + } + if statusErr := checkStatus(resp, http.StatusOK); statusErr != nil { + return statusErr + } + resp.Body.Close() + return nil + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "after", + "error should contain retry exhaustion context") + assert.Contains(t, err.Error(), "attempts", + "error should indicate number of attempts") +}