fix(forge): retry 5xx server errors at the HTTP client level by ralphbean · Pull Request #2342 · fullsend-ai/fullsend

ralphbean · 2026-06-16T16:15:27Z

Summary

Moves 5xx (500-504) retry handling from retryOnTransient down into isRetryable in do(), so all GitHub API calls automatically retry on transient server errors
Renames retryOnTransient → retryOnRepoRace and narrows isTransientStatus to only 404/409, reflecting its actual purpose (repo init races and branch ref conflicts)
Fixes the 502 Bad Gateway failure seen in this run, where GetPullRequestHeadSHA had no retry coverage

Test plan

New TestIsRetryable_ServerErrors — verifies isRetryable returns true for 500/502/503/504
New TestDo_RetriesOnServerError — verifies do() retries a 502 and succeeds on next attempt
Updated existing 5xx retry tests to reflect new call patterns (retry happens inside do(), not at the wrapper level)
Full ./internal/forge/... test suite passes

🤖 Generated with Claude Code

Move 5xx retry handling from the higher-level retryOnTransient wrapper (now renamed retryOnRepoRace) down into isRetryable, which is used by do(). This ensures all GitHub API calls automatically retry on transient server errors (500-504), not just the handful of call sites that were wrapped in retryOnTransient. This fixes a 502 Bad Gateway failure in post-review's GetPullRequestHeadSHA, which had no retry coverage because it called get() directly. Rename retryOnTransient to retryOnRepoRace and narrow isTransientStatus to only cover 404 (async repo init) and 409 (branch ref conflict), which are the race conditions that wrapper actually exists for. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

github-actions · 2026-06-16T16:17:23Z

Site preview

Preview: https://8ae8c2d3-site.fullsend-ai.workers.dev

Commit: 1e985c93b2a6e17e55a17460f50d8507903c53f7

fullsend-ai-review · 2026-06-16T16:20:13Z

🤖 Finished Review · ✅ Success · Started 4:20 PM UTC · Completed 4:32 PM UTC
Commit: 78302ba · View workflow run →

codecov · 2026-06-16T16:20:51Z

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/forge/github/github.go	84.61%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

fullsend-ai-review · 2026-06-16T16:31:54Z

Review

Findings

Medium

[logic-error] internal/forge/github/github.go:174 — The new 5xx check resp.StatusCode >= 500 && resp.StatusCode <= 504 inadvertently retries HTTP 501 (Not Implemented), which is not a transient error and was not retried by the previous isTransientStatus implementation (which explicitly listed 500, 502, 503, 504). The doc comment also says "500, 502, 503, 504" but the range includes 501. Retrying a 501 wastes time and delays the eventual error.
Remediation: Use an explicit switch or check for the four specific codes (500, 502, 503, 504) instead of a range, matching the original behavior and the doc comment.

Low

[error-message-consistency] internal/forge/github/github.go:149 — The error message says "retryable error after %d attempts" but line 164 says "exhausted retries". Minor inconsistency, though line 164 is unreachable.
[error-message-accuracy] internal/forge/github/github.go:149 — The new error message "retryable error after %d attempts" is generic and loses the distinction between rate limits (429/403) and server errors (5xx). The APIError struct carries the status code programmatically, so this is a minor diagnostic convenience issue.
[test-adequacy] internal/forge/github/github_test.go:1429 — TestCreateOrUpdateFile_MaxRetriesExceeded does not assert the total HTTP call count. Other tests in this PR (e.g., TestCreateOrUpdateFile_RetriesOn504, TestDo_RetriesOnServerError) do assert on call counts, making this omission inconsistent.

Info

[naming-alignment] internal/forge/github/github.go:596 — isTransientStatus now only checks 404/409 repo race conditions. Consider renaming to isRepoRaceStatus for full consistency with retryOnRepoRace.
[error-handling-gap] internal/forge/github/github.go:148 — The error message format string has a missing closing parenthesis in the base string, with ) appended later via concatenation. Output is correct but the pattern is fragile. Pre-existing, not introduced by this PR.
[terminology-consistency] internal/forge/github/github.go:148 — Error message uses "attempts" but the constant is maxRetries. The semantics are correct (maxRetries=3 means 3 attempts).

Labels: PR fixes a 502 Bad Gateway retry bug in the forge GitHub client.

Previous run

Review

Findings

Critical

[logic-error] internal/forge/github/github.go:823 — The diff renames retryOnTransient to retryOnRepoRace but does not update two call sites in DeleteFiles: line 823 (c.retryOnTransient(ctx, "get branch ref", ...)) and line 931 (c.retryOnTransient(ctx, "update ref", ...)). Since the method definition is renamed, these two references will cause a compilation error.
Remediation: Update both call sites in DeleteFiles to use c.retryOnRepoRace(ctx, ...) instead of c.retryOnTransient(ctx, ...).

Low

[test-adequacy] internal/forge/github/github_test.go:1415 — TestCreateOrUpdateFile_MaxRetriesExceeded should verify the total HTTP call count (assert.Equal(t, 4, callNum) for 1 GET + 3 PUT attempts) to confirm the expected retry behavior matches the new architecture where do() handles 5xx retries.
[test-adequacy] internal/forge/github/github_test.go:1440 — TestDo_RetriesOnServerError incurs real wall-clock delay from the exponential backoff in do(), with no overridable hook (unlike secondaryRateLimitBackoff). This will accumulate test-suite runtime.
[scope-coherence] internal/forge/github/github.go — The PR uses fix(forge) prefix but includes architectural refactoring (rename, narrowing isTransientStatus, moving 5xx retry to do()). The fix prefix is defensible since the change addresses a concrete gap where GetPullRequestHeadSHA had no 5xx retry coverage.

Info

[missing-authorization] internal/forge/github/github.go — No linked issue for a non-trivial change. The PR body references a specific CI failure which provides sufficient context.
[naming-alignment] internal/forge/github/github.go — isTransientStatus now only checks 404/409 repo race conditions. Consider renaming to isRepoRaceStatus for full consistency with retryOnRepoRace.
[terminology-consistency] internal/forge/github/github.go:148 — Error message uses "attempts" but the constant is maxRetries. The semantics are correct (maxRetries=3 means 3 attempts).
[error-handling-gap] internal/forge/github/github.go:148 — The error message format string has a missing closing parenthesis in the base string, with ) appended later via concatenation. Output is correct but the pattern is fragile. Pre-existing, not introduced by this PR.

Previous run (2)

Review

Findings

Medium

[error-handling-gap] internal/forge/github/github.go:148 — After this PR, do() retries 5xx errors up to maxRetries (3) times and then returns an APIError with the 5xx status code. retryOnRepoRace calls isTransientStatus which no longer matches 5xx, so there is no double-retry today. However, if someone later re-adds 5xx to isTransientStatus, a 5xx failure would be retried 3 times in do() and then up to 5 more times in retryOnRepoRace (potentially 15 total inner attempts). Adding a comment documenting this invariant would be a low-cost safeguard.
Remediation: Add a code comment on the APIError returned by do() at line 153 noting that the caller must not retry 5xx because do() has already exhausted retries, or wrap it in a sentinel type that retryOnRepoRace explicitly excludes.

Low

[documentation-completeness] internal/forge/github/github.go:170 — The comment for isRetryable() says "Server errors (500, 502, 503, 504)" but the range check resp.StatusCode >= 500 && resp.StatusCode <= 504 also includes 501 (Not Implemented), which is not documented. HTTP 501 is generally not transient and arguably should not be retried.
Remediation: Either update the comment to list all five codes (500–504 inclusive) or change the condition to explicitly check only the intended codes.
[test-adequacy] internal/forge/github/github_test.go — TestDo_RetriesOnServerError incurs at least 1s of real wall-clock delay from the exponential backoff in do(), with no overridable hook (unlike secondaryRateLimitBackoff which tests can override). This will accumulate test-suite runtime.
[terminology-consistency] internal/forge/github/github.go:148 — Error message uses "attempts" but the constant is maxRetries. Minor inconsistency, though the value semantics are correct.
[scope-coherence] internal/forge/github/github.go:147 — The generalized error message "retryable error after %d attempts" is more accurate than the old "rate limited" message now that do() retries more than just rate limits. Consider whether distinguishing rate-limit vs server-error exhaustion would aid debugging.
[missing-authorization] internal/forge/github/github.go — No linked issue for a non-trivial change. The PR body references a specific CI failure which provides sufficient context, but linking an issue would improve traceability.
[commit-message-alignment] — PR title uses fix(forge) which is defensible since the change corrects a defect (missing retry on GetPullRequestHeadSHA), though the structural refactoring (renaming retryOnTransient, narrowing isTransientStatus) could also support refactor(forge).

Info

[architectural-alignment] internal/forge/github/github.go:167 — Moving 5xx retry into do() strengthens the forge abstraction by centralizing retry logic. All forge.Client methods automatically benefit. The separation of concerns — do() for HTTP-level retries, retryOnRepoRace for GitHub-specific race conditions — is architecturally sound.
[edge-case] internal/forge/github/github.go:173 — The 500–504 range check correctly excludes 505+ status codes, which are typically non-transient.

fullsend-ai-review · 2026-06-18T15:28:37Z

🤖 Finished Review · ✅ Success · Started 3:28 PM UTC · Completed 3:40 PM UTC
Commit: 8e9ba31 · View workflow run →

fullsend-ai-review

See the review comment for full details.

fullsend-ai-review · 2026-06-18T15:40:16Z

+	// 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) {


[low] test-adequacy

TestCreateOrUpdateFile_MaxRetriesExceeded should verify the total HTTP call count to confirm the expected retry behavior matches the new architecture where do() handles 5xx retries.

fullsend-ai-review · 2026-06-18T15:40:17Z

+	// 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)


[low] test-adequacy

TestDo_RetriesOnServerError incurs real wall-clock delay from the exponential backoff in do(), with no overridable hook (unlike secondaryRateLimitBackoff). This will accumulate test-suite runtime.

fullsend-ai-review · 2026-06-18T15:40:17Z

@@ -146,7 +146,7 @@ func (c *LiveClient) do(ctx context.Context, method, path string, body any) (*ht
 		retryAfter := resp.Header.Get("Retry-After")


[low] scope-coherence

The PR uses fix(forge) prefix but includes architectural refactoring. The fix prefix is defensible since the change addresses a concrete gap where GetPullRequestHeadSHA had no 5xx retry coverage.

fullsend-ai-review · 2026-06-18T15:40:17Z

@@ -591,16 +597,13 @@ func (c *LiveClient) retryOnTransient(ctx context.Context, label string, fn func
 }


[info] naming-alignment

isTransientStatus now only checks 404/409 repo race conditions. Consider renaming to isRepoRaceStatus for full consistency with retryOnRepoRace.

fullsend-ai-review · 2026-06-18T15:40:17Z

@@ -146,7 +146,7 @@ func (c *LiveClient) do(ctx context.Context, method, path string, body any) (*ht
 		retryAfter := resp.Header.Get("Retry-After")

 		if attempt == maxRetries-1 {


[info] terminology-consistency

Error message uses 'attempts' but the constant is maxRetries. The semantics are correct (maxRetries=3 means 3 attempts).

fullsend-ai-review · 2026-06-18T15:40:17Z

@@ -146,7 +146,7 @@ func (c *LiveClient) do(ctx context.Context, method, path string, body any) (*ht
 		retryAfter := resp.Header.Get("Retry-After")

 		if attempt == maxRetries-1 {


[info] error-handling-gap

The error message format string has a missing closing parenthesis in the base string, with ) appended later via concatenation. Output is correct but the pattern is fragile. Pre-existing, not introduced by this PR.

Two call sites in commitFilesTo were missed during the rename, causing build failures. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

fullsend-ai-review · 2026-06-18T16:21:38Z

🤖 Finished Review · ❌ Failure · Started 4:21 PM UTC · Completed 4:33 PM UTC
Commit: 1e985c9 · View workflow run →

fullsend-ai-retro · 2026-06-18T17:56:44Z

🤖 Finished Retro · ✅ Success · Started 5:56 PM UTC · Completed 6:05 PM UTC
Commit: 1e985c9 · View workflow run →

fullsend-ai-retro · 2026-06-18T18:05:34Z

Retro: PR #2342 — fix(forge): retry 5xx server errors at the HTTP client level

What happened

Human-authored PR by ralphbean to move 5xx retry handling from retryOnTransient into do(), fixing a 502 Bad Gateway gap where GetPullRequestHeadSHA had no retry coverage. The review agent ran 3 times across 3 commits.

Timeline:

First review (commit 78302ba) — found medium/low findings, no critical issues at this stage.
Second review (commit 8e9ba31) — caught a Critical compilation error: two DeleteFiles call sites still referenced the old retryOnTransient after the rename. This was a genuine, high-value catch that would have broken the build.
Human reviewer approved at 07:32 UTC on June 18.
Third review (commit 1e985c9) — posted CHANGES_REQUESTED at 15:40 UTC (8+ hours after human approval) with only medium/low findings about the >= 500 && <= 504 range including HTTP 501.
One review run (27773457418) failed for unclear reasons.
PR merged at 17:51 UTC.

Assessment

Review quality was strong. The critical compilation error catch on the second review was exactly the kind of finding that justifies automated reviews — it prevented a broken build from merging.

Three patterns worth noting, all covered by existing issues:

Severity inconsistency — The 501 range-check finding was classified Low ("documentation-completeness") in one run and Medium ("logic-error") in the next, on unchanged code. → Already tracked in #1389.
CHANGES_REQUESTED after human approval — The bot posted CHANGES_REQUESTED with only medium/low findings, overriding a human approval made 8 hours earlier. → Already tracked in #2115.
Repeated findings across runs — After the critical was fixed, subsequent reviews largely repeated the same medium/low findings without adding new value. → Covered by #1500 and #963.

No new proposals filed — all identified improvements are already tracked in existing open issues.

github-actions Bot deployed to site-preview June 16, 2026 16:17 View deployment

fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 16, 2026

guyoron1 mentioned this pull request Jun 17, 2026

fix(forge): retry 5xx server errors at the HTTP client level guyoron1/fullsend#24

Closed

rh-hemartin approved these changes Jun 18, 2026

View reviewed changes

ralphbean added this pull request to the merge queue Jun 18, 2026

ralphbean removed this pull request from the merge queue due to a manual request Jun 18, 2026

Merge branch 'main' into fix/retry-5xx-in-do

8e9ba31

ralphbean enabled auto-merge June 18, 2026 15:25

github-actions Bot deployed to site-preview June 18, 2026 15:26 View deployment

fullsend-ai-review Bot suggested changes Jun 18, 2026

View reviewed changes

fullsend-ai-review Bot removed the requires-manual-review Review requires human judgment label Jun 18, 2026

fix: rename remaining retryOnTransient calls to retryOnRepoRace

1e985c9

Two call sites in commitFilesTo were missed during the rename, causing build failures. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

github-actions Bot deployed to site-preview June 18, 2026 16:19 View deployment

ralphbean added this pull request to the merge queue Jun 18, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 18, 2026

ralphbean added this pull request to the merge queue Jun 18, 2026

Merged via the queue into main with commit 4d83c42 Jun 18, 2026
10 checks passed

ralphbean deleted the fix/retry-5xx-in-do branch June 18, 2026 17:51

qodo-code-review Bot mentioned this pull request Jun 22, 2026

fix(#2489): retry scaffold push on non-fast-forward (auto_init race) #2491

Merged

qodo-code-review Bot mentioned this pull request Jun 29, 2026

fix(github): increase retry attempts and add jitter to backoff #2764

Merged

6 tasks

qodo-code-review Bot mentioned this pull request Jun 30, 2026

fix(github): detect primary rate limits returned as 403 #2776

Merged

5 tasks

		@@ -146,7 +146,7 @@ func (c LiveClient) do(ctx context.Context, method, path string, body any) (ht
		retryAfter := resp.Header.Get("Retry-After")

		@@ -591,16 +597,13 @@ func (c *LiveClient) retryOnTransient(ctx context.Context, label string, fn func
		}

		@@ -146,7 +146,7 @@ func (c LiveClient) do(ctx context.Context, method, path string, body any) (ht
		retryAfter := resp.Header.Get("Retry-After")

		if attempt == maxRetries-1 {

Uh oh!

Conversation

ralphbean commented Jun 16, 2026

Summary

Test plan

Uh oh!

github-actions Bot commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Site preview

Uh oh!

fullsend-ai-review Bot commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fullsend-ai-review Bot commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

Medium

Low

Info

Review

Findings

Critical

Low

Info

Review

Findings

Medium

Low

Info

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fullsend-ai-retro Bot commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-retro Bot commented Jun 18, 2026

Retro: PR #2342 — fix(forge): retry 5xx server errors at the HTTP client level

What happened

Assessment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

github-actions Bot commented Jun 16, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 16, 2026 •

edited

Loading

codecov Bot commented Jun 16, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 16, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 18, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 18, 2026 •

edited

Loading

fullsend-ai-retro Bot commented Jun 18, 2026 •

edited

Loading