ci: cancel obsolete PR checks even when new checks are skipped (#29364)

* ci: cancel superseded PR checks even when replacement checks are filtered out

* test(ci): verify cleanup keys against every automatic PR workflow

.github/workflows/ci-cache-warmup.yml

@@ -57,6 +57,10 @@ on:
 permissions:
   contents: read
 
+concurrency:
+  group: ci-cache-warmup-${{ github.event.pull_request.number || github.run_id }}
+  cancel-in-progress: ${{ github.event_name == 'pull_request' }}
+
 jobs:
   warm:
     concurrency:

.github/workflows/ci-closed-pr-caches.yml

@@ -1,8 +1,8 @@
-name: Clean closed PR work
+name: Clean obsolete PR work
 
 on:
   pull_request_target:
-    types: [closed]
+    types: [closed, synchronize]
 
 permissions:
   actions: write
@@ -10,37 +10,65 @@ permissions:
 
 jobs:
   clean:
-    runs-on: ubuntu-latest
+    runs-on: blacksmith-2vcpu-ubuntu-2204
     timeout-minutes: 5
     steps:
-      - name: Cancel checks for an unmerged closed PR
-        if: github.event.pull_request.merged == false
+      - name: Cancel obsolete PR checks
+        if: github.event.action == 'synchronize' || github.event.pull_request.merged == false
         uses: actions/github-script@v8
         with:
           script: |
-            const closed = context.payload.pull_request
+            const pr = context.payload.pull_request
+            const superseded = context.payload.action === 'synchronize'
             const targets = [
               ['pr-checks', 'pr.yml'],
               ['node-server', 'node-server-tests.yml'],
               ['ssh-windows-hosts', 'ssh-windows-hosts.yml'],
               ['ssh-hostile-hosts', 'ssh-hostile-hosts.yml'],
               ['mobile', 'mobile.yml'],
-              ['computer-e2e', 'computer-e2e.yml']
+              ['computer-e2e', 'computer-e2e.yml'],
+              ['wsl-orcad-e2e', 'wsl-orcad-e2e.yml'],
+              ['performance-contracts', 'performance-contracts.yml'],
+              ['pr-test-loc', 'pr-test-loc.yml'],
+              ['preflight-critical-path', 'preflight-critical-path-benchmark.yml'],
+              ['preflight-cli', 'preflight-cli-platforms.yml'],
+              ['macos-updater', 'macos-updater-tests.yml'],
+              ['preflight-comparison', 'preflight-benchmark.yml'],
+              ['windows-typecheck-parity', 'windows-typecheck-parity.yml'],
+              ['zcode-transcript-contract', 'zcode-transcript-contract.yml'],
+              ['release-ref-validation', 'release-ref-validation.yml'],
+              ['docs-pr', 'docs.yml'],
+              ['cloud-verify-refs/pull', 'cloud-verify.yml', '/merge'],
+              ['pnpm-verification-pilot', 'ci-pnpm-verification-pilot.yml'],
+              ['win-orcad-serve-switch-e2e', 'win-orcad-serve-switch-e2e.yml'],
+              ['pi-provider-runtime', 'pi-provider-runtime.yml'],
+              ['coding-plan-contract', 'zcode-credential-contract.yml', '-credentials'],
+              ['release-javascript-comparison', 'release-javascript-benchmark.yml'],
+              ['ci-cache-warmup-platforms', 'ci-cache-warmup-platforms.yml'],
+              ['pi-owner-runtime', 'pi-owner-runtime.yml'],
+              ['git-command-termination', 'git-command-termination-runtime.yml'],
+              ['skill-roundtrip-pull_request', 'skill-update-roundtrip.yml'],
+              ['ci-cache-warmup', 'ci-cache-warmup.yml']
             ]
-            const stillClosed = async () => {
+            const stillCurrent = async () => {
               const { data: current } = await github.rest.pulls.get({
-                ...context.repo, pull_number: closed.number
+                ...context.repo, pull_number: pr.number
               })
+              if (superseded) {
+                return current.state === 'open' && current.head.sha === pr.head.sha
+              }
               return current.state === 'closed' && current.merged_at === null &&
-                current.closed_at === closed.closed_at
+                current.closed_at === pr.closed_at
             }
-            if (!Number.isSafeInteger(closed.number) || closed.number <= 0 ||
-                !Number.isFinite(Date.parse(closed.closed_at))) {
-              throw new Error('Missing closed PR identity')
+            if (!Number.isSafeInteger(pr.number) || pr.number <= 0 ||
+                (superseded ? !/^[a-f0-9]{40}$/.test(pr.head?.sha ?? '') :
+                  !Number.isFinite(Date.parse(pr.closed_at)))) {
+              throw new Error('Missing PR identity')
             }
-            if (!await stillClosed()) return
-            for (const [prefix, workflow] of targets) {
-              const group = `${prefix}-${closed.number}`
+            if (!await stillCurrent()) return
+            for (const [prefix, workflow, suffix = ''] of targets) {
+              const group = prefix === 'cloud-verify-refs/pull'
+                ? `${prefix}/${pr.number}${suffix}` : `${prefix}-${pr.number}${suffix}`
               let data
               try {
                 const response = await github.request(
@@ -65,9 +93,11 @@ jobs:
                 })
                 if (run.event !== 'pull_request' || run.path !== `.github/workflows/${workflow}` ||
                     run.status === 'completed' ||
-                    !Number.isFinite(Date.parse(run.created_at)) ||
-                    Date.parse(run.created_at) > Date.parse(closed.closed_at)) continue
-                if (!await stillClosed()) return
+                    (superseded ? !/^[a-f0-9]{40}$/.test(run.head_sha ?? '') ||
+                      run.head_sha === pr.head.sha :
+                      !Number.isFinite(Date.parse(run.created_at)) ||
+                      Date.parse(run.created_at) > Date.parse(pr.closed_at))) continue
+                if (!await stillCurrent()) return
                 try {
                   await github.rest.actions.cancelWorkflowRun({
                     ...context.repo, run_id: member.run_id
@@ -80,7 +110,7 @@ jobs:
             }
       # No checkout: this runs trusted default-branch code, including for fork PRs.
       - uses: actions/github-script@v8
-        if: '!cancelled()'
+        if: ${{ !cancelled() && github.event.action == 'closed' }}
         with:
           script: |
             const ref = `refs/pull/${context.payload.pull_request.number}/merge`

config/scripts/ci-closed-pr-caches.test.mjs

@@ -22,7 +22,10 @@ function run(caches, remove = vi.fn().mockResolvedValue(undefined)) {
 }
 
 it('uses default-branch code without checking out a closed PR', () => {
-  expect(workflow.on).toEqual({ pull_request_target: { types: ['closed'] } })
+  expect(workflow.on).toEqual({ pull_request_target: { types: ['closed', 'synchronize'] } })
+  expect(workflow.jobs.clean.steps[1].if).toBe(
+    "${{ !cancelled() && github.event.action == 'closed' }}"
+  )
   expect(workflow.permissions).toEqual({ actions: 'write', 'pull-requests': 'read' })
   expect(workflow.jobs.clean.steps).toHaveLength(2)
   expect(workflow.jobs.clean.steps.every((step) => step.uses === 'actions/github-script@v8')).toBe(

config/scripts/ci-closed-pr-cancellation.test.mjs

@@ -31,10 +31,10 @@ function execute(options = {}) {
     }
   })
   const getPr = vi.fn(async () => ({
-    data: options.current ?? current
+    data: options.nextCurrent ?? options.current ?? current
   }))
-  if (options.reopened) {
-    getPr.mockResolvedValueOnce({ data: current })
+  if (options.reopened || options.nextCurrent) {
+    getPr.mockResolvedValueOnce({ data: options.reopened ? current : options.current })
   }
   const getRun = vi.fn(async () => ({ data: { ...run, ...options.run } }))
   const cancel = vi.fn(async () => {
@@ -45,7 +45,7 @@ function execute(options = {}) {
   const result = runInNewContext(`(async () => { ${step.with.script} })()`, {
     context: {
       repo: { owner: 'owner', repo: 'repo' },
-      payload: { pull_request: options.closed ?? closed }
+      payload: { action: options.action ?? 'closed', pull_request: options.closed ?? closed }
     },
     github: {
       request,
@@ -60,8 +60,10 @@ function execute(options = {}) {
 }
 
 it('uses trusted inline code and only enters cancellation on an unmerged close', () => {
-  expect(workflow.on).toEqual({ pull_request_target: { types: ['closed'] } })
-  expect(step.if).toBe('github.event.pull_request.merged == false')
+  expect(workflow.on).toEqual({ pull_request_target: { types: ['closed', 'synchronize'] } })
+  expect(step.if).toBe(
+    "github.event.action == 'synchronize' || github.event.pull_request.merged == false"
+  )
   expect(step.uses).toBe('actions/github-script@v8')
   expect(
     workflow.jobs.clean.steps.some((entry) => entry.uses?.startsWith('actions/checkout'))
@@ -77,7 +79,29 @@ it('looks up exact PR groups without branch or head-SHA inference', async () =>
     'ssh-windows-hosts-123',
     'ssh-hostile-hosts-123',
     'mobile-123',
-    'computer-e2e-123'
+    'computer-e2e-123',
+    'wsl-orcad-e2e-123',
+    'performance-contracts-123',
+    'pr-test-loc-123',
+    'preflight-critical-path-123',
+    'preflight-cli-123',
+    'macos-updater-123',
+    'preflight-comparison-123',
+    'windows-typecheck-parity-123',
+    'zcode-transcript-contract-123',
+    'release-ref-validation-123',
+    'docs-pr-123',
+    'cloud-verify-refs/pull/123/merge',
+    'pnpm-verification-pilot-123',
+    'win-orcad-serve-switch-e2e-123',
+    'pi-provider-runtime-123',
+    'coding-plan-contract-123-credentials',
+    'release-javascript-comparison-123',
+    'ci-cache-warmup-platforms-123',
+    'pi-owner-runtime-123',
+    'git-command-termination-123',
+    'skill-roundtrip-pull_request-123',
+    'ci-cache-warmup-123'
   ])
   expect(
     request.mock.calls.every(
@@ -146,7 +170,59 @@ it('surfaces lookup and cancellation permission failures', async () => {
 
 it('validates the event identity before looking up any work', async () => {
   const { result, request, getPr } = execute({ closed: { ...closed, number: '123' } })
-  await expect(result).rejects.toThrow('Missing closed PR identity')
+  await expect(result).rejects.toThrow('Missing PR identity')
   expect(request).not.toHaveBeenCalled()
   expect(getPr).not.toHaveBeenCalled()
 })
+
+const head = 'a'.repeat(40)
+const pushed = { number: 123, head: { sha: head } }
+const open = { ...pushed, state: 'open' }
+
+it('cancels an older commit even when a path filter prevents a replacement run', async () => {
+  const { result, cancel } = execute({
+    action: 'synchronize',
+    closed: pushed,
+    current: open,
+    run: { head_sha: 'b'.repeat(40) }
+  })
+  await result
+  expect(cancel).toHaveBeenCalledOnce()
+})
+
+it.each([head, undefined, 'invalid'])(
+  'preserves the current or unidentifiable commit: %s',
+  async (sha) => {
+    const { result, cancel } = execute({
+      action: 'synchronize',
+      closed: pushed,
+      current: open,
+      run: { head_sha: sha }
+    })
+    await result
+    expect(cancel).not.toHaveBeenCalled()
+  }
+)
+
+it('ignores a delayed push event after another push', async () => {
+  const { result, request, cancel } = execute({
+    action: 'synchronize',
+    closed: pushed,
+    current: { ...open, head: { sha: 'c'.repeat(40) } }
+  })
+  await result
+  expect(request).not.toHaveBeenCalled()
+  expect(cancel).not.toHaveBeenCalled()
+})
+
+it('rechecks the current head before cancelling when another push arrives during lookup', async () => {
+  const { result, cancel } = execute({
+    action: 'synchronize',
+    closed: pushed,
+    current: open,
+    nextCurrent: { ...open, head: { sha: 'b'.repeat(40) } },
+    run: { head_sha: 'b'.repeat(40) }
+  })
+  await result
+  expect(cancel).not.toHaveBeenCalled()
+})

config/scripts/ci-pr-concurrency-keys.test.mjs

@@ -0,0 +1,70 @@
+import { readFileSync, readdirSync } from 'node:fs'
+import { runInNewContext } from 'node:vm'
+import { expect, it, vi } from 'vitest'
+import { parse } from 'yaml'
+
+const directory = '.github/workflows'
+const number = 123
+const sha = 'a'.repeat(40)
+const githubContext = {
+  event_name: 'pull_request',
+  event: { pull_request: { number, head: { sha } } },
+  ref: `refs/pull/${number}/merge`,
+  sha: 'b'.repeat(40),
+  run_id: 456
+}
+
+function evaluate(expression) {
+  return expression.replace(/\$\{\{(.*?)\}\}/g, (_, source) =>
+    runInNewContext(source, {
+      github: githubContext,
+      inputs: {},
+      format: (template, value) => template.replace('{0}', value)
+    })
+  )
+}
+
+it('cleanup looks up the actual unique concurrency key of every automatic PR workflow', async () => {
+  const expected = new Map()
+  for (const file of readdirSync(directory).filter((name) => /\.ya?ml$/.test(name))) {
+    const workflow = parse(readFileSync(`${directory}/${file}`, 'utf8'))
+    if (!Object.hasOwn(workflow.on ?? {}, 'pull_request')) {
+      continue
+    }
+    expect(workflow.concurrency, file).toBeDefined()
+    const key = evaluate(workflow.concurrency.group)
+    expect(expected.has(key), `${file}: duplicate PR concurrency key ${key}`).toBe(false)
+    expect(String(evaluate(String(workflow.concurrency['cancel-in-progress']))), file).toBe('true')
+    expected.set(key, `.github/workflows/${file}`)
+  }
+  const cleanup = parse(readFileSync(`${directory}/ci-closed-pr-caches.yml`, 'utf8'))
+  const request = vi.fn()
+  let path
+  const getRun = vi.fn(async () => ({
+    data: { event: 'pull_request', status: 'queued', head_sha: 'c'.repeat(40), path }
+  }))
+  request.mockImplementation(async (_route, args) => {
+    path = expected.get(args.concurrency_group_name)
+    expect(path).toBeDefined()
+    return { data: { group_name: args.concurrency_group_name, group_members: [{ run_id: 42 }] } }
+  })
+  const cancel = vi.fn()
+  await runInNewContext(`(async () => { ${cleanup.jobs.clean.steps[0].with.script} })()`, {
+    context: {
+      repo: { owner: 'owner', repo: 'repo' },
+      payload: { action: 'synchronize', pull_request: githubContext.event.pull_request }
+    },
+    github: {
+      request,
+      rest: {
+        pulls: { get: async () => ({ data: { state: 'open', head: { sha } } }) },
+        actions: { getWorkflowRun: getRun, cancelWorkflowRun: cancel }
+      }
+    },
+    core: { info: vi.fn() }
+  })
+  expect(new Set(request.mock.calls.map(([, args]) => args.concurrency_group_name))).toEqual(
+    new Set(expected.keys())
+  )
+  expect(cancel).toHaveBeenCalledTimes(expected.size)
+})