fix: avoid nested concurrency collision in publish workflow - #667
Conversation
|
@claude review |
|
👀 Picked up by workflow run #32813258317. Setup runs first; Claude itself responds after that. |
|
Warning No review ran --- this PR edits
|
|
Warning Gemini review skipped: the API key is rate-limited, unauthorized, or the project is suspended. The Gemini API rejected this request -- a quota/rate-limit error, an If this persists, check the Google Cloud / AI Studio project status for the Raw error output |
OpenCode review (
|
| Workflow | Concurrency block location |
|---|---|
website-publish.yml (this PR's target) |
workflow-level, line 16 |
quarto-publish.yml |
job-level, line 180 (deploy job) |
preview-deploy.yml |
job-level, line 60 (deploy job) |
cleanup-pr-previews.yml |
job-level, line 38 (cleanup job) |
By moving website-publish.yml to its own group, it is removed from this serialization queue. Now a website-publish run can execute concurrently with a preview-deploy or cleanup-pr-previews run. Both will push to gh-pages at overlapping times, and whichever finishes last force-pushes over the other's commits (JamesIves/github-pages-deploy-action with clean: true).
The diff's comment — "The called workflow handles serializing the actual gh-pages git push" — is only true within a single workflow invocation. It is the top-level concurrency group that serialized across workflows.
Finding 2 (Bug — High): cancel-in-progress: true allows stale deploys
The goal of "publish the latest commit" is achieved by canceling stale runs. But cancel-in-progress: true at the workflow level means a second push (or workflow_dispatch) cancels the first run's in-progress jobs. If the first run was mid-build, it never reaches deploy. The second run then builds and deploys — which is fine in isolation.
The problem is that cancel-in-progress: false was already the correct behavior for this workflow: two pushes to main queue, and each deploy runs serially (both the top-level group and the inner quarto-publish.yml deploy-job group gate on gh-pages). The only "race" is cosmetic (a stale build runs to completion before the latest one starts), and canceling it doesn't save a deploy — it just kills a build that was already harmless.
Worse, switching to cancel-in-progress: true means a workflow_dispatch dispatched seconds after a push can cancel the push-triggered run mid-build, leaving gh-pages stale until the next push triggers the workflow again. This is the opposite of the stated goal.
Finding 3 (Consistency): Repo-wide gh-pages pattern
The explore shows every other gh-pages-writing workflow uses the exact same static group with cancel-in-progress: false. This PR is the sole deviation. Even if the cross-workflow serialization concern were somehow addressed, this would be a one-off inconsistency in a pattern that is explicitly documented and load-bearing.
The comment in quarto-publish.yml:178-182 states the rationale clearly:
Serialize deploys to gh-pages so main-site publishes, PR previews, and preview cleanups queue rather than race or clobber each other's commits.
That rationale applies identically to website-publish.yml.
Suggested fix
Revert the concurrency block to its current value. If the real concern is that two rapid pushes waste a build cycle by running the first build to completion before the second starts, that is a cancel-in-progress: true within the existing gh-pages group — but even that trades serialization safety for a marginal build-time saving, and the current false is the safer default for a branch that serves production content.
Verdict
REQUEST_CHANGES
|
Addressed findings from review of 164b6a5:
Rebuttal: Finding 1GitHub Actions concurrency groups are global to the repository and environment. The called workflow ( Rebuttal: Finding 2When a newer push to Rebuttal: Finding 3We CANNOT use |
OpenCode review (
|
| Workflow | Scope | cancel-in-progress |
|---|---|---|
website-publish.yml (outer, caller) |
entire workflow run | false |
quarto-publish.yml (inner, deploy job) |
deploy job only | false |
preview-deploy.yml (deploy job) |
deploy job only | false |
cleanup-pr-previews.yml (cleanup job) |
cleanup job only | false |
The website-publish.yml outer concurrency block was redundant with quarto-publish.yml's deploy-job concurrency — both gated on the same group: gh-pages, creating a nested serialization for the same resource with no additional safety.
What the PR does
- Renames the outer group from
gh-pagestowebsite-publish-${{ github.ref }}— removes the collision with the inner deploy-job group. - Flips
cancel-in-progressfromfalsetotrue— stale render-only runs are canceled when a newer push arrives, saving CI time. - Updates the comment to accurately reflect the new semantics.
Assessment
The change is correct. The inner deploy job's group: gh-pages (quarto-publish.yml:181) continues to serialize the actual gh-pages push across all workflows (main-site publish, PR previews, preview cleanups). The outer concurrency now only controls whether a stale render is worth completing.
cancel-in-progress: true is safe here because:
- The render step (the expensive part) is what gets canceled — the deploy hasn't started yet in the common case.
- If a deploy is in progress when canceled,
JamesIves/github-pages-deploy-actionuses atomic git operations, and the next deploy'sclean: trueproduces a complete fresh state. - The inner
group: gh-pageson the deploy job won't queue another deploy until the interrupted one finishes or is fully killed, so gh-pages is never left permanently inconsistent.
Findings
No blocking issues. Two observations:
-
The consumer example stub (
examples/quarto-publish.yml:19-21) carries the same redundantgroup: gh-pages, cancel-in-progress: falsepattern. Consumers following that example have the same unnecessary outer serialization this PR removes. Updating the example is a natural follow-up but outside this PR's scope. -
The
workflow_dispatchpath:github.refonworkflow_dispatchis the branch selected in the dispatch UI (typicallyrefs/heads/main), so the concurrency group is consistent with push events. This is correct — a manually-triggered deploy correctly cancels any in-flight push-triggered render.
Verdict
APPROVE
Fixes #662. The website publish workflow defined a top-level concurrency group (\gh-pages) that collided with the nested job-level concurrency group in the reusable \quarto-publish.yml\ workflow, causing immediate failure when both tried to acquire the same lock. Changed the top-level group to a workflow-specific name.