Repository navigation
ci(autofix): fan out review targets and stop route-scan starvation #7127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
28c3e23
6607548
8d391ca
13676f6
d26e2e0
1373512
a35245e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,9 @@ env: | |
| # Hard cap on automated review-address rounds per PR. After this the bot stops | ||
| # and leaves the PR for a human. | ||
| MAX_ROUNDS: '5' | ||
| # Upper bound on review targets emitted per scan (fan-out defense-in-depth; | ||
| # excess is logged and deferred to the next scan). | ||
| MAX_TARGETS_PER_SCAN: '10' | ||
| # Do not claim more issues when too many existing autofix PRs are still open. | ||
| MAX_OPEN_AUTOFIX_PRS: '5' | ||
|
|
||
|
|
@@ -90,8 +93,19 @@ jobs: | |
| runs-on: 'ubuntu-latest' | ||
| timeout-minutes: 5 | ||
| concurrency: | ||
| group: 'qwen-autofix-route' | ||
| cancel-in-progress: true | ||
| # Concurrency is keyed by TARGET, not shared and not fully unique: | ||
| # • cron ticks share one group (a newer tick supersedes a queued one) | ||
| # • review events coalesce PER PR (two reviews on the same PR seconds | ||
| # apart route once — the old shared group's one useful side effect, | ||
| # kept, without letting events on OTHER PRs cancel this one) | ||
| # • issue events coalesce PER issue | ||
| # • dispatches are unique per run and are never cancelled | ||
| # The old single shared cancel-in-progress group let ANY newer event kill | ||
| # pending full scans while route jobs sat queued behind runner backlog — | ||
| # observed as hours of scan starvation during review-event storms. | ||
| group: "${{ github.event_name == 'schedule' && 'qwen-autofix-route-cron' || (github.event_name == 'pull_request_review' && format('qwen-autofix-route-pr-{0}', github.event.pull_request.number)) || (github.event_name == 'issues' && format('qwen-autofix-route-issue-{0}', github.event.issue.number)) || format('qwen-autofix-route-{0}', github.run_id) }}" | ||
| cancel-in-progress: |- | ||
| ${{ github.event_name != 'workflow_dispatch' }} | ||
| permissions: | ||
| contents: 'read' | ||
| outputs: | ||
|
|
@@ -1004,8 +1018,33 @@ jobs: | |
| # check and double-process the feedback). | ||
| PENDING_STALE_MIN=240 | ||
| PENDING_CUTOFF="$(date -u -d "${PENDING_STALE_MIN} minutes ago" +%Y-%m-%dT%H:%M:%SZ)" | ||
|
|
||
| # PRs whose review-address is already RUNNING OR QUEUED in any live | ||
| # autofix run must not be re-targeted. Schedule/dispatch runs execute | ||
| # against main's SHA, so their matrix jobs never appear in the PR's | ||
| # statusCheckRollup — and a fanned-out matrix holds queued jobs well | ||
| # past a 10-minute tick, so without this the next scan re-emits the | ||
| # same PRs and the per-PR address groups accumulate duplicates that | ||
| # later replay stale watermarks. One runs-list plus one jobs-view per | ||
| # live run is cheap (there are at most a few). | ||
| BUSY_PRS=' ' | ||
| while IFS= read -r LIVE_RUN; do | ||
| [[ -z "${LIVE_RUN}" ]] && continue | ||
| while IFS= read -r BUSY; do | ||
| [[ -n "${BUSY}" ]] && BUSY_PRS="${BUSY_PRS}${BUSY} " | ||
| done < <(gh run view "${LIVE_RUN}" --repo "${REPO}" --json jobs \ | ||
| --jq '.jobs[] | select(.status != "completed") | .name | capture("^review-address \\((?<pr>[0-9]+),") | .pr' 2> /dev/null) | ||
| done < <(gh run list --repo "${REPO}" --workflow qwen-autofix.yml \ | ||
|
wenshao marked this conversation as resolved.
Resolved
|
||
| --limit 15 --json databaseId,status \ | ||
| --jq '.[] | select(.status != "completed") | .databaseId' 2> /dev/null) | ||
|
wenshao marked this conversation as resolved.
Resolved
|
||
| [[ "${BUSY_PRS}" != ' ' ]] && echo "🚧 address in flight/queued for PR(s):${BUSY_PRS}" | ||
|
|
||
| TARGETS='[]' | ||
| for PR in ${CANDIDATES}; do | ||
| if [[ "${BUSY_PRS}" == *" ${PR} "* ]]; then | ||
| echo "⏳ #${PR}: review-address already in flight or queued — skipping" | ||
| continue | ||
| fi | ||
| # One PR fetch for the branch name, check rollup, and creation time (the | ||
| # watermark floor below) — avoids extra round-trips per candidate PR. | ||
| PR_META="$(gh pr view "${PR}" --repo "${REPO}" \ | ||
|
|
@@ -1152,7 +1191,21 @@ jobs: | |
| --arg round "${ROUND}" --arg wm "${EFF_WM}" \ | ||
| '. + [{pr: $pr, branch: $branch, issue: $issue, round: $round, watermark: $wm}]' \ | ||
| <<< "${TARGETS}")" | ||
| break # one PR per scheduled scan | ||
| # Fan out: emit EVERY eligible PR up to the per-scan budget. The | ||
| # address matrix bounds simultaneity (max-parallel) and the per-PR | ||
| # concurrency groups plus the busy-PR skip above prevent duplicate | ||
| # same-PR runs, so one scan drains the whole backlog instead of | ||
| # serving a single newest-first target per tick (which starved | ||
| # older PRs for hours when cron ticks were sparse). The budget | ||
| # break bounds this loop's RUNTIME and API usage too — each | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Follow-up only: this target-count guard does not bound candidate-loop work when fewer than ten PRs are actionable. Nine actionable plus ninety-one quiet candidates still perform all serial reads, so the claimed runtime/API bound is absent. Add a separate fair candidate-inspection budget or batching; otherwise narrow the comment to the matrix-size guarantee. — Codex GPT-5 via Qwen Code /review |
||
| # candidate costs several serial API reads, so scanning past a | ||
| # full budget would spend hundreds of calls for nothing. Never a | ||
| # silent cap: the deferral is logged and the next scan picks up | ||
| # the remainder (their signals persist). | ||
| if [[ "$(jq 'length' <<< "${TARGETS}")" -ge "${MAX_TARGETS_PER_SCAN}" ]]; then | ||
| echo "⚠️ target budget (${MAX_TARGETS_PER_SCAN}) reached; deferring the remaining candidates to the next scan" | ||
| break | ||
| fi | ||
| done | ||
|
|
||
| COUNT="$(jq 'length' <<< "${TARGETS}")" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -200,7 +200,7 @@ describe('qwen-autofix workflow', () => { | |
| expect(workflow).toContain('.[0:10] | map(. + {autofixTier: 1})'); | ||
| }); | ||
|
|
||
| it('runs scheduled autofix as a 10-minute single-target worker', () => { | ||
| it('runs scheduled autofix as a 10-minute multi-target fan-out worker', () => { | ||
| expect(workflow).toContain("cron: '*/10 * * * *'"); | ||
| expect(workflow).not.toContain("cron: '0 0,12 * * *'"); | ||
| expect(workflow).not.toContain("cron: '0 4,8,16,20 * * *'"); | ||
|
|
@@ -215,7 +215,30 @@ describe('qwen-autofix workflow', () => { | |
| expect(reviewScanJob).toContain('isCrossRepository'); | ||
| expect(reviewScanJob).toContain('not an open in-repo main-targeting PR'); | ||
| expect(reviewScanJob).toContain('.isCrossRepository != true'); | ||
| expect(reviewScanJob).toContain('break # one PR per scheduled scan'); | ||
| // Fan-out: one scan emits EVERY eligible PR (no single-target break). The | ||
| // address matrix's max-parallel bounds simultaneity and per-PR concurrency | ||
| // groups prevent duplicate same-PR runs; a single-target break starved | ||
| // older PRs for hours whenever cron ticks were sparse. | ||
| expect(reviewScanJob).not.toContain('break # one PR per scheduled scan'); | ||
| expect(reviewScanJob).toContain('Fan out: emit EVERY eligible PR'); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Follow-up only: this assertion pins explanatory prose rather than behavior, so a harmless comment rewording fails the suite while neighboring checks already cover the fan-out controls. Remove this literal-comment assertion or replace it with an executable loop behavior check. — Codex GPT-5 via Qwen Code /review |
||
| expect(workflow).toContain("max-parallel: 3"); | ||
| // Pathological-backlog bound: the budget BREAKS the candidate loop (so it | ||
| // bounds runtime and API usage, not just matrix size), the deferral is | ||
| // LOGGED, and the next scan picks up the remainder. | ||
| expect(workflow).toContain("MAX_TARGETS_PER_SCAN: '10'"); | ||
| expect(reviewScanJob).toContain( | ||
| 'deferring the remaining candidates to the next scan', | ||
| ); | ||
| expect(reviewScanJob).toMatch(/target budget \(\$\{MAX_TARGETS_PER_SCAN\}\) reached[\s\S]{0,120}break/); | ||
| // Fanned-out matrices hold QUEUED jobs past a tick and schedule/dispatch | ||
| // runs never appear in the PR's checks — the scan must skip PRs whose | ||
| // review-address is already running or queued in any live autofix run. | ||
| expect(reviewScanJob).toContain( | ||
| 'review-address already in flight or queued — skipping', | ||
| ); | ||
| expect(reviewScanJob).toContain( | ||
| 'capture("^review-address \\\\((?<pr>[0-9]+),")', | ||
| ); | ||
| expect(reviewScanJob).toContain('statusCheckRollup'); | ||
| expect(reviewScanJob).toContain('HAS_PENDING_CHECKS'); | ||
| expect(reviewScanJob).toContain('N_FAILED_CHECKS'); | ||
|
|
@@ -303,8 +326,28 @@ describe('qwen-autofix workflow', () => { | |
| "ASSIGNEE_LOGIN: '${{ github.event.assignee.login }}'", | ||
| ); | ||
| expect(workflow).toContain("permissions:\n contents: 'read'"); | ||
| expect(routeJob).toContain("group: 'qwen-autofix-route'"); | ||
| expect(routeJob).toContain('cancel-in-progress: true'); | ||
| // Route concurrency: cron ticks share one group and supersede each other, | ||
| // but dispatches and review/issue events get unique per-run groups — a | ||
| // shared cancel-in-progress group let any newer event kill pending full | ||
| // scans while route jobs sat queued behind runner backlog. | ||
| // Per-TARGET keys: cron ticks coalesce with each other; review events | ||
| // coalesce per PR (near-simultaneous reviews on one PR route once, without | ||
| // events on OTHER PRs cancelling this one); issue events per issue; | ||
| // dispatches unique and never cancelled. | ||
| expect(routeJob).toContain("'qwen-autofix-route-cron'"); | ||
| expect(routeJob).toContain( | ||
| "format('qwen-autofix-route-pr-{0}', github.event.pull_request.number)", | ||
| ); | ||
| expect(routeJob).toContain( | ||
| "format('qwen-autofix-route-issue-{0}', github.event.issue.number)", | ||
| ); | ||
| expect(routeJob).toContain( | ||
| "format('qwen-autofix-route-{0}', github.run_id)", | ||
| ); | ||
| expect(routeJob).toContain( | ||
| "cancel-in-progress: |-\n ${{ github.event_name != 'workflow_dispatch' }}", | ||
| ); | ||
| expect(routeJob).not.toContain("group: 'qwen-autofix-route'"); | ||
| expect(workflow).toContain( | ||
| 'gh api "repos/${REPO}/collaborators/${SENDER_LOGIN}/permission"', | ||
| ); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Critical] This concurrency group is entered before the reviewer trust check. On a public PR, an untrusted submitted review can cancel or replace a trusted per-PR route; the replacement then fails authorization in
Decide phases, so the legitimate real-time scan is lost until an independent trigger recovers it. Authenticate in a prerequisite job before entering this target group, or ensure untrusted review payloads use an isolated run-specific group while retaining the live permission check.— Codex GPT-5 via Qwen Code /review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in follow-up #7163: the per-PR group is granted only when the review payload already looks trusted (
OWNER/MEMBER/COLLABORATORassociation, or the review bot); anything else gets a run-unique group — cancels nothing, still fully authorized insideDecide phases. Will resolve once #7163 merges.