Skip to content

Commit 91ea751

Browse files
roomoteedelauna
roomote
authored and
edelauna
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 8dd8ef3 commit 91ea751

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
@@ -45,33 +45,138 @@ permissions:
4545
checks: read
4646
statuses: write
4747

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

6880
jobs:
69-
reconcile:
70-
name: Zoo Code / reconcile PR review state
81+
resolve:
82+
name: Zoo Code / resolve PRs to reconcile
7183
if: >-
7284
github.event_name != 'issue_comment' ||
7385
(github.event.issue.pull_request && github.event.comment.user.login == 'coderabbitai[bot]')
7486
runs-on: ubuntu-latest
87+
outputs:
88+
pr_numbers: ${{ steps.resolve.outputs.pr_numbers }}
89+
steps:
90+
- name: Resolve target PR numbers
91+
id: resolve
92+
uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8
93+
with:
94+
retries: 3
95+
script: |
96+
const { owner, repo } = context.repo;
97+
98+
if (context.eventName === 'issue_comment' &&
99+
(!context.payload.issue?.pull_request ||
100+
context.payload.comment?.user?.login?.toLowerCase() !== 'coderabbitai[bot]')) {
101+
core.info('Ignoring untrusted issue comment event');
102+
core.setOutput('pr_numbers', '[]');
103+
return;
104+
}
105+
106+
// Resolve which PRs this event targets. Single-PR events carry the
107+
// number in the payload; sweep triggers (hourly schedule, pushes to
108+
// main) list every open PR; workflow_run events use the run's
109+
// associated PRs, with a fork-safe backfill by exact head owner,
110+
// branch, and SHA when GitHub left pull_requests empty.
111+
let prNumbers = [];
112+
if (context.payload.pull_request?.number) {
113+
prNumbers = [context.payload.pull_request.number];
114+
} else if (context.eventName === 'issue_comment') {
115+
prNumbers = [context.payload.issue.number];
116+
} else if (context.eventName === 'workflow_dispatch') {
117+
prNumbers = [Number(context.payload.inputs.pull_request_number)];
118+
} else if (context.payload.workflow_run?.pull_requests?.length > 0) {
119+
prNumbers = context.payload.workflow_run.pull_requests.map(pr => pr.number);
120+
} else if (context.eventName === 'workflow_run') {
121+
const run = context.payload.workflow_run;
122+
const headOwner = run.head_repository?.owner?.login;
123+
if (headOwner && run.head_branch && run.head_sha) {
124+
const candidates = await github.paginate(github.rest.pulls.list, {
125+
owner,
126+
repo,
127+
state: 'open',
128+
head: `${headOwner}:${run.head_branch}`,
129+
per_page: 100,
130+
});
131+
const baseRepository = `${owner}/${repo}`.toLowerCase();
132+
prNumbers = candidates
133+
.filter(pr =>
134+
pr.head?.sha === run.head_sha &&
135+
pr.base?.repo?.full_name?.toLowerCase() === baseRepository
136+
)
137+
.map(pr => pr.number);
138+
core.info(
139+
`Resolved workflow_run ${run.id ?? '(unknown)'} to ` +
140+
`${prNumbers.length} PR(s) by exact head owner, branch, and SHA`
141+
);
142+
}
143+
} else {
144+
const openPrs = await github.paginate(github.rest.pulls.list, {
145+
owner, repo, state: 'open', per_page: 100,
146+
});
147+
prNumbers = openPrs.map(pr => pr.number);
148+
}
149+
150+
core.info(
151+
`Reconciliation targets: ` +
152+
`${prNumbers.length > 0 ? prNumbers.join(', ') : '(none)'}`
153+
);
154+
core.setOutput('pr_numbers', JSON.stringify(prNumbers));
155+
156+
reconcile:
157+
name: Zoo Code / reconcile PR review state
158+
needs: resolve
159+
if: >-
160+
(github.event_name != 'issue_comment' ||
161+
(github.event.issue.pull_request && github.event.comment.user.login == 'coderabbitai[bot]')) &&
162+
needs.resolve.outputs.pr_numbers != '[]'
163+
strategy:
164+
fail-fast: false
165+
matrix:
166+
pr_number: ${{ fromJSON(needs.resolve.outputs.pr_numbers) }}
167+
# One mutating shard per PR, serialized per PR across every trigger path:
168+
# direct-event shards, sweep-resolved shards, and workflow_run-resolved
169+
# shards all share this group namespace, so their non-atomic writes can
170+
# never interleave. cancel-in-progress stays false — within one PR,
171+
# newest-pending-wins is exactly freshest-state-wins. The run_id fallback
172+
# only applies to the skipped empty-resolution case so empty runs never
173+
# contend.
174+
concurrency:
175+
group: label-pr-review-state-${{ matrix.pr_number || github.run_id }}
176+
cancel-in-progress: false
177+
runs-on: ubuntu-latest
178+
env:
179+
PR_NUMBER: ${{ matrix.pr_number }}
75180
steps:
76181
- name: Reconcile PR review state labels
77182
uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8
@@ -133,56 +238,15 @@ jobs:
133238
return;
134239
}
135240
136-
// When triggered by a single PR event, only reconcile that PR.
137-
// Main-branch pushes, the hourly schedule, and workflow_dispatch reconcile all open PRs.
138-
let prs;
139-
let eventPrNumbers = [];
140-
if (context.payload.pull_request?.number) {
141-
eventPrNumbers = [context.payload.pull_request.number];
142-
} else if (context.eventName === 'issue_comment') {
143-
eventPrNumbers = [context.payload.issue.number];
144-
} else if (context.eventName === 'workflow_dispatch') {
145-
eventPrNumbers = [Number(context.payload.inputs.pull_request_number)];
146-
} else if (context.payload.workflow_run?.pull_requests) {
147-
eventPrNumbers = context.payload.workflow_run.pull_requests.map(pr => pr.number);
148-
}
149-
if (context.eventName === 'workflow_run' && eventPrNumbers.length === 0) {
150-
const run = context.payload.workflow_run;
151-
const headOwner = run.head_repository?.owner?.login;
152-
if (headOwner && run.head_branch && run.head_sha) {
153-
const candidates = await github.paginate(github.rest.pulls.list, {
154-
owner,
155-
repo,
156-
state: 'open',
157-
head: `${headOwner}:${run.head_branch}`,
158-
per_page: 100,
159-
});
160-
const baseRepository = `${owner}/${repo}`.toLowerCase();
161-
eventPrNumbers = candidates
162-
.filter(pr =>
163-
pr.head?.sha === run.head_sha &&
164-
pr.base?.repo?.full_name?.toLowerCase() === baseRepository
165-
)
166-
.map(pr => pr.number);
167-
core.info(
168-
`Resolved workflow_run ${run.id ?? '(unknown)'} to ` +
169-
`${eventPrNumbers.length} PR(s) by exact head owner, branch, and SHA`
170-
);
171-
}
172-
}
173-
if (eventPrNumbers.length > 0) {
174-
prs = await Promise.all(eventPrNumbers.map(async pull_number => {
175-
const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number });
176-
return pr;
177-
}));
178-
prs = prs.filter(pr => pr.state === 'open');
179-
} else if (context.eventName === 'workflow_run') {
180-
prs = [];
181-
} else {
182-
prs = await github.paginate(github.rest.pulls.list, {
183-
owner, repo, state: 'open', per_page: 100,
184-
});
185-
}
241+
// The resolve job fans out one shard per target PR; this job
242+
// reconciles exactly this matrix shard's PR. The single-PR GET
243+
// endpoint returns mergeable fields that list endpoints omit.
244+
const { data: shardPr } = await github.rest.pulls.get({
245+
owner,
246+
repo,
247+
pull_number: Number(process.env.PR_NUMBER),
248+
});
249+
const prs = shardPr.state === 'open' ? [shardPr] : [];
186250
187251
// pull_request_review runs originating from forks can still receive a read-only
188252
// token. pull_request_target, schedule, and workflow_dispatch runs execute in the
@@ -203,7 +267,12 @@ jobs:
203267
await github.rest.issues.getLabel({ owner, repo, name: label.name });
204268
} catch (error) {
205269
if (error.status !== 404) throw error;
206-
await github.rest.issues.createLabel({ owner, repo, ...label });
270+
try {
271+
await github.rest.issues.createLabel({ owner, repo, ...label });
272+
} catch (createError) {
273+
// Concurrent shards for other PRs can win the creation race.
274+
if (createError.status !== 422) throw createError;
275+
}
207276
}
208277
}
209278
}
@@ -564,8 +633,13 @@ jobs:
564633
for (const pr of prs) {
565634
let failurePhase = 'ci-pending';
566635
try {
636+
// The matrix job reports one check per shard named
637+
// "<reconciliationCheckName> (<pr_number>)", so catch both the
638+
// bare name and any matrix-suffixed variant.
567639
const selfReferentialRequirements = (requiredChecks ?? []).filter(check =>
568-
check.context === reviewGateName || check.context === reconciliationCheckName
640+
check.context === reviewGateName ||
641+
check.context === reconciliationCheckName ||
642+
check.context.startsWith(`${reconciliationCheckName} (`)
569643
);
570644
if (selfReferentialRequirements.length > 0) {
571645
failurePhase = 'configuration-error';
@@ -584,15 +658,12 @@ jobs:
584658
585659
let existingGuide = await findReviewGuide(pr);
586660
587-
// `mergeable`/`mergeable_state` are only returned by the single-PR GET
588-
// endpoint, and are computed asynchronously by GitHub — a PR fetched via
589-
// pulls.list (schedule/workflow_dispatch runs) never has them, and even a
590-
// single-PR fetch can return `null`/"unknown" if the merge check hasn't
591-
// finished yet. Re-fetch the single PR to get a fresh value, and treat
592-
// "unknown" as not-yet-computed rather than as conflicting.
593-
const prDetail = eventPrNumbers.length > 0
594-
? pr
595-
: (await github.rest.pulls.get({ owner, repo, pull_number: pr.number })).data;
661+
// `mergeable`/`mergeable_state` are only returned by the single-PR
662+
// GET endpoint and are computed asynchronously by GitHub — this
663+
// shard's own pulls.get above provides them, but even that can
664+
// return `null`/"unknown" while the merge check is still running.
665+
// Treat "unknown" as not-yet-computed rather than as conflicting.
666+
const prDetail = pr;
596667
597668
if (prDetail.mergeable === false && prDetail.mergeable_state === 'dirty') {
598669
core.info(`PR #${pr.number}: has merge conflicts — labeling has-conflicts`);
@@ -772,12 +843,14 @@ jobs:
772843
}
773844
}
774845
775-
// Clear blockers the author explicitly re-requested. Only a
776-
// review_requested timeline event whose actor is the PR author and
777-
// whose requested reviewer is the blocking maintainer clears that
778-
// maintainer's blocker. Team requests carry no requested_reviewer
779-
// and never clear an individual blocker; review_request_removed
780-
// events only trigger reconciliation and are not clearing evidence.
846+
// Clear blockers that were explicitly re-requested. A
847+
// review_requested timeline event clears that maintainer's
848+
// blocker when the actor is the PR author re-requesting the
849+
// blocking maintainer, or the blocking maintainer re-requesting
850+
// themselves (signaling intent to re-review). Team requests
851+
// carry no requested_reviewer and never clear an individual
852+
// blocker; review_request_removed events only trigger
853+
// reconciliation and are not clearing evidence.
781854
// If the timeline cannot be reconstructed, fail closed: keep every
782855
// blocker so awaiting-author is preserved.
783856
if (maintainerBlockers.size > 0) {

0 commit comments

Comments
 (0)