Repository navigation
ci: cancel obsolete merge queue runs - #26760
kumarUjjawal merged 4 commits into
Conversation
comphead
left a comment
There was a problem hiding this comment.
Thanks for the analysis. A few design questions before this starts cancelling runs. I'd suggest shipping with cancellation off for a week and comparing the "would cancel" log against the analysis.
- Does cancelling a queued job release a
runs-oninstance that is already starting? That decides how much of the 312 late-starting jobs is real savings. - Filtered run listings cap at 1,000 results.
- A manual dry run shares the concurrency group, so it can replace a pending real scan.
|
|
||
| const head = await refSha(github, repo, current.head_branch); | ||
| if (head === current.head_sha) continue; | ||
| const reason = head === null ? "queue ref removed" : "queue ref replaced"; |
There was a problem hiding this comment.
A merged group also loses its ref. Non-required jobs still running at merge (e.g. cargo check datafusion-ffi features) get cancelled here, and main shows a cancelled run. Could this keep runs already on the base branch, e.g. compareCommitsWithBasehead status identical or behind?
| on: | ||
| # A replacement group starts cleanup promptly; the sweep also catches groups | ||
| # removed without a replacement and runs that appear after an earlier scan. | ||
| merge_group: |
There was a problem hiding this comment.
Does this event arrive before or after the old group's ref is deleted? If before, this scan finds nothing and everything waits for the cron (17.9h vs 9.4h saved by the PR's numbers).
| // Called on each new merge group and on a schedule. The queue's exact Git | ||
| // refs, rather than PR state or failing checks, determine which runs are stale. |
There was a problem hiding this comment.
Why refs rather than GraphQL mergeQueue(branch: "main").entries { headCommit { oid } }? One query gives the active set and removes the per-ref lookups, cache and regex. Also, the ref suffix is the base SHA, so "queue ref replaced" seems to need the same PR re-queued on the same base. Is that the intended case?
| concurrency: | ||
| group: merge-queue-cleanup | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
These runs attach checks to group SHAs that become main commits. Pending runs replaced here show as cancelled, and setFailed on API errors shows red. Would warnings be enough?
There was a problem hiding this comment.
The setFailed part is fixed, thanks. The cancelled part remains. A queue rebuild creates several groups at once, so one scan runs, the next waits as pending and a third replaces it. The replaced run shows as cancelled on a group commit that can land on main. From Oct 19 to 27, three Rust merge-group runs started within 60 s of each other 15 times. Could merge_group scans use a per-SHA group, or drop that trigger if the observation week shows it rarely finds anything?
| !eligible(current, fullName, context.runId) || | ||
| current.head_branch !== candidate.head_branch || | ||
| current.head_sha !== candidate.head_sha || | ||
| current.run_attempt !== candidate.run_attempt |
There was a problem hiding this comment.
The next sweep cancels the rerun anyway, so this guard (and the 409 recheck) lasts at most 15 minutes. Exempt run_attempt > 1 for real, or drop the attempt logic?
| // A 409 can mean a completion race, but also an active run that | ||
| // GitHub cannot cancel. Only suppress it after confirming the race. | ||
| if (error.status === 409) { |
There was a problem hiding this comment.
What does cancel return for a run that is already cancelling? If 409, the run is still in_progress and the scan fails every sweep until it finishes. I haven't verified.
| persist-credentials: false | ||
| - name: Cancel obsolete merge-group runs | ||
| # During the PR that introduces this workflow, main has no script yet. | ||
| # Always execute main's script, never code from the merge group. |
There was a problem hiding this comment.
For merge_group the workflow file comes from the group commit, so a queued PR editing this file runs its own script. Maybe soften to "Execute main's copy of the script".
| assert.equal(new Set(result.cancellations).size, 205); | ||
| }); | ||
|
|
||
| test("workflow loads default-branch code and supports prompt cleanup, sweeping, and dry runs", () => { |
There was a problem hiding this comment.
This re-asserts the YAML literals and won't catch a behavior bug. Could it go?
|
Thank you @comphead Pushed new changes to address the comments |
comphead
left a comment
There was a problem hiding this comment.
Thanks for the update. Merged commits, reruns, observation mode, the dry-run concurrency split and the merge-group warnings all look good to me.
- Optional:
compareCommitsWithBaseheadruns once per workflow while the ref lookup is cached per branch. Caching the comparison by SHA would save a few calls per group. - Optional: the contributor guide section now reads like a rollout runbook. The observation checklist might fit better in #25148.
|
|
||
| // Filtered Actions listings are capped at 1,000 results even with pagination. | ||
| // Split full windows by creation time before trusting the result set. | ||
| async function listRuns(github, repo, status, start, end) { |
There was a problem hiding this comment.
Thanks for handling the cap. Active merge-group runs stay far below 1,000 (167 Rust merge-group runs in all of Oct 19 to 27). Would failing the scan when total_count >= 1000 be enough? It drops the recursive split and its two tests.
| assert.equal(`${owner}/${repo}`, fullName); | ||
| calls.push(`cancel:${run_id}`); | ||
| if (cancelErrors[run_id]) throw cancelErrors[run_id]; | ||
| calls.push(`cancel:${run_id}`); |
There was a problem hiding this comment.
This pushes cancel:${run_id} a second time. Was it meant to record something else?
comphead
left a comment
There was a problem hiding this comment.
Thanks @kumarUjjawal just some nits left
|
Thank you @comphead for the review. Let's test this |
Which issue does this PR close?
Related to #25148. Inspired by apache/airflow#75619.
Rationale for this change
Merge queue runs continue using runners after their group is removed or replaced. The Rust workflow gives each merge-group SHA a separate concurrency group, so a replacement does not cancel the old run. Jobs still waiting for a runner can start long after the queue has stopped using their results.
A read-only analysis of runs created on October 19–25, 2026 (UTC) found:
For the Rust and extended-test workflows, this was 19.0 of 369.4 runner-hours, or 5.2% of their sampled runner time. Cancellation taking effect one minute after obsolescence would have avoided 17.9 runner-hours; at five minutes, 11.9 hours; at a fixed fifteen-minute delay, 9.4 hours. A separate model using the next exact quarter-hour boundary estimates 11.2 hours.
Examples:
The cutoff was an exact-SHA queue-removal event or the creation of a replacement group for the same PR. For each job, the analysis summed time after the later of its start and the cutoff plus the assumed cancellation delay. It excluded skipped jobs, deduplicated successful jobs copied into rerun records, and excluded seven rerun attempts started after obsolescence that could have been deliberate debugging. All Rust and extended-test jobs were fetched for the denominator; other workflows were inspected when their run metadata showed activity after the cutoff. Merged groups and all rerun attempts contributed zero to the reported avoidable time, so preserving both leaves these estimates unchanged.
These are historical runner times and estimated avoidable portions, not deployed savings or billing estimates. Of the 312 late-starting jobs, 42 used Runs-On (2.4 runner-hours); 270 used GitHub Linux runners. We have not measured whether cancelling queued jobs immediately terminates instances already starting. The models also exclude ref-deletion latency, cancellation cleanup, scheduler delays, and cleanup-workflow overhead. Recent runner changes make this sample unsuitable as an annual forecast.
What changes are included in this PR?
MERGE_QUEUE_CLEANUP_ENABLED=true. Document at least one week of observation before enabling it; there is no automatic activation.merge_groupruns inapache/datafusionwhose exact queue ref is missing or points elsewhere. Recheck the run and ref, then compare with its target branch to preserve commits already merged. Preserve reruns, other event types, and cleanup itself.What is the testing strategy for this PR?
cargo fmt --all,cargo clippy --all-targets --all-features -- -D warnings, and the complete./dev/rust_lint.shsuite pass, including generated docs, links, dependency checks, Rust docs, and the HTML documentation build.Are there any user-facing changes?
Initially the workflow reports candidates only. Once enabled by a maintainer, it cancels obsolete first-attempt merge-queue runs while preserving merged commits and deliberate reruns. Contributors can rerun obsolete jobs for debugging. No query behavior or public API changes.