Skip to content

Commit 663c6cb

Browse files
committed
fix: address review findings on the release workflow
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. Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
1 parent 70988d1 commit 663c6cb

2 files changed

Lines changed: 64 additions & 15 deletions

File tree

‎.github/workflows/release-project-in-dir.yml‎

Lines changed: 64 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,36 @@ jobs:
2929
uses: actions/checkout@v7
3030
with:
3131
ref: "${{ inputs.version_branch }}"
32+
# Full history and tags are needed to check where the release tag
33+
# currently points relative to the branch.
34+
fetch-depth: 0
35+
fetch-tags: true
3236

3337
- name: Resolve checked-out commit
3438
id: resolve-sha
3539
run: echo "commit=$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT"
3640

41+
- name: Check the release tag can be moved onto this branch
42+
env:
43+
RELEASE_TAG: ${{ inputs.release_tag }}
44+
TARGET_BRANCH: ${{ inputs.version_branch }}
45+
run: |
46+
set -euo pipefail
47+
48+
# finalize-release moves the tag onto a commit built on top of this
49+
# one, which is only legitimate if the tag already points into this
50+
# branch's history. Checking it here rather than after the deploy
51+
# means a release cut from an unrelated commit fails while it can
52+
# still be retried - once artifacts are in Maven Central they are
53+
# immutable.
54+
if git rev-parse -q --verify "refs/tags/${RELEASE_TAG}^{commit}" >/dev/null; then
55+
CURRENT_TAGGED="$(git rev-parse "refs/tags/${RELEASE_TAG}^{commit}")"
56+
if ! git merge-base --is-ancestor "${CURRENT_TAGGED}" HEAD; then
57+
echo "Tag ${RELEASE_TAG} points at ${CURRENT_TAGGED}, which is not an ancestor of ${TARGET_BRANCH}"
58+
exit 1
59+
fi
60+
fi
61+
3762
- name: Set up Java and Maven
3863
uses: actions/setup-java@v6
3964
with:
@@ -106,6 +131,7 @@ jobs:
106131
id: commits
107132
env:
108133
RELEASE_TAG: ${{ inputs.release_tag }}
134+
TARGET_BRANCH: ${{ inputs.version_branch }}
109135
run: |
110136
set -euo pipefail
111137
@@ -117,6 +143,7 @@ jobs:
117143
}
118144
119145
RELEASE_VERSION="${RELEASE_TAG#v}"
146+
DEVELOPMENT_VERSION="$(pom_version)"
120147
121148
git config --local user.email "action@github.com"
122149
git config --local user.name "GitHub Action"
@@ -160,6 +187,17 @@ jobs:
160187
;;
161188
esac
162189
190+
# The bump is derived from the released version, so releasing a tag
191+
# older than the branch's own development version would move the
192+
# branch backwards - onto versions that have already been released.
193+
# That means the wrong branch was selected for the tag, so stop.
194+
if [ "${NEXT_VERSION}" != "${DEVELOPMENT_VERSION}" ] && \
195+
[ "$(printf '%s\n%s\n' "${NEXT_VERSION}" "${DEVELOPMENT_VERSION}" | sort -V | head -1)" = "${NEXT_VERSION}" ]; then
196+
echo "Next development version ${NEXT_VERSION} would move ${TARGET_BRANCH} back from ${DEVELOPMENT_VERSION}"
197+
echo "Is ${RELEASE_TAG} being released from the right branch?"
198+
exit 1
199+
fi
200+
163201
if git diff --quiet; then
164202
echo "Branch would be left on release version ${RELEASE_VERSION}"
165203
exit 1
@@ -168,6 +206,7 @@ jobs:
168206
echo "Next development version: ${NEXT_VERSION}"
169207
170208
- name: Move release tag onto the release commit
209+
id: tag
171210
env:
172211
RELEASE_TAG: ${{ inputs.release_tag }}
173212
RELEASE_COMMIT: ${{ steps.commits.outputs.release_commit }}
@@ -176,30 +215,46 @@ jobs:
176215
177216
# GitHub created the tag on whatever the branch tip was when the
178217
# release was published, so it is expected to move - but only forward,
179-
# onto a descendant. Anything else means the release was cut from a
180-
# commit this workflow did not build, and silently discarding it would
181-
# lose the tagged state.
182-
if git rev-parse -q --verify "refs/tags/${RELEASE_TAG}^{commit}" >/dev/null; then
218+
# onto a descendant. publish already checked this against the branch;
219+
# repeat it here because the release commit is what actually gets
220+
# tagged.
221+
#
222+
# The OID recorded here is the tag ref itself, not the commit it peels
223+
# to, because that is what the push has to lease against. An empty
224+
# value means the tag did not exist, which --force-with-lease reads as
225+
# "must still not exist".
226+
if git rev-parse -q --verify "refs/tags/${RELEASE_TAG}" >/dev/null; then
227+
EXPECTED_TAG_OID="$(git rev-parse "refs/tags/${RELEASE_TAG}")"
183228
CURRENT_TAGGED="$(git rev-parse "refs/tags/${RELEASE_TAG}^{commit}")"
184229
if ! git merge-base --is-ancestor "${CURRENT_TAGGED}" "${RELEASE_COMMIT}"; then
185230
echo "Tag ${RELEASE_TAG} points at ${CURRENT_TAGGED}, which is not an ancestor of ${RELEASE_COMMIT}"
186231
exit 1
187232
fi
233+
else
234+
EXPECTED_TAG_OID=""
188235
fi
236+
echo "expected_tag_oid=${EXPECTED_TAG_OID}" >> "$GITHUB_OUTPUT"
189237
190238
git tag -f -a "${RELEASE_TAG}" "${RELEASE_COMMIT}" -m "Release ${RELEASE_TAG}"
191239
192240
- name: Push release commit, next development commit and tag
193241
env:
194242
RELEASE_TAG: ${{ inputs.release_tag }}
195243
TARGET_BRANCH: ${{ inputs.version_branch }}
244+
EXPECTED_TAG_OID: ${{ steps.tag.outputs.expected_tag_oid }}
196245
run: |
197246
set -euo pipefail
198247
199248
# One atomic push so the branch is never left holding the release
200-
# commit without the SNAPSHOT commit that follows it. The branch
201-
# refspec is not forced: if something landed on the branch while the
202-
# release was being deployed, this fails instead of clobbering it.
203-
git push --atomic origin \
249+
# commit without the SNAPSHOT commit that follows it.
250+
#
251+
# Neither ref can be clobbered: the branch refspec is not forced, so a
252+
# commit landing during the deploy fails the push, and the tag is
253+
# leased against the OID observed during the ancestry check, so a tag
254+
# moved by anyone else since then fails it too. --atomic means either
255+
# both refs update or neither does.
256+
git push --atomic \
257+
--force-with-lease="refs/tags/${RELEASE_TAG}:${EXPECTED_TAG_OID}" \
258+
origin \
204259
"HEAD:refs/heads/${TARGET_BRANCH}" \
205-
"+refs/tags/${RELEASE_TAG}"
260+
"refs/tags/${RELEASE_TAG}:refs/tags/${RELEASE_TAG}"

‎.github/workflows/release.yml‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,6 @@ on:
55
release:
66
types: [ released ]
77

8-
# Releases push commits to the branch they are cut from, so run them one at a
9-
# time rather than letting two overlap on the same branch.
10-
concurrency:
11-
group: ${{ github.workflow }}
12-
cancel-in-progress: false
13-
148
permissions:
159
contents: read
1610

0 commit comments

Comments
 (0)