fix(forge): retry 5xx server errors at the HTTP client level

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 {

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"))

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 |