diff --git a/README.md b/README.md index 7e4e5f56..52d16867 100644 --- a/README.md +++ b/README.md @@ -77,6 +77,15 @@ and set it as an environment variable named `BUILD_NUMBER`, and as a GitHub Acti The build number is unique per workflow run ID. It is not incremented on workflow reruns. +Concurrent runs (e.g. several GitHub Stacked PRs opened at once) claim a build number atomically: the +action reads the `build_number` property as a starting hint, then claims that number exclusively by +creating a Git reference under `refs/build-locks/` — ref creation fails if the ref already +exists, so it acts as a compare-and-swap. On a collision the action retries with the next number; no +two concurrent runs can ever claim the same number. The `build_number` property is still updated +afterwards, but only as a best-effort hint for where the next run should start searching — correctness +never depends on it. `refs/build-locks/*` accumulate over time and are not currently pruned; cleanup is +tracked separately and does not affect correctness. + During execution the action temporarily writes `.build_number.txt` at the repository root (for `actions/cache`); the file is removed before the action completes. Do not track a file named `.build_number.txt` in your repository. @@ -94,7 +103,9 @@ The action authenticates `gh` with a Vault-issued GitHub token. It sets both `GI #### Required Vault Permissions -- `build-number`: GitHub preset to read and write the build number property. This is built-in to the Vault `auth.github` permission. +- `build-number`: GitHub preset to read and write the build number property, and to create the + `refs/build-locks/*` references used to claim it atomically. This is built-in to the Vault + `auth.github` permission. ### Usage diff --git a/get-build-number/get_build_number.sh b/get-build-number/get_build_number.sh index 8a534bf6..2c471ccf 100755 --- a/get-build-number/get_build_number.sh +++ b/get-build-number/get_build_number.sh @@ -4,11 +4,23 @@ set -euo pipefail : "${GITHUB_REPOSITORY:?}" +: "${GITHUB_SHA:?}" GH_API_VERSION_HEADER="X-GitHub-Api-Version: 2022-11-28" BUILD_NUMBER_FILE="${BUILD_NUMBER_FILE:-.build_number.txt}" +PROPERTIES_API_URL="repos/${GITHUB_REPOSITORY}/properties/values" +REFS_API_URL="repos/${GITHUB_REPOSITORY}/git/refs" +# The custom-properties API has no conditional/atomic update (no If-Match support), so a plain +# GET-increment-PATCH on build_number races under concurrent runs (e.g. GitHub Stacked PRs) and can +# hand out the same number twice. Git ref creation IS atomic (it fails if the ref already exists), so +# it's used here as a compare-and-swap: exclusively claim refs/build-locks/ before trusting N. +# See PREQ-7781. +MAX_ATTEMPTS="${MAX_ATTEMPTS:-50}" + +claim_build_number() { + gh api --method POST -H "$GH_API_VERSION_HEADER" "$REFS_API_URL" -f "ref=refs/build-locks/$1" -f "sha=${GITHUB_SHA}" 2>&1 +} echo "Fetching build number from repository properties..." -PROPERTIES_API_URL="repos/${GITHUB_REPOSITORY}/properties/values" BUILD_NUMBER=$(gh api -H "$GH_API_VERSION_HEADER" "$PROPERTIES_API_URL" --jq '.[] | select(.property_name == "build_number") | .value') echo "Current build number from repo: ${BUILD_NUMBER:=0}" if ! [[ "$BUILD_NUMBER" =~ ^[0-9]+$ ]]; then @@ -16,9 +28,39 @@ if ! [[ "$BUILD_NUMBER" =~ ^[0-9]+$ ]]; then exit 1 fi -BUILD_NUMBER=$((BUILD_NUMBER + 1)) +# BUILD_NUMBER above is only a starting hint for where to search; it may be stale (e.g. a concurrent +# run claimed further ahead and hasn't updated the property yet). Correctness never depends on it +# being accurate, only on the ref-creation compare-and-swap below. +attempt=1 +CANDIDATE=$((BUILD_NUMBER + 1)) +while true; do + RESPONSE=$(claim_build_number "$CANDIDATE") && CLAIM_STATUS=0 || CLAIM_STATUS=$? + + if [[ "$CLAIM_STATUS" -eq 0 ]]; then + echo "Claimed build number ${CANDIDATE} (refs/build-locks/${CANDIDATE})" + break + fi + + if [[ "$RESPONSE" != *"Reference already exists"* ]]; then + echo "::error title=Build number claim failed::${RESPONSE}" >&2 + exit 1 + fi + + if (( attempt >= MAX_ATTEMPTS )); then + echo "::error title=Build number race::Could not claim a build number after ${MAX_ATTEMPTS} attempts (concurrent claims)." >&2 + exit 1 + fi + + echo "Build number ${CANDIDATE} already claimed; trying $((CANDIDATE + 1)) (attempt $((attempt + 1))/${MAX_ATTEMPTS})..." + CANDIDATE=$((CANDIDATE + 1)) + attempt=$((attempt + 1)) +done + +# Best-effort: keep the custom property as a hint for the next run's starting point. Correctness +# never depends on this succeeding or being accurate. gh api --method PATCH -H "$GH_API_VERSION_HEADER" "$PROPERTIES_API_URL" \ -f "properties[][property_name]=build_number" \ - -f "properties[][value]=${BUILD_NUMBER}" -echo "Incremented 'build_number' repository property to ${BUILD_NUMBER}" -echo "${BUILD_NUMBER}" > "$BUILD_NUMBER_FILE" + -f "properties[][value]=${CANDIDATE}" \ + || echo "::warning title=Build number hint not updated::Failed to update the build_number property; this does not affect correctness." + +echo "${CANDIDATE}" > "$BUILD_NUMBER_FILE" diff --git a/spec/get_build_number_spec.sh b/spec/get_build_number_spec.sh index 54222de8..72166975 100755 --- a/spec/get_build_number_spec.sh +++ b/spec/get_build_number_spec.sh @@ -2,6 +2,7 @@ eval "$(shellspec - -c) exit 1" export GITHUB_REPOSITORY="my org/my-repo" +export GITHUB_SHA="deadbeefcafef00dfeed" TEMP_DIR="${SHELLSPEC_TMPBASE:-/tmp}" export BUILD_NUMBER_FILE="${TEMP_DIR}/build_number.txt" @@ -10,26 +11,24 @@ Mock gh End Describe 'get_build_number.sh' - It 'should increment and return the build number' + It 'should claim the next build number when there is no contention' Mock gh - if [[ "$*" =~ "api --method PATCH" ]]; then + if [[ "$*" == *"git/refs"* ]]; then echo "gh $*" - elif [[ "$*" =~ "properties/values" ]]; then + elif [[ "$*" == *"--method PATCH"* ]]; then + echo "gh $*" + elif [[ "$*" == *"properties/values"* ]]; then echo '42' else echo "gh $*" fi End - # shellcheck disable=SC2317 -# preserve() { %preserve BUILD_NUMBER; } -# AfterRun preserve When run script get-build-number/get_build_number.sh The line 1 should include "Fetching build number" The line 2 should equal "Current build number from repo: 42" - The line 3 should include "43" + The output should include "Claimed build number 43" The path "$BUILD_NUMBER_FILE" should be file The contents of file "$BUILD_NUMBER_FILE" should equal "43" -# The variable BUILD_NUMBER should equal "43" End It 'should return an error if BUILD_NUMBER is invalid' @@ -44,12 +43,100 @@ Describe 'get_build_number.sh' It 'should handle empty build number' Mock gh + if [[ "$*" == *"properties/values"* && "$*" != *"--method PATCH"* ]]; then echo '' + else + echo "gh $*" + fi End When run script get-build-number/get_build_number.sh The status should be success The line 2 should equal "Current build number from repo: 0" - # Ignore empty line from second call to gh - The line 4 should include "1" + The output should include "Claimed build number 1" + The contents of file "$BUILD_NUMBER_FILE" should equal "1" + End + + It 'should retry when a concurrent run already claimed the next number, then succeed' + export GH_REFS_CALLS_FILE="${TEMP_DIR}/gh_refs_calls_retry.txt" + rm -f "$GH_REFS_CALLS_FILE" + Mock gh + if [[ "$*" == *"git/refs"* ]]; then + count=$(($(cat "$GH_REFS_CALLS_FILE" 2>/dev/null || echo 0) + 1)) + echo "$count" > "$GH_REFS_CALLS_FILE" + if [[ "$count" -eq 1 ]]; then + echo '{"message":"Reference already exists"}' >&2 + exit 1 + else + echo "gh $*" + fi + elif [[ "$*" == *"--method PATCH"* ]]; then + echo "gh $*" + elif [[ "$*" == *"properties/values"* ]]; then + echo '42' + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be success + The output should include "Build number 43 already claimed; trying 44" + The output should include "Claimed build number 44" + The path "$BUILD_NUMBER_FILE" should be file + The contents of file "$BUILD_NUMBER_FILE" should equal "44" + End + + It 'should fail after exhausting retries under permanent contention' + export MAX_ATTEMPTS=3 + Mock gh + if [[ "$*" == *"git/refs"* ]]; then + echo '{"message":"Reference already exists"}' >&2 + exit 1 + elif [[ "$*" == *"properties/values"* ]]; then + echo '42' + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be failure + The output should include "already claimed" + The stderr should include "::error title=Build number race::Could not claim a build number after 3 attempts" + End + + It 'should fail immediately on an unexpected API error, not treat it as a collision' + Mock gh + if [[ "$*" == *"git/refs"* ]]; then + echo '{"message":"Internal Server Error"}' >&2 + exit 1 + elif [[ "$*" == *"properties/values"* ]]; then + echo '42' + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be failure + The output should include "Current build number from repo: 42" + The stderr should include "::error title=Build number claim failed::" + The stderr should include "Internal Server Error" + End + + It 'should still succeed even if updating the build_number property hint fails' + Mock gh + if [[ "$*" == *"git/refs"* ]]; then + echo "gh $*" + elif [[ "$*" == *"--method PATCH"* ]]; then + exit 1 + elif [[ "$*" == *"properties/values"* ]]; then + echo '42' + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be success + The output should include "Claimed build number 43" + The output should include "Build number hint not updated" + The contents of file "$BUILD_NUMBER_FILE" should equal "43" End End