fix(#2432): retry merge on 409 after updating PR branch#2434
Conversation
When MergeChangeProposal gets a 409 "Head branch is out of date", call GitHub's update-branch endpoint to sync the PR branch with the base, wait briefly, then retry the merge. Up to 3 attempts before giving up. Non-409 errors are still returned immediately. Closes #2432 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
PR Summary by QodoRetry PR merge after updating branch on 409 “out of date” conflicts Description
Diagram
High-Level Assessment
Files changed (2)
|
Site previewPreview: https://f9b65d81-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1. range maxAttempts invalid loop
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| mergePath := fmt.Sprintf("/repos/%s/%s/pulls/%d/merge", owner, repo, number) | ||
| updatePath := fmt.Sprintf("/repos/%s/%s/pulls/%d/update-branch", owner, repo, number) | ||
|
|
||
| for attempt := range maxAttempts { |
There was a problem hiding this comment.
1. range maxattempts invalid loop 📘 Rule violation ≡ Correctness
MergeChangeProposal uses for attempt := range maxAttempts, which is invalid Go syntax and will fail compilation. This will cause make go-vet (and therefore linting in CI) to fail for this PR.
Agent Prompt
## Issue description
`MergeChangeProposal` contains an invalid Go loop: `for attempt := range maxAttempts { ... }`, which will not compile.
## Issue Context
This change is intended to retry merge attempts up to `maxAttempts` times, so the loop should iterate `attempt` from 0 to `maxAttempts-1`.
## Fix Focus Areas
- internal/forge/github/github.go[2060-2091]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Update the PR branch to incorporate base branch changes. | ||
| updateResp, updateErr := c.do(ctx, http.MethodPut, updatePath, map[string]string{}) | ||
| if updateErr == nil { | ||
| updateResp.Body.Close() | ||
| } | ||
|
|
||
| if attempt < maxAttempts-1 { | ||
| select { | ||
| case <-time.After(3 * time.Second): | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| } | ||
| } |
There was a problem hiding this comment.
2. Update-branch errors ignored 🐞 Bug ≡ Correctness
MergeChangeProposal calls the PR update-branch endpoint via do() but never checks the HTTP status code nor returns updateErr, so it can keep retrying merges even when the branch update was rejected/rate-limited and ultimately return a misleading “branch remained out of date” error. It also performs an update-branch call on the final 409 attempt even though no subsequent merge retry will occur.
Agent Prompt
## Issue description
`MergeChangeProposal` uses `c.do()` for `PUT .../update-branch` and ignores both (a) non-2xx HTTP status codes and (b) `updateErr`. Since `do()` does not treat non-2xx as errors, the code can proceed as if the branch update succeeded when it actually failed, and it may perform an unnecessary final `update-branch` call even when no further merge retry will happen.
## Issue Context
- `do()` returns `(*http.Response, nil)` for non-2xx responses; status validation must be done via `checkStatus` (or an equivalent helper).
- `update-branch` commonly returns `202 Accepted`, so the status checker should accept 202.
## Fix Focus Areas
- internal/forge/github/github.go[2059-2092]
- internal/forge/github/github.go[95-165]
- internal/forge/github/github.go[216-236]
## Suggested fix approach
1. Only call `update-branch` when `attempt < maxAttempts-1` (i.e., when a retry will actually occur).
2. After `updateResp, updateErr := c.do(...)`:
- If `updateErr != nil`, return a wrapped error (e.g., `fmt.Errorf("update pull request #%d branch: %w", number, updateErr)`).
- Otherwise `defer updateResp.Body.Close()` and validate status with `checkStatus(updateResp, http.StatusAccepted, http.StatusOK, http.StatusNoContent)` (whatever is correct for your usage).
- If status validation fails, return a wrapped error so the caller sees the real cause.
3. Consider preserving the last merge 409 `err` and including it in the final failure message for better diagnostics (optional but helpful).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
🤖 Finished Review · ✅ Success · Started 4:34 PM UTC · Completed 4:56 PM UTC |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Low
Labels: PR fixes a bug in the GitHub forge client that caused flaky e2e test failures |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| return fmt.Errorf("merge pull request #%d: %w", number, err) | ||
| } | ||
|
|
||
| // Update the PR branch to incorporate base branch changes. |
There was a problem hiding this comment.
[medium] error-handling-gap
The update-branch call's error is silently discarded. If c.do() returns an error (network failure, context cancellation, rate limit exhaustion), the code proceeds to sleep and retry the merge with no indication that the update failed. When all retries are exhausted, the final error message says 'branch remained out of date' with no context about update-branch failures.
Suggested fix: At minimum, log the update error. Consider including the last update-branch error in the final returned error message when all retries are exhausted.
| return fmt.Errorf("merge pull request #%d: %w", number, err) | ||
| } | ||
|
|
||
| // Update the PR branch to incorporate base branch changes. |
There was a problem hiding this comment.
[low] edge-case
On the final loop iteration (attempt == maxAttempts-1), when the merge fails with 409, the code still calls update-branch even though the loop will not execute another merge attempt. Wasted work and unnecessary API call.
Suggested fix: Move the update-branch call inside the if attempt < maxAttempts-1 block.
| default: | ||
| t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) | ||
| w.WriteHeader(http.StatusNotFound) | ||
| } |
There was a problem hiding this comment.
[low] test-inadequate
TestMergeChangeProposal_409PersistsAfterRetries asserts mergeAttempts.Load() > 1 but the contract is exactly 3 attempts. A weaker assertion would pass even if retry logic changed to only retry once.
Suggested fix: Use assert.Equal(t, int32(3), mergeAttempts.Load()) for precise verification.
| updateResp, updateErr := c.do(ctx, http.MethodPut, updatePath, map[string]string{}) | ||
| if updateErr == nil { | ||
| updateResp.Body.Close() | ||
| } |
There was a problem hiding this comment.
[low] pattern-inconsistency
Body close is conditional (if updateErr == nil). The safer defensive pattern is if updateResp != nil { updateResp.Body.Close() }.
| @@ -2054,13 +2054,41 @@ func (c *LiveClient) DismissPullRequestReview(ctx context.Context, owner, repo s | |||
| } | |||
|
|
|||
| // MergeChangeProposal squash-merges a pull request by number. | |||
There was a problem hiding this comment.
[low] doc-style
Function comment documents retry behavior but omits the delay duration (3 seconds) and total potential delay (~9s). Compare with retryOnTransient which documents timing details.
Suggested fix: Add timing details to the comment: 3-second delay, up to 3 retries (~9s total).
fullsend-ai-review
Bot
added
requires-manual-review
1 participant
Summary
MergeChangeProposalgets a 409 "Head branch is out of date", it now calls GitHub'sPUT .../pulls/{n}/update-branchto sync the PR with the base, waits 3s, then retries the merge (up to 3 attempts).TestAdminInstallUninstallfailure at the enrollment PR merge step.Closes #2432
Test plan
TestMergeChangeProposal_Success— happy path unchangedTestMergeChangeProposal_409UpdatesBranchAndRetries— 409 triggers update-branch then successful retryTestMergeChangeProposal_NonConflictErrorNotRetried— 422 not retriedTestMergeChangeProposal_409PersistsAfterRetries— gives up after max attempts with clear error🤖 Generated with Claude Code