ci: pr-states no longer overwrites running run state when label is removed (#25586)

.github/workflows/pr-states.yml

@@ -1,16 +1,15 @@
 name: PR States
-# Maintains a CI-states block at the bottom of the PR body. Triggered by
-# `pull_request_target` (not `pull_request`) so fork PRs get a write-enabled
-# GITHUB_TOKEN. Safe because the workflow never checks out PR head code —
-# only reads metadata via API and PATCHes the PR body.
+# Maintains the CI-states block at the bottom of the PR body. Uses
+# pull_request_target (not pull_request) for fork-PR write access; safe
+# because we never check out PR head code, only API-read and PATCH the body.
 
 on:
   pull_request_target:
     types: [opened, synchronize, reopened, labeled, unlabeled]
   workflow_run:
     # Listen to pr-test* lifecycle so reruns initiated by slash commands
-    # (run.rerun / run.rerun_failed_jobs via GITHUB_TOKEN) — which don't
-    # refire pull_request_target — still refresh the CI-states block.
+    # (run.rerun / run.rerun_failed_jobs via GITHUB_TOKEN) -- which don't
+    # refire pull_request_target -- still refresh the CI-states block.
     workflows: ["PR Test", "PR Test Extra"]
     types: [requested, completed]
 
@@ -33,14 +32,18 @@ jobs:
         uses: actions/github-script@v7
         with:
           script: |
+            const RETRY_ATTEMPTS = 3;
+            const RETRY_DELAY_MS = 6000;
+            const LABEL_ON_PRESLEEP_MS = 3000;
+            const LABEL_OFF_PRESLEEP_MS = 6000;
+            const sleep = (ms) => new Promise(r => setTimeout(r, ms));
+
             // Event-agnostic PR lookup.
             // - pull_request_target: PR is in the payload.
             // - workflow_run: pull_requests[] is populated for same-repo PRs;
-            //   for fork PRs it's empty and we reverse-lookup by head ref
-            //   ("forkOwner:branch"), which is the GH-documented fork-PR query
-            //   pattern and avoids the "commit must be in default branch" caveat
-            //   of listPullRequestsAssociatedWithCommit. Bail cleanly when no
-            //   open PR matches (e.g. workflow run from a push to main).
+            //   fork PRs need a head-ref reverse lookup ("forkOwner:branch")
+            //   because listPullRequestsAssociatedWithCommit requires the commit
+            //   to be in the default branch. Bail when no open PR matches.
             let prNumber;
             if (context.payload.pull_request) {
               prNumber = context.payload.pull_request.number;
@@ -79,7 +82,11 @@ jobs:
             const hasCI = labels.includes('run-ci');
             const hasExtra = labels.includes('run-ci-extra');
 
-            // Retry briefly: pr-test* may not be API-visible yet when we start.
+            // Pre-sleep below only matters when a label was just removed;
+            // other no-label fires don't race against fresh indexing.
+            const justRemovedLabel = context.eventName === 'pull_request_target'
+                                     && context.payload.action === 'unlabeled';
+
             async function findRunBySha(workflowFile) {
               const { data } = await github.rest.actions.listWorkflowRuns({
                 owner: context.repo.owner,
@@ -90,38 +97,61 @@ jobs:
               });
               return data.workflow_runs[0] || null;
             }
-            async function findRunWithRetry(workflowFile, maxAttempts = 6, delayMs = 5000) {
-              for (let i = 0; i < maxAttempts; i++) {
+            async function findRun(workflowFile, { preSleepMs = 0, attempts = 1, delayMs = 0 }) {
+              if (preSleepMs > 0) await sleep(preSleepMs);
+              for (let i = 0; i < attempts; i++) {
                 const run = await findRunBySha(workflowFile);
                 if (run) return run;
-                if (i < maxAttempts - 1) await new Promise(r => setTimeout(r, delayMs));
+                if (i < attempts - 1) await sleep(delayMs);
               }
               return null;
             }
 
-            const ptRun = hasCI ? await findRunWithRetry('pr-test.yml') : null;
-            // pr-test-extra's gate requires BOTH run-ci AND run-ci-extra
-            // (see pr-test-extra.yml check-changes if-condition), so without
-            // run-ci the extra workflow doesn't run either.
-            const peRun = (hasCI && hasExtra) ? await findRunWithRetry('pr-test-extra.yml') : null;
+            // Query unconditionally: removing a label does NOT cancel an
+            // in-flight workflow, so a run from before removal can still be
+            // live. Label state only picks the fallback text. labelOnOpts
+            // covers fresh-push indexing race; labelOffOpts only pre-sleeps
+            // on just-removed-label to catch removal-after-push.
+            const labelOnOpts  = { preSleepMs: LABEL_ON_PRESLEEP_MS, attempts: RETRY_ATTEMPTS, delayMs: RETRY_DELAY_MS };
+            const labelOffOpts = { preSleepMs: justRemovedLabel ? LABEL_OFF_PRESLEEP_MS : 0, attempts: 1 };
+            const ptRun = await findRun('pr-test.yml', hasCI ? labelOnOpts : labelOffOpts);
+            // pr-test-extra gates on BOTH labels (see check-changes there).
+            const peRun = await findRun('pr-test-extra.yml', (hasCI && hasExtra) ? labelOnOpts : labelOffOpts);
 
-            // Treat a fully-skipped run as "no real run" — happens when the PR
-            // was opened without the label and label was added later (GHA does
-            // not retrigger on `labeled`).
+            // Skipped run = "no real run" -- happens when a label is added
+            // after a commit, because GHA doesn't retrigger on `labeled`.
             const isReal = (run) => run && run.conclusion !== 'skipped';
 
-            const missingCIText = ':x: **Missing `run-ci` label** — add it to run CI tests.';
-            const peBlockedByCIText = ':x: **Blocked** — `run-ci` is required first.';
-            const notExtraEnabledText = ':warning: **Not enabled** — add `run-ci-extra` label to opt in.';
-            const stalePushText = ':warning: **Not run on latest push** — push again to dispatch.';
-            const ptText = !hasCI
-              ? missingCIText
-              : (isReal(ptRun) ? `[Run #${ptRun.id}](${ptRun.html_url})` : '_Not run yet_');
-            const peText = !hasCI
+            const missingCIText = ':x: **Missing `run-ci` label** -- add it to run CI tests.';
+            const peBlockedByCIText = ':x: **Blocked** -- `run-ci` is required first.';
+            const notExtraEnabledText = ':warning: **Not enabled** -- add `run-ci-extra` label to opt in.';
+            const stalePushText = ':warning: **Not run on latest push** -- push again to dispatch.';
+
+            // Status icon: body otherwise looks uniformly "dispatched" even
+            // for gate-failed runs (call-gate exits on missing label).
+            function runIcon(run) {
+              if (run.status !== 'completed') return ':hourglass_flowing_sand:';
+              switch (run.conclusion) {
+                case 'success': return ':white_check_mark:';
+                case 'failure': return ':x:';
+                case 'cancelled': return ':no_entry_sign:';
+                case 'timed_out': return ':alarm_clock:';
+                case 'action_required': return ':warning:';
+                case 'neutral': return ':grey_question:';
+                default: return ':grey_question:';
+              }
+            }
+            const runLink = (run) => `${runIcon(run)} [Run #${run.id}](${run.html_url})`;
+            const ptText = isReal(ptRun)
+              ? runLink(ptRun)
+              : (!hasCI ? missingCIText : '_Not run yet_');
+            const peText = isReal(peRun)
+              ? runLink(peRun)
+              : !hasCI
                 ? peBlockedByCIText
                 : !hasExtra
                   ? notExtraEnabledText
-                : (isReal(peRun) ? `[Run #${peRun.id}](${peRun.html_url})` : stalePushText);
+                  : stalePushText;
 
             const outerStart = '<!-- pr-states:start -->';
             const outerEnd   = '<!-- pr-states:end -->';