Skip to content

Commit 74a5d72

Browse files
roomoteroomote
committed
fix(ci): serialize PR review-state reconciliation per PR
Split the workflow into a resolve job (determines target PRs from the event payload, or every open PR for sweep triggers) and a fan-out reconcile job that runs one matrix shard per PR. The shards carry a job-level concurrency group keyed by PR number that is identical for direct-event, sweep-resolved, and workflow_run-resolved shards, so their non-atomic label, status, and comment writes serialize per PR instead of racing; previously a sweep could overwrite fresher state written by a direct per-PR run. Workflow-level keys no longer starve multi-PR events: workflow_run runs key by the unique triggering run id instead of the first associated PR, single-PR events keep per-PR keys, sweeps share one group, and anything unresolved falls back to a unique per-run group. cancel-in-progress stays false at both levels, so within one PR newest-pending-wins remains freshest-state-wins. Also tolerate 422 label-creation races between concurrent shards and match matrix-suffixed check names in the self-referential required-check guard. Fixes #1707 Co-authored-by: Roomote <[email protected]>
1 parent d890af6 commit 74a5d72

2 files changed

Lines changed: 619 additions & 227 deletions

File tree

‎.github/workflows/label-pr-review-state.yml‎

Lines changed: 151 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -34,33 +34,138 @@ permissions:
3434
checks: read
3535
statuses: write
3636

37-
# One concurrency group per PR so bursts of activity on unrelated PRs cannot
38-
# starve an individual PR's reconciliation (issue #1707). GitHub keeps only the
39-
# newest pending run per group; keyed per PR that becomes freshest-state-wins
40-
# within a PR. Issue/PR numbers share one GitHub sequence, so issue-comment runs
41-
# can never collide with a different PR's group. Sweep triggers (hourly
42-
# schedule, pushes to main) share a single group, and events that cannot be
43-
# tied to a PR (fork workflow_run without backfilled pull_requests) fall back
44-
# to a unique per-run group so they are never superseded.
37+
# Concurrency is keyed at two levels (issue #1707):
38+
#
39+
# Workflow level (below): keeps bursts of activity on unrelated PRs from
40+
# starving whole runs. GitHub keeps only the newest pending run per group even
41+
# with cancel-in-progress: false, so keying per PR makes that freshest-state-
42+
# wins within one PR. Issue/PR numbers share one GitHub sequence, so
43+
# issue-comment runs can never collide with a different PR's group.
44+
# workflow_run events key by the triggering run's id instead of a PR number: a
45+
# single run can reconcile several associated PRs (and fork runs backfill
46+
# none), so a PR-number key would let an unrelated event for the first PR
47+
# supersede reconciliation of the rest. Sweep triggers (hourly schedule,
48+
# pushes to main) share one group, and anything unresolved falls back to a
49+
# unique per-run group.
50+
#
51+
# Job level (on the reconcile job): every mutating shard keys per PR in one
52+
# shared group namespace, so direct-event shards, sweep-resolved shards, and
53+
# workflow_run-resolved shards for the same PR serialize against each other
54+
# instead of racing non-atomic label, status, and comment writes. Only the
55+
# reconcile job mutates PR state, so no workflow-level key can bypass that
56+
# per-PR serialization.
4557
concurrency:
4658
group: >-
4759
label-pr-review-state-${{
4860
github.event.pull_request.number ||
4961
github.event.issue.number ||
50-
github.event.workflow_run.pull_requests[0].number ||
5162
github.event.inputs.pull_request_number ||
63+
github.event.workflow_run.id ||
5264
((github.event_name == 'schedule' || github.event_name == 'push') && 'sweep') ||
5365
github.run_id
5466
}}
5567
cancel-in-progress: false
5668

5769
jobs:
58-
reconcile:
59-
name: Zoo Code / reconcile PR review state
70+
resolve:
71+
name: Zoo Code / resolve PRs to reconcile
6072
if: >-
6173
github.event_name != 'issue_comment' ||
6274
(github.event.issue.pull_request && github.event.comment.user.login == 'coderabbitai[bot]')
6375
runs-on: ubuntu-latest
76+
outputs:
77+
pr_numbers: ${{ steps.resolve.outputs.pr_numbers }}
78+
steps:
79+
- name: Resolve target PR numbers
80+
id: resolve
81+
uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8
82+
with:
83+
retries: 3
84+
script: |
85+
const { owner, repo } = context.repo;
86+
87+
if (context.eventName === 'issue_comment' &&
88+
(!context.payload.issue?.pull_request ||
89+
context.payload.comment?.user?.login?.toLowerCase() !== 'coderabbitai[bot]')) {
90+
core.info('Ignoring untrusted issue comment event');
91+
core.setOutput('pr_numbers', '[]');
92+
return;
93+
}
94+
95+
// Resolve which PRs this event targets. Single-PR events carry the
96+
// number in the payload; sweep triggers (hourly schedule, pushes to
97+
// main) list every open PR; workflow_run events use the run's
98+
// associated PRs, with a fork-safe backfill by exact head owner,
99+
// branch, and SHA when GitHub left pull_requests empty.
100+
let prNumbers = [];
101+
if (context.payload.pull_request?.number) {
102+
prNumbers = [context.payload.pull_request.number];
103+
} else if (context.eventName === 'issue_comment') {
104+
prNumbers = [context.payload.issue.number];
105+
} else if (context.eventName === 'workflow_dispatch') {
106+
prNumbers = [Number(context.payload.inputs.pull_request_number)];
107+
} else if (context.payload.workflow_run?.pull_requests?.length > 0) {
108+
prNumbers = context.payload.workflow_run.pull_requests.map(pr => pr.number);
109+
} else if (context.eventName === 'workflow_run') {
110+
const run = context.payload.workflow_run;
111+
const headOwner = run.head_repository?.owner?.login;
112+
if (headOwner && run.head_branch && run.head_sha) {
113+
const candidates = await github.paginate(github.rest.pulls.list, {
114+
owner,
115+
repo,
116+
state: 'open',
117+
head: `${headOwner}:${run.head_branch}`,
118+
per_page: 100,
119+
});
120+
const baseRepository = `${owner}/${repo}`.toLowerCase();
121+
prNumbers = candidates
122+
.filter(pr =>
123+
pr.head?.sha === run.head_sha &&
124+
pr.base?.repo?.full_name?.toLowerCase() === baseRepository
125+
)
126+
.map(pr => pr.number);
127+
core.info(
128+
`Resolved workflow_run ${run.id ?? '(unknown)'} to ` +
129+
`${prNumbers.length} PR(s) by exact head owner, branch, and SHA`
130+
);
131+
}
132+
} else {
133+
const openPrs = await github.paginate(github.rest.pulls.list, {
134+
owner, repo, state: 'open', per_page: 100,
135+
});
136+
prNumbers = openPrs.map(pr => pr.number);
137+
}
138+
139+
core.info(
140+
`Reconciliation targets: ` +
141+
`${prNumbers.length > 0 ? prNumbers.join(', ') : '(none)'}`
142+
);
143+
core.setOutput('pr_numbers', JSON.stringify(prNumbers));
144+
145+
reconcile:
146+
name: Zoo Code / reconcile PR review state
147+
needs: resolve
148+
if: >-
149+
(github.event_name != 'issue_comment' ||
150+
(github.event.issue.pull_request && github.event.comment.user.login == 'coderabbitai[bot]')) &&
151+
needs.resolve.outputs.pr_numbers != '[]'
152+
strategy:
153+
fail-fast: false
154+
matrix:
155+
pr_number: ${{ fromJSON(needs.resolve.outputs.pr_numbers) }}
156+
# One mutating shard per PR, serialized per PR across every trigger path:
157+
# direct-event shards, sweep-resolved shards, and workflow_run-resolved
158+
# shards all share this group namespace, so their non-atomic writes can
159+
# never interleave. cancel-in-progress stays false — within one PR,
160+
# newest-pending-wins is exactly freshest-state-wins. The run_id fallback
161+
# only applies to the skipped empty-resolution case so empty runs never
162+
# contend.
163+
concurrency:
164+
group: label-pr-review-state-${{ matrix.pr_number || github.run_id }}
165+
cancel-in-progress: false
166+
runs-on: ubuntu-latest
167+
env:
168+
PR_NUMBER: ${{ matrix.pr_number }}
64169
steps:
65170
- name: Reconcile PR review state labels
66171
uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8
@@ -114,56 +219,15 @@ jobs:
114219
return;
115220
}
116221
117-
// When triggered by a single PR event, only reconcile that PR.
118-
// Main-branch pushes, the hourly schedule, and workflow_dispatch reconcile all open PRs.
119-
let prs;
120-
let eventPrNumbers = [];
121-
if (context.payload.pull_request?.number) {
122-
eventPrNumbers = [context.payload.pull_request.number];
123-
} else if (context.eventName === 'issue_comment') {
124-
eventPrNumbers = [context.payload.issue.number];
125-
} else if (context.eventName === 'workflow_dispatch') {
126-
eventPrNumbers = [Number(context.payload.inputs.pull_request_number)];
127-
} else if (context.payload.workflow_run?.pull_requests) {
128-
eventPrNumbers = context.payload.workflow_run.pull_requests.map(pr => pr.number);
129-
}
130-
if (context.eventName === 'workflow_run' && eventPrNumbers.length === 0) {
131-
const run = context.payload.workflow_run;
132-
const headOwner = run.head_repository?.owner?.login;
133-
if (headOwner && run.head_branch && run.head_sha) {
134-
const candidates = await github.paginate(github.rest.pulls.list, {
135-
owner,
136-
repo,
137-
state: 'open',
138-
head: `${headOwner}:${run.head_branch}`,
139-
per_page: 100,
140-
});
141-
const baseRepository = `${owner}/${repo}`.toLowerCase();
142-
eventPrNumbers = candidates
143-
.filter(pr =>
144-
pr.head?.sha === run.head_sha &&
145-
pr.base?.repo?.full_name?.toLowerCase() === baseRepository
146-
)
147-
.map(pr => pr.number);
148-
core.info(
149-
`Resolved workflow_run ${run.id ?? '(unknown)'} to ` +
150-
`${eventPrNumbers.length} PR(s) by exact head owner, branch, and SHA`
151-
);
152-
}
153-
}
154-
if (eventPrNumbers.length > 0) {
155-
prs = await Promise.all(eventPrNumbers.map(async pull_number => {
156-
const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number });
157-
return pr;
158-
}));
159-
prs = prs.filter(pr => pr.state === 'open');
160-
} else if (context.eventName === 'workflow_run') {
161-
prs = [];
162-
} else {
163-
prs = await github.paginate(github.rest.pulls.list, {
164-
owner, repo, state: 'open', per_page: 100,
165-
});
166-
}
222+
// The resolve job fans out one shard per target PR; this job
223+
// reconciles exactly this matrix shard's PR. The single-PR GET
224+
// endpoint returns mergeable fields that list endpoints omit.
225+
const { data: shardPr } = await github.rest.pulls.get({
226+
owner,
227+
repo,
228+
pull_number: Number(process.env.PR_NUMBER),
229+
});
230+
const prs = shardPr.state === 'open' ? [shardPr] : [];
167231
168232
// pull_request_review runs originating from forks can still receive a read-only
169233
// token. pull_request_target, schedule, and workflow_dispatch runs execute in the
@@ -184,7 +248,12 @@ jobs:
184248
await github.rest.issues.getLabel({ owner, repo, name: label.name });
185249
} catch (error) {
186250
if (error.status !== 404) throw error;
187-
await github.rest.issues.createLabel({ owner, repo, ...label });
251+
try {
252+
await github.rest.issues.createLabel({ owner, repo, ...label });
253+
} catch (createError) {
254+
// Concurrent shards for other PRs can win the creation race.
255+
if (createError.status !== 422) throw createError;
256+
}
188257
}
189258
}
190259
}
@@ -500,8 +569,13 @@ jobs:
500569
for (const pr of prs) {
501570
let failurePhase = 'ci-pending';
502571
try {
572+
// The matrix job reports one check per shard named
573+
// "<reconciliationCheckName> (<pr_number>)", so catch both the
574+
// bare name and any matrix-suffixed variant.
503575
const selfReferentialRequirements = (requiredChecks ?? []).filter(check =>
504-
check.context === reviewGateName || check.context === reconciliationCheckName
576+
check.context === reviewGateName ||
577+
check.context === reconciliationCheckName ||
578+
check.context.startsWith(`${reconciliationCheckName} (`)
505579
);
506580
if (selfReferentialRequirements.length > 0) {
507581
failurePhase = 'configuration-error';
@@ -519,15 +593,12 @@ jobs:
519593
520594
let existingGuide = await findReviewGuide(pr);
521595
522-
// `mergeable`/`mergeable_state` are only returned by the single-PR GET
523-
// endpoint, and are computed asynchronously by GitHub — a PR fetched via
524-
// pulls.list (schedule/workflow_dispatch runs) never has them, and even a
525-
// single-PR fetch can return `null`/"unknown" if the merge check hasn't
526-
// finished yet. Re-fetch the single PR to get a fresh value, and treat
527-
// "unknown" as not-yet-computed rather than as conflicting.
528-
const prDetail = eventPrNumbers.length > 0
529-
? pr
530-
: (await github.rest.pulls.get({ owner, repo, pull_number: pr.number })).data;
596+
// `mergeable`/`mergeable_state` are only returned by the single-PR
597+
// GET endpoint and are computed asynchronously by GitHub — this
598+
// shard's own pulls.get above provides them, but even that can
599+
// return `null`/"unknown" while the merge check is still running.
600+
// Treat "unknown" as not-yet-computed rather than as conflicting.
601+
const prDetail = pr;
531602
532603
if (prDetail.mergeable === false && prDetail.mergeable_state === 'dirty') {
533604
core.info(`PR #${pr.number}: has merge conflicts — labeling has-conflicts`);
@@ -702,12 +773,14 @@ jobs:
702773
}
703774
}
704775
705-
// Clear blockers the author explicitly re-requested. Only a
706-
// review_requested timeline event whose actor is the PR author and
707-
// whose requested reviewer is the blocking maintainer clears that
708-
// maintainer's blocker. Team requests carry no requested_reviewer
709-
// and never clear an individual blocker; review_request_removed
710-
// events only trigger reconciliation and are not clearing evidence.
776+
// Clear blockers that were explicitly re-requested. A
777+
// review_requested timeline event clears that maintainer's
778+
// blocker when the actor is the PR author re-requesting the
779+
// blocking maintainer, or the blocking maintainer re-requesting
780+
// themselves (signaling intent to re-review). Team requests
781+
// carry no requested_reviewer and never clear an individual
782+
// blocker; review_request_removed events only trigger
783+
// reconciliation and are not clearing evidence.
711784
// If the timeline cannot be reconstructed, fail closed: keep every
712785
// blocker so awaiting-author is preserved.
713786
if (maintainerBlockers.size > 0) {

0 commit comments

Comments
 (0)