Skip to content

Add retries when setting maintainer-approval success check - #5921

Open
janniklasrose wants to merge 1 commit into
mainfrom
janniklasrose/retry-setting-maintainer-approval
Open

Add retries when setting maintainer-approval success check#5921
janniklasrose wants to merge 1 commit into
mainfrom
janniklasrose/retry-setting-maintainer-approval

Conversation

@janniklasrose

Copy link
Copy Markdown
Contributor

Problem

The maintainer-approval.js script (run by the "PR approval" workflow) creates the required maintainer-approval status check on the PR head SHA when approval is satisfied:

awaitgithub.rest.checks.create({ ...checkParams,conclusion: "success", ... });

There is no verification that this write actually took effect. When checks.create returns 2xx but the check does not register on the head commit (GitHub eventual consistency), the workflow run still exits green while the required maintainer-approval status stays stuck as "Expected" in the UI. Because the pending path deliberately posts no check at all, nothing re-establishes it — the PR is silently blocked from the merge queue even though a maintainer approved, until some unrelated event happens to re-run the workflow.

This was observed on #5868: the "PR approval" run passed, a maintainer had approved, yet maintainer-approval remained "Expected" and the PR would not enter the merge queue.

Fix

Route all three success paths through a new createApprovalCheck helper that:

  1. Creates the success check.
  2. Re-reads the check on the head SHA via checks.listForRef to confirm it is visible.
  3. Retries up to 3 times (with a short backoff) if it has not registered.
  4. Calls core.setFailed if the check never lands, so the run goes red with an actionable message ("re-run this workflow to unblock the merge") instead of green-but-blocking.

A red, re-runnable check is far easier to notice and recover from than a silently stuck "Expected" status.

Tests

The test mock now models create → persist → read, with a flakyCreates knob to simulate non-persisting writes. Two new cases:

  • retries when the first write does not persist — second create lands, no failure.
  • fails when the check never persists — exhausts attempts and records setFailed.

The retry backoff is overridable via MAINTAINER_APPROVAL_VERIFY_DELAY_MS (set to 0 in tests). All 24 tests pass.

This pull request and its description were written by Isaac, an AI coding agent.

@github-actions

Copy link
Copy Markdown
Contributor

Waiting for approval

Based on git history, these people are best suited to review:

  • @simonfaltum -- recent work in .github/workflows/

Eligible reviewers: @andrewnester, @anton-107, @denik, @lennartkats-db, @pietern, @renaudhartert-db, @shreyas-goenka

Suggestions based on git history. See OWNERS for ownership rules.

@eng-dev-ecosystem-bot

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: 50cb996

Run: 29321453138

Env💚​RECOVERED🙈​SKIP✅​pass🙈​skipTime
💚​aws linux4423010845:31
💚​aws windows4423210827:54
💚​aws-ucws linux4431610015:18
💚​aws-ucws windows443189997:24
💚​azure linux4423010836:16
💚​azure windows4423210817:11
💚​azure-ucws linux443189985:58
💚​azure-ucws windows443209967:29
💚​gcp linux4422910855:08
💚​gcp windows4423110836:54
8 interesting tests: 4 RECOVERED, 4 SKIP
Test Nameaws linuxaws windowsaws-ucws linuxaws-ucws windowsazure linuxazure windowsazure-ucws linuxazure-ucws windowsgcp linuxgcp windows
💚​TestAccept💚​R💚​R💚​R💚​R💚​R💚​R💚​R💚​R💚​R💚​R
🙈​TestAccept/bundle/invariant/no_drift🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S
🙈​TestAccept/bundle/resources/vector_search_endpoints/drift/recreated_same_name🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S
🙈​TestAccept/bundle/resources/vector_search_indexes/recreate/embedding_dimension🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S
🙈​TestAccept/ssh/connection🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S🙈​S
💚​TestFetchRepositoryInfoAPI_FromRepo💚​R💚​R💚​R💚​R💚​R💚​R💚​R💚​R💚​R💚​R
💚​TestFetchRepositoryInfoAPI_FromRepo/root💚​R💚​R💚​R💚​R💚​R💚​R💚​R💚​R💚​R💚​R
💚​TestFetchRepositoryInfoAPI_FromRepo/subdir💚​R💚​R💚​R💚​R💚​R💚​R💚​R💚​R💚​R💚​R
Top 10 slowest tests (at least 2 minutes):
durationenvtestname
6:47aws windowsTestAccept
6:27azure-ucws windowsTestAccept
6:19aws-ucws windowsTestAccept
6:02azure windowsTestAccept
6:00gcp windowsTestAccept
3:00gcp linuxTestAccept
2:58azure linuxTestAccept
2:52aws linuxTestAccept
2:47azure-ucws linuxTestAccept
2:47aws-ucws linuxTestAccept

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({

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.

checks.create doesn't seem to be idempotent, so retrying might create duplicate runs, should we do listForRef first and re-create if it's not there?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@janniklasrose@eng-dev-ecosystem-bot@radakam