Conversation
Follow-up to #3631. Remove the concurrency group. It was added to serialize releases, but the default queue keeps only one pending run: with a release running and a second pending, a third cancels the second, which would then be tagged on GitHub but never deployed or finalized - silently. The group was not load-bearing anyway, since the atomic non-forced branch push already makes a concurrent release fail visibly. Removing it trades a silent failure back for a loud one. Check the tag ancestry in publish instead of finalize-release. The check is equivalent against the branch tip, because the release commit is always a child of it, and running it before the deploy means a release cut from an unrelated commit fails while it can still be retried. Maven Central artifacts are immutable once published. Lease the tag update. The ancestry check reads the tag as fetched at checkout, but `+refs/tags/...` would then overwrite whatever the remote holds at push time. The push now leases against the OID observed during the check - the tag ref itself, not the commit it peels to - with an empty expectation when the tag did not exist. A tag moved by anyone else in between fails the push, and --atomic means the branch does not move either. Guard against a release moving the branch backwards. The next development version is derived from the released version, so releasing a tag older than the branch's own version - which means the wrong branch was selected - would set the branch to versions that are already released. Deriving is still right for the common case: releasing v5.7.0 from main on 5.6.2-SNAPSHOT must land on 5.7.1-SNAPSHOT, not go back to 5.6.2-SNAPSHOT.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe release workflow now validates tag ancestry, prevents backward version moves, and updates the release branch and tag with an atomic lease. A shared POM version script replaces inline XML parsing. Release serialization documentation now describes deployment and push conflicts. ChangesRelease safety controls
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The workflow adds release-safety safeguards, and no actionable production risk remains identified for this change. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 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 |
csviri
marked this pull request as ready for review
September 22, 2026 14:08
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical release-safety issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 4
Open (4)
What changed in this PR
This PR hardens Maven release finalization with improved tag validation, version safeguards, and atomic ref updates.
Changes:
- Removes workflow-level release concurrency.
- Adds ancestry and monotonic-version checks.
- Uses leased atomic tag and branch updates.
| File | Summary |
|---|---|
.github/workflows/release.yml |
Removes release concurrency configuration. |
.github/workflows/release-project-in-dir.yml |
Adds validation, version guards, and leased atomic pushes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+256
to
+260
| git push --atomic \ | ||
| --force-with-lease="refs/tags/${RELEASE_TAG}:${EXPECTED_TAG_OID}" \ | ||
| origin \ | ||
| "HEAD:refs/heads/${TARGET_BRANCH}" \ | ||
| "+refs/tags/${RELEASE_TAG}" | ||
| "refs/tags/${RELEASE_TAG}:refs/tags/${RELEASE_TAG}" |
Two review findings, both about checks running after the deploy has already
put immutable artifacts in Maven Central.
The tag ancestry check tested `refs/tags/<tag>^{commit}` for existence, so a tag
that resolves to something other than a commit looked the same as an absent tag
and skipped the check entirely - then finalize-release failed peeling it, after
publishing. Test the ref and peel it separately so that case fails loudly.
The monotonicity guard ran in finalize-release. A tag older than the branch's
development version but still an ancestor of it would pass publish, deploy, and
only then abort. Move the equivalent check - released version against the
branch's own version - into publish, ahead of the deploy. The guard in
finalize-release stays as a postcondition on the derived version.
Extract pom-version.py, now that both jobs need to read a pom version.
Removing it was the wrong call. Without serialization two releases cut from the same branch both pass publish and deploy different immutable versions to Maven Central; only then does the non-forced branch push fail one of them, leaving published artifacts with no release commit and the tag still on the snapshot commit. Maven Central does not allow that to be redone. The failure mode serialization does have - a third release arriving while one runs and one is pending replaces the pending one - leaves nothing published, so that release can just be re-run. A recoverable failure beats an unrecoverable one, which is the opposite of the trade described when this was removed.
| # develops would set the branch back onto versions that are already | ||
| # released. That means the wrong branch was selected for this tag. | ||
| # Checked here because the deploy that follows cannot be undone. | ||
| DEVELOPMENT_VERSION="$(python3 .github/scripts/pom-version.py "${ROOT_POM}")" |
Comment on lines
+8
to
14
| # Two releases cut from the same branch must not overlap: both would deploy to | ||
| # Maven Central, then one would lose the branch push and be left with immutable | ||
| # artifacts and no release commit. Serializing means the worst case is instead a | ||
| # release that does not start, which can simply be re-run. | ||
| concurrency: | ||
| group: ${{ github.workflow }} | ||
| cancel-in-progress: false |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #3631, addressing review findings. v5.6.2 already released cleanly through #3631 — the tag points at the
Release v5.6.2commit with a non-SNAPSHOT pom — so none of this is a live breakage.Remove the concurrency group. I added it in #3631 to serialize releases, but the default queue keeps only one pending run: with a release running and a second pending, a third cancels the second, which would then be tagged on GitHub but never deployed or finalized — silently. The group was never load-bearing, since the atomic non-forced branch push already makes a concurrent release fail visibly. Removing it trades a silent failure back for a loud one. (
queue: maxwould be the alternative; I couldn't verify its availability, so this takes the unambiguously safe route.)Check the tag ancestry in
publish, notfinalize-release. The check is equivalent against the branch tip — the release commit is always a child of it — and running it before the deploy means a release cut from an unrelated commit fails while it's still retryable. Maven Central artifacts are immutable once published.Lease the tag update. The ancestry check reads the tag as fetched at checkout, but
+refs/tags/...would then overwrite whatever the remote holds at push time. The push now leases against the OID observed during the check — the tag ref itself, not the commit it peels to — with an empty expectation when the tag didn't exist. A tag moved by anyone else in between fails the push, and--atomicmeans the branch doesn't move either.Guard against a release moving the branch backwards. The next development version is derived from the released version, so releasing a tag older than the branch's own version would set the branch to versions already released. Note this is deliberately not CodeRabbit's suggested fix of restoring the checked-out version: that breaks the common case, since releasing
v5.7.0from main on5.6.2-SNAPSHOTmust land on5.7.1-SNAPSHOT, not go back to5.6.2-SNAPSHOT. A monotonicity guard covers both.5.6.2-SNAPSHOTv5.6.25.6.3-SNAPSHOT5.6.2-SNAPSHOTv5.7.05.7.1-SNAPSHOT5.6.3-SNAPSHOTv5.0.05.0.1-SNAPSHOTNot changed
Copilot also flagged that the push uses
GITHUB_TOKEN, sosnapshot-releases.ymlwon't run for the release commits. That's accurate — verified, it last ran on832857dfaand not on0d612f32a/70988d132— but it isn't a regression (the previousad-m/github-push-actionused the same token) and it's arguably desirable. It also doesn't affect #3631's rationale, where the trigger was always a subsequent human merge onto a branch left at a release version.Testing
Run-blocks extracted from the YAML and executed verbatim against scratch repos with a stubbed
mvnw, using annotated tags to exercise the tag-object lease path:v5.7.0from5.7.0-SNAPSHOT→ tagged5.7.0, branch5.7.1-SNAPSHOTv5.8.0from5.7.3-SNAPSHOT→ tagged5.8.0, branch5.8.1-SNAPSHOTv6.0.0from5.7.3-SNAPSHOT→ tagged6.0.0, branch6.0.1-SNAPSHOTGuards, all aborting as intended:
publish, before any deploy.v5.0.0from a branch on5.6.3-SNAPSHOT→ monotonicity guard fires.--force-with-leasesemantics were verified separately against real git (2.55.0): empty lease succeeds when the tag is absent and is rejected when it exists, a matching OID performs the forced move, and a stale OID is rejected with--atomicholding the branch back.Summary by CodeRabbit
Bug Fixes
Release Process