Skip to content

Retry upload-asset pushes after concurrent branch updates #51893

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 8 commits into from
Aug 11, 2026
Merged

Retry upload-asset pushes after concurrent branch updates #51893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f16e685
Initial plan
Copilot Aug 11, 2026
59503c0
Retry concurrent asset branch pushes
Copilot Aug 11, 2026
d67cbba
Limit asset retries to branch races
Copilot Aug 11, 2026
75bd2cf
Merge branch 'main' into copilot/upload-asset-fix-concurrent-race
github-actions[bot] Aug 11, 2026
56402d0
Add git rebase --abort cleanup on rebase failure and add test coverage
Copilot Aug 11, 2026
a11b1f3
Merge branch 'main' into copilot/upload-asset-fix-concurrent-race
github-actions[bot] Aug 11, 2026
00ce891
Merge branch 'main' into copilot/upload-asset-fix-concurrent-race
github-actions[bot] Aug 11, 2026
5e1da83
Fix porcelain stdout/stderr order and align test mocks with actual gi…
Copilot Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion actions/setup/js/upload_assets.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,27 @@ async function main() {
if (isStaged) {
core.summary.addRaw("## 🎭 Staged Mode: Asset Publication Preview");
} else {
await exec.exec("git", ["push", "origin", normalizedBranchName]);
const maxPushAttempts = 3;
for (let attempt = 1; attempt <= maxPushAttempts; attempt++) {
const pushResult = await exec.getExecOutput("git", ["push", "--porcelain", "origin", normalizedBranchName], { ignoreReturnCode: true });
if (pushResult.exitCode === 0) {
break;
}
const pushError = [pushResult.stdout, pushResult.stderr].filter(Boolean).join("\n").trim() || `git push exited with code ${pushResult.exitCode}`;
const isNonFastForward = /non-fast-forward|fetch first/i.test(pushError);
if (!isNonFastForward || attempt === maxPushAttempts) {

@github-actions github-actions Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] The error message is built by joining stderr then stdout, but --porcelain output (including rejection lines like ! [rejected] ...) goes to stdout, not stderr. On a rejection, stderr is often empty and stdout carries the diagnostic — so the order should be stdout first, or both should be shown clearly.

💡 Suggested fix 
const pushError = [pushResult.stdout, pushResult.stderr].filter(Boolean).join("\n").trim()
  || `git push exited with code ${pushResult.exitCode}`;

This ensures the porcelain rejection line (! [rejected]) is the leading text in the error message and in the regex test for isNonFastForward.

@copilot please address this.

Copilot AI Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5e1da83. Swapped the join order to [pushResult.stdout, pushResult.stderr] so the porcelain rejection line (! [rejected]) leads the error message and is the primary input to the isNonFastForward regex. Test mocks updated to place the rejection text in stdout to match actual --porcelain behaviour.

throw new Error(pushError);
}
core.warning(`Asset push attempt ${attempt}/${maxPushAttempts} was rejected because the branch changed; rebasing onto the latest ${normalizedBranchName} branch before retrying`);
const remoteBranch = `refs/remotes/origin/${normalizedBranchName}`;
await exec.exec("git", ["fetch", "--no-tags", "origin", `+refs/heads/${normalizedBranchName}:${remoteBranch}`]);
try {
await exec.exec("git", ["rebase", remoteBranch]);
} catch (rebaseError) {
await exec.exec("git", ["rebase", "--abort"], { ignoreReturnCode: true });
throw rebaseError;
}
}

@github-actions github-actions Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] If git rebase fails (e.g. merge conflict), the process throws without aborting the in-progress rebase, leaving the working tree in a broken REBASE_HEAD state for any subsequent Git operations in the same runner.

💡 Suggested fix: abort rebase on failure 
try {
  await exec.exec('git', ['rebase', remoteBranch]);
} catch (rebaseErr) {
  await exec.exec('git', ['rebase', '--abort'], { ignoreReturnCode: true });
  throw rebaseErr;
}

Without --abort, a rebase conflict corrupts the workspace for any further Git operations.

@copilot please address this.

Copilot AI Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 56402d0. Added try/catch around git rebase that calls git rebase --abort with ignoreReturnCode: true before rethrowing, so the working tree is never left in a broken REBASE_HEAD state. A test for this path was also added.

core.summary.addRaw("## Assets").addRaw(`Successfully uploaded **${uploadCount}** assets to branch \`${normalizedBranchName}\``).addRaw("");
core.info(`Successfully uploaded ${uploadCount} assets to branch ${normalizedBranchName}`);
}
Expand Down
70 changes: 69 additions & 1 deletion actions/setup/js/upload_assets.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,10 @@ describe("upload_assets.cjs", () => {
process.env.GH_AW_ASSETS_DIR = getAssetsDir();

uploadAssetsScript = fs.readFileSync(path.join(__dirname, "upload_assets.cjs"), "utf8");
mockExec = { exec: vi.fn().mockResolvedValue(0) };
mockExec = {
exec: vi.fn().mockResolvedValue(0),
getExecOutput: vi.fn().mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }),
};
});

afterEach(() => {
Expand Down Expand Up @@ -353,6 +356,71 @@ describe("upload_assets.cjs", () => {
});
});

describe("concurrent push recovery", () => {
const prepareAsset = () => {
process.env.GH_AW_ASSETS_BRANCH = "assets/test-workflow";
process.env.GH_AW_SAFE_OUTPUTS_STAGED = "false";
const assetDir = getAssetsDir();
fs.mkdirSync(assetDir, { recursive: true });
const { sha, size } = makeAsset(assetDir, "test.png", "fake png data");
setAgentOutput({
items: [{ type: "upload_asset", fileName: "test.png", sha, size, targetFileName: "test.png", url: "https://example.com/test.png" }],
});
trackCwdArtifact("test.png");
};

it("should fetch, rebase, and retry after a concurrent push", async () => {
prepareAsset();
mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 1, stdout: "! [rejected] assets/test-workflow -> assets/test-workflow (fetch first)", stderr: "" }).mockResolvedValueOnce({ exitCode: 0, stdout: "", stderr: "" });

await executeScript();

expect(mockCore.setFailed).not.toHaveBeenCalled();
expect(mockExec.getExecOutput).toHaveBeenCalledTimes(2);
expect(mockExec.exec).toHaveBeenCalledWith("git", ["fetch", "--no-tags", "origin", "+refs/heads/assets/test-workflow:refs/remotes/origin/assets/test-workflow"]);
expect(mockExec.exec).toHaveBeenCalledWith("git", ["rebase", "refs/remotes/origin/assets/test-workflow"]);
});

@github-actions github-actions Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The retry-exhaustion test verifies getExecOutput is called 3 times, but doesn't assert the final error message thrown (via mockCore.setFailed) contains the right content. If the error were swallowed or reformatted, the test would still pass.

💡 Suggested assertion 
expect(mockCore.setFailed).toHaveBeenCalledWith(
  expect.stringContaining('non-fast-forward')
);

This is partially present but the test name says "stop after three attempts" — adding an explicit assertion about what error is surfaced makes the specification complete.

@copilot please address this.

Copilot AI Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("non-fast-forward")) was already present in the test as of d67cbba. The test mock was also updated in 5e1da83 to put the rejection text in stdout (matching --porcelain output), so the assertion now exercises the correct code path end-to-end.

it("should stop after three failed push attempts", async () => {
prepareAsset();
mockExec.getExecOutput.mockResolvedValue({ exitCode: 1, stdout: "! [rejected] assets/test-workflow -> assets/test-workflow (non-fast-forward)", stderr: "" });

await executeScript();

expect(mockExec.getExecOutput).toHaveBeenCalledTimes(3);
expect(mockExec.exec.mock.calls.filter(call => isGitCommand(call[0], call[1], "fetch"))).toHaveLength(2);
expect(mockExec.exec.mock.calls.filter(call => isGitCommand(call[0], call[1], "rebase"))).toHaveLength(2);
expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("non-fast-forward"));
});

it("should not retry a non-concurrent push failure", async () => {
prepareAsset();
mockExec.getExecOutput.mockResolvedValue({ exitCode: 1, stdout: "", stderr: "remote: permission denied" });

await executeScript();

expect(mockExec.getExecOutput).toHaveBeenCalledTimes(1);
expect(mockExec.exec.mock.calls.some(call => isGitCommand(call[0], call[1], "fetch"))).toBe(false);
expect(mockExec.exec.mock.calls.some(call => isGitCommand(call[0], call[1], "rebase"))).toBe(false);
expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("permission denied"));
});

it("should abort the rebase and rethrow when rebase fails with conflicts", async () => {
prepareAsset();
mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 1, stdout: "! [rejected] assets/test-workflow -> assets/test-workflow (non-fast-forward)", stderr: "" });
mockExec.exec.mockImplementation(async (command, args) => {
if (isGitCommand(command, args, "rebase") && args[1] !== "--abort") {
throw new Error("CONFLICT (content): Merge conflict in test.png");
}
});

await executeScript();

expect(mockExec.exec.mock.calls.some(call => isGitCommand(call[0], call[1], "rebase") && call[1].includes("--abort"))).toBe(true);
expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("CONFLICT"));
});
});

describe("git commit message security", () => {
it("should not wrap commit message in extra quotes to prevent command injection", async () => {
process.env.GH_AW_ASSETS_BRANCH = "assets/test-workflow";
Expand Down
Loading