fix(ci): serialise the two workflows that push to a PR head branch - #7392
Conversation
`@qwen-code /resolve` and the autofix loop's own conflict path both merge the base branch and push to the PR's head, but they live in different workflows, so their per-PR concurrency groups only guarded each against itself. On #7355 they ran together: /resolve pushed at 03:51, the autofix leg pushed at 04:05 and was rejected `fetch first`, discarding a full agent run and leaving no marker behind. GitHub concurrency groups are repository-scoped, so both jobs now use the same `qwen-pr-head-write-<pr>` group and queue behind each other instead of racing. The prefix has to be a literal in both files — job-level `concurrency` cannot read the `env` context — so a test pins the two equal; renaming one side alone would silently re-open the race.
|
@qwen-code /takeover |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。移除 |
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with concrete evidence — the #7355 timeline (03:43 scan → 03:51 /resolve push → 04:05 autofix push rejected Direction: CI reliability, clearly in scope. The shared concurrency group approach is the right primitive — GitHub's groups are repo-scoped, so an identical name across workflows serialises them. No CHANGELOG reference needed for internal CI tooling. Size: not applicable (no core module paths touched — Approach: the scope feels exactly right. Two lines of actual change (rename the group prefix in each workflow), the rest is comments explaining the invariant and a test pinning both sides equal. The PR is honest about what it does not buy (the loser may still spend an agent run) and defers the recheck improvement deliberately. No unrelated changes, no scope creep. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的 bug,有具体证据——#7355 时间线(03:43 扫描 → 03:51 /resolve 推送 → 04:05 autofix 推送被 方向:CI 可靠性修复,明确在范围内。共享 concurrency 组是正确的原语——GitHub 的组是仓库级的,因此跨 workflow 使用相同名称即可串行化。 规模:不适用(未触及核心模块路径——仅 方案:范围恰到好处。实际改动只有两行(重命名各 workflow 的组前缀),其余是解释不变量的注释和一个钉住两侧相等的测试。PR 坦诚说明了它不能买到什么(输的一方仍可能花掉一次 agent 运行),并刻意推迟了 recheck 改进。无无关改动,无范围蔓延。 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewIndependent proposal: two jobs in different workflows push to the same PR head branch with different concurrency group names, so each only serialises against itself. The simplest fix is to give them the same group name — GitHub's concurrency groups are repo-scoped, so an identical name across workflows makes them mutually exclusive. Alternatives (check-before-push, file-based lock) add complexity without preventing the wasted agent run. Comparison with the diff: the PR does exactly this, and nothing else. Two lines of actual change — rename the group prefix in each workflow from No critical blockers. No convention violations. The comments are verbose for a two-line change, but this is a subtle cross-file invariant where a one-sided rename silently re-opens the race — the verbosity earns its place. TestingThis is a CI-only change (GitHub Actions concurrency groups) — no user-facing CLI behavior to drive in tmux. Before/After: N/A. Test suite — 26/26 pass: YAML validation — both workflow files parse cleanly under js-yaml: Mutation tests — verified the new test catches regressions:
Concurrency group audit — no other jobs in either workflow push to an existing PR's head branch:
中文说明代码审查独立方案: 两个不同 workflow 中的作业以不同的 concurrency 组名推送同一 PR head 分支,因此各自只与自身互斥。最简方案是统一组名——GitHub 的 concurrency 组是仓库级的,跨 workflow 使用相同名称即可串行化。替代方案(推送前检查、文件锁)增加复杂度却不能避免浪费的 agent 运行。 与 diff 对比: PR 正是这样做的,且仅此而已。实际改动两行——将两个 workflow 的组前缀从 无关键阻塞项。无规范违反。注释对于两行改动来说偏多,但这是一个微妙的跨文件不变量——单侧重命名会悄悄复活竞态——冗长是有价值的。 测试这是纯 CI 改动(GitHub Actions concurrency 组)——无用户可见的 CLI 行为可在 tmux 中驱动。Before/After:不适用。 测试套件 26/26 通过。YAML 验证通过。变异测试确认新测试能捕获回归(重命名单侧前缀 → 红;翻转 cancel-in-progress → 红)。Concurrency 组审计确认无其他作业推送已有 PR 的 head 分支。 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 5/5 — observed race, minimal fix, test pins the invariant, nothing to second-guess. This is the textbook version of what a CI fix should look like. Two lines of actual change — rename the concurrency group prefix so GitHub's repo-scoped serialisation kicks in — wrapped in enough comment to explain why the prefix must stay byte-identical across files, and a test that makes a one-sided rename fail loudly instead of silently re-opening the race. The #7355 timeline is concrete evidence, not a hypothesis. The approach matches what I'd propose independently: GitHub concurrency groups are the right primitive here, and No unrelated changes, no scope creep, no over-engineering. The concurrency group audit confirms no other jobs in either workflow push to an existing PR head, so the two-job serialisation is complete. If I had to maintain this in six months, the comments and the test would tell me exactly what's going on and what not to break. 中文说明置信度:5/5 — 已观测到的竞态,最小化修复,测试钉住不变量,无可置疑。 这是 CI 修复的教科书版本。实际改动两行——重命名 concurrency 组前缀以启用 GitHub 仓库级串行化——配以足够的注释解释为何前缀必须跨文件保持字节一致,以及一个让单侧重命名大声失败而非悄悄复活竞态的测试。 #7355 时间线是具体证据,不是假设。方案与我独立提出的一致:GitHub concurrency 组是正确的原语, 无无关改动,无范围蔓延,无过度工程。Concurrency 组审计确认两个 workflow 中无其他作业推送已有 PR 的 head,因此两作业串行化是完备的。 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
doudouOUC
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Clean fix — shared concurrency group serialises the two writers, test pins both sides equal. Well-documented rationale from the #7355 incident.
|
Released in v0.20.1. |
|
PR #7392 已合并,最新评论仅是发布机器人通知已随 v0.20.1 发布。无需代码变更,无需回复。确认收到。 ✅ completed |
What this PR does
Makes the two workflows that push to a PR's head branch take turns instead of racing.
Why it's needed
Two independent jobs resolve conflicts on the same branch:
review-address(autofix'sconflict=truepath)qwen-autofix.ymlqwen-autofix-review-<pr>resolve-pr(@qwen-code /resolve)qwen-code-pr-review.ymlqwen-resolve-<pr>Both merge the base branch and push. Different group names means neither guards against the other — each only serialises against itself.
Observed on #7355:
The loser had already spent its agent run. Its job died in
Push and report, so it wrote no eval marker — the round is simply gone, and from outside the PR just looks idle. This is the same failure I had previously mis-filed as a generic "push race"; #7262's was a merge race, but this one is two owners with no lock.How
GitHub concurrency groups are repository-scoped, not workflow-scoped, so two jobs in different workflows sharing a group name do serialise. Both now use
qwen-pr-head-write-<pr>, keepingcancel-in-progress: falseso the loser queues rather than being discarded.issue-autofixis deliberately left out: it pushes a branch it created for a new PR, so it has no contender.The prefix is a literal in both files because job-level
concurrencycannot read theenvcontext, so the two cannot share a constant. A test pins them equal instead — renaming one side alone would re-open the race with nothing failing.What this does and does not buy
review-addressfetches the head by name at job start (git fetch … refs/heads/${BRANCH}→git checkout -B "${BRANCH}" FETCH_HEAD), so the second run reads the winner's result rather than a stale base: its work is usable and its push lands.It may still spend an agent run. The address-time recheck re-verifies lifecycle and consent (state, labels, author, base, head branch) but not whether the conflict is still there, so the loser does the work and finds little to do rather than discarding for free. Making the recheck re-test the trigger condition is the natural follow-up and is deliberately not in this PR.
Reviewer Test Plan
How to verify
npx vitest run scripts/tests/qwen-resolve-workflow.test.js— 26/26. The new test reads both workflow files, extracts each job's group prefix by regex, and asserts they are equal, that each still keys on the PR number, and that both queue rather than cancel.Mutation-verified, three reverts each turning it red:
qwen-autofix-review-expected 'qwen-autofix-review' to be 'qwen-pr-head-write'github.run_id(shared name, serialises nothing)resolve-prtocancel-in-progress: trueStatic, run locally with the exact CI toolchain: both files parse under
js-yaml; all 37 + 17run:blocks passbash -n; actionlint 1.7.12 clean on both; the added lines are prettier-clean.Post-merge smoke: issue
@qwen-code /resolveon a PR while an autofixreview-addressleg is running for it — the resolve job should sit queued and start only after the leg finishes.Evidence (Before & After)
Tested on
Risk & Scope
/resolvecan now sit queued behind areview-addressleg for that PR, whose timeout cap is 120 minutes. That is the intended semantics — the alternative is the current wasted run — but it is a real latency change, and GitHub shows nothing to the commenter while a job is pending. If that proves annoying, the follow-up above (cheap early discard) reduces how long the lock is held rather than removing it./resolvearriving while one is pending cancels the pending one, which is latest-intent and correct here.qwen-resolve-workflow,qwen-autofix-workflowandqwen-pr-review-workflowtogether reliably failseligibility recheckandtakeover-command toggle— a load flake. A/B on this machine: unmodifiedmainfails the same two, twice, in the same combined run, and both pass in isolation. Not introduced here.Linked Issues
Found while analysing the takeover fleet's operating state — #7355's round-6 work had vanished with no marker. Part of the autofix-reliability line: #7330, #7350, #7351, #7354, #7355, #7358, #7364.
中文说明
本 PR 做了什么
让两个会向 PR head 分支推送的 workflow 排队,而不是互相竞争。
为什么需要
有两个互不相识的作业在解同一个分支的冲突:
review-address(autofix 的conflict=true路径)qwen-autofix.ymlqwen-autofix-review-<pr>resolve-pr(@qwen-code /resolve)qwen-code-pr-review.ymlqwen-resolve-<pr>两者都会合并 base 分支并推送。组名不同,意味着谁也拦不住谁 —— 各自只与自己互斥。
在 #7355 上实测到(见上方英文时间线):03:43 扫描选中 → 03:47 有人发
/resolve→ 03:51/resolve推送成功 → 04:05 autofix 推送被fetch first拒绝。输的一方已经把 agent 运行花掉了。它的 job 死在
Push and report,因此没有写下任何 eval 标记 —— 这一轮就这么消失了,从 PR 外面看只是"没动静"。这也是我此前笼统归类为"推送竞态"的两例中真正的那一类;#7262 是合并竞态,而这一例是两个主人、没有锁。怎么做
GitHub 的 concurrency 组是仓库级而非 workflow 级,因此不同 workflow 的两个作业只要组名相同就会串行。两者现在统一使用
qwen-pr-head-write-<pr>,并保持cancel-in-progress: false,让输的一方排队而不是被丢弃。issue-autofix刻意不纳入:它推送的是自己为新 PR 创建的分支,不存在竞争者。前缀在两个文件里都是字面量,因为 job 级
concurrency读不到env上下文,两边无法共享常量。改由测试钉住两者相等 —— 只改一边会让竞态悄悄复活且无人报警。它买到了什么、没买到什么
review-address在作业开始时按分支名拉取 head(git fetch … refs/heads/${BRANCH}→git checkout -B),因此第二个运行读到的是赢家的结果而非陈旧的 base:它的工作可用,推送也能落地。但它仍可能花掉一次 agent 运行。address 阶段的复核只重验生命周期与授权(state、labels、author、base、head 分支),不重验冲突是否还在,所以输的一方会照常干活、然后发现没什么可做,而不是免费丢弃。让复核重验触发条件是自然的后续,刻意不放进本 PR。
评审验证
npx vitest run scripts/tests/qwen-resolve-workflow.test.js—— 26/26。新测试同时读取两个 workflow 文件,用正则提取各自作业的组前缀并断言相等,同时断言两边仍以 PR 号为 key、且都是排队而非取消。github.run_id作 key / 把resolve-pr改成cancel-in-progress: true)。js-yaml解析;37 + 17 个run:块全过bash -n;actionlint 1.7.12 对两者均 clean;新增行本身符合 prettier。review-address腿正在运行时对它发@qwen-code /resolve,该 resolve 作业应处于 queued,待那条腿结束后才启动。风险与范围
/resolve现在可能排在该 PR 的review-address腿之后,后者超时上限为 120 分钟。这正是预期语义 —— 替代方案是现在这种白跑一轮 —— 但它确实改变了延迟,且作业处于 pending 时 GitHub 不会给评论者任何提示。若实际体验不佳,上文的后续(让复核提前廉价丢弃)可缩短持锁时间,而非移除锁。/resolve会取消前一个 pending,这里属于"最新意图优先",是正确的。qwen-resolve-workflow、qwen-autofix-workflow、qwen-pr-review-workflow三个套件一起跑时,eligibility recheck与takeover-command toggle会稳定变红 —— 这是负载 flake。本机 A/B:未修改的main在同样的组合运行下同样红这两个,两次皆然,而单独跑均通过。并非本 PR 引入。关联 Issue
在盘点接管机群运行状况时发现 —— #7355 的第 6 轮工作连标记都没留下就消失了。属 autofix 可靠性主线:#7330、#7350、#7351、#7354、#7355、#7358、#7364。