mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-04 02:57:27 +09:00
Aleksei Sviridkin
cf6bf2e379
## What this PR does A run joins its concurrency group before any job-level `if` is evaluated, so `prepare`'s guard in `backport.yaml` is too late to help. By the time it works out that a label event is irrelevant, that run has already cancelled whatever was in flight for the same PR. Labels set with the default `GITHUB_TOKEN` start no run, which is why the in-repo labelers never showed this. Third-party GitHub Apps do start runs, and they arrive in bursts. The delivery survived, which is why this went unnoticed for so long. The old guard admitted a run on the PR's cumulative label set, so the last label of a burst, the one nothing cancels, requalified and redid the work. In the bursts on record the kill lands within seconds of the merge run starting, well before the cherry-pick, so the cost was a restart rather than a lost backport, recorded as nothing more than a cancelled run sitting next to a green one. Two changes to the concurrency key, and both are needed. A label event that requests no backport now goes into a group of its own, so it can neither cancel nor displace the run doing the work. A label event that does request one queues behind that run instead of killing it, because `backport` and `backport-previous` are separate requests and both have to finish. Splitting alone would let `backport` cancel the merge-triggered run. Queuing alone would let an unrelated label evict a pending request, since the group holds a single pending run and a newer one replaces it. `prepare`'s condition is scoped in the same commit. It read the PR's cumulative label set, so once a merged PR carried `backport`, any later unrelated label re-entered the job to redo a backport already delivered. That run used to get cancelled by the next label in the burst. After the change above it runs to completion, so the scoping has to land here rather than as a follow-up. The coupling runs the other way too, and it is the sharper half. That redundant run is exactly what redelivered the backport the burst had just killed, so narrowing the guard on its own would convert a noisy delivery into a missing one. Neither change is safe to land without the other. Both jobs also gain a `timeout-minutes` ceiling, which is a consequence of the queuing rather than housekeeping. A cancelling key disposed of a stuck run by killing it; queuing makes the next genuine request wait behind it instead, on the six-hour job default, which turns one wedged job into a six-hour hole in the release line. That ceiling reaches a stuck job and nothing else: a run can also wedge at the run level with every job already finished, and `timeout-minutes` has no run-level equivalent, so nothing in the workflow reaches that state. `docs/release.md` now tells an operator to look for a run still in flight and clear it before re-applying the label, because the retry queues behind it rather than replacing it. The ceiling bounds execution, not the wait for a runner, and thirty minutes is generous rather than calculated. Execution on run 29844315422 was six seconds for `prepare` and two and sixteen for the matrix legs, against 9m24s, 5m28s and 5m45s of runner queue for the same three jobs. That queue is a third unbounded case: GitHub's limits page says "job execution time" for the six-hour job cap while spelling out that the 35-day run cap "includes execution duration, and time spent on waiting and approval", and [orgs/community#50926](https://github.com/orgs/community/discussions/50926) shows a job sitting six hours on "Waiting for a runner to pick up this job" under `timeout-minutes: 5`. So the value is picked to leave room for a cherry-pick far larger than any on record while replacing the six-hour default, and nothing here is sized against the queue, because nothing here can be. This removes a mitigation, and the trade is worth naming on both sides. Under an unconditional cancel a run wedged at the run level cleared itself, because the next event killed whatever was sitting in the group. That is the same behaviour that killed live backports, so the mitigation is inseparable from the bug being fixed: keeping it means keeping label events that cancel a backport in flight. What it buys is a change of failure mode rather than one fewer failure. A backport lost silently, discovered weeks later as a fix missing from a release line, becomes a delay the operator can see in the run list, with the recovery written into `docs/release.md`. There is no lossless alternative here: `queue: max` removes the eviction but only by dropping the conditional cancel entirely, and the group key cannot tell a manual retry from a second genuine request. The run cited above, 31232645133, is still live and retained on purpose: it is the only observed instance of the run-level wedge, so clearing it would remove the evidence for the paragraph that describes it. Its exposure is one pull request, #3262, which carries no backport label, so nothing is queued behind it. The old condition also carried a disjunct that could never decide anything. A `labeled` payload already lists the new label in `pull_request.labels`, so `contains` was true whenever the explicit `github.event.label.name` check was, and the name check only ever agreed with a `contains` that had already matched. The restructure drops it by construction rather than by deletion: the name check is now the whole of what a `labeled` event may match on. ### Why this no longer touches `pull-requests.yaml` `main` fixed it there by splitting the group key, then refined that so a `full-e2e` label stays in the main group instead of publishing a second `E2E Tests` status on one SHA. That is the better mechanism, it neither queues nor delays, and `gate-concurrency-contract.bats` now pins `cancel-in-progress: true` in that file, so what this PR originally did would turn it red. Nothing was left to add there, so the hunk is dropped and its pin with it. The split-key idea is applied in `backport.yaml` instead, where it did not exist. One claim from the earlier description is withdrawn. It said the leftover risk was a duplicate backport PR, opened when the original had merged and its branch had been deleted before the queued run pushed. `main` has since gained a guard that lists closed PRs on the `backport-<n>-to-<target>` branch and skips the target when one of them merged. Branch deletion does not defeat it: `GET /branches/backport-3510-to-release-1.6` is 404 while the same head filter on `/pulls?state=closed` still returns the merged PR. So a redundant run only cost runner minutes, and the scoping above removes the run. ### Screenshots Not applicable, no UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. The only workflow it cites is `tags.yaml`, for the release-prep behaviour `cozystack/ccp` documents. Every `hack/` trigger it lists either names a specific file another repository mirrors or gates on, or covers moving and renaming under `hack/`. This PR appends tests to a contract file the map does not name, and moves nothing. No package, chart, values file, CRD, image reference, namespace, variant or node prerequisite in the diff. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(ci): stop `labeled` pull request events from cancelling the automatic backport run already in flight. A burst of labels from a third-party app could previously kill the backport the merge had just started, leaving the work to be redone by whichever label arrived last. ```