ci: never cancel main runs, and make publish concurrency-safe - #93
ci: never cancel main runs, and make publish concurrency-safe#93shaharkazaz wants to merge 2 commits into
Conversation
Pushes to main shared a single concurrency group with cancel-in-progress enabled, so each new commit cancelled the run for the previous one -- including the push-only publish job. Append the commit sha to the concurrency group for refs/heads/main, giving every commit on main its own group. Nothing cancels it and nothing queues behind it, so each commit runs the full matrix. Pull request runs keep the existing per-ref group, so superseded PR runs are still cancelled.
📝 WalkthroughWalkthroughThe CI workflow now preserves concurrent main runs, serializes publishing, recalculates versions from refreshed tags, atomically claims release tags, gates npm publication, and retries pushing version commits after rebasing. ChangesRelease publishing concurrency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant OriginMain
participant GitTags
participant Npm
GitHubActions->>OriginMain: Force-fetch main and tags
GitHubActions->>GitTags: Atomically push version tag
GitTags-->>GitHubActions: Return claim result
GitHubActions->>Npm: Generate and publish packages when claimed
GitHubActions->>OriginMain: Push version commit
OriginMain-->>GitHubActions: Reject or accept push
GitHubActions->>OriginMain: Re-fetch and rebase after rejection
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Preview Release AvailableA preview release has been published for commit ee309e0. Installationnpm install https://github.com/frontops-dev/domino/releases/download/pr-93-ee309e0/front-ops-domino-2.0.2.tgzRunning the previewnpx https://github.com/frontops-dev/domino/releases/download/pr-93-ee309e0/front-ops-domino-2.0.2.tgz affectedDetails |
Removing cancellation on main means several publish jobs can be in flight, and they previously all derived the next version from their own package.json. Two merges landing together would compute the same version, and the loser would fail after the winner had already published to npm. - Queue publishes (job concurrency group, cancel-in-progress: false) so they never overlap. - Re-fetch tags and derive the next version from the last tag rather than from package.json, since a queued run's checkout predates the version commit an earlier run pushed. - Claim the version by pushing the tag on its own before publishing. Creating a remote ref is atomic and reversible, so the winner owns the version and the losers exit 0 before npm publish, the only irreversible step. Re-pushing the same ref from the same commit is a no-op, so retrying a flaky publish works. - Push the version commit after publishing, rebasing onto main if a feature commit landed meanwhile. The version commit cannot be pushed before npm publish: generate-packages.js points optionalDependencies at the new platform package versions, so the yarn.lock refresh in that commit can only resolve once those are on the registry. Splitting the old atomic commit+tag push is what keeps the claim ahead of the publish.
shaharkazaz
changed the title
|
All three review items are in — the PR is now two commits. What went inItem 3 (keep per-commit no-cancel): unchanged, Item 2 (the real fix), all inside
One deviation, and whyThe feedback asked for
So the atomic push is split: the tag is the CAS and goes first; the commit follows the publish. This preserves the property the feedback is actually after — a reversible claim ahead of any irreversible side effect. Consequence worth flagging: the release tag now points at the commit that was released, rather than at the version-bump commit that follows it. Verified by execution, not inspection
Not verified: an end-to-end release against the real npm registry. The first merge after this lands is the remaining check. One behavior to decide onGitHub's documented rule for a shared concurrency group is that only one run may be pending — a newly queued run cancels the previously pending one. So with three merges in rapid succession, the middle commit's publish job can be cancelled while pending. Nothing is lost when that happens (its CAS would have skipped anyway, and no verification job is ever cancelled), but the run shows a cancelled job rather than a green skip — which superficially resembles the symptom this PR set out to fix. Dropping the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/CI.yml (1)
492-492: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRoute computed values through
env:instead of interpolating${{ }}directly intorun:.Static analysis flags direct template expansion into shell at these three new lines (
npm version ${{ ... }},TAG="v${{ ... }}", and the::error::v${{ ... }}message). The values are internally computed semver strings today, so exploitability is low, but this is the standard GitHub Actions hardening pattern to eliminate any script-injection surface, and the fix is trivial. Note line 553 (git commit -m "${{ steps.bump.outputs.new_version }}", pre-existing/unchanged) has the same pattern.🔧 Suggested fix
- name: Bump version id: bump if: steps.version.outputs.is_release != 'true' + env: + NEXT_VERSION: ${{ steps.version.outputs.next_version }} run: | - npm version ${{ steps.version.outputs.next_version }} --no-git-tag-version --ignore-scripts --allow-same-version + npm version "$NEXT_VERSION" --no-git-tag-version --ignore-scripts --allow-same-version node scripts/sync-cargo-version.js NEW_VERSION=$(node -p "require('./package.json').version") echo "new_version=$NEW_VERSION" >> $GITHUB_OUTPUT echo "Bumped to $NEW_VERSION" - name: Claim version id: claim if: steps.version.outputs.is_release != 'true' + env: + NEW_VERSION: ${{ steps.bump.outputs.new_version }} run: | - TAG="v${{ steps.bump.outputs.new_version }}" + TAG="v${NEW_VERSION}"Also applies to: 501-501, 563-563
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/CI.yml at line 492, Replace direct GitHub Actions expression interpolation in the new npm version, TAG assignment, and error-message run commands with environment-variable references, and define those values through each step’s env configuration. Preserve the existing computed semver values and command behavior while avoiding `${{ }}` expansion inside shell scripts; do not modify the pre-existing commit command.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/CI.yml:
- Line 492: Replace direct GitHub Actions expression interpolation in the new
npm version, TAG assignment, and error-message run commands with
environment-variable references, and define those values through each step’s env
configuration. Preserve the existing computed semver values and command behavior
while avoiding `${{ }}` expansion inside shell scripts; do not modify the
pre-existing commit command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ab92e2f-1741-44aa-bc77-2555273b40d9
📒 Files selected for processing (1)
.github/workflows/CI.yml
2 participants
Problem
Two related problems, one caused by fixing the other.
1. Runs on
mainwere cancelled. The workflow used a single concurrency group perref with cancellation enabled, so every push to
mainproduced the same group key(
CI-refs/heads/main) and commit N+1 cancelled the still-running job for commit N.Back-to-back merges left
mainwith no full verification, and could abort thepublishjob partway through a release.2. Removing that cancellation exposes a publish race.
cancel-in-progresswasquietly doing double duty: as a CI cost saver and as a de-facto debounce on the
release pipeline. Only the newest commit ever reached
npm publish, so theauto-versioning logic never had to be concurrency-safe. Without cancellation, two
merges landing together would both publish:
package.json(2.0.1)package.json(2.0.1)npm viewguardThe
npm viewcheck is a TOCTOU test, not a lock. Worse than the red job: 2.0.2 isbuilt from A's tree, and B's own changes may never get published at all.
Solution
Verification: per-commit groups (commit 1)
Append the commit sha to the concurrency group only for
refs/heads/main:main@ abc123CI-refs/heads/main-abc123main@ def456CI-refs/heads/main-def456CI-refs/pull/42/mergeEach
maincommit is alone in its group, so it is never cancelled and never queuedbehind another — the full matrix runs for every commit. PR runs keep the existing
per-ref group, so pushing to a PR branch still cancels the superseded run.
Note that
cancel-in-progress: falseon a shared group would not work here: itqueues the newer run rather than running it, so pushes to
mainwould serializebehind the whole build matrix.
Release: compare-and-swap (commit 2)
All inside the
publishjob:concurrency: { group: publish-main, cancel-in-progress: false }.package.json— re-fetch tags first, since aqueued run's checkout predates the tag an earlier run pushed while it waited.
remote ref is atomic and reversible, so the run that wins owns the version;
losers
exit 0beforenpm publish, the only irreversible step.mainif a featurecommit landed meanwhile.
The
npm viewguard is retained, so a retry after a flaky publish stays idempotent:re-pushing the same tag from the same commit is a no-op success, which lets a re-run
proceed rather than skip.
Deviation from review feedback
The review asked for
git push --atomic origin main v<ver>(commit and tag)before publishing. That is not implementable here — the release has an ordering cycle:
scripts/generate-packages.jsrepoints rootoptionalDependenciesat@front-ops/domino-<platform>@<new version>, andprepublishOnly(
napi prepublish) is what publishes those platform packages. So a version commitpushed before
npm publishcarries ayarn.lockthat cannot resolve, and thebot's own CI run on that commit goes red. (This is what the existing retry loop
commented "handle npm registry propagation delay after publish" is about.)
The atomic push is therefore split: the tag is the CAS and goes first, the commit
follows the publish. This keeps the property the feedback is actually after — a
reversible claim ahead of any irreversible side effect.
Consequence to be aware of: the release tag now points at the commit that was
released, rather than at the version-bump commit that follows it.
git describeandbump derivation are unaffected (the tag is still an ancestor). A stale tag from a run
that died before publishing is self-healing: the next run simply bumps past it.
Key Changes
.github/workflows/CI.ymlmainpublish: job concurrency group, tag re-fetch, version derived from last tag,new
Claim versionCAS step, side-effecting steps gated on the claim, and arebase-retry on the version commit push
Testing
v2.0.1+patch→2.0.2, +minor→2.1.0, +major→3.0.0, tagless→package.json+1.sha is
rejected (already exists)→exit 0with no npm side effect; re-claim fromthe same sha is
Everything up-to-date→ the idempotent retry path.feature commit that landed during the publish; both changes survive).
is_release=truethe claim step is skipped, soclaimedis empty and every side-effecting step skips.
cargo/yarnsuites are unaffected.after merge is the remaining verification.
Out of Scope
pages.yml— docs deploy where only the newest build matters, cancellation is correct.preview-release.yml— gated toworkflow_run.event == 'pull_request', never runsfor
mainpushes.Summary by CodeRabbit