Retry transient git-fetch failures during evals-state branch checkout - #50058
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage: #50058
Draft PR, no CI runs yet. Addresses an intermittent git-fetch flake affecting the evals-state pipeline.
|
There was a problem hiding this comment.
Pull request overview
Adds retry protection for transient failures when checking out eval-state branches.
Changes:
- Adds exponential-backoff checkout retries.
- Adds unit tests for success, recovery, and exhaustion.
- Exports the retry helper for testing.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/push_experiment_state.cjs |
Implements and invokes checkout retries. |
actions/setup/js/push_experiment_state.test.cjs |
Tests retry behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| } catch (err) { | ||
| lastError = err; | ||
| if (attempt < maxRetries) { |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #50058 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). The PR modifies only 2 files with no changes in src/, lib/, pkg/, internal/, app/, core/, domain/, services/, or api/ directories. |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two issues.
📋 Key Themes & Highlights
Key Themes
- Retry scope too broad:
checkoutOrCreateBranchWithRetrycatches and retries every error fromcheckoutOrCreateBranch, including non-transient failures (auth errors, invalid repo, permission denied). The PR description says only transient fetch failures are retried, but the implementation does not enforce that boundary. - Test coverage gap: Tests cover success, transient-retry-then-succeed, and exhausted-retries paths, but no test documents/pins the behaviour for non-transient errors.
Positive Highlights
- ✅ Clean injection of
checkoutFnvia options — makes unit testing straightforward without mocking globals - ✅
baseDelayMs: 0in tests eliminates real waiting — good test hygiene - ✅ Exponential backoff matches the existing push-step pattern, keeping the codebase consistent
- ✅ Detailed JSDoc explaining what is and is not retried
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 24 AIC · ⌖ 10.8 AIC · ⊞ 7.1K
Comment /matt to run again
| expect(mockCore.warning).toHaveBeenCalledTimes(2); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] Missing test: non-transient errors (auth, invalid repo) are retried just like transient network errors — a test should document this behaviour or the implementation should distinguish error types.
💡 Suggested test
it("retries permission errors even though they are non-transient (documents current behaviour)", async () => {
const checkoutFn = vi.fn().mockImplementation(() => {
throw new Error("ERROR: Repository not found.");
});
await expect(
checkoutOrCreateBranchWithRetry("evals/myworkflow", "...url...", "/tmp/workdir", {
checkoutFn, baseDelayMs: 0, maxRetries: 2,
})
).rejects.toThrow();
expect(checkoutFn).toHaveBeenCalledTimes(3); // retried 3x despite being non-transient
});If the intent is to retry everything, this test locks in that design choice. If only transient errors should be retried, an error classifier is needed.
@copilot please address this.
There was a problem hiding this comment.
The retry wrapper is a good fix and the test coverage is solid. The existing inline comment correctly identifies the one gap: checkoutOrCreateBranchWithRetry retries all non-missing-ref errors, not just transient network errors — auth failures, bad token, or local workspace errors will also be retried 3× before failing. Consider adding a retryable error predicate (matching patterns like 502, timeout, RPC failed) to avoid masking fast-fail conditions with spurious retries and delays.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21.3 AIC · ⌖ 9.28 AIC · ⊞ 5.4K
There was a problem hiding this comment.
REQUEST_CHANGES: retry wrapper can corrupt checkout state on partial-mutation failures
The new checkoutOrCreateBranchWithRetry fixes the reported transient-fetch-failure bug, but introduces a real correctness risk: it retries the entire checkoutOrCreateBranch function, including its orphan-branch creation path, which performs non-idempotent local mutations (checkout --orphan, read-tree --empty, workspace file deletion). A failure partway through that path (e.g. a filesystem error during rmSync) leaves the workspace in a state where blindly re-invoking checkoutOrCreateBranch from scratch can throw a different, confusing error or silently proceed against an inconsistent checkout.
Themes and additional notes
- There is also a pre-existing open review comment (from Copilot, unaddressed) flagging that the retry wrapper retries all errors (auth failures, invalid refs, etc.), not just transient network failures — this compounds the above issue since non-transient errors could also trigger retries against a half-mutated workspace.
- Test coverage for the new retry wrapper is solid for the "all transient" and "all persistent" cases, but doesn't cover the orphan-branch/partial-mutation retry scenario described above.
- The push-step retry pattern this mirrors doesn't have this same idempotency risk since a push failure doesn't mutate local git state destructively before failing.
🔎 Code quality review by PR Code Quality Reviewer · auto · 42.8 AIC · ⌖ 3.79 AIC · ⊞ 7.9K
Comment /review to run again
| * Wraps checkoutOrCreateBranch with retry-with-backoff so transient network | ||
| * failures during the initial `git fetch` (e.g. HTTP 502s or timeouts against | ||
| * the git remote) don't immediately fail the job. Genuine "missing ref" | ||
| * conditions are handled inside checkoutOrCreateBranch itself and are not | ||
| * retried here since they are resolved deterministically (orphan branch | ||
| * creation), not by retrying. | ||
| * | ||
| * @param {string} branchName | ||
| * @param {string} repoUrl | ||
| * @param {string} workspaceDir | ||
| * @param {{maxRetries?: number, baseDelayMs?: number, checkoutFn?: typeof checkoutOrCreateBranch}} [options] | ||
| * @returns {Promise<string>} | ||
| */ | ||
| async function checkoutOrCreateBranchWithRetry(branchName, repoUrl, workspaceDir, options = {}) { | ||
| const { maxRetries = 3, baseDelayMs = 1000, checkoutFn = checkoutOrCreateBranch } = options; | ||
| let lastError; | ||
| for (let attempt = 0; attempt <= maxRetries; attempt++) { | ||
| try { | ||
| return checkoutFn(branchName, repoUrl, workspaceDir); |
There was a problem hiding this comment.
Retrying after a partial orphan-branch mutation can leave the workspace/repo in a state where the retry itself fails differently or corrupts the checkout.
💡 Explanation and suggested fix
checkoutOrCreateBranch is not idempotent: on the orphan-branch path it runs checkout --orphan, read-tree --empty, and deletes workspace files as side effects before returning. If any of these steps throws (e.g. disk error, git lock contention) after the branch has already been switched to orphan and/or files removed, the wrapper will blindly retry the whole function from scratch. On retry, checkoutOrCreateBranch re-enters the fetch path but the workspace is already in the orphan-branch/partially-cleared state from the prior attempt, so the retry can hit a different error (e.g. "already on branch X", stale index state) instead of a clean transient-network retry, or silently proceed with a half-cleaned working tree.
Suggested fix: make checkoutOrCreateBranch resilient to being re-entered mid-mutation (e.g. git reset --hard / re-checkout at the start of each attempt), or restrict the retry wrapper to only retry errors thrown by the initial fetch step (before any local mutation occurs), rather than retrying the whole function.
// current: retries whole function including orphan-branch mutation
return checkoutFn(branchName, repoUrl, workspaceDir);
// safer: only retry the network fetch, keep local mutation single-attempt
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (3 tests)
Test Classification
DetailsAll three tests verify genuine behavioral contracts of the retry-with-exponential-backoff mechanism:
Mocking strategy is sound (dependency injection via Verdict
|
|
@copilot run pr-finisher skill |
|
@copilot apply fix to push_evals_state - refactor helper. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the pr-finisher flow.
Hand-off: CI needs to be re-triggered by a maintainer before merge. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 2159ac7. The retry logic is now a shared helper in
|
|
@copilot This PR still needs another pass before maintainers can investigate quickly.
|
…ch path Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the remaining blocking The two open concerns were (1) retrying non-transient errors and (2) replaying the non-idempotent orphan-branch path. Both are now structurally impossible:
Merge readiness for head ff553f1:
Hand-off: CI must be re-triggered by a maintainer before merge. |
|
@copilot This PR still needs another pass before maintainers can investigate quickly.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
The
push_evals_statejob's "Push evals results to git" step failed independently across three unrelated workflows within a ~10h window. Logs showed the identical signature in all three: an HTTP 502 / git-fetch timeout during branch checkout in the sharedpush_experiment_state.cjsscript.Root cause
checkoutOrCreateBranch()(initialgit fetch+checkoutof the targetevals/<workflow>branch) had no retry logic, unlike the laterpushSignedCommits()push, which already retries 3x with exponential backoff.Fix
checkoutOrCreateBranchWithRetry(), applying the same retry-with-backoff pattern (3 retries, exponential backoff starting at 1s) already used for the push step, around the checkout call inmain().Tests
Added unit tests for
checkoutOrCreateBranchWithRetrycovering immediate success, retry-then-succeed on transient errors, and exhausting all retries.Branch refresh requested by PR Sous Chef.
Run: https://github.com/github/gh-aw/actions/runs/30851084299
run: https://github.com/github/gh-aw/actions/runs/30855215929