fix: evaluate backport approval from live labels to prevent check race

spec/checks-util.spec.ts

@@ -0,0 +1,69 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest';
+
+import { BACKPORT_APPROVAL_CHECK } from '../src/constants';
+import { queueBackportApprovalCheck } from '../src/utils/checks-util';
+
+const backportPROpenedEvent = require('./fixtures/backport_pull_request.opened.json');
+
+describe('checks-util', () => {
+  describe('queueBackportApprovalCheck', () => {
+    const octokit = {
+      checks: {
+        listForRef: vi.fn(),
+        create: vi.fn().mockResolvedValue({ data: {} }),
+        update: vi.fn().mockResolvedValue({ data: {} }),
+      },
+    };
+
+    const context = {
+      ...backportPROpenedEvent,
+      octokit,
+      repo: vi.fn((obj) => obj),
+    };
+
+    beforeEach(() => vi.clearAllMocks());
+
+    it('creates a new queued check run when none exists', async () => {
+      octokit.checks.listForRef.mockResolvedValue({
+        data: { check_runs: [] },
+      });
+
+      await queueBackportApprovalCheck(context);
+
+      expect(octokit.checks.create).toHaveBeenCalledTimes(1);
+      expect(octokit.checks.create).toHaveBeenCalledWith(
+        expect.objectContaining({
+          name: BACKPORT_APPROVAL_CHECK,
+          status: 'queued',
+        }),
+      );
+      expect(octokit.checks.update).not.toHaveBeenCalled();
+    });
+
+    it('resets the existing check run to queued instead of creating a duplicate', async () => {
+      octokit.checks.listForRef.mockResolvedValue({
+        data: {
+          check_runs: [
+            {
+              id: 12345,
+              name: BACKPORT_APPROVAL_CHECK,
+              status: 'completed',
+              conclusion: 'success',
+            },
+          ],
+        },
+      });
+
+      await queueBackportApprovalCheck(context);
+
+      expect(octokit.checks.create).not.toHaveBeenCalled();
+      expect(octokit.checks.update).toHaveBeenCalledTimes(1);
+      expect(octokit.checks.update).toHaveBeenCalledWith(
+        expect.objectContaining({
+          check_run_id: 12345,
+          status: 'queued',
+        }),
+      );
+    });
+  });
+});

spec/index.spec.ts

@@ -554,12 +554,44 @@ describe('trop', () => {
         .get(
           `/repos/codebytere/probot-test/issues/${event.payload.pull_request.number}/labels?per_page=100&page=1`,
         )
-        .reply(200, []);
+        .reply(200, [backportRequestedLabel]);
 
       await robot.receive(event);
 
-      expect(checkUtils.queueBackportApprovalCheck).toHaveBeenCalledTimes(1);
+      // Once to create the missing check run, once to (re-)assert the
+      // pending state after evaluating the live labels.
+      expect(checkUtils.queueBackportApprovalCheck).toHaveBeenCalledTimes(2);
+      expect(checkUtils.updateBackportApprovalCheck).not.toHaveBeenCalled();
+    });
+
+    it('does not conclude "Not Required" when "backport/requested" was added after the payload snapshot', async () => {
+      // Regression test for the manual-backport race: the `opened` handler
+      // itself adds `backport/requested` via updateManualBackport, so the
+      // payload's label snapshot can never contain it. The verdict must be
+      // evaluated from the live labels, not the stale snapshot.
+      nock(GH_API)
+        .persist()
+        .get('/repos/codebytere/probot-test/pulls/12345')
+        .reply(200, MOCK_PR);
+
+      nock(GH_API)
+        .get('/repos/codebytere/probot-test/branches?protected=true')
+        .reply(200, BRANCHES);
+
+      // Live labels include `backport/requested` even though the payload
+      // snapshot (fixture) does not.
+      nock(GH_API)
+        .persist()
+        .get(
+          '/repos/codebytere/probot-test/issues/7/labels?per_page=100&page=1',
+        )
+        .reply(200, [backportRequestedLabel]);
+
+      await robot.receive(backportPROpenedEvent);
+
+      expect(updateManualBackport).toHaveBeenCalled();
       expect(checkUtils.updateBackportApprovalCheck).not.toHaveBeenCalled();
+      expect(checkUtils.queueBackportApprovalCheck).toHaveBeenCalledTimes(2);
     });
 
     it('passes the backport approval check if the "backport/approved" label is on a new backport PR', async () => {
@@ -583,7 +615,7 @@ describe('trop', () => {
         .get(
           `/repos/codebytere/probot-test/issues/${event.payload.pull_request.number}/labels?per_page=100&page=1`,
         )
-        .reply(200, []);
+        .reply(200, [backportApprovedLabel]);
 
       await robot.receive(event);
 
@@ -624,7 +656,7 @@ describe('trop', () => {
         await fs.readFile(backportPRLabeledEventPath, 'utf-8'),
       );
 
-      event.payload.label = backportApprovedLabel;
+      event.payload.label = backportRequestedLabel;
       event.payload.pull_request.labels = [backportRequestedLabel];
 
       nock(GH_API)
@@ -654,14 +686,113 @@ describe('trop', () => {
         .get(
           `/repos/codebytere/probot-test/issues/${event.payload.pull_request.number}/labels?per_page=100&page=1`,
         )
-        .reply(200, MOCK_PR.labels);
+        .reply(200, [backportRequestedLabel]);
+
+      await robot.receive(event);
+
+      expect(checkUtils.queueBackportApprovalCheck).toHaveBeenCalledTimes(1);
+      expect(checkUtils.updateBackportApprovalCheck).not.toHaveBeenCalled();
+    });
+
+    it('re-asserts the pending state even when the check run is already queued', async () => {
+      // Regression test: this previously no-op'd when the run was already
+      // `queued`, letting a slower concurrent invocation holding a stale
+      // payload complete the run last with an incorrect conclusion.
+      const event = JSON.parse(
+        await fs.readFile(backportPRLabeledEventPath, 'utf-8'),
+      );
+
+      event.payload.label = backportRequestedLabel;
+      event.payload.pull_request.labels = [backportRequestedLabel];
+
+      nock(GH_API)
+        .persist()
+        .get(
+          '/repos/codebytere/probot-test/commits/ABC/check-runs?per_page=100',
+        )
+        .reply(200, {
+          check_runs: [
+            {
+              name: BACKPORT_APPROVAL_CHECK,
+              status: 'queued',
+            },
+          ],
+        });
+
+      nock(GH_API)
+        .persist()
+        .get('/repos/codebytere/probot-test/pulls/12345')
+        .reply(200, MOCK_PR);
+
+      nock(GH_API)
+        .get('/repos/codebytere/probot-test/branches?protected=true')
+        .reply(200, BRANCHES);
+
+      nock(GH_API)
+        .persist()
+        .get(
+          `/repos/codebytere/probot-test/issues/${event.payload.pull_request.number}/labels?per_page=100&page=1`,
+        )
+        .reply(200, [backportRequestedLabel]);
 
       await robot.receive(event);
 
       expect(checkUtils.queueBackportApprovalCheck).toHaveBeenCalledTimes(1);
       expect(checkUtils.updateBackportApprovalCheck).not.toHaveBeenCalled();
     });
 
+    it('completes a queued check when the "backport/approved" label is added', async () => {
+      const event = JSON.parse(
+        await fs.readFile(backportPRLabeledEventPath, 'utf-8'),
+      );
+
+      event.payload.label = backportApprovedLabel;
+      event.payload.pull_request.labels = [backportApprovedLabel];
+
+      nock(GH_API)
+        .persist()
+        .get(
+          '/repos/codebytere/probot-test/commits/ABC/check-runs?per_page=100',
+        )
+        .reply(200, {
+          check_runs: [
+            {
+              name: BACKPORT_APPROVAL_CHECK,
+              status: 'queued',
+            },
+          ],
+        });
+
+      nock(GH_API)
+        .persist()
+        .get('/repos/codebytere/probot-test/pulls/12345')
+        .reply(200, MOCK_PR);
+
+      nock(GH_API)
+        .get('/repos/codebytere/probot-test/branches?protected=true')
+        .reply(200, BRANCHES);
+
+      nock(GH_API)
+        .persist()
+        .get(
+          `/repos/codebytere/probot-test/issues/${event.payload.pull_request.number}/labels?per_page=100&page=1`,
+        )
+        .reply(200, [backportApprovedLabel]);
+
+      await robot.receive(event);
+
+      expect(checkUtils.queueBackportApprovalCheck).not.toHaveBeenCalled();
+
+      const updatePayload = vi.mocked(checkUtils.updateBackportApprovalCheck)
+        .mock.calls[0][2];
+
+      expect(updatePayload).toMatchObject({
+        title: 'Backport Approved',
+        summary: 'This PR has been approved for backporting.',
+        conclusion: CheckRunStatus.SUCCESS,
+      });
+    });
+
     it('passes the backport approval check if the "backport/approved" label is added', async () => {
       const event = JSON.parse(
         await fs.readFile(backportPRLabeledEventPath, 'utf-8'),
@@ -691,7 +822,7 @@ describe('trop', () => {
         .get(
           `/repos/codebytere/probot-test/issues/${event.payload.pull_request.number}/labels?per_page=100&page=1`,
         )
-        .reply(200, MOCK_PR.labels);
+        .reply(200, [backportApprovedLabel]);
 
       await robot.receive(event);
 

src/index.ts

@@ -378,11 +378,21 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => {
           }
         }
 
-        const isBackportApproved = pr.labels.some(
-          (prLabel) => prLabel.name === BACKPORT_APPROVED_LABEL,
+        // Evaluate the approval verdict from the live labels on the PR
+        // rather than the webhook payload's label snapshot. The payload is
+        // frozen at delivery time, so it can never contain a label added
+        // mid-handler (e.g. `backport/requested` added for a manual backport
+        // by updateManualBackport above) and a slow invocation racing with
+        // other deliveries would otherwise stamp a stale verdict last.
+        const isBackportApproved = await labelExistsOnPR(
+          context,
+          pr.number,
+          BACKPORT_APPROVED_LABEL,
         );
-        const isBackportRequested = pr.labels.some(
-          (prLabel) => prLabel.name === BACKPORT_REQUESTED_LABEL,
+        const isBackportRequested = await labelExistsOnPR(
+          context,
+          pr.number,
+          BACKPORT_REQUESTED_LABEL,
         );
 
         if (isBackportApproved) {
@@ -396,22 +406,30 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => {
             conclusion: CheckRunStatus.SUCCESS,
           });
         } else if (!isBackportRequested) {
-          // If backport/approved was removed, add backport/requested back
           if (
             action === 'unlabeled' &&
             label?.name === BACKPORT_APPROVED_LABEL
           ) {
+            // If backport/approved was removed, add backport/requested back
+            // and keep the check pending - the same invocation that flags a
+            // PR as needing approval must never conclude that approval is
+            // not required.
             await addLabels(context, pr.number, [BACKPORT_REQUESTED_LABEL]);
-          }
-          await updateBackportApprovalCheck(context, backportApprovalCheck, {
-            title: 'Backport Approval Not Required',
-            summary: 'This PR does not need backport approval.',
-            conclusion: CheckRunStatus.SUCCESS,
-          });
-        } else {
-          if (backportApprovalCheck.status !== 'queued') {
             await queueBackportApprovalCheck(context);
+          } else {
+            await updateBackportApprovalCheck(context, backportApprovalCheck, {
+              title: 'Backport Approval Not Required',
+              summary: 'This PR does not need backport approval.',
+              conclusion: CheckRunStatus.SUCCESS,
+            });
           }
+        } else {
+          // Always (re-)assert the pending state, even when the check run
+          // already appears queued in our snapshot - a concurrent invocation
+          // holding a stale payload may have completed it (or may be about
+          // to). An idempotent write of the queued state corrects a stale
+          // conclusion instead of silently trusting the snapshot.
+          await queueBackportApprovalCheck(context);
         }
       } else {
         // If we're somehow targeting main and have a check run,

src/utils/checks-util.ts

@@ -141,16 +141,39 @@ export async function updateBackportApprovalCheck(
 export async function queueBackportApprovalCheck(context: WebHookPRContext) {
   const pr = context.payload.pull_request;
 
+  const output = {
+    title: 'Needs Backport Approval',
+    summary: 'This PR requires backport approval.',
+  };
+
+  // Re-fetch the existing check run immediately before writing - concurrent
+  // webhook deliveries race through check-then-create and would otherwise
+  // create duplicate check runs for the same head SHA (branch protection
+  // only consults the latest run per name). If a run already exists - even
+  // one a stale invocation already completed - reset it to queued by id
+  // instead of creating another.
+  const existingCheck = await getBackportApprovalCheck(context);
+
+  if (existingCheck) {
+    await context.octokit.checks.update(
+      context.repo({
+        check_run_id: existingCheck.id,
+        name: existingCheck.name,
+        status: 'queued' as 'queued',
+        details_url: 'https://github.com/electron/trop',
+        output,
+      }),
+    );
+    return;
+  }
+
   await context.octokit.checks.create(
     context.repo({
       name: BACKPORT_APPROVAL_CHECK,
       head_sha: pr.head.sha,
       status: 'queued',
       details_url: 'https://github.com/electron/trop',
-      output: {
-        title: 'Needs Backport Approval',
-        summary: 'This PR requires backport approval.',
-      },
+      output,
     }),
   );
 }