diff --git a/.github/workflows/ci-cache-warmup.yml b/.github/workflows/ci-cache-warmup.yml index 882dfac0bab..9310d87a133 100644 --- a/.github/workflows/ci-cache-warmup.yml +++ b/.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: diff --git a/.github/workflows/ci-closed-pr-caches.yml b/.github/workflows/ci-closed-pr-caches.yml index 468559ccd4d..a5a3a9f33f2 100644 --- a/.github/workflows/ci-closed-pr-caches.yml +++ b/.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` diff --git a/config/scripts/ci-closed-pr-caches.test.mjs b/config/scripts/ci-closed-pr-caches.test.mjs index dbab17e889a..08a321ea363 100644 --- a/config/scripts/ci-closed-pr-caches.test.mjs +++ b/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( diff --git a/config/scripts/ci-closed-pr-cancellation.test.mjs b/config/scripts/ci-closed-pr-cancellation.test.mjs index 13f933932ca..bb118b5a9a8 100644 --- a/config/scripts/ci-closed-pr-cancellation.test.mjs +++ b/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() +}) diff --git a/config/scripts/ci-pr-concurrency-keys.test.mjs b/config/scripts/ci-pr-concurrency-keys.test.mjs new file mode 100644 index 00000000000..3150d3448da --- /dev/null +++ b/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) +})