From 50cb9966d60f49371b6a4ec751eec26e0c7e06cc Mon Sep 17 00:00:00 2001 From: Jan Rose Date: Tue, 14 Jul 2026 11:14:44 +0200 Subject: [PATCH] Add retries when setting maintainer-approval success --- .github/workflows/maintainer-approval.js | 72 ++++++++++++++----- .github/workflows/maintainer-approval.test.js | 63 +++++++++++++++- 2 files changed, 116 insertions(+), 19 deletions(-) diff --git a/.github/workflows/maintainer-approval.js b/.github/workflows/maintainer-approval.js index 93ce4de78c7..ba537c27d0f 100644 --- a/.github/workflows/maintainer-approval.js +++ b/.github/workflows/maintainer-approval.js @@ -430,6 +430,57 @@ async function upsertComment(github, owner, repo, prNumber, newBody) { }); } +// --- Approval check --- + +const CHECK_VERIFY_ATTEMPTS = 3; + +/** + * Create the maintainer-approval success check and confirm it took effect. + * + * checks.create can return 2xx yet leave the required "maintainer-approval" + * status stuck as "Expected" (yellow dot): the write is lost to GitHub + * eventual consistency while the workflow run itself still goes green. Because + * the pending path deliberately posts no check at all, nothing re-establishes + * it until an unrelated event happens to re-run this workflow, so the merge + * queue is silently blocked even though a maintainer approved (see PR #5868). + * Re-read the check from the head SHA after each create and retry until it is + * visible; fail the run if it never lands so the breakage surfaces loudly + * instead of blocking the merge with a green run. + */ +async function createApprovalCheck(github, checkParams, summary, core) { + const { owner, repo, head_sha: headSha, name } = checkParams; + const delayMs = Number(process.env.MAINTAINER_APPROVAL_VERIFY_DELAY_MS ?? 2000); + + for (let attempt = 1; attempt <= CHECK_VERIFY_ATTEMPTS; attempt++) { + await github.rest.checks.create({ + ...checkParams, + status: "completed", + conclusion: "success", + output: { title: name, summary }, + }); + + const { data } = await github.rest.checks.listForRef({ + owner, repo, ref: headSha, check_name: name, + }); + if (data.check_runs.some(r => r.conclusion === "success")) { + return; + } + + core.warning( + `${name} not visible on ${headSha} after create ` + + `(attempt ${attempt}/${CHECK_VERIFY_ATTEMPTS})` + ); + if (attempt < CHECK_VERIFY_ATTEMPTS) { + await new Promise(resolve => setTimeout(resolve, delayMs)); + } + } + + core.setFailed( + `${name} could not be set on ${headSha} after ${CHECK_VERIFY_ATTEMPTS} ` + + `attempts; re-run this workflow to unblock the merge` + ); +} + // --- Main --- module.exports = async ({ github, context, core }) => { @@ -475,12 +526,7 @@ module.exports = async ({ github, context, core }) => { if (maintainerApproval) { const approver = maintainerApproval.user.login; core.info(`Maintainer approval from @${approver}`); - await github.rest.checks.create({ - ...checkParams, - status: "completed", - conclusion: "success", - output: { title: STATUS_CONTEXT, summary: `Approved by @${approver}` }, - }); + await createApprovalCheck(github, checkParams, `Approved by @${approver}`, core); await deleteMarkerComments(github, owner, repo, prNumber); return; } @@ -493,12 +539,7 @@ module.exports = async ({ github, context, core }) => { ); if (hasAnyApproval) { core.info(`Maintainer-authored PR approved by a reviewer.`); - await github.rest.checks.create({ - ...checkParams, - status: "completed", - conclusion: "success", - output: { title: STATUS_CONTEXT, summary: "Approved (maintainer-authored PR)" }, - }); + await createApprovalCheck(github, checkParams, "Approved (maintainer-authored PR)", core); await deleteMarkerComments(github, owner, repo, prNumber); return; } @@ -533,12 +574,7 @@ module.exports = async ({ github, context, core }) => { // the GitHub UI, which blocks the merge until approval is granted. if (result.allCovered && approverLogins.length > 0) { core.info("All ownership groups have per-path approval."); - await github.rest.checks.create({ - ...checkParams, - status: "completed", - conclusion: "success", - output: { title: STATUS_CONTEXT, summary: "All ownership groups approved" }, - }); + await createApprovalCheck(github, checkParams, "All ownership groups approved", core); await deleteMarkerComments(github, owner, repo, prNumber); return; } diff --git a/.github/workflows/maintainer-approval.test.js b/.github/workflows/maintainer-approval.test.js index 2866dc9d3d7..0394aa74730 100644 --- a/.github/workflows/maintainer-approval.test.js +++ b/.github/workflows/maintainer-approval.test.js @@ -60,12 +60,16 @@ function makeCore() { * @param {Array} opts.files - PR files to return (objects with .filename) * @param {Object} opts.teamMembers - { teamSlug: [logins] } * @param {Array} opts.existingComments - Existing PR comments to return + * @param {number} opts.flakyCreates - Number of leading checks.create calls that + * return 2xx but do not persist (not visible to the follow-up listForRef read), + * simulating GitHub eventual-consistency flakes. */ -function makeGithub({ reviews = [], files = [], teamMembers = {}, existingComments = [] } = {}) { +function makeGithub({ reviews = [], files = [], teamMembers = {}, existingComments = [], flakyCreates = 0 } = {}) { const listReviews = Symbol("listReviews"); const listFiles = Symbol("listFiles"); const listComments = Symbol("listComments"); const checkRuns = []; + const persistedCheckRuns = []; const createdComments = []; const updatedComments = []; const deletedCommentIds = []; @@ -85,7 +89,17 @@ function makeGithub({ reviews = [], files = [], teamMembers = {}, existingCommen checks: { create: async (params) => { checkRuns.push(params); + if (checkRuns.length > flakyCreates) { + persistedCheckRuns.push(params); + } }, + listForRef: async ({ ref, check_name }) => ({ + data: { + check_runs: persistedCheckRuns.filter( + (r) => r.head_sha === ref && r.name === check_name + ), + }, + }), }, issues: { listComments, @@ -111,6 +125,7 @@ function makeGithub({ reviews = [], files = [], teamMembers = {}, existingCommen }, }, _checkRuns: checkRuns, + _persistedCheckRuns: persistedCheckRuns, _comments: createdComments, _updatedComments: updatedComments, _deletedCommentIds: deletedCommentIds, @@ -128,6 +143,8 @@ describe("maintainer-approval", () => { originalWorkspace = process.env.GITHUB_WORKSPACE; tmpDir = makeTmpOwners(OWNERS_CONTENT, OWNERTEAMS_CONTENT); process.env.GITHUB_WORKSPACE = tmpDir; + // Skip the retry backoff so flake tests do not sleep. + process.env.MAINTAINER_APPROVAL_VERIFY_DELAY_MS = "0"; }); after(() => { @@ -136,6 +153,7 @@ describe("maintainer-approval", () => { } else { delete process.env.GITHUB_WORKSPACE; } + delete process.env.MAINTAINER_APPROVAL_VERIFY_DELAY_MS; fs.rmSync(tmpDir, { recursive: true }); }); @@ -558,4 +576,47 @@ describe("maintainer-approval", () => { assert.ok(body.includes("4 files changed"), "should show count for bundle group"); assert.ok(!body.includes("`bundle/a.go`"), "should not list individual bundle files"); }); + + // --- Check persistence (PR #5868 flake) --- + + it("retries checks.create when the first write does not persist", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "maintainer1" } }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + flakyCreates: 1, + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + // First create was lost; second landed. No failure recorded. + assert.equal(github._checkRuns.length, 2); + assert.equal(github._persistedCheckRuns.length, 1); + assert.equal(github._persistedCheckRuns[0].conclusion, "success"); + assert.equal(core._log.failed.length, 0); + assert.ok(core._log.warning.length >= 1); + }); + + it("fails the run when the check never persists", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "maintainer1" } }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + flakyCreates: Infinity, + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + // Exhausted all attempts without the required check becoming visible. + assert.equal(github._checkRuns.length, 3); + assert.equal(github._persistedCheckRuns.length, 0); + assert.equal(core._log.failed.length, 1); + assert.ok(core._log.failed[0].includes("could not be set")); + }); });