Skip to content

Commit a488f1e

Browse files
roomoteroomote
committed
fix(ci): namespace reconcile shard concurrency groups
Workflow-level and job-level concurrency groups share one repo-wide namespace, so for direct single-PR triggers the reconcile shard's job-level group was byte-identical to the workflow-level group its own run already held, and GitHub failed the shard at startup with no logs (run 35548066989, job 106177532582). Give shard groups their own label-pr-review-state-reconcile- namespace, which also covers the theoretical numeric overlap between a workflow-level workflow_run.id key and a shard's PR-number key. Per-PR serialization across direct, sweep-resolved, and workflow_run-resolved shards is unchanged. Adds a regression test asserting workflow-level and job-level groups are disjoint for every trigger path. Fixes #1707 Co-authored-by: Roomote <[email protected]>
1 parent c4136e9 commit a488f1e

2 files changed

Lines changed: 59 additions & 15 deletions

File tree

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,9 +159,14 @@ jobs:
159159
# never interleave. cancel-in-progress stays false — within one PR,
160160
# newest-pending-wins is exactly freshest-state-wins. The run_id fallback
161161
# only applies to the skipped empty-resolution case so empty runs never
162-
# contend.
162+
# contend. The `-reconcile-` infix keeps job-level groups in their own
163+
# namespace: concurrency groups are one repo-wide namespace across
164+
# workflow and job levels, so a shard group equal to the workflow-level
165+
# group its own run holds (e.g. label-pr-review-state-<pr> for direct
166+
# triggers, or a workflow_run.id that numerically matches a PR number)
167+
# fails the job at startup.
163168
concurrency:
164-
group: label-pr-review-state-${{ matrix.pr_number || github.run_id }}
169+
group: label-pr-review-state-reconcile-${{ matrix.pr_number || github.run_id }}
165170
cancel-in-progress: false
166171
runs-on: ubuntu-latest
167172
env:

‎src/services/__tests__/pr-review-state-workflow.test.ts‎

Lines changed: 52 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2676,10 +2676,47 @@ describe("PR review-state workflow concurrency groups (#1707)", () => {
26762676
]
26772677

26782678
for (const context of triggerContexts) {
2679-
expect(shardGroupFor(context, { pr_number: 1664 })).toBe("label-pr-review-state-1664")
2679+
expect(shardGroupFor(context, { pr_number: 1664 })).toBe("label-pr-review-state-reconcile-1664")
2680+
}
2681+
expect(shardGroupFor(triggerContexts[0], { pr_number: 1707 })).toBe("label-pr-review-state-reconcile-1707")
2682+
expect(shardGroupFor(triggerContexts[0], { pr_number: 1707 })).not.toBe("label-pr-review-state-reconcile-1664")
2683+
})
2684+
2685+
it("keeps workflow-level and job-level groups disjoint for every trigger path", () => {
2686+
// Live regression (run 35548066989): for direct single-PR triggers the
2687+
// shard group equaled the workflow-level group its own run held, and
2688+
// GitHub failed the job at startup with no logs — concurrency groups are
2689+
// one repo-wide namespace across workflow and job levels. The same
2690+
// collision would apply to a workflow_run.id that numerically matches a
2691+
// PR number. The `-reconcile-` infix makes the two levels disjoint.
2692+
const contexts = [
2693+
contextFor({ event_name: "pull_request_target", event: { pull_request: { number: 1664 } } }),
2694+
contextFor({ event_name: "pull_request_review", event: { pull_request: { number: 1664 } } }),
2695+
contextFor({ event_name: "issue_comment", event: { issue: { number: 1664 } } }),
2696+
contextFor({ event_name: "workflow_dispatch", event: { inputs: { pull_request_number: 1664 } } }),
2697+
// workflow_run.id numerically equal to the PR number: the exact
2698+
// overlap the distinct namespace also has to cover.
2699+
contextFor({
2700+
event_name: "workflow_run",
2701+
event: { workflow_run: { id: 1664, pull_requests: [{ number: 1664 }] } },
2702+
}),
2703+
contextFor({
2704+
event_name: "workflow_run",
2705+
event: { workflow_run: { id: 999, pull_requests: [{ number: 1664 }, { number: 1707 }] } },
2706+
}),
2707+
contextFor({ event_name: "schedule" }),
2708+
contextFor({ event_name: "push" }),
2709+
]
2710+
2711+
for (const context of contexts) {
2712+
const workflowGroup = workflowGroupFor(context)
2713+
// The disjointness is structural: workflow-level groups never carry
2714+
// the reconcile infix that every shard group carries.
2715+
expect(workflowGroup).not.toContain("-reconcile-")
2716+
for (const prNumber of [1664, 1707, 1437, 999]) {
2717+
expect(shardGroupFor(context, { pr_number: prNumber })).not.toBe(workflowGroup)
2718+
}
26802719
}
2681-
expect(shardGroupFor(triggerContexts[0], { pr_number: 1707 })).toBe("label-pr-review-state-1707")
2682-
expect(shardGroupFor(triggerContexts[0], { pr_number: 1707 })).not.toBe("label-pr-review-state-1664")
26832720
})
26842721

26852722
it("keeps the empty-resolution shard fallback unique per run", () => {
@@ -2696,10 +2733,10 @@ describe("PR review-state workflow concurrency groups (#1707)", () => {
26962733

26972734
const groupA = shardGroupFor(runA, { pr_number: null })
26982735
const groupB = shardGroupFor(runB, { pr_number: null })
2699-
expect(groupA).toBe("label-pr-review-state-555000111")
2700-
expect(groupB).toBe("label-pr-review-state-555000222")
2736+
expect(groupA).toBe("label-pr-review-state-reconcile-555000111")
2737+
expect(groupB).toBe("label-pr-review-state-reconcile-555000222")
27012738
expect(groupA).not.toBe(groupB)
2702-
expect(groupA).not.toBe("label-pr-review-state-1664")
2739+
expect(groupA).not.toBe("label-pr-review-state-reconcile-1664")
27032740
})
27042741

27052742
it("fans out one serialized shard per resolved PR", () => {
@@ -2719,7 +2756,9 @@ describe("PR review-state workflow concurrency groups (#1707)", () => {
27192756
expect(resolution.prNumbers()).toEqual([1437])
27202757

27212758
const context = contextFor({ event_name: "pull_request_target", event: { pull_request: { number: 1437 } } })
2722-
expect(shardGroupFor(context, { pr_number: resolution.prNumbers()?.[0] })).toBe("label-pr-review-state-1437")
2759+
expect(shardGroupFor(context, { pr_number: resolution.prNumbers()?.[0] })).toBe(
2760+
"label-pr-review-state-reconcile-1437",
2761+
)
27232762
})
27242763

27252764
it("resolves a sweep to one shard per open PR, each serializing with direct runs for that PR", async () => {
@@ -2732,9 +2771,9 @@ describe("PR review-state workflow concurrency groups (#1707)", () => {
27322771
shardGroupFor(sweepContext, { pr_number: prNumber }),
27332772
)
27342773
expect(groups).toEqual([
2735-
"label-pr-review-state-1664",
2736-
"label-pr-review-state-1665",
2737-
"label-pr-review-state-1666",
2774+
"label-pr-review-state-reconcile-1664",
2775+
"label-pr-review-state-reconcile-1665",
2776+
"label-pr-review-state-reconcile-1666",
27382777
])
27392778

27402779
// A direct event for one of those PRs lands in the identical group, so the
@@ -2756,8 +2795,8 @@ describe("PR review-state workflow concurrency groups (#1707)", () => {
27562795
event: { workflow_run: { id: 123456, pull_requests: [{ number: 1664 }, { number: 1707 }] } },
27572796
})
27582797
expect(workflowGroupFor(context)).toBe("label-pr-review-state-123456")
2759-
expect(shardGroupFor(context, { pr_number: 1664 })).toBe("label-pr-review-state-1664")
2760-
expect(shardGroupFor(context, { pr_number: 1707 })).toBe("label-pr-review-state-1707")
2798+
expect(shardGroupFor(context, { pr_number: 1664 })).toBe("label-pr-review-state-reconcile-1664")
2799+
expect(shardGroupFor(context, { pr_number: 1707 })).toBe("label-pr-review-state-reconcile-1707")
27612800
})
27622801

27632802
it("demonstrates the stale-overwrite race that per-PR shard serialization prevents", async () => {
@@ -2816,7 +2855,7 @@ describe("PR review-state workflow concurrency groups (#1707)", () => {
28162855
event: { pull_request: { number: 1437 } },
28172856
})
28182857
const sweepContext = contextFor({ event_name: "schedule" })
2819-
expect(shardGroupFor(directContext, { pr_number: 1437 })).toBe("label-pr-review-state-1437")
2858+
expect(shardGroupFor(directContext, { pr_number: 1437 })).toBe("label-pr-review-state-reconcile-1437")
28202859
expect(shardGroupFor(sweepContext, { pr_number: 1437 })).toBe(shardGroupFor(directContext, { pr_number: 1437 }))
28212860
})
28222861

0 commit comments

Comments
 (0)