fix: handle race condition when PR is merged before agent job checks out branch#25581
fix: handle race condition when PR is merged before agent job checks out branch#25581
Conversation
…R race condition Agent-Logs-Url: https://github.com/github/gh-aw/sessions/91e7d084-780b-4652-b435-d4833c7555d0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Update tests |
…after trigger Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2f44d3d1-8b2e-4d87-bac6-6464a26b05ca Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added 7 new tests in a
All 48 tests pass (41 existing + 7 new). |
There was a problem hiding this comment.
Pull request overview
Fixes a race condition where a pull_request workflow can start with a stale "open" PR state in the webhook payload, but the PR is merged/closed (and branch deleted) before the agent job reaches checkout—causing an unnecessary hard failure.
Changes:
- On checkout failure for payload-
openPRs, re-query GitHub’s PR API to confirm the current PR state. - If the PR is now closed, log a warning and exit successfully (
checkout_pr_success: true) instead of failing the job. - Adds a dedicated test suite covering the race-condition scenarios and API re-check warning behavior (including HTTP status reporting).
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/checkout_pr_branch.cjs | Adds PR state re-check via GitHub API on checkout failure to gracefully handle “PR closed after trigger” races. |
| actions/setup/js/checkout_pr_branch.test.cjs | Adds tests for race-condition handling, API re-check failures, status code logging, and correct API call args. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
actions/setup/js/checkout_pr_branch.cjs:289
- The new closed-PR race-condition path hard-codes "merged after workflow was triggered" in both the log line and the step summary, but
currentPR.state === "closed"also covers PRs that were simply closed (not merged). This makes the warning/summary misleading. Consider either (a) wording it as "closed after workflow was triggered" or (b) checking the merge status from the API response (e.g.,merged_at/merged) and tailoring the message accordingly.
core.warning(`PR state: closed (merged after workflow was triggered)`);
core.warning(`Checkout failed (expected for closed PR): ${errorMsg}`);
if (pullRequest.head?.ref) {
core.warning(`Branch likely deleted: ${pullRequest.head.ref}`);
}
core.warning("This is expected behavior when a PR is closed - the branch may have been deleted.");
core.endGroup();
// Set output to indicate successful handling of closed PR
core.setOutput("checkout_pr_success", "true");
const warningMessage = `## ⚠️ Closed Pull Request
Pull request #${pullRequest.number} was merged after this workflow was triggered. The checkout failed because the branch has been deleted, which is expected behavior.
**This is not an error** - workflows targeting closed PRs will continue normally.`;
actions/setup/js/checkout_pr_branch.cjs:293
- This change introduces a second block that emits the "Closed PR Checkout Warning", sets
checkout_pr_success, and writes a closed-PR summary (the original block handlesisClosedfrom payload; the new one handlesisNowClosedfrom API). The duplicated logic increases the risk of the two paths drifting (e.g., different messages/fields). Consider extracting a small helper to emit the warning + summary so both paths share the same implementation, with only the state text differing.
if (isNowClosed) {
core.startGroup("⚠️ Closed PR Checkout Warning");
core.warning(`Event type: ${eventName}`);
core.warning(`PR number: ${pullRequest.number}`);
core.warning(`PR state: closed (merged after workflow was triggered)`);
core.warning(`Checkout failed (expected for closed PR): ${errorMsg}`);
if (pullRequest.head?.ref) {
core.warning(`Branch likely deleted: ${pullRequest.head.ref}`);
}
core.warning("This is expected behavior when a PR is closed - the branch may have been deleted.");
core.endGroup();
// Set output to indicate successful handling of closed PR
core.setOutput("checkout_pr_success", "true");
const warningMessage = `## ⚠️ Closed Pull Request
Pull request #${pullRequest.number} was merged after this workflow was triggered. The checkout failed because the branch has been deleted, which is expected behavior.
**This is not an error** - workflows targeting closed PRs will continue normally.`;
await core.summary.addRaw(warningMessage).write();
return;
}
- Files reviewed: 2/2 changed files
- Comments generated: 2
| } | ||
| } catch (apiError) { | ||
| const apiErrorMsg = getErrorMessage(apiError); | ||
| const statusCode = /** @type {any} */ apiError?.status; |
There was a problem hiding this comment.
JSDoc type assertions in this repo appear to consistently use the parenthesized form (e.g. const err = /** @type {any} */ (error); in actions/setup/js/assign_agent_helpers.cjs:400). Here, the cast is written as /** @type {any} */ apiError?.status, which is inconsistent and can be less reliably understood by TS tooling. Suggest casting apiError first (or using the repo’s existing pattern) before reading .status.
This issue also appears in the following locations of the same file:
- line 268
- line 272
| const statusCode = /** @type {any} */ apiError?.status; | |
| const typedApiError = /** @type {any} */ (apiError); | |
| const statusCode = typedApiError?.status; |
| // Should log as warning, not error | ||
| expect(mockCore.startGroup).toHaveBeenCalledWith("⚠️ Closed PR Checkout Warning"); | ||
| expect(mockCore.warning).toHaveBeenCalledWith("Event type: pull_request"); | ||
| expect(mockCore.warning).toHaveBeenCalledWith("PR number: 123"); | ||
| expect(mockCore.warning).toHaveBeenCalledWith("PR state: closed (merged after workflow was triggered)"); | ||
| expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("couldn't find remote ref")); | ||
| expect(mockCore.warning).toHaveBeenCalledWith("Branch likely deleted: feature-branch"); | ||
| expect(mockCore.warning).toHaveBeenCalledWith("This is expected behavior when a PR is closed - the branch may have been deleted."); | ||
|
|
||
| // Should write summary with the "merged after" message | ||
| expect(mockCore.summary.addRaw).toHaveBeenCalled(); | ||
| const summaryCall = mockCore.summary.addRaw.mock.calls[0][0]; | ||
| expect(summaryCall).toContain("⚠️ Closed Pull Request"); | ||
| expect(summaryCall).toContain("was merged after this workflow was triggered"); | ||
| expect(summaryCall).toContain("This is not an error"); | ||
|
|
There was a problem hiding this comment.
These new assertions hard-code "merged after workflow was triggered" for the closed-PR race condition, but the implementation currently only checks state === "closed" (which also covers unmerged closures). If the production message is adjusted to distinguish closed vs merged (or to use more neutral wording), update these expectations to match the corrected behavior.
🧪 Test Quality Sentinel ReportTest Quality Score: 84/100✅ Excellent test quality
Test Classification Details
Flagged Tests — Requires Review
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 84/100. Test quality is excellent — 14% of new tests are implementation tests (threshold: 30%). Seven tests comprehensively cover the race condition fix across all key behavioral scenarios. One minor improvement suggested: add a behavioral assertion to the API parameter verification test.
When a PR is merged within seconds of triggering a
pull_requestworkflow, the webhook payload capturesstate: "open"— but by the time theagentjob reaches "Checkout PR branch", the branch is already deleted. The existingisClosedguard reads only the stale payload state, so it falls through to the error path instead of the expected graceful closed-PR handling.Changes
checkout_pr_branch.cjs: In thecatchblock, when the payload says the PR is"open"but checkout fails, re-query the GitHub API for the current PR state. If the PR is now closed/merged, emit a warning and return cleanly (checkout_pr_success: true) instead of callingsetFailed.The API failure warning now includes the HTTP status code for easier triage.
checkout_pr_branch.test.cjs: Added 7 new tests in a"race condition - PR merged after workflow trigger"describe block covering: graceful handling when PR is merged mid-flight, still-failing when PR is genuinely open, API re-check failure with HTTP status in warning, HTTP status code included/omitted correctly, correct API call arguments, and the race condition on thegh pr checkoutpath (e.g.issue_commentevents).