mirror of
https://github.com/stablyai/orca.git
synced 2026-10-07 00:02:29 +00:00
Stop expensive checks when an unmerged PR closes (#25829)
* Cancel active checks when an unmerged PR closes * Register owned-branch cancellation qualification * Keep temporary cancellation qualification outside the review diff
This commit is contained in:
@@ -1,4 +1,4 @@
|
|||||||
name: Clean closed PR caches
|
name: Clean closed PR work
|
||||||
|
|
||||||
on:
|
on:
|
||||||
pull_request_target:
|
pull_request_target:
|
||||||
@@ -6,14 +6,81 @@ on:
|
|||||||
|
|
||||||
permissions:
|
permissions:
|
||||||
actions: write
|
actions: write
|
||||||
|
pull-requests: read
|
||||||
|
|
||||||
jobs:
|
jobs:
|
||||||
clean:
|
clean:
|
||||||
runs-on: ubuntu-latest
|
runs-on: ubuntu-latest
|
||||||
timeout-minutes: 5
|
timeout-minutes: 5
|
||||||
steps:
|
steps:
|
||||||
|
- name: Cancel checks for an unmerged closed PR
|
||||||
|
if: github.event.pull_request.merged == false
|
||||||
|
uses: actions/github-script@v8
|
||||||
|
with:
|
||||||
|
script: |
|
||||||
|
const closed = context.payload.pull_request
|
||||||
|
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']
|
||||||
|
]
|
||||||
|
const stillClosed = async () => {
|
||||||
|
const { data: current } = await github.rest.pulls.get({
|
||||||
|
...context.repo, pull_number: closed.number
|
||||||
|
})
|
||||||
|
return current.state === 'closed' && current.merged_at === null &&
|
||||||
|
current.closed_at === closed.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 (!await stillClosed()) return
|
||||||
|
for (const [prefix, workflow] of targets) {
|
||||||
|
const group = `${prefix}-${closed.number}`
|
||||||
|
let data
|
||||||
|
try {
|
||||||
|
const response = await github.request(
|
||||||
|
'GET /repos/{owner}/{repo}/actions/concurrency_groups/{concurrency_group_name}', {
|
||||||
|
...context.repo, concurrency_group_name: group,
|
||||||
|
headers: { 'X-GitHub-Api-Version': '2026-03-10' }
|
||||||
|
}
|
||||||
|
)
|
||||||
|
data = response.data
|
||||||
|
} catch (error) {
|
||||||
|
if (error.status === 404) continue
|
||||||
|
throw error
|
||||||
|
}
|
||||||
|
if (data.group_name !== group || !Array.isArray(data.group_members)) {
|
||||||
|
throw new Error(`Unexpected concurrency group: ${group}`)
|
||||||
|
}
|
||||||
|
for (const member of data.group_members) {
|
||||||
|
// Only whole PR runs; release/manual jobs never share this identity.
|
||||||
|
if (member.job_id !== undefined || !Number.isSafeInteger(member.run_id)) continue
|
||||||
|
const { data: run } = await github.rest.actions.getWorkflowRun({
|
||||||
|
...context.repo, run_id: member.run_id
|
||||||
|
})
|
||||||
|
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
|
||||||
|
try {
|
||||||
|
await github.rest.actions.cancelWorkflowRun({
|
||||||
|
...context.repo, run_id: member.run_id
|
||||||
|
})
|
||||||
|
core.info(`Requested cancellation of ${group}: ${member.run_id}`)
|
||||||
|
} catch (error) {
|
||||||
|
if (error.status !== 409) throw error
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
# No checkout: this runs trusted default-branch code, including for fork PRs.
|
# No checkout: this runs trusted default-branch code, including for fork PRs.
|
||||||
- uses: actions/github-script@v8
|
- uses: actions/github-script@v8
|
||||||
|
if: '!cancelled()'
|
||||||
with:
|
with:
|
||||||
script: |
|
script: |
|
||||||
const ref = `refs/pull/${context.payload.pull_request.number}/merge`
|
const ref = `refs/pull/${context.payload.pull_request.number}/merge`
|
||||||
|
|||||||
@@ -4,7 +4,7 @@ import { expect, it, vi } from 'vitest'
|
|||||||
import { parse } from 'yaml'
|
import { parse } from 'yaml'
|
||||||
|
|
||||||
const workflow = parse(readFileSync('.github/workflows/ci-closed-pr-caches.yml', 'utf8'))
|
const workflow = parse(readFileSync('.github/workflows/ci-closed-pr-caches.yml', 'utf8'))
|
||||||
const script = workflow.jobs.clean.steps[0].with.script
|
const script = workflow.jobs.clean.steps[1].with.script
|
||||||
const ref = 'refs/pull/123/merge'
|
const ref = 'refs/pull/123/merge'
|
||||||
|
|
||||||
function run(caches, remove = vi.fn().mockResolvedValue(undefined)) {
|
function run(caches, remove = vi.fn().mockResolvedValue(undefined)) {
|
||||||
@@ -23,9 +23,11 @@ function run(caches, remove = vi.fn().mockResolvedValue(undefined)) {
|
|||||||
|
|
||||||
it('uses default-branch code without checking out a closed PR', () => {
|
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'] } })
|
||||||
expect(workflow.permissions).toEqual({ actions: 'write' })
|
expect(workflow.permissions).toEqual({ actions: 'write', 'pull-requests': 'read' })
|
||||||
expect(workflow.jobs.clean.steps).toHaveLength(1)
|
expect(workflow.jobs.clean.steps).toHaveLength(2)
|
||||||
expect(workflow.jobs.clean.steps[0].uses).toBe('actions/github-script@v8')
|
expect(workflow.jobs.clean.steps.every((step) => step.uses === 'actions/github-script@v8')).toBe(
|
||||||
|
true
|
||||||
|
)
|
||||||
})
|
})
|
||||||
|
|
||||||
it('lists and deletes only caches scoped to the closed merge ref', async () => {
|
it('lists and deletes only caches scoped to the closed merge ref', async () => {
|
||||||
|
|||||||
@@ -0,0 +1,152 @@
|
|||||||
|
import { readFileSync } from 'node:fs'
|
||||||
|
import { runInNewContext } from 'node:vm'
|
||||||
|
import { expect, it, vi } from 'vitest'
|
||||||
|
import { parse } from 'yaml'
|
||||||
|
|
||||||
|
const workflow = parse(readFileSync('.github/workflows/ci-closed-pr-caches.yml', 'utf8'))
|
||||||
|
const step = workflow.jobs.clean.steps[0]
|
||||||
|
const closed = { number: 123, closed_at: '2026-10-06T00:00:00Z' }
|
||||||
|
const current = { ...closed, state: 'closed', merged_at: null }
|
||||||
|
const run = {
|
||||||
|
id: 42,
|
||||||
|
event: 'pull_request',
|
||||||
|
path: '.github/workflows/pr.yml',
|
||||||
|
status: 'in_progress',
|
||||||
|
created_at: '2026-10-05T23:59:00Z'
|
||||||
|
}
|
||||||
|
|
||||||
|
function execute(options = {}) {
|
||||||
|
const request = vi.fn(async (_route, args) => {
|
||||||
|
if (options.requestError) {
|
||||||
|
throw options.requestError
|
||||||
|
}
|
||||||
|
if (args.concurrency_group_name !== 'pr-checks-123') {
|
||||||
|
throw { status: 404 }
|
||||||
|
}
|
||||||
|
return {
|
||||||
|
data: {
|
||||||
|
group_name: options.group ?? 'pr-checks-123',
|
||||||
|
group_members: options.members ?? [{ run_id: 42 }]
|
||||||
|
}
|
||||||
|
}
|
||||||
|
})
|
||||||
|
const getPr = vi.fn(async () => ({
|
||||||
|
data: options.current ?? current
|
||||||
|
}))
|
||||||
|
if (options.reopened) {
|
||||||
|
getPr.mockResolvedValueOnce({ data: current })
|
||||||
|
}
|
||||||
|
const getRun = vi.fn(async () => ({ data: { ...run, ...options.run } }))
|
||||||
|
const cancel = vi.fn(async () => {
|
||||||
|
if (options.cancelError) {
|
||||||
|
throw options.cancelError
|
||||||
|
}
|
||||||
|
})
|
||||||
|
const result = runInNewContext(`(async () => { ${step.with.script} })()`, {
|
||||||
|
context: {
|
||||||
|
repo: { owner: 'owner', repo: 'repo' },
|
||||||
|
payload: { pull_request: options.closed ?? closed }
|
||||||
|
},
|
||||||
|
github: {
|
||||||
|
request,
|
||||||
|
rest: {
|
||||||
|
pulls: { get: getPr },
|
||||||
|
actions: { getWorkflowRun: getRun, cancelWorkflowRun: cancel }
|
||||||
|
}
|
||||||
|
},
|
||||||
|
core: { info: vi.fn() }
|
||||||
|
})
|
||||||
|
return { result, request, getPr, getRun, cancel }
|
||||||
|
}
|
||||||
|
|
||||||
|
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(step.uses).toBe('actions/github-script@v8')
|
||||||
|
expect(
|
||||||
|
workflow.jobs.clean.steps.some((entry) => entry.uses?.startsWith('actions/checkout'))
|
||||||
|
).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
it('looks up exact PR groups without branch or head-SHA inference', async () => {
|
||||||
|
const { result, request, cancel } = execute()
|
||||||
|
await result
|
||||||
|
expect(request.mock.calls.map(([, args]) => args.concurrency_group_name)).toEqual([
|
||||||
|
'pr-checks-123',
|
||||||
|
'node-server-123',
|
||||||
|
'ssh-windows-hosts-123',
|
||||||
|
'ssh-hostile-hosts-123',
|
||||||
|
'mobile-123',
|
||||||
|
'computer-e2e-123'
|
||||||
|
])
|
||||||
|
expect(
|
||||||
|
request.mock.calls.every(
|
||||||
|
([route, args]) =>
|
||||||
|
route === 'GET /repos/{owner}/{repo}/actions/concurrency_groups/{concurrency_group_name}' &&
|
||||||
|
args.headers['X-GitHub-Api-Version'] === '2026-03-10'
|
||||||
|
)
|
||||||
|
).toBe(true)
|
||||||
|
expect(cancel.mock.calls).toEqual([[{ owner: 'owner', repo: 'repo', run_id: 42 }]])
|
||||||
|
})
|
||||||
|
|
||||||
|
it.each([
|
||||||
|
{ event: 'push' },
|
||||||
|
{ event: 'workflow_dispatch' },
|
||||||
|
{ path: '.github/workflows/release-cut.yml' },
|
||||||
|
{ status: 'completed' },
|
||||||
|
{ created_at: '2026-10-06T00:00:01Z' },
|
||||||
|
{ created_at: 'invalid' }
|
||||||
|
])('retains unrelated, completed and post-close runs: %j', async (otherRun) => {
|
||||||
|
const { result, cancel } = execute({ run: otherRun })
|
||||||
|
await result
|
||||||
|
expect(cancel).not.toHaveBeenCalled()
|
||||||
|
})
|
||||||
|
|
||||||
|
it.each([
|
||||||
|
{ ...current, state: 'open' },
|
||||||
|
{ ...current, merged_at: closed.closed_at },
|
||||||
|
{ ...current, closed_at: '2026-10-06T00:01:00Z' }
|
||||||
|
])('retains checks if the closure is no longer current: %j', async (pr) => {
|
||||||
|
const { result, request, cancel } = execute({ current: pr })
|
||||||
|
await result
|
||||||
|
expect(request).not.toHaveBeenCalled()
|
||||||
|
expect(cancel).not.toHaveBeenCalled()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('rechecks closure before cancelling when a PR reopens during lookup', async () => {
|
||||||
|
const { result, getRun, cancel } = execute({
|
||||||
|
current: { ...current, state: 'open' },
|
||||||
|
reopened: true
|
||||||
|
})
|
||||||
|
await result
|
||||||
|
expect(getRun).toHaveBeenCalledOnce()
|
||||||
|
expect(cancel).not.toHaveBeenCalled()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('ignores job-level leases and invalid run identities', async () => {
|
||||||
|
const { result, getRun, cancel } = execute({
|
||||||
|
members: [{ run_id: 42, job_id: 1 }, { run_id: '42' }]
|
||||||
|
})
|
||||||
|
await result
|
||||||
|
expect(getRun).not.toHaveBeenCalled()
|
||||||
|
expect(cancel).not.toHaveBeenCalled()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('refuses a mismatched concurrency group', async () => {
|
||||||
|
const { result, cancel } = execute({ group: 'pr-checks-124' })
|
||||||
|
await expect(result).rejects.toThrow('Unexpected concurrency group')
|
||||||
|
expect(cancel).not.toHaveBeenCalled()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('surfaces lookup and cancellation permission failures', async () => {
|
||||||
|
await expect(execute({ requestError: { status: 403 } }).result).rejects.toEqual({ status: 403 })
|
||||||
|
await expect(execute({ cancelError: { status: 403 } }).result).rejects.toEqual({ status: 403 })
|
||||||
|
await expect(execute({ cancelError: { status: 409 } }).result).resolves.toBeUndefined()
|
||||||
|
})
|
||||||
|
|
||||||
|
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')
|
||||||
|
expect(request).not.toHaveBeenCalled()
|
||||||
|
expect(getPr).not.toHaveBeenCalled()
|
||||||
|
})
|
||||||
Reference in New Issue
Block a user