From 8b1e0654dca8374dcaeef2380a74f753aabad1b9 Mon Sep 17 00:00:00 2001 From: Julien Carsique Date: Thu, 6 Aug 2026 15:28:07 +0200 Subject: [PATCH 1/3] PREQ-7781 Claim build numbers atomically via Git refs only Replaces the verify-after-write + retry approach (#335, closed) with a genuine atomic claim: creating refs/build-locks/ fails if the ref already exists, so it acts as a compare-and-swap instead of a probabilistic race-window narrowing. Drops the build_number custom property and actions/cache entirely. The starting candidate now comes from scanning refs/build-locks/* instead of the property hint, and rerun/same-run reuse is tracked via a best-effort refs/build-runs// marker instead of a cache entry. The claim itself uses the calling workflow's own ambient token (contents: write); Vault is only used to read the legacy build_number property once, as a migration seed for repos with existing history, and can be dropped entirely once every repo has migrated. Updates this repo's own CI workflows (pre-commit, test-shell-scripts, test-update-release-channel, test-build-number) to grant contents: write instead of contents: read, since get-build-number (and anything that calls it, e.g. config-npm) now needs it for the ambient-token claim. Co-Authored-By: Claude Sonnet 4.5 --- .github/workflows/pre-commit.yml | 2 +- .github/workflows/test-build-number.yml | 28 ++-- .github/workflows/test-shell-scripts.yml | 2 +- .../workflows/test-update-release-channel.yml | 2 +- README.md | 26 ++- get-build-number/action.yml | 44 ++---- get-build-number/check_existing_claim.sh | 36 +++++ get-build-number/get_build_number.sh | 87 ++++++++-- spec/check_existing_claim_spec.sh | 69 ++++++++ spec/get_build_number_spec.sh | 148 +++++++++++++++--- 10 files changed, 354 insertions(+), 90 deletions(-) create mode 100755 get-build-number/check_existing_claim.sh create mode 100644 spec/check_existing_claim_spec.sh diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml index 5a423022..9a60cfb5 100644 --- a/.github/workflows/pre-commit.yml +++ b/.github/workflows/pre-commit.yml @@ -3,7 +3,7 @@ on: pull_request: permissions: id-token: write - contents: read + contents: write jobs: pre-commit: runs-on: warp-custom-ubuntu-24-04 diff --git a/.github/workflows/test-build-number.yml b/.github/workflows/test-build-number.yml index 0670aa96..50d388ec 100644 --- a/.github/workflows/test-build-number.yml +++ b/.github/workflows/test-build-number.yml @@ -14,7 +14,7 @@ jobs: runs-on: warp-custom-ubuntu-24-04 permissions: id-token: write - contents: read + contents: write outputs: BUILD_NUMBER: ${{ steps.get_build_number.outputs.BUILD_NUMBER }} steps: @@ -46,12 +46,12 @@ jobs: exit 1 fi - test-build-number-reuse-from-cache: + test-build-number-reuse-same-run: needs: test-build-number-generation runs-on: warp-custom-ubuntu-24-04 permissions: id-token: write - contents: read + contents: write steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: @@ -64,15 +64,20 @@ jobs: echo -e "::error title=test-build-number-reuse::Build number '${BUILD_NUMBER}' does not match the previous job build number" \ "'${{ needs.test-build-number-generation.outputs.BUILD_NUMBER }}' despite it is the same workflow run.\n" \ "Prefer using the output from SonarSource/ci-github-actions/get-build-number instead of calling it from distinct jobs." - # exit 1 # flaky test + # Enforced (PREQ-7781): reuse is now backed by strongly-consistent refs/build-runs//* + # instead of eventually-consistent actions/cache, which is why this was flaky before. This + # doesn't guarantee two jobs racing truly concurrently will converge (pre-existing, unchanged + # limitation - see get-build-number/check_existing_claim.sh) but a sequential same-run reuse + # like this one should now be reliable. + exit 1 fi - test-build-number-reuse-from-cache-windows: + test-build-number-reuse-same-run-windows: needs: test-build-number-generation runs-on: github-windows-latest-s permissions: id-token: write - contents: read + contents: write steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: @@ -86,7 +91,8 @@ jobs: echo -e "::error title=test-build-number-reuse-from-cache-windows::Build number '${BUILD_NUMBER}' does not match the previous" \ "job build number '${{ needs.test-build-number-generation.outputs.BUILD_NUMBER }}' despite it is the same workflow run.\n" \ "Prefer using the output from SonarSource/ci-github-actions/get-build-number instead of calling it from distinct jobs." - # exit 1 # flaky test + # Enforced (PREQ-7781): see test-build-number-reuse-same-run above. + exit 1 fi test-build-number-reuse-from-env: @@ -94,7 +100,7 @@ jobs: runs-on: warp-custom-ubuntu-24-04 permissions: id-token: write - contents: read + contents: write env: BUILD_NUMBER: ${{ needs.test-build-number-generation.outputs.BUILD_NUMBER }} steps: @@ -119,13 +125,13 @@ jobs: if: always() needs: - test-build-number-generation - - test-build-number-reuse-from-cache - - test-build-number-reuse-from-cache-windows + - test-build-number-reuse-same-run + - test-build-number-reuse-same-run-windows - test-build-number-reuse-from-env runs-on: warp-custom-ubuntu-24-04 permissions: id-token: write - contents: read + contents: write steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - uses: ./config-npm diff --git a/.github/workflows/test-shell-scripts.yml b/.github/workflows/test-shell-scripts.yml index 6e0f5020..c888f1e8 100644 --- a/.github/workflows/test-shell-scripts.yml +++ b/.github/workflows/test-shell-scripts.yml @@ -14,7 +14,7 @@ jobs: runs-on: warp-custom-ubuntu-24-04 permissions: id-token: write - contents: read + contents: write steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: diff --git a/.github/workflows/test-update-release-channel.yml b/.github/workflows/test-update-release-channel.yml index 8bd3266a..3dbb2bfb 100644 --- a/.github/workflows/test-update-release-channel.yml +++ b/.github/workflows/test-update-release-channel.yml @@ -14,7 +14,7 @@ jobs: runs-on: sonar-xs permissions: id-token: write - contents: read + contents: write steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - name: Update release channel (dry-run, happy path) diff --git a/README.md b/README.md index 7e4e5f56..ed74fee6 100644 --- a/README.md +++ b/README.md @@ -70,31 +70,25 @@ These badges show the status of workflows in dummy repositories that use (or sho ## `get-build-number` -Manage the build number in GitHub Actions. +Get a unique, strictly increasing build number for a repository, reusing one already claimed by the current workflow run when applicable. +It sets `BUILD_NUMBER` as both an environment variable and a GitHub Actions output. Safe to call from multiple jobs in the same workflow +run, and from concurrent workflow runs (e.g. several GitHub Stacked PRs opened at once) - no two calls will ever return the same number. -The build number is stored in the GitHub repository property named `build_number`. This action will reuse or increment the build number, -and set it as an environment variable named `BUILD_NUMBER`, and as a GitHub Actions output variable also named `BUILD_NUMBER`. - -The build number is unique per workflow run ID. It is not incremented on workflow reruns. - -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. - -The action authenticates `gh` with a Vault-issued GitHub token. It sets both `GITHUB_TOKEN` and -`GH_TOKEN` for that step so a workflow-exported `GH_TOKEN` cannot shadow the Vault credential -(`gh` prefers `GH_TOKEN` over `GITHUB_TOKEN`). +During execution the action temporarily writes `.build_number.txt` at the repository root; the file is removed before the action +completes. Do not track a file named `.build_number.txt` in your repository. ### Requirements #### Required GitHub Permissions - `id-token: write` -- `contents: read` +- `contents: write` #### 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 used to read the legacy `build_number` repository property, needed only for repositories that predate this + action's current design. Built-in to the Vault `auth.github` permission. This dependency will be dropped once no repository needs it + anymore. ### Usage @@ -104,7 +98,7 @@ jobs: runs-on: sonar-xs permissions: id-token: write - contents: read + contents: write steps: - uses: SonarSource/ci-github-actions/get-build-number@v1 ``` diff --git a/get-build-number/action.yml b/get-build-number/action.yml index 6d7807af..4df76d06 100644 --- a/get-build-number/action.yml +++ b/get-build-number/action.yml @@ -3,7 +3,7 @@ name: Get build number description: GitHub Action to get the build number for a repository outputs: BUILD_NUMBER: - description: The build number, incremented or reused if already cached + description: The build number, newly claimed or reused if this workflow run already claimed one value: ${{ steps.export.outputs.BUILD_NUMBER }} inputs: host-actions-root: @@ -38,36 +38,34 @@ runs: if: env.BUILD_NUMBER != '' shell: bash run: | - echo "BUILD_NUMBER ${BUILD_NUMBER} provided from environment, skipping both increment and save to cache." + echo "BUILD_NUMBER ${BUILD_NUMBER} provided from environment, skipping increment." echo "${BUILD_NUMBER}" > "$BUILD_NUMBER_FILE" echo "skip=true" >> $GITHUB_OUTPUT - # Reuse current build number in case of rerun - - name: Get cached build number + # Reuse the build number already claimed by this workflow run (e.g. a rerun, or an earlier job in the same run) + - name: Check for existing build number from this workflow run + id: existing-claim if: steps.from-env.outputs.skip != 'true' - uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 - id: current-build-number - with: - path: ${{ env.BUILD_NUMBER_FILE }} - key: build-number-${{ github.run_id }} - enableCrossOsArchive: true + shell: bash + env: + GITHUB_TOKEN: ${{ github.token }} + GH_TOKEN: ${{ github.token }} + run: ${ACTION_PATH_GET_BUILD_NUMBER}/check_existing_claim.sh - # Otherwise, increment the build number + # Otherwise, claim a new build number + # Vault is only needed to read the legacy build_number property during the migration to refs/build-locks/* - uses: SonarSource/vault-action-wrapper@881045d830534a70ec3c7c275fa3714412c8ff6e # 3.6.1 id: secrets - if: steps.from-env.outputs.skip != 'true' && steps.current-build-number.outputs.cache-hit != 'true' + if: steps.from-env.outputs.skip != 'true' && steps.existing-claim.outputs.skip != 'true' with: secrets: development/github/token/{REPO_OWNER_NAME_DASH}-build-number token | github_token; - name: Get new build number - if: steps.from-env.outputs.skip != 'true' && steps.current-build-number.outputs.cache-hit != 'true' + if: steps.from-env.outputs.skip != 'true' && steps.existing-claim.outputs.skip != 'true' shell: bash env: - # gh prefers GH_TOKEN over GITHUB_TOKEN. Set both so a workflow-exported - # GH_TOKEN (e.g. a bot token) cannot shadow the Vault build-number token. - GITHUB_TOKEN: ${{ steps.current-build-number.outputs.cache-hit != 'true' && - steps.secrets.outputs.vault && fromJSON(steps.secrets.outputs.vault).github_token || '' }} - GH_TOKEN: ${{ steps.current-build-number.outputs.cache-hit != 'true' && - steps.secrets.outputs.vault && fromJSON(steps.secrets.outputs.vault).github_token || '' }} + GITHUB_TOKEN: ${{ github.token }} + GH_TOKEN: ${{ github.token }} + LEGACY_PROPERTY_TOKEN: ${{ steps.secrets.outputs.vault && fromJSON(steps.secrets.outputs.vault).github_token || '' }} run: ${ACTION_PATH_GET_BUILD_NUMBER}/get_build_number.sh - name: Export build number @@ -83,14 +81,6 @@ runs: echo "BUILD_NUMBER=${BUILD_NUMBER}" >> "$GITHUB_ENV" echo "BUILD_NUMBER=${BUILD_NUMBER}" >> "$GITHUB_OUTPUT" - - name: Save build number to cache - uses: actions/cache/save@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 - if: steps.from-env.outputs.skip != 'true' && steps.current-build-number.outputs.cache-hit != 'true' - with: - path: ${{ env.BUILD_NUMBER_FILE }} - key: build-number-${{ github.run_id }} - enableCrossOsArchive: true - - name: Remove build number file from workspace if: always() shell: bash diff --git a/get-build-number/check_existing_claim.sh b/get-build-number/check_existing_claim.sh new file mode 100755 index 00000000..76a66446 --- /dev/null +++ b/get-build-number/check_existing_claim.sh @@ -0,0 +1,36 @@ +#!/bin/bash +# Check whether this workflow run already claimed a build number (e.g. a rerun, or another job in the same run) and reuse it if so. +# Read-only check against refs/build-runs//*; see get_build_number.sh for the actual claim. + +set -euo pipefail + +: "${GITHUB_REPOSITORY:?}" "${GITHUB_RUN_ID:?}" "${GITHUB_OUTPUT:?}" + +GH_API_VERSION_HEADER="X-GitHub-Api-Version: 2022-11-28" +BUILD_NUMBER_FILE="${BUILD_NUMBER_FILE:-.build_number.txt}" +MATCHING_REFS_API_URL="repos/${GITHUB_REPOSITORY}/git/matching-refs" +RUNS_NS="build-runs/${GITHUB_RUN_ID}" + +echo "::group::Check for an existing build number claim from this workflow run" +echo "::debug::Checking for an existing claim by this workflow run (refs/${RUNS_NS}/*)" +RUN_CLAIMS=$(gh api -H "$GH_API_VERSION_HEADER" "${MATCHING_REFS_API_URL}/${RUNS_NS}/" --jq '.[].ref') +EXISTING="" +if [[ -n "$RUN_CLAIMS" ]]; then + while IFS= read -r ref; do + [[ "$ref" =~ ^refs/${RUNS_NS}/([0-9]+)$ ]] || continue + n="${BASH_REMATCH[1]}" + # TODO PREQ-7781: two jobs racing here before either writes a marker can each claim a different number; this just picks the lowest. + if [[ -z "$EXISTING" ]] || ((n < EXISTING)); then + EXISTING="$n" + fi + done <<<"$RUN_CLAIMS" +fi + +if [[ -n "$EXISTING" ]]; then + echo "::debug::Reusing build number ${EXISTING}, already claimed by this workflow run (refs/${RUNS_NS}/${EXISTING})" + echo "${EXISTING}" >"$BUILD_NUMBER_FILE" + echo "skip=true" >>"$GITHUB_OUTPUT" +else + echo "::debug::No existing claim found for this workflow run" +fi +echo "::endgroup::" diff --git a/get-build-number/get_build_number.sh b/get-build-number/get_build_number.sh index 8a534bf6..35d6f3de 100755 --- a/get-build-number/get_build_number.sh +++ b/get-build-number/get_build_number.sh @@ -1,24 +1,83 @@ #!/bin/bash # Get the build number for a GitHub repository and save the incremented value to .build_number.txt +# See check_existing_claim.sh for the read-only check for an existing claim by this workflow run. +# refs/build-locks/ is the only source of truth for uniqueness (atomic create). +# refs/build-runs// is a best-effort marker so a rerun can reuse its number; it is not authoritative. +# All ref reads/writes use the ambient GITHUB_TOKEN. +# LEGACY_PROPERTY_TOKEN (from Vault) is only used to read the legacy build_number property during migration - see README. set -euo pipefail -: "${GITHUB_REPOSITORY:?}" +: "${GITHUB_REPOSITORY:?}" "${GITHUB_SHA:?}" "${GITHUB_RUN_ID:?}" + GH_API_VERSION_HEADER="X-GitHub-Api-Version: 2022-11-28" BUILD_NUMBER_FILE="${BUILD_NUMBER_FILE:-.build_number.txt}" - -echo "Fetching build number from repository properties..." +REFS_API_URL="repos/${GITHUB_REPOSITORY}/git/refs" +MATCHING_REFS_API_URL="repos/${GITHUB_REPOSITORY}/git/matching-refs" 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 - echo "::error title=Invalid build number::Build number '${BUILD_NUMBER}' is not a valid positive integer." >&2 - exit 1 +LOCKS_NS="build-locks" +RUNS_NS="build-runs/${GITHUB_RUN_ID}" +MAX_ATTEMPTS="${MAX_ATTEMPTS:-100}" # retries when a concurrent claim beats us to the next number + +claim_ref() { + gh api --method POST -H "$GH_API_VERSION_HEADER" "$REFS_API_URL" -f "ref=refs/$1" -f "sha=${GITHUB_SHA}" 2>&1 +} + +echo "::group::Claim build number" +echo "::debug::Scanning refs/${LOCKS_NS}/* for the highest claimed build number" +LOCK_REFS=$(gh api --paginate -H "$GH_API_VERSION_HEADER" "${MATCHING_REFS_API_URL}/${LOCKS_NS}/" --jq '.[].ref') +MAX_CLAIMED=0 +if [[ -n "$LOCK_REFS" ]]; then + while IFS= read -r ref; do + [[ "$ref" =~ ^refs/${LOCKS_NS}/([0-9]+)$ ]] || continue + n="${BASH_REMATCH[1]}" + ((n > MAX_CLAIMED)) && MAX_CLAIMED=$n + done <<<"$LOCK_REFS" +fi + +if [[ "$MAX_CLAIMED" -eq 0 ]]; then + echo "::debug::No refs/${LOCKS_NS}/* found yet; checking the legacy build_number property as a migration seed" + LEGACY_BUILD_NUMBER=$(GH_TOKEN="${LEGACY_PROPERTY_TOKEN:-}" gh api -H "$GH_API_VERSION_HEADER" "$PROPERTIES_API_URL" \ + --jq '.[] | select(.property_name == "build_number") | .value') + if [[ -n "$LEGACY_BUILD_NUMBER" ]]; then + if ! [[ "$LEGACY_BUILD_NUMBER" =~ ^[0-9]+$ ]]; then + echo "::error title=Invalid build number::Legacy build_number property '${LEGACY_BUILD_NUMBER}' is not a valid positive integer." >&2 + exit 1 + fi + echo "Seeding from legacy build_number property: ${LEGACY_BUILD_NUMBER}" + MAX_CLAIMED=$((LEGACY_BUILD_NUMBER + 1000)) # Add a buffer to avoid collisions with legacy numbers + fi fi +echo "::debug::Highest known build number: ${MAX_CLAIMED}" + +attempt=1 +CANDIDATE=$((MAX_CLAIMED + 1)) +while true; do + RESPONSE=$(claim_ref "${LOCKS_NS}/${CANDIDATE}") && CLAIM_STATUS=0 || CLAIM_STATUS=$? + + if [[ "$CLAIM_STATUS" -eq 0 ]]; then + echo "Claimed build number ${CANDIDATE} (refs/${LOCKS_NS}/${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 "::debug::Build number ${CANDIDATE} already claimed; trying $((CANDIDATE + 1)) (attempt $((attempt + 1))/${MAX_ATTEMPTS})" + CANDIDATE=$((CANDIDATE + 1)) + attempt=$((attempt + 1)) +done + +claim_ref "${RUNS_NS}/${CANDIDATE}" >/dev/null || + echo "::warning title=Build number run-marker not recorded::Failed to record refs/${RUNS_NS}/${CANDIDATE}; a rerun of this workflow" \ + "run may claim a new build number instead of reusing this one." +echo "::endgroup::" -BUILD_NUMBER=$((BUILD_NUMBER + 1)) -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" +echo "${CANDIDATE}" >"$BUILD_NUMBER_FILE" diff --git a/spec/check_existing_claim_spec.sh b/spec/check_existing_claim_spec.sh new file mode 100644 index 00000000..bcbb1b78 --- /dev/null +++ b/spec/check_existing_claim_spec.sh @@ -0,0 +1,69 @@ +#!/bin/bash +eval "$(shellspec - -c) exit 1" + +export GITHUB_REPOSITORY="my org/my-repo" +export GITHUB_RUN_ID="123456789" +TEMP_DIR="${SHELLSPEC_TMPBASE:-/tmp}" +export BUILD_NUMBER_FILE="${TEMP_DIR}/build_number_existing_claim.txt" + +Mock gh + echo "gh $*" +End + +Describe 'check_existing_claim.sh' + BeforeEach 'rm -f "$BUILD_NUMBER_FILE"' + + It 'should not skip when no existing claim is found for this run' + GITHUB_OUTPUT="${TEMP_DIR}/github_output_none.txt" + export GITHUB_OUTPUT + : > "$GITHUB_OUTPUT" + Mock gh + echo '' + End + When run script get-build-number/check_existing_claim.sh + The status should be success + The output should include "No existing claim found for this workflow run" + The path "$BUILD_NUMBER_FILE" should not be file + The contents of file "$GITHUB_OUTPUT" should not include "skip=true" + End + + It 'should reuse the build number already claimed by this run and set skip=true' + GITHUB_OUTPUT="${TEMP_DIR}/github_output_found.txt" + export GITHUB_OUTPUT + : > "$GITHUB_OUTPUT" + Mock gh + echo "refs/build-runs/${GITHUB_RUN_ID}/7" + End + When run script get-build-number/check_existing_claim.sh + The status should be success + The output should include "Reusing build number 7" + The path "$BUILD_NUMBER_FILE" should be file + The contents of file "$BUILD_NUMBER_FILE" should equal "7" + The contents of file "$GITHUB_OUTPUT" should include "skip=true" + End + + It 'should deterministically pick the lowest number when multiple entries exist for this run' + GITHUB_OUTPUT="${TEMP_DIR}/github_output_multi.txt" + export GITHUB_OUTPUT + : > "$GITHUB_OUTPUT" + Mock gh + printf 'refs/build-runs/%s/9\nrefs/build-runs/%s/8\n' "$GITHUB_RUN_ID" "$GITHUB_RUN_ID" + End + When run script get-build-number/check_existing_claim.sh + The status should be success + The output should include "Reusing build number 8" + The contents of file "$BUILD_NUMBER_FILE" should equal "8" + End + + It 'should fail on an API error while listing existing claims' + GITHUB_OUTPUT="${TEMP_DIR}/github_output_error.txt" + export GITHUB_OUTPUT + : > "$GITHUB_OUTPUT" + Mock gh + echo '{"message":"Internal Server Error"}' >&2 + exit 1 + End + When run script get-build-number/check_existing_claim.sh + The status should be failure + End +End diff --git a/spec/get_build_number_spec.sh b/spec/get_build_number_spec.sh index 54222de8..48b4903c 100755 --- a/spec/get_build_number_spec.sh +++ b/spec/get_build_number_spec.sh @@ -2,6 +2,8 @@ eval "$(shellspec - -c) exit 1" export GITHUB_REPOSITORY="my org/my-repo" +export GITHUB_SHA="deadbeefcafef00dfeed" +export GITHUB_RUN_ID="123456789" TEMP_DIR="${SHELLSPEC_TMPBASE:-/tmp}" export BUILD_NUMBER_FILE="${TEMP_DIR}/build_number.txt" @@ -10,46 +12,154 @@ Mock gh End Describe 'get_build_number.sh' - It 'should increment and return the build number' + It 'should claim build number 1 when there are no locks and no legacy build_number property' Mock gh - if [[ "$*" =~ "api --method PATCH" ]]; then + if [[ "$*" == *"matching-refs/build-locks/"* ]]; then + echo '' + elif [[ "$*" == *"properties/values"* ]]; then + echo '' + else echo "gh $*" - elif [[ "$*" =~ "properties/values" ]]; then + fi + End + When run script get-build-number/get_build_number.sh + The output should include "Claimed build number 1" + The path "$BUILD_NUMBER_FILE" should be file + The contents of file "$BUILD_NUMBER_FILE" should equal "1" + End + + It 'should seed the starting candidate (with a safety gap) from the legacy property when no locks exist yet' + Mock gh + if [[ "$*" == *"matching-refs/build-locks/"* ]]; then + echo '' + 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 path "$BUILD_NUMBER_FILE" should be file - The contents of file "$BUILD_NUMBER_FILE" should equal "43" -# The variable BUILD_NUMBER should equal "43" + The output should include "Seeding from legacy build_number property: 42" + The output should include "Claimed build number 1043" + The contents of file "$BUILD_NUMBER_FILE" should equal "1043" + End + + It 'should claim the next number after the highest existing lock, ignoring gaps, without consulting the legacy property' + Mock gh + if [[ "$*" == *"matching-refs/build-locks/"* ]]; then + printf 'refs/build-locks/1\nrefs/build-locks/2\nrefs/build-locks/3\nrefs/build-locks/5\n' + elif [[ "$*" == *"properties/values"* ]]; then + echo '999' + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The output should include "Claimed build number 6" + The contents of file "$BUILD_NUMBER_FILE" should equal "6" End - It 'should return an error if BUILD_NUMBER is invalid' + It 'should return an error if the legacy build number property is invalid' Mock gh + if [[ "$*" == *"matching-refs/build-locks/"* ]]; then + echo '' + elif [[ "$*" == *"properties/values"* ]]; then echo 'notANumber' + else + echo "gh $*" + fi End When run script get-build-number/get_build_number.sh The status should be failure - The line 2 should equal "Current build number from repo: notANumber" - The stderr should include "::error title=Invalid build number::Build number 'notANumber'" + The stderr should include "::error title=Invalid build number::Legacy build_number property 'notANumber'" End - It 'should handle empty build number' + 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 [[ "$*" == *"matching-refs/build-locks/"* ]]; then echo '' + elif [[ "$*" == *"properties/values"* ]]; then + echo '42' + elif [[ "$*" == *"ref=refs/build-locks/"* ]]; 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 + 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 1043 already claimed; trying 1044" + The output should include "Claimed build number 1044" + The path "$BUILD_NUMBER_FILE" should be file + The contents of file "$BUILD_NUMBER_FILE" should equal "1044" + End + + It 'should fail after exhausting retries under permanent contention' + export MAX_ATTEMPTS=3 + Mock gh + if [[ "$*" == *"matching-refs/build-locks/"* ]]; then + echo '' + elif [[ "$*" == *"properties/values"* ]]; then + echo '42' + elif [[ "$*" == *"ref=refs/build-locks/"* ]]; then + echo '{"message":"Reference already exists"}' >&2 + exit 1 + 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 [[ "$*" == *"matching-refs/build-locks/"* ]]; then + echo '' + elif [[ "$*" == *"properties/values"* ]]; then + echo '42' + elif [[ "$*" == *"ref=refs/build-locks/"* ]]; then + echo '{"message":"Internal Server Error"}' >&2 + exit 1 + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be failure + The stderr should include "::error title=Build number claim failed::" + The stderr should include "Internal Server Error" + End + + It 'should still succeed even if recording the run marker fails' + Mock gh + if [[ "$*" == *"matching-refs/build-locks/"* ]]; then + echo '' + elif [[ "$*" == *"properties/values"* ]]; then + echo '42' + elif [[ "$*" == *"ref=refs/build-runs/"* ]]; then + exit 1 + elif [[ "$*" == *"ref=refs/build-locks/"* ]]; then + echo "gh $*" + 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 1043" + The output should include "Build number run-marker not recorded" + The contents of file "$BUILD_NUMBER_FILE" should equal "1043" End End From 7e4f48f6c3937b3f5571e03a006ee2ef93a7bdf8 Mon Sep 17 00:00:00 2001 From: Julien Carsique Date: Mon, 24 Aug 2026 18:59:51 +0200 Subject: [PATCH 2/3] PREQ-7781 Serialize same-run build number claims with a real lock check_existing_claim.sh was a plain read: if two jobs in the same workflow run both checked before either had claimed, both would independently claim different numbers under the same run_id - each individually valid, but not the intended "one run, one number" outcome. That's a lock-free CAS allocator (every caller races for a different key), not a mutex (one shared key, losers wait) - and the previous behavior on divergence was silent, not a failure. Renamed to acquire_run_lock.sh and rebuilt around a real, exclusive, one-shot lock: refs/build-run-locks/. The winner proceeds straight into get_build_number.sh (scan, claim, write the refs/build-runs// marker) with nothing else in between. A loser - whether a true concurrent race or a rerun replaying an already-acquired lock - polls for that marker and reuses its number. Reruns resolve near-instantly (the marker is already there); only genuine concurrent overlaps actually wait. A wait that times out fails loudly instead of falling back to an independent claim. Consequently, get_build_number.sh's marker write is now a hard failure rather than a best-effort warning, since other jobs may be blocked on it. This also resolves the "prefer using the output instead of calling it from distinct jobs" caveat - that limitation of the previous implementation is gone, so it's removed from test-build-number.yml and the README. Never delete refs/build-number/: an earlier revision of this commit kept only the latest ref, deleting superseded ones on the reasoning that the claim loop only ever scans forward from the current max. gitar-bot's review caught the actual race: a claim stalled between scanning and posting can resume after two other, faster claims have superseded and deleted the exact number it's about to post to, letting it succeed against that freed slot and duplicate an already-published number. There is no external authority that can rule this out (unlike the run-existence check below), so the ref is kept forever instead of bounded by a guess about how long an API call could stay in flight. Document contents: write as a breaking change (claiming now writes Git references with the calling workflow's own ambient token instead of a Vault-issued one) and have the claim failure message name the missing permission directly when the API denies access, instead of only dumping the raw response. Co-Authored-By: Claude Sonnet 4.5 --- .github/workflows/test-build-number.yml | 16 +- README.md | 27 +- get-build-number/action.yml | 23 +- get-build-number/check_existing_claim.sh | 36 --- get-build-number/get_build_number.sh | 169 +++++++++--- spec/check_existing_claim_spec.sh | 69 ----- spec/get_build_number_spec.sh | 312 +++++++++++++++++++---- 7 files changed, 439 insertions(+), 213 deletions(-) delete mode 100755 get-build-number/check_existing_claim.sh delete mode 100644 spec/check_existing_claim_spec.sh diff --git a/.github/workflows/test-build-number.yml b/.github/workflows/test-build-number.yml index 50d388ec..833e375e 100644 --- a/.github/workflows/test-build-number.yml +++ b/.github/workflows/test-build-number.yml @@ -61,14 +61,8 @@ jobs: run: | echo "BUILD_NUMBER: ${BUILD_NUMBER}" if [[ "${BUILD_NUMBER}" != "${{ needs.test-build-number-generation.outputs.BUILD_NUMBER }}" ]]; then - echo -e "::error title=test-build-number-reuse::Build number '${BUILD_NUMBER}' does not match the previous job build number" \ - "'${{ needs.test-build-number-generation.outputs.BUILD_NUMBER }}' despite it is the same workflow run.\n" \ - "Prefer using the output from SonarSource/ci-github-actions/get-build-number instead of calling it from distinct jobs." - # Enforced (PREQ-7781): reuse is now backed by strongly-consistent refs/build-runs//* - # instead of eventually-consistent actions/cache, which is why this was flaky before. This - # doesn't guarantee two jobs racing truly concurrently will converge (pre-existing, unchanged - # limitation - see get-build-number/check_existing_claim.sh) but a sequential same-run reuse - # like this one should now be reliable. + echo "::error title=test-build-number-reuse::Build number '${BUILD_NUMBER}' does not match the previous job build number" \ + "'${{ needs.test-build-number-generation.outputs.BUILD_NUMBER }}' despite it is the same workflow run." exit 1 fi @@ -88,10 +82,8 @@ jobs: run: | echo "BUILD_NUMBER: ${BUILD_NUMBER}" if [[ "${BUILD_NUMBER}" != "${{ needs.test-build-number-generation.outputs.BUILD_NUMBER }}" ]]; then - echo -e "::error title=test-build-number-reuse-from-cache-windows::Build number '${BUILD_NUMBER}' does not match the previous" \ - "job build number '${{ needs.test-build-number-generation.outputs.BUILD_NUMBER }}' despite it is the same workflow run.\n" \ - "Prefer using the output from SonarSource/ci-github-actions/get-build-number instead of calling it from distinct jobs." - # Enforced (PREQ-7781): see test-build-number-reuse-same-run above. + echo "::error title=test-build-number-reuse-same-run-windows::Build number '${BUILD_NUMBER}' does not match the previous" \ + "job build number '${{ needs.test-build-number-generation.outputs.BUILD_NUMBER }}' despite it is the same workflow run." exit 1 fi diff --git a/README.md b/README.md index ed74fee6..6dec5e29 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,10 @@ These badges show the status of workflows in dummy repositories that use (or sho Get a unique, strictly increasing build number for a repository, reusing one already claimed by the current workflow run when applicable. It sets `BUILD_NUMBER` as both an environment variable and a GitHub Actions output. Safe to call from multiple jobs in the same workflow -run, and from concurrent workflow runs (e.g. several GitHub Stacked PRs opened at once) - no two calls will ever return the same number. +run - only one of them claims a number, the others wait for it and reuse it - and from concurrent workflow runs (e.g. several GitHub +Stacked PRs opened at once), where no two runs will ever get the same number. A job waiting for another job's claim fails after a bounded +timeout (a few minutes) if that claim never completes, rather than claiming an independent number - see [Git References](#git-references) +below for how this is coordinated. During execution the action temporarily writes `.build_number.txt` at the repository root; the file is removed before the action completes. Do not track a file named `.build_number.txt` in your repository. @@ -84,6 +87,11 @@ completes. Do not track a file named `.build_number.txt` in your repository. - `id-token: write` - `contents: write` +> **Breaking change:** this action used to require only `contents: read`. Claiming now needs `contents: write` to create the +> [Git references](#git-references) below - `contents: read` alone will fail with a 403 on every claim except the narrow case where this +> exact workflow run already has a marker to reuse (e.g. certain reruns), since that path is read-only. There is no working +> read-only/rerun-only mode: any run that needs a genuinely new number will fail until the caller's `permissions:` block is updated. + #### Required Vault Permissions - `build-number`: GitHub preset used to read the legacy `build_number` repository property, needed only for repositories that predate this @@ -125,6 +133,23 @@ No inputs are required for this action. |----------------------|--------------------------| | `BUILD_NUMBER` | The current build number | +### Git References + +This action coordinates purely through Git references on the repository - no external state, cache, or database. Each reference is created +pointing at `$GITHUB_SHA` (the commit that triggered the claim); that target is never read back - only the reference's existence matters +(for `build-number` and `build-run-locks`), or its name, which encodes the claimed number (for `build-runs`). + +| Reference | Lifetime | Purpose | +|-------------------------------------|--------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `refs/build-number/` | Permanent, never deleted | The atomic claim itself and the sole source of truth for uniqueness. Creating it fails if it already exists, which is what makes a claim a genuine compare-and-swap. There is no safe way to delete these: unlike `build-runs` below, nothing external can ever confirm "no in-flight claim is still targeting this number" - a stalled claim for an older number could resurrect it after deletion, reopening a number already published elsewhere. A time-based margin (e.g. "not superseded for the last N minutes") would only lower the odds, not eliminate them, which isn't an acceptable trade for a bug class this action exists to remove. The unbounded growth this causes is a real but separate concern - see [Known limitations](#known-limitations) below. | +| `refs/build-runs//` | Not automatically deleted | Marker recording which number a workflow run claimed. Checked first, so a rerun or another job in the same run reuses it instead of claiming a new one. A completed run can still be re-run much later, so "the run finished" does not make its marker safe to delete - only the run's own record being gone from GitHub (confirmed via a 404, never inferred from age) does, since that's what makes re-running it impossible. | +| `refs/build-run-locks/` | Seconds: created, used, and deleted again within a single claim, not a persistent artifact | Exclusive lock ensuring only one job per workflow run claims and publishes a number at a time; every other job waits for the marker above instead of racing to claim its own. | + +### Known limitations + +- `refs/build-number/` is never deleted (see the table above), so the scan that finds the next candidate grows with the + repository's total historical claim count. Not expected to matter in practice for a long time; no bound is implemented today. + --- ## `config-maven` diff --git a/get-build-number/action.yml b/get-build-number/action.yml index 4df76d06..cb4bdb63 100644 --- a/get-build-number/action.yml +++ b/get-build-number/action.yml @@ -42,25 +42,18 @@ runs: echo "${BUILD_NUMBER}" > "$BUILD_NUMBER_FILE" echo "skip=true" >> $GITHUB_OUTPUT - # Reuse the build number already claimed by this workflow run (e.g. a rerun, or an earlier job in the same run) - - name: Check for existing build number from this workflow run - id: existing-claim - if: steps.from-env.outputs.skip != 'true' - shell: bash - env: - GITHUB_TOKEN: ${{ github.token }} - GH_TOKEN: ${{ github.token }} - run: ${ACTION_PATH_GET_BUILD_NUMBER}/check_existing_claim.sh - - # Otherwise, claim a new build number - # Vault is only needed to read the legacy build_number property during the migration to refs/build-locks/* + # Vault is only needed to read the legacy build_number property during migration to refs/build-number/* (see README); fetched + # unconditionally for simplicity, since this whole dependency is temporary and slated for removal once migration is complete. - uses: SonarSource/vault-action-wrapper@881045d830534a70ec3c7c275fa3714412c8ff6e # 3.6.1 id: secrets - if: steps.from-env.outputs.skip != 'true' && steps.existing-claim.outputs.skip != 'true' + if: steps.from-env.outputs.skip != 'true' with: secrets: development/github/token/{REPO_OWNER_NAME_DASH}-build-number token | github_token; - - name: Get new build number - if: steps.from-env.outputs.skip != 'true' && steps.existing-claim.outputs.skip != 'true' + + # Reuses this run's own claim if one already exists (a rerun, or another job in the same run); otherwise claims a new one under + # an exclusive per-run lock, released as soon as it's no longer needed - see get_build_number.sh. + - name: Get build number + if: steps.from-env.outputs.skip != 'true' shell: bash env: GITHUB_TOKEN: ${{ github.token }} diff --git a/get-build-number/check_existing_claim.sh b/get-build-number/check_existing_claim.sh deleted file mode 100755 index 76a66446..00000000 --- a/get-build-number/check_existing_claim.sh +++ /dev/null @@ -1,36 +0,0 @@ -#!/bin/bash -# Check whether this workflow run already claimed a build number (e.g. a rerun, or another job in the same run) and reuse it if so. -# Read-only check against refs/build-runs//*; see get_build_number.sh for the actual claim. - -set -euo pipefail - -: "${GITHUB_REPOSITORY:?}" "${GITHUB_RUN_ID:?}" "${GITHUB_OUTPUT:?}" - -GH_API_VERSION_HEADER="X-GitHub-Api-Version: 2022-11-28" -BUILD_NUMBER_FILE="${BUILD_NUMBER_FILE:-.build_number.txt}" -MATCHING_REFS_API_URL="repos/${GITHUB_REPOSITORY}/git/matching-refs" -RUNS_NS="build-runs/${GITHUB_RUN_ID}" - -echo "::group::Check for an existing build number claim from this workflow run" -echo "::debug::Checking for an existing claim by this workflow run (refs/${RUNS_NS}/*)" -RUN_CLAIMS=$(gh api -H "$GH_API_VERSION_HEADER" "${MATCHING_REFS_API_URL}/${RUNS_NS}/" --jq '.[].ref') -EXISTING="" -if [[ -n "$RUN_CLAIMS" ]]; then - while IFS= read -r ref; do - [[ "$ref" =~ ^refs/${RUNS_NS}/([0-9]+)$ ]] || continue - n="${BASH_REMATCH[1]}" - # TODO PREQ-7781: two jobs racing here before either writes a marker can each claim a different number; this just picks the lowest. - if [[ -z "$EXISTING" ]] || ((n < EXISTING)); then - EXISTING="$n" - fi - done <<<"$RUN_CLAIMS" -fi - -if [[ -n "$EXISTING" ]]; then - echo "::debug::Reusing build number ${EXISTING}, already claimed by this workflow run (refs/${RUNS_NS}/${EXISTING})" - echo "${EXISTING}" >"$BUILD_NUMBER_FILE" - echo "skip=true" >>"$GITHUB_OUTPUT" -else - echo "::debug::No existing claim found for this workflow run" -fi -echo "::endgroup::" diff --git a/get-build-number/get_build_number.sh b/get-build-number/get_build_number.sh index 35d6f3de..2e9269a2 100755 --- a/get-build-number/get_build_number.sh +++ b/get-build-number/get_build_number.sh @@ -1,10 +1,18 @@ #!/bin/bash -# Get the build number for a GitHub repository and save the incremented value to .build_number.txt -# See check_existing_claim.sh for the read-only check for an existing claim by this workflow run. -# refs/build-locks/ is the only source of truth for uniqueness (atomic create). -# refs/build-runs// is a best-effort marker so a rerun can reuse its number; it is not authoritative. -# All ref reads/writes use the ambient GITHUB_TOKEN. -# LEGACY_PROPERTY_TOKEN (from Vault) is only used to read the legacy build_number property during migration - see README. +# Get the build number for a GitHub repository and save it to .build_number.txt, reusing one already claimed by this workflow run. +# +# refs/build-number/: the atomic claim itself, the sole source of truth for uniqueness. Never deleted: a claim in flight for +# candidate N can stall arbitrarily long before its POST executes, and if some other, older N had since been deleted, that stale +# POST would succeed against the now-free slot - reopening an already-published number. Pruning (out-of-band, not here) may only +# ever remove refs so far below the current claim that no in-flight run could plausibly still be targeting them. +# refs/build-runs//: marker recording which number this workflow run claimed. Checked first, so reruns and other jobs in +# the same run reuse it instead of racing to claim their own. +# refs/build-run-locks/: exclusive, transient lock serializing "check the marker, then claim and publish one" for this run. +# Released immediately after use (success or failure) via the trap below, not held for the run's lifetime. +# +# Every ref points at $GITHUB_SHA purely because creation requires some valid target; that target is never read back. +# All ref reads/writes use the ambient GITHUB_TOKEN. LEGACY_PROPERTY_TOKEN (from Vault) is only used to read the legacy +# build_number property during migration - see README. set -euo pipefail @@ -15,37 +23,129 @@ BUILD_NUMBER_FILE="${BUILD_NUMBER_FILE:-.build_number.txt}" REFS_API_URL="repos/${GITHUB_REPOSITORY}/git/refs" MATCHING_REFS_API_URL="repos/${GITHUB_REPOSITORY}/git/matching-refs" PROPERTIES_API_URL="repos/${GITHUB_REPOSITORY}/properties/values" -LOCKS_NS="build-locks" -RUNS_NS="build-runs/${GITHUB_RUN_ID}" -MAX_ATTEMPTS="${MAX_ATTEMPTS:-100}" # retries when a concurrent claim beats us to the next number +PROPS_JQ='.[] | select(.property_name == "build_number") | .value' +NUMBER_NS="build-number" +RUN_NS="build-runs/${GITHUB_RUN_ID}" +RUN_LOCK_REF="build-run-locks/${GITHUB_RUN_ID}" +LOCK_POLL_INTERVAL_SECONDS="${LOCK_POLL_INTERVAL_SECONDS:-3}" +LOCK_WAIT_MAX_ATTEMPTS="${LOCK_WAIT_MAX_ATTEMPTS:-40}" # ~2 minutes at the default interval +MAX_ATTEMPTS="${MAX_ATTEMPTS:-100}" # retries when a concurrent run beats us to the next number -claim_ref() { - gh api --method POST -H "$GH_API_VERSION_HEADER" "$REFS_API_URL" -f "ref=refs/$1" -f "sha=${GITHUB_SHA}" 2>&1 +create_ref() { + local ref="$1" + gh api --method POST -H "$GH_API_VERSION_HEADER" "$REFS_API_URL" -f "ref=refs/${ref}" -f "sha=${GITHUB_SHA}" 2>&1 } -echo "::group::Claim build number" -echo "::debug::Scanning refs/${LOCKS_NS}/* for the highest claimed build number" -LOCK_REFS=$(gh api --paginate -H "$GH_API_VERSION_HEADER" "${MATCHING_REFS_API_URL}/${LOCKS_NS}/" --jq '.[].ref') +delete_ref() { + local ref="$1" + gh api --method DELETE -H "$GH_API_VERSION_HEADER" "${REFS_API_URL}/${ref}" >/dev/null 2>&1 +} + +# Reports a fatal ref-creation failure, calling out the common "caller still has contents: read" cause by name instead of only +# dumping the raw API response, since that response alone reads as a generic permission error, not a fix. +fail_claim() { + local response="$1" + if [[ "$response" == *"Resource not accessible by integration"* ]]; then + echo "::error title=Build number claim failed::${response} This action requires 'contents: write' in the calling workflow's" \ + "permissions (contents: read is no longer sufficient)." >&2 + else + echo "::error title=Build number claim failed::${response}" >&2 + fi + exit 1 +} + +find_run_marker() { + local output + # Tolerate a transient/network failure of this specific call: treat it the same as "no marker yet" and retry on the next poll, + # but log it so a *permanent* failure (e.g. an auth error) is visible instead of only ever surfacing as a wait timeout. + if ! output=$(gh api -H "$GH_API_VERSION_HEADER" "${MATCHING_REFS_API_URL}/${RUN_NS}/" --jq '.[0].ref // empty' 2>&1); then + echo "::debug::Marker check failed, treating as not-yet-published and retrying: ${output}" >&2 + return 0 + fi + echo "$output" +} + +# Exits 0 (and tells the caller to stop) if this run already has a claim; exits 1 on a malformed marker. +try_reuse_existing_claim() { + local marker existing + marker=$(find_run_marker) + [[ -n "$marker" ]] || return 1 + if ! [[ "$marker" =~ ^refs/${RUN_NS}/([0-9]+)$ ]]; then + echo "::error title=Build number claim failed::Unexpected ref format: ${marker}" >&2 + exit 1 + fi + existing="${BASH_REMATCH[1]}" + echo "Reusing build number ${existing}, already claimed by this workflow run (refs/${RUN_NS}/${existing})" + echo "${existing}" >"$BUILD_NUMBER_FILE" +} + +HELD_LOCK="" +release_lock() { + [[ -n "$HELD_LOCK" ]] || return 0 + delete_ref "$RUN_LOCK_REF" || echo "::warning title=Build number lock not released::Failed to delete refs/${RUN_LOCK_REF}; a" \ + "later attempt for this workflow run may have to wait out its timeout before claiming a number." >&2 +} +trap release_lock EXIT + +echo "::group::Get build number" + +attempt=1 +while true; do + try_reuse_existing_claim && { echo "::endgroup::" && exit 0; } + + RESPONSE=$(create_ref "$RUN_LOCK_REF") && LOCK_STATUS=0 || LOCK_STATUS=$? + if [[ "$LOCK_STATUS" -eq 0 ]]; then + HELD_LOCK=1 + break + fi + if [[ "$RESPONSE" != *"Reference already exists"* ]]; then + fail_claim "$RESPONSE" + fi + + if ((attempt >= LOCK_WAIT_MAX_ATTEMPTS)); then + echo "::error title=Build number claim timed out::Waited ${LOCK_WAIT_MAX_ATTEMPTS} attempts for refs/${RUN_NS}/* to appear; the" \ + "job holding refs/${RUN_LOCK_REF} may have failed before publishing its claim." >&2 + exit 1 + fi + echo "::debug::refs/${RUN_LOCK_REF} already held; waiting for its marker (attempt $((attempt + 1))/${LOCK_WAIT_MAX_ATTEMPTS})" + sleep "$LOCK_POLL_INTERVAL_SECONDS" + attempt=$((attempt + 1)) +done + +# We now hold the lock, but someone else may have finished between our last check above and acquiring it. +try_reuse_existing_claim && { echo "::endgroup::" && exit 0; } + +# O(total historical claims), not O(1) - see README's Known limitations. A cheaper alternative (e.g. binary search over +# individual ref lookups) needs claimed numbers to have no permanent gaps, which the migration seed below can violate: two +# different runs seeding from the legacy property concurrently can each claim a distinct, non-adjacent number (see the +# "ignoring gaps" test) - so a gap can't be assumed away, and this scan can't be replaced without addressing that first. +echo "::debug::Scanning refs/${NUMBER_NS}/* for the highest claimed build number" +NUMBER_REFS=$(gh api --paginate -H "$GH_API_VERSION_HEADER" "${MATCHING_REFS_API_URL}/${NUMBER_NS}/" --jq '.[].ref') MAX_CLAIMED=0 -if [[ -n "$LOCK_REFS" ]]; then +if [[ -n "$NUMBER_REFS" ]]; then while IFS= read -r ref; do - [[ "$ref" =~ ^refs/${LOCKS_NS}/([0-9]+)$ ]] || continue + [[ "$ref" =~ ^refs/${NUMBER_NS}/([0-9]+)$ ]] || continue n="${BASH_REMATCH[1]}" ((n > MAX_CLAIMED)) && MAX_CLAIMED=$n - done <<<"$LOCK_REFS" + done <<<"$NUMBER_REFS" fi if [[ "$MAX_CLAIMED" -eq 0 ]]; then - echo "::debug::No refs/${LOCKS_NS}/* found yet; checking the legacy build_number property as a migration seed" - LEGACY_BUILD_NUMBER=$(GH_TOKEN="${LEGACY_PROPERTY_TOKEN:-}" gh api -H "$GH_API_VERSION_HEADER" "$PROPERTIES_API_URL" \ - --jq '.[] | select(.property_name == "build_number") | .value') - if [[ -n "$LEGACY_BUILD_NUMBER" ]]; then - if ! [[ "$LEGACY_BUILD_NUMBER" =~ ^[0-9]+$ ]]; then - echo "::error title=Invalid build number::Legacy build_number property '${LEGACY_BUILD_NUMBER}' is not a valid positive integer." >&2 - exit 1 + if [[ -z "${LEGACY_PROPERTY_TOKEN:-}" ]]; then + echo "::warning title=Legacy build number not checked::No refs/${NUMBER_NS}/* exist yet and no migration token is available;" \ + "starting from 1. If this repository has a legacy build_number property, its numbers will be reused." >&2 + else + echo "::debug::No refs/${NUMBER_NS}/* found yet; checking the legacy build_number property as a migration seed" + LEGACY_BUILD_NUMBER=$(GH_TOKEN="${LEGACY_PROPERTY_TOKEN}" gh api -H "$GH_API_VERSION_HEADER" "$PROPERTIES_API_URL" --jq "$PROPS_JQ") + if [[ -n "$LEGACY_BUILD_NUMBER" ]]; then + if ! [[ "$LEGACY_BUILD_NUMBER" =~ ^[0-9]+$ ]]; then + echo "::error title=Invalid build number::Legacy build_number property '${LEGACY_BUILD_NUMBER}' is not a valid positive" \ + "integer." >&2 + exit 1 + fi + echo "Seeding from legacy build_number property: ${LEGACY_BUILD_NUMBER}" + MAX_CLAIMED=$((LEGACY_BUILD_NUMBER + 1000)) # buffer against the legacy property still advancing elsewhere during migration fi - echo "Seeding from legacy build_number property: ${LEGACY_BUILD_NUMBER}" - MAX_CLAIMED=$((LEGACY_BUILD_NUMBER + 1000)) # Add a buffer to avoid collisions with legacy numbers fi fi echo "::debug::Highest known build number: ${MAX_CLAIMED}" @@ -53,16 +153,15 @@ echo "::debug::Highest known build number: ${MAX_CLAIMED}" attempt=1 CANDIDATE=$((MAX_CLAIMED + 1)) while true; do - RESPONSE=$(claim_ref "${LOCKS_NS}/${CANDIDATE}") && CLAIM_STATUS=0 || CLAIM_STATUS=$? + RESPONSE=$(create_ref "${NUMBER_NS}/${CANDIDATE}") && CLAIM_STATUS=0 || CLAIM_STATUS=$? if [[ "$CLAIM_STATUS" -eq 0 ]]; then - echo "Claimed build number ${CANDIDATE} (refs/${LOCKS_NS}/${CANDIDATE})" + echo "Claimed build number ${CANDIDATE} (refs/${NUMBER_NS}/${CANDIDATE})" break fi if [[ "$RESPONSE" != *"Reference already exists"* ]]; then - echo "::error title=Build number claim failed::${RESPONSE}" >&2 - exit 1 + fail_claim "$RESPONSE" fi if ((attempt >= MAX_ATTEMPTS)); then @@ -75,9 +174,11 @@ while true; do attempt=$((attempt + 1)) done -claim_ref "${RUNS_NS}/${CANDIDATE}" >/dev/null || - echo "::warning title=Build number run-marker not recorded::Failed to record refs/${RUNS_NS}/${CANDIDATE}; a rerun of this workflow" \ - "run may claim a new build number instead of reusing this one." -echo "::endgroup::" +if ! create_ref "${RUN_NS}/${CANDIDATE}" >/dev/null; then + echo "::error title=Build number claim failed::Failed to record refs/${RUN_NS}/${CANDIDATE}; other jobs/reruns of this workflow" \ + "run waiting on refs/${RUN_LOCK_REF} would otherwise time out instead of reusing it." >&2 + exit 1 +fi +echo "::endgroup::" echo "${CANDIDATE}" >"$BUILD_NUMBER_FILE" diff --git a/spec/check_existing_claim_spec.sh b/spec/check_existing_claim_spec.sh deleted file mode 100644 index bcbb1b78..00000000 --- a/spec/check_existing_claim_spec.sh +++ /dev/null @@ -1,69 +0,0 @@ -#!/bin/bash -eval "$(shellspec - -c) exit 1" - -export GITHUB_REPOSITORY="my org/my-repo" -export GITHUB_RUN_ID="123456789" -TEMP_DIR="${SHELLSPEC_TMPBASE:-/tmp}" -export BUILD_NUMBER_FILE="${TEMP_DIR}/build_number_existing_claim.txt" - -Mock gh - echo "gh $*" -End - -Describe 'check_existing_claim.sh' - BeforeEach 'rm -f "$BUILD_NUMBER_FILE"' - - It 'should not skip when no existing claim is found for this run' - GITHUB_OUTPUT="${TEMP_DIR}/github_output_none.txt" - export GITHUB_OUTPUT - : > "$GITHUB_OUTPUT" - Mock gh - echo '' - End - When run script get-build-number/check_existing_claim.sh - The status should be success - The output should include "No existing claim found for this workflow run" - The path "$BUILD_NUMBER_FILE" should not be file - The contents of file "$GITHUB_OUTPUT" should not include "skip=true" - End - - It 'should reuse the build number already claimed by this run and set skip=true' - GITHUB_OUTPUT="${TEMP_DIR}/github_output_found.txt" - export GITHUB_OUTPUT - : > "$GITHUB_OUTPUT" - Mock gh - echo "refs/build-runs/${GITHUB_RUN_ID}/7" - End - When run script get-build-number/check_existing_claim.sh - The status should be success - The output should include "Reusing build number 7" - The path "$BUILD_NUMBER_FILE" should be file - The contents of file "$BUILD_NUMBER_FILE" should equal "7" - The contents of file "$GITHUB_OUTPUT" should include "skip=true" - End - - It 'should deterministically pick the lowest number when multiple entries exist for this run' - GITHUB_OUTPUT="${TEMP_DIR}/github_output_multi.txt" - export GITHUB_OUTPUT - : > "$GITHUB_OUTPUT" - Mock gh - printf 'refs/build-runs/%s/9\nrefs/build-runs/%s/8\n' "$GITHUB_RUN_ID" "$GITHUB_RUN_ID" - End - When run script get-build-number/check_existing_claim.sh - The status should be success - The output should include "Reusing build number 8" - The contents of file "$BUILD_NUMBER_FILE" should equal "8" - End - - It 'should fail on an API error while listing existing claims' - GITHUB_OUTPUT="${TEMP_DIR}/github_output_error.txt" - export GITHUB_OUTPUT - : > "$GITHUB_OUTPUT" - Mock gh - echo '{"message":"Internal Server Error"}' >&2 - exit 1 - End - When run script get-build-number/check_existing_claim.sh - The status should be failure - End -End diff --git a/spec/get_build_number_spec.sh b/spec/get_build_number_spec.sh index 48b4903c..87ea73c2 100755 --- a/spec/get_build_number_spec.sh +++ b/spec/get_build_number_spec.sh @@ -4,6 +4,7 @@ eval "$(shellspec - -c) exit 1" export GITHUB_REPOSITORY="my org/my-repo" export GITHUB_SHA="deadbeefcafef00dfeed" export GITHUB_RUN_ID="123456789" +export LOCK_POLL_INTERVAL_SECONDS=0 TEMP_DIR="${SHELLSPEC_TMPBASE:-/tmp}" export BUILD_NUMBER_FILE="${TEMP_DIR}/build_number.txt" @@ -11,26 +12,58 @@ Mock gh echo "gh $*" End +remove_build_number_file() { + rm -f "$BUILD_NUMBER_FILE" + return 0 +} + Describe 'get_build_number.sh' - It 'should claim build number 1 when there are no locks and no legacy build_number property' + BeforeEach 'remove_build_number_file' + + It 'should reuse an existing claim immediately, without touching the lock or scanning build-number refs' + export GH_LOCK_CALLS_FILE="${TEMP_DIR}/gh_lock_calls_reuse.txt" + rm -f "$GH_LOCK_CALLS_FILE" Mock gh - if [[ "$*" == *"matching-refs/build-locks/"* ]]; then + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then + echo "refs/build-runs/${GITHUB_RUN_ID}/5" + elif [[ "$*" == *"build-run-locks/"* ]]; then + echo "1" >>"$GH_LOCK_CALLS_FILE" + echo "gh $*" + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be success + The output should include "Reusing build number 5" + The contents of file "$BUILD_NUMBER_FILE" should equal "5" + The path "$GH_LOCK_CALLS_FILE" should not be file + End + + It 'should claim build number 1 and warn when there are no build-number refs and no migration token' + Mock gh + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then echo '' - elif [[ "$*" == *"properties/values"* ]]; then + elif [[ "$*" == *"matching-refs/build-number/"* ]]; then echo '' 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 1" - The path "$BUILD_NUMBER_FILE" should be file + The output should not include "checking the legacy build_number property" + The stderr should include "::warning title=Legacy build number not checked::" The contents of file "$BUILD_NUMBER_FILE" should equal "1" End - It 'should seed the starting candidate (with a safety gap) from the legacy property when no locks exist yet' + It 'should seed the starting candidate (with a safety buffer) from the legacy property when no build-number refs exist yet' + export LEGACY_PROPERTY_TOKEN="vault-issued-token-placeholder" Mock gh - if [[ "$*" == *"matching-refs/build-locks/"* ]]; then + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then + echo '' + elif [[ "$*" == *"matching-refs/build-number/"* ]]; then echo '' elif [[ "$*" == *"properties/values"* ]]; then echo '42' @@ -39,52 +72,64 @@ Describe 'get_build_number.sh' fi End When run script get-build-number/get_build_number.sh + The status should be success The output should include "Seeding from legacy build_number property: 42" The output should include "Claimed build number 1043" The contents of file "$BUILD_NUMBER_FILE" should equal "1043" End - It 'should claim the next number after the highest existing lock, ignoring gaps, without consulting the legacy property' + It 'should return an error if the legacy build number property is invalid' + export LEGACY_PROPERTY_TOKEN="vault-issued-token-placeholder" Mock gh - if [[ "$*" == *"matching-refs/build-locks/"* ]]; then - printf 'refs/build-locks/1\nrefs/build-locks/2\nrefs/build-locks/3\nrefs/build-locks/5\n' + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then + echo '' + elif [[ "$*" == *"matching-refs/build-number/"* ]]; then + echo '' elif [[ "$*" == *"properties/values"* ]]; then - echo '999' + echo 'notANumber' else echo "gh $*" fi End When run script get-build-number/get_build_number.sh - The output should include "Claimed build number 6" - The contents of file "$BUILD_NUMBER_FILE" should equal "6" + The status should be failure + The output should include "::group::Get build number" + The stderr should include "::error title=Invalid build number::Legacy build_number property 'notANumber'" End - It 'should return an error if the legacy build number property is invalid' + It 'should claim the next number after the highest existing build-number ref, ignoring gaps, and never delete any of them' + export GH_DELETE_CALLS_FILE="${TEMP_DIR}/gh_delete_calls_number.txt" + rm -f "$GH_DELETE_CALLS_FILE" Mock gh - if [[ "$*" == *"matching-refs/build-locks/"* ]]; then + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then echo '' - elif [[ "$*" == *"properties/values"* ]]; then - echo 'notANumber' + elif [[ "$*" == *"matching-refs/build-number/"* ]]; then + printf 'refs/build-number/1\nrefs/build-number/2\nrefs/build-number/3\nrefs/build-number/5\n' + elif [[ "$*" == *"--method DELETE"* && "$*" == *"build-number/"* ]]; then + echo "$*" >>"$GH_DELETE_CALLS_FILE" + echo "gh $*" else echo "gh $*" fi End When run script get-build-number/get_build_number.sh - The status should be failure - The stderr should include "::error title=Invalid build number::Legacy build_number property 'notANumber'" + The status should be success + The output should include "Claimed build number 6" + The contents of file "$BUILD_NUMBER_FILE" should equal "6" + The path "$GH_DELETE_CALLS_FILE" should not be file End - It 'should retry when a concurrent run already claimed the next number, then succeed' + It 'should retry when a concurrent run claims the same candidate first, then succeed' export GH_REFS_CALLS_FILE="${TEMP_DIR}/gh_refs_calls_retry.txt" rm -f "$GH_REFS_CALLS_FILE" Mock gh - if [[ "$*" == *"matching-refs/build-locks/"* ]]; then + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then echo '' - elif [[ "$*" == *"properties/values"* ]]; then - echo '42' - elif [[ "$*" == *"ref=refs/build-locks/"* ]]; then + elif [[ "$*" == *"matching-refs/build-number/"* ]]; then + echo '' + elif [[ "$*" == *"ref=refs/build-number/"* ]]; then count=$(($(cat "$GH_REFS_CALLS_FILE" 2>/dev/null || echo 0) + 1)) - echo "$count" > "$GH_REFS_CALLS_FILE" + echo "$count" >"$GH_REFS_CALLS_FILE" if [[ "$count" -eq 1 ]]; then echo '{"message":"Reference already exists"}' >&2 exit 1 @@ -97,20 +142,20 @@ Describe 'get_build_number.sh' End When run script get-build-number/get_build_number.sh The status should be success - The output should include "Build number 1043 already claimed; trying 1044" - The output should include "Claimed build number 1044" - The path "$BUILD_NUMBER_FILE" should be file - The contents of file "$BUILD_NUMBER_FILE" should equal "1044" + The output should include "Build number 1 already claimed; trying 2" + The output should include "Claimed build number 2" + The stderr should include "::warning title=Legacy build number not checked::" + The contents of file "$BUILD_NUMBER_FILE" should equal "2" End - It 'should fail after exhausting retries under permanent contention' + It 'should fail after exhausting retries under permanent contention on the build-number namespace' export MAX_ATTEMPTS=3 Mock gh - if [[ "$*" == *"matching-refs/build-locks/"* ]]; then + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then echo '' - elif [[ "$*" == *"properties/values"* ]]; then - echo '42' - elif [[ "$*" == *"ref=refs/build-locks/"* ]]; then + elif [[ "$*" == *"matching-refs/build-number/"* ]]; then + echo '' + elif [[ "$*" == *"ref=refs/build-number/"* ]]; then echo '{"message":"Reference already exists"}' >&2 exit 1 else @@ -123,13 +168,13 @@ Describe 'get_build_number.sh' 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' + It 'should fail immediately on an unexpected API error while claiming, not treat it as a collision' Mock gh - if [[ "$*" == *"matching-refs/build-locks/"* ]]; then + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then echo '' - elif [[ "$*" == *"properties/values"* ]]; then - echo '42' - elif [[ "$*" == *"ref=refs/build-locks/"* ]]; then + elif [[ "$*" == *"matching-refs/build-number/"* ]]; then + echo '' + elif [[ "$*" == *"ref=refs/build-number/"* ]]; then echo '{"message":"Internal Server Error"}' >&2 exit 1 else @@ -138,28 +183,203 @@ Describe 'get_build_number.sh' End When run script get-build-number/get_build_number.sh The status should be failure + The output should include "::group::Get build number" The stderr should include "::error title=Build number claim failed::" The stderr should include "Internal Server Error" End - It 'should still succeed even if recording the run marker fails' + It 'should call out the contents:write permission requirement when the API denies access' Mock gh - if [[ "$*" == *"matching-refs/build-locks/"* ]]; then + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then + echo '' + elif [[ "$*" == *"matching-refs/build-number/"* ]]; then + echo '' + elif [[ "$*" == *"ref=refs/build-number/"* ]]; then + echo '{"message":"Resource not accessible by integration"}' >&2 + exit 1 + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be failure + The output should include "::group::Get build number" + The stderr should include "Resource not accessible by integration" + The stderr should include "requires 'contents: write'" + End + + It 'should fail if recording the run marker fails, since other jobs/reruns may be waiting on it' + Mock gh + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then + echo '' + elif [[ "$*" == *"matching-refs/build-number/"* ]]; then echo '' - elif [[ "$*" == *"properties/values"* ]]; then - echo '42' elif [[ "$*" == *"ref=refs/build-runs/"* ]]; then exit 1 - elif [[ "$*" == *"ref=refs/build-locks/"* ]]; then + elif [[ "$*" == *"ref=refs/build-number/"* ]]; then + echo "gh $*" + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be failure + The output should include "Claimed build number 1" + The stderr should include "::error title=Build number claim failed::Failed to record" + The path "$BUILD_NUMBER_FILE" should not be file + End + + It 'should release the lock even when the claim ultimately fails' + export GH_UNLOCK_CALLS_FILE="${TEMP_DIR}/gh_unlock_calls_failure.txt" + rm -f "$GH_UNLOCK_CALLS_FILE" + Mock gh + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then + echo '' + elif [[ "$*" == *"matching-refs/build-number/"* ]]; then + echo '' + elif [[ "$*" == *"ref=refs/build-number/"* ]]; then + echo '{"message":"Internal Server Error"}' >&2 + exit 1 + elif [[ "$*" == *"--method DELETE"* && "$*" == *"build-run-locks/"* ]]; then + echo "1" >>"$GH_UNLOCK_CALLS_FILE" + echo "gh $*" + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be failure + The output should include "::group::Get build number" + The stderr should include "::error title=Build number claim failed::" + The path "$GH_UNLOCK_CALLS_FILE" should be file + End + + It 'should wait for the lock holder to publish a marker, then reuse it' + export GH_MARKER_CALLS_FILE="${TEMP_DIR}/gh_marker_calls_poll.txt" + rm -f "$GH_MARKER_CALLS_FILE" + Mock gh + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then + count=$(($(cat "$GH_MARKER_CALLS_FILE" 2>/dev/null || echo 0) + 1)) + echo "$count" >"$GH_MARKER_CALLS_FILE" + if [[ "$count" -lt 3 ]]; then + echo '' + else + echo "refs/build-runs/${GITHUB_RUN_ID}/9" + fi + elif [[ "$*" == *"ref=refs/build-run-locks/"* ]]; then + echo '{"message":"Reference already exists"}' >&2 + exit 1 + else echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be success + The output should include "Reusing build number 9" + The contents of file "$BUILD_NUMBER_FILE" should equal "9" + End + + It 'should tolerate a transient error while polling for the marker and retry instead of aborting the wait' + export GH_MARKER_CALLS_FILE="${TEMP_DIR}/gh_marker_calls_transient.txt" + rm -f "$GH_MARKER_CALLS_FILE" + Mock gh + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then + count=$(($(cat "$GH_MARKER_CALLS_FILE" 2>/dev/null || echo 0) + 1)) + echo "$count" >"$GH_MARKER_CALLS_FILE" + if [[ "$count" -eq 1 ]]; then + echo '{"message":"Internal Server Error"}' >&2 + exit 1 + elif [[ "$count" -lt 3 ]]; then + echo '' + else + echo "refs/build-runs/${GITHUB_RUN_ID}/11" + fi + elif [[ "$*" == *"ref=refs/build-run-locks/"* ]]; then + echo '{"message":"Reference already exists"}' >&2 + exit 1 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 1043" - The output should include "Build number run-marker not recorded" - The contents of file "$BUILD_NUMBER_FILE" should equal "1043" + The output should include "Reusing build number 11" + The stderr should include "Internal Server Error" + The contents of file "$BUILD_NUMBER_FILE" should equal "11" + End + + It 'should fail after exhausting the wait if the lock holder never publishes a marker' + export LOCK_WAIT_MAX_ATTEMPTS=3 + Mock gh + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then + echo '' + elif [[ "$*" == *"ref=refs/build-run-locks/"* ]]; then + echo '{"message":"Reference already exists"}' >&2 + exit 1 + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be failure + The output should include "::group::Get build number" + The stderr should include "::error title=Build number claim timed out::Waited 3 attempts" + End + + It 'should fail immediately on an unexpected error acquiring the lock, not treat it as a collision' + Mock gh + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then + echo '' + elif [[ "$*" == *"ref=refs/build-run-locks/"* ]]; then + echo '{"message":"Internal Server Error"}' >&2 + exit 1 + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be failure + The output should include "::group::Get build number" + The stderr should include "::error title=Build number claim failed::" + The stderr should include "Internal Server Error" + End + + It 'should reuse the marker if it appears between winning the lock and scanning build-number refs' + export GH_MARKER_CALLS_FILE="${TEMP_DIR}/gh_marker_calls_race.txt" + rm -f "$GH_MARKER_CALLS_FILE" + Mock gh + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then + count=$(($(cat "$GH_MARKER_CALLS_FILE" 2>/dev/null || echo 0) + 1)) + echo "$count" >"$GH_MARKER_CALLS_FILE" + if [[ "$count" -eq 1 ]]; then + echo '' + else + echo "refs/build-runs/${GITHUB_RUN_ID}/13" + fi + elif [[ "$*" == *"matching-refs/build-number/"* ]]; then + echo "::error title=test-bug::build-number should never be scanned in this scenario" >&2 + exit 1 + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be success + The output should include "Reusing build number 13" + The contents of file "$BUILD_NUMBER_FILE" should equal "13" + End + + It 'should fail if a marker ref has an unexpected format' + Mock gh + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then + echo "refs/build-runs/${GITHUB_RUN_ID}/notanumber" + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be failure + The output should include "::group::Get build number" + The stderr should include "::error title=Build number claim failed::Unexpected ref format" End End From cf0354f7dde70957be6d34176117ed45b7605a3b Mon Sep 17 00:00:00 2001 From: Julien Carsique Date: Tue, 25 Aug 2026 17:35:03 +0200 Subject: [PATCH 3/3] PREQ-7781 Serialize all claims to allow deleting build numbers refs/build-number/ was never deleted: without exclusive access, a claim stalled in flight for an older candidate could resume after two other, faster claims had superseded and deleted the exact number it was about to post to, letting it succeed against that freed slot and duplicate an already-published number - the ABA bug fixed two commits ago by simply never deleting anything. That traded correctness for an unbounded matching-refs scan that grows with the repository's total historical claim count. Add refs/build-number-lock/global: an exclusive, repository-wide, transient lock (same create/trap-release pattern as the marker write) serializing every new claim, not just those within one run. Under it, no other claim can be scanning, posting, or deleting at the same time, so deleting a superseded build-number ref is safe again - at most one ref exists at a time, keeping the scan effectively O(1) regardless of history. This assumes claim volume stays low enough, relative to how fast one claim completes, that the resulting wait queue stays short. (The trailing /global segment isn't decorative: the Git References API rejects a ref name with fewer than three slash-separated components, caught by real CI against this branch, not by the mocked unit tests.) The single global lock also replaces the per-run build-run-locks/ lock entirely: serializing every claim naturally serializes same-run job races too, so a separate run-scoped lock added nothing. And under exclusive access, a collision on the candidate ref is no longer an expected race - it's now a fatal, actionable error instead of something MAX_ATTEMPTS retried past. As a side effect, serializing the migration-seed branch too closes a second bug: two different concurrent runs could previously each independently seed from the legacy build_number property and claim distinct, non-adjacent numbers, since that seeding was only serialized within a single run. That gap was harmless to uniqueness (still covered by the "ignoring gaps" test) but ruled out ever finding the max more cheaply than a full scan; it can no longer form. refs/build-runs// markers are never automatically deleted (no pruning mechanism, by design - see below); README now documents git ls-remote as the way to inspect the current build number and the run-to-number history directly, instead. Review fixes (real CI + gitar-bot/copilot on this same commit): - A leaked repository-wide lock (runner killed before its cleanup trap runs) previously had no recovery path; the timeout error now names the exact `gh api --method DELETE` command to clear it. Auto-steal via holder run_id + Actions API status is a real, structurally sound follow-up, deliberately not folded in here. - Marker check and marker write each retry once/twice (RETRY_INTERVAL_SECONDS apart) before giving up, narrowing (not closing) the window where a transient API blip could make a same-run job miss an existing marker and claim its own, second number instead of waiting/reusing. - Ref-name arithmetic now forces base-10 (`10#$n`) so a leading-zero ref name can't be misread as an invalid octal literal under `set -e`. - action.yml: the Vault fetch step is now continue-on-error, so a missing/misconfigured secret falls through to the existing no-migration-token warning instead of hard-failing the whole action before that path is ever reached. - README: config-maven/config-pip/config-uv corrected from a stale contents: read to contents: write (each calls get-build-number internally, same breaking change as get-build-number itself). - The stale-refs deletion loop guards against an empty STALE_REFS array before expanding it: bash before 4.4 treats "${STALE_REFS[@]}" on a zero-element array as an unbound variable under `set -u`, and macOS ships bash 3.2 by default (this action also runs there via config-*/build-* wrappers). Only reachable on a repository's very first claim (no refs/build-number/* yet), but real - every repository migrating to this design hits it once. A prune-markers maintenance mode (deleting refs/build-runs// markers for runs no longer on GitHub) was built and tested against this repository's own real history in an earlier revision of this commit, but removed before merge: the naive implementation doesn't scale (listing the entire refs/build-runs/* namespace is unbounded per invocation regardless of how many markers are actually stale), and a correctly bounded version needs a different ref-naming scheme entirely. Left as a real, undocumented follow-up rather than shipped half-solved; git ls-remote covers the "what's the current state" need in the meantime. Co-Authored-By: Claude Sonnet 4.5 --- README.md | 47 ++++++++--- get-build-number/action.yml | 6 +- get-build-number/get_build_number.sh | 114 ++++++++++++++------------- spec/get_build_number_spec.sh | 86 ++++++++++++-------- 4 files changed, 152 insertions(+), 101 deletions(-) diff --git a/README.md b/README.md index 6dec5e29..13faa798 100644 --- a/README.md +++ b/README.md @@ -137,18 +137,39 @@ No inputs are required for this action. This action coordinates purely through Git references on the repository - no external state, cache, or database. Each reference is created pointing at `$GITHUB_SHA` (the commit that triggered the claim); that target is never read back - only the reference's existence matters -(for `build-number` and `build-run-locks`), or its name, which encodes the claimed number (for `build-runs`). +(for `build-number` and `build-number-lock`), or its name, which encodes the claimed number (for `build-runs`). -| Reference | Lifetime | Purpose | -|-------------------------------------|--------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `refs/build-number/` | Permanent, never deleted | The atomic claim itself and the sole source of truth for uniqueness. Creating it fails if it already exists, which is what makes a claim a genuine compare-and-swap. There is no safe way to delete these: unlike `build-runs` below, nothing external can ever confirm "no in-flight claim is still targeting this number" - a stalled claim for an older number could resurrect it after deletion, reopening a number already published elsewhere. A time-based margin (e.g. "not superseded for the last N minutes") would only lower the odds, not eliminate them, which isn't an acceptable trade for a bug class this action exists to remove. The unbounded growth this causes is a real but separate concern - see [Known limitations](#known-limitations) below. | -| `refs/build-runs//` | Not automatically deleted | Marker recording which number a workflow run claimed. Checked first, so a rerun or another job in the same run reuses it instead of claiming a new one. A completed run can still be re-run much later, so "the run finished" does not make its marker safe to delete - only the run's own record being gone from GitHub (confirmed via a 404, never inferred from age) does, since that's what makes re-running it impossible. | -| `refs/build-run-locks/` | Seconds: created, used, and deleted again within a single claim, not a persistent artifact | Exclusive lock ensuring only one job per workflow run claims and publishes a number at a time; every other job waits for the marker above instead of racing to claim its own. | +| Reference | Lifetime | Purpose | +| ------------------------------------- | -------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `refs/build-number/` | Deleted once superseded by the next claim | The atomic claim itself and the sole source of truth for uniqueness. Deleting the superseded ref is only safe because it happens while holding the exclusive `build-number-lock` below - without that, a claim stalled in flight for an older number could resurrect it after deletion, republishing a number already used elsewhere (a real bug found in review before this lock existed). At most one ref (rarely a couple, if a past deletion failed and self-heals on the next claim) exists at a time. | +| `refs/build-runs//` | Not automatically deleted - see [Inspecting current refs](#inspecting-current-refs) below | Marker recording which number a workflow run claimed. Checked first, and lock-free, so a rerun or another job in the same run reuses it without ever touching `build-number-lock`. A completed run can still be re-run much later, so "the run finished" does not make its marker safe to delete - only the run's own record being gone from GitHub (confirmed via a 404, never inferred from age) does, since that's what makes re-running it impossible. | +| `refs/build-number-lock/global` | Seconds: created, used, and deleted again within a single claim, not a persistent artifact | Exclusive, repository-wide lock serializing every new claim (not just those within one run) - what makes deleting a superseded `build-number` ref above safe. Assumes claims are infrequent and each is fast enough that the resulting wait queue stays short; every other claimant waits for either this lock or the marker above instead of racing independently. | ### Known limitations -- `refs/build-number/` is never deleted (see the table above), so the scan that finds the next candidate grows with the - repository's total historical claim count. Not expected to matter in practice for a long time; no bound is implemented today. +- `refs/build-runs//` markers are never automatically deleted, so they accumulate for the lifetime of the + repository - see [Inspecting current refs](#inspecting-current-refs) below to check what currently exists. Not expected to + matter in practice for a long time; no automated cleanup is implemented today. + +### Inspecting current refs + +These refs aren't pulled by a normal `git fetch`/`git clone` (they live outside the default `refs/heads/*`/`refs/tags/*` +namespaces), but can be listed directly with `git ls-remote` or the GitHub API. + +Current build number - only one `refs/build-number/*` ref exists at a time, since each new claim deletes the one it supersedes: + +```shell +$ git ls-remote origin 'refs/build-number/*' +7345fe785a3fc8705e3d8204a28ca4f909628e7e refs/build-number/12106 +``` + +History - mapping a workflow run to the build number it claimed or reused, one `refs/build-runs//` ref per run: + +```shell +$ git ls-remote origin 'refs/build-runs/*' +3cfd2386fdd965247c0e465ff4774239036894ec refs/build-runs/32867023550/12093 +002e0fb9a88da75c89316635b1f0a62a6ad9aff4 refs/build-runs/32868621162/12095 +``` --- @@ -196,7 +217,7 @@ By default, Maven caches `~/.m2/repository`. You can customize this behavior: #### Required GitHub Permissions - `id-token: write` -- `contents: read` +- `contents: write` #### Required Vault Permissions @@ -1201,7 +1222,7 @@ This action configures pip to pull packages from the internal JFrog Artifactory #### Required GitHub Permissions - `id-token: write` -- `contents: read` +- `contents: write` #### Required Vault Permissions @@ -1212,7 +1233,7 @@ This action configures pip to pull packages from the internal JFrog Artifactory ```yaml permissions: id-token: write - contents: read + contents: write steps: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 - uses: SonarSource/ci-github-actions/config-pip@v1 @@ -1311,7 +1332,7 @@ default = true #### Required GitHub Permissions - `id-token: write` -- `contents: read` +- `contents: write` #### Required Vault Permissions @@ -1326,7 +1347,7 @@ The `uv` tool must be pre-installed. Use of `mise` is recommended. ```yaml permissions: id-token: write - contents: read + contents: write steps: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 - uses: jdx/mise-action@1648a7812b9aeae629881980618f079932869151 # v4.0.1 diff --git a/get-build-number/action.yml b/get-build-number/action.yml index cb4bdb63..cdbd1c16 100644 --- a/get-build-number/action.yml +++ b/get-build-number/action.yml @@ -44,14 +44,18 @@ runs: # Vault is only needed to read the legacy build_number property during migration to refs/build-number/* (see README); fetched # unconditionally for simplicity, since this whole dependency is temporary and slated for removal once migration is complete. + # continue-on-error: a repository with no build_number history (or no {REPO_OWNER_NAME_DASH}-build-number preset configured + # yet) has nothing to migrate - a failure here must not block claiming, only fall through to get_build_number.sh's own + # no-migration-token warning path (an empty LEGACY_PROPERTY_TOKEN below, same as if this step is skipped entirely). - uses: SonarSource/vault-action-wrapper@881045d830534a70ec3c7c275fa3714412c8ff6e # 3.6.1 id: secrets if: steps.from-env.outputs.skip != 'true' + continue-on-error: true with: secrets: development/github/token/{REPO_OWNER_NAME_DASH}-build-number token | github_token; # Reuses this run's own claim if one already exists (a rerun, or another job in the same run); otherwise claims a new one under - # an exclusive per-run lock, released as soon as it's no longer needed - see get_build_number.sh. + # an exclusive, repository-wide lock, released as soon as it's no longer needed - see get_build_number.sh. - name: Get build number if: steps.from-env.outputs.skip != 'true' shell: bash diff --git a/get-build-number/get_build_number.sh b/get-build-number/get_build_number.sh index 2e9269a2..eb38f35a 100755 --- a/get-build-number/get_build_number.sh +++ b/get-build-number/get_build_number.sh @@ -1,14 +1,12 @@ #!/bin/bash # Get the build number for a GitHub repository and save it to .build_number.txt, reusing one already claimed by this workflow run. # -# refs/build-number/: the atomic claim itself, the sole source of truth for uniqueness. Never deleted: a claim in flight for -# candidate N can stall arbitrarily long before its POST executes, and if some other, older N had since been deleted, that stale -# POST would succeed against the now-free slot - reopening an already-published number. Pruning (out-of-band, not here) may only -# ever remove refs so far below the current claim that no in-flight run could plausibly still be targeting them. -# refs/build-runs//: marker recording which number this workflow run claimed. Checked first, so reruns and other jobs in -# the same run reuse it instead of racing to claim their own. -# refs/build-run-locks/: exclusive, transient lock serializing "check the marker, then claim and publish one" for this run. -# Released immediately after use (success or failure) via the trap below, not held for the run's lifetime. +# refs/build-number/: the atomic claim itself and the sole source of truth for uniqueness. Claiming a number and deleting the +# one it superseded both happen while holding build-number-lock below. +# refs/build-runs//: marker recording which number this workflow run claimed. Checked first, and lock-free, so reruns +# and other jobs in the same run reuse it without ever touching build-number-lock. +# refs/build-number-lock: exclusive, transient, repository-wide lock serializing every new claim, not just those within one run. +# Released immediately after use (success or failure) via the trap below, not held for a run's lifetime. # # Every ref points at $GITHUB_SHA purely because creation requires some valid target; that target is never read back. # All ref reads/writes use the ambient GITHUB_TOKEN. LEGACY_PROPERTY_TOKEN (from Vault) is only used to read the legacy @@ -26,10 +24,10 @@ PROPERTIES_API_URL="repos/${GITHUB_REPOSITORY}/properties/values" PROPS_JQ='.[] | select(.property_name == "build_number") | .value' NUMBER_NS="build-number" RUN_NS="build-runs/${GITHUB_RUN_ID}" -RUN_LOCK_REF="build-run-locks/${GITHUB_RUN_ID}" +NUMBER_LOCK_REF="build-number-lock/global" # git/refs requires at least 3 slash-separated components; a bare name is rejected LOCK_POLL_INTERVAL_SECONDS="${LOCK_POLL_INTERVAL_SECONDS:-3}" LOCK_WAIT_MAX_ATTEMPTS="${LOCK_WAIT_MAX_ATTEMPTS:-40}" # ~2 minutes at the default interval -MAX_ATTEMPTS="${MAX_ATTEMPTS:-100}" # retries when a concurrent run beats us to the next number +RETRY_INTERVAL_SECONDS="${RETRY_INTERVAL_SECONDS:-1}" # between internal retries of a single marker check or marker write create_ref() { local ref="$1" @@ -54,15 +52,20 @@ fail_claim() { exit 1 } +# A false "no marker" here would make this run claim a second, unnecessary number instead of reusing an existing one, so a +# single transient failure isn't enough to conclude that - retried once before falling back to "not yet published". find_run_marker() { - local output - # Tolerate a transient/network failure of this specific call: treat it the same as "no marker yet" and retry on the next poll, - # but log it so a *permanent* failure (e.g. an auth error) is visible instead of only ever surfacing as a wait timeout. - if ! output=$(gh api -H "$GH_API_VERSION_HEADER" "${MATCHING_REFS_API_URL}/${RUN_NS}/" --jq '.[0].ref // empty' 2>&1); then - echo "::debug::Marker check failed, treating as not-yet-published and retrying: ${output}" >&2 - return 0 - fi - echo "$output" + local output check_attempt + for check_attempt in 1 2; do + if output=$(gh api -H "$GH_API_VERSION_HEADER" "${MATCHING_REFS_API_URL}/${RUN_NS}/" --jq '.[0].ref // empty' 2>&1); then + echo "$output" + return 0 + fi + ((check_attempt < 2)) && sleep "$RETRY_INTERVAL_SECONDS" + done + echo "::warning title=Marker check inconclusive::Could not confirm whether refs/${RUN_NS}/* exists after 2 attempts (${output});" \ + "proceeding as if not yet published." >&2 + return 0 } # Exits 0 (and tells the caller to stop) if this run already has a claim; exits 1 on a malformed marker. @@ -82,8 +85,8 @@ try_reuse_existing_claim() { HELD_LOCK="" release_lock() { [[ -n "$HELD_LOCK" ]] || return 0 - delete_ref "$RUN_LOCK_REF" || echo "::warning title=Build number lock not released::Failed to delete refs/${RUN_LOCK_REF}; a" \ - "later attempt for this workflow run may have to wait out its timeout before claiming a number." >&2 + delete_ref "$NUMBER_LOCK_REF" || echo "::warning title=Build number lock not released::Failed to delete refs/${NUMBER_LOCK_REF};" \ + "a later claim may have to wait out its timeout before proceeding." >&2 } trap release_lock EXIT @@ -93,7 +96,7 @@ attempt=1 while true; do try_reuse_existing_claim && { echo "::endgroup::" && exit 0; } - RESPONSE=$(create_ref "$RUN_LOCK_REF") && LOCK_STATUS=0 || LOCK_STATUS=$? + RESPONSE=$(create_ref "$NUMBER_LOCK_REF") && LOCK_STATUS=0 || LOCK_STATUS=$? if [[ "$LOCK_STATUS" -eq 0 ]]; then HELD_LOCK=1 break @@ -103,30 +106,31 @@ while true; do fi if ((attempt >= LOCK_WAIT_MAX_ATTEMPTS)); then - echo "::error title=Build number claim timed out::Waited ${LOCK_WAIT_MAX_ATTEMPTS} attempts for refs/${RUN_NS}/* to appear; the" \ - "job holding refs/${RUN_LOCK_REF} may have failed before publishing its claim." >&2 + echo "::error title=Build number claim timed out::Waited ${LOCK_WAIT_MAX_ATTEMPTS} attempts for refs/${NUMBER_LOCK_REF} to" \ + "be released; the job holding it may have failed before completing its claim. If no claim is genuinely in progress, the" \ + "lock leaked (e.g. a runner killed without running its cleanup) and blocks every claim in this repository until removed:" \ + "gh api --method DELETE ${REFS_API_URL}/${NUMBER_LOCK_REF}" >&2 exit 1 fi - echo "::debug::refs/${RUN_LOCK_REF} already held; waiting for its marker (attempt $((attempt + 1))/${LOCK_WAIT_MAX_ATTEMPTS})" + echo "::debug::refs/${NUMBER_LOCK_REF} already held; waiting (attempt $((attempt + 1))/${LOCK_WAIT_MAX_ATTEMPTS})" sleep "$LOCK_POLL_INTERVAL_SECONDS" attempt=$((attempt + 1)) done -# We now hold the lock, but someone else may have finished between our last check above and acquiring it. +# We now hold the lock, but this run's own marker may have appeared while we were waiting (another job in the same run). try_reuse_existing_claim && { echo "::endgroup::" && exit 0; } -# O(total historical claims), not O(1) - see README's Known limitations. A cheaper alternative (e.g. binary search over -# individual ref lookups) needs claimed numbers to have no permanent gaps, which the migration seed below can violate: two -# different runs seeding from the legacy property concurrently can each claim a distinct, non-adjacent number (see the -# "ignoring gaps" test) - so a gap can't be assumed away, and this scan can't be replaced without addressing that first. echo "::debug::Scanning refs/${NUMBER_NS}/* for the highest claimed build number" NUMBER_REFS=$(gh api --paginate -H "$GH_API_VERSION_HEADER" "${MATCHING_REFS_API_URL}/${NUMBER_NS}/" --jq '.[].ref') MAX_CLAIMED=0 +STALE_REFS=() if [[ -n "$NUMBER_REFS" ]]; then while IFS= read -r ref; do [[ "$ref" =~ ^refs/${NUMBER_NS}/([0-9]+)$ ]] || continue n="${BASH_REMATCH[1]}" - ((n > MAX_CLAIMED)) && MAX_CLAIMED=$n + STALE_REFS+=("${NUMBER_NS}/${n}") + # Force base-10: a leading-zero ref name like build-number/08 would otherwise be parsed as an (invalid) octal literal. + ((10#$n > MAX_CLAIMED)) && MAX_CLAIMED=$((10#$n)) done <<<"$NUMBER_REFS" fi @@ -144,39 +148,43 @@ if [[ "$MAX_CLAIMED" -eq 0 ]]; then exit 1 fi echo "Seeding from legacy build_number property: ${LEGACY_BUILD_NUMBER}" - MAX_CLAIMED=$((LEGACY_BUILD_NUMBER + 1000)) # buffer against the legacy property still advancing elsewhere during migration + # 10# forces base-10: a leading-zero property value would otherwise be parsed as an (invalid) octal literal. + MAX_CLAIMED=$((10#$LEGACY_BUILD_NUMBER + 1000)) # buffer against the legacy property still advancing elsewhere during migration fi fi fi echo "::debug::Highest known build number: ${MAX_CLAIMED}" -attempt=1 CANDIDATE=$((MAX_CLAIMED + 1)) -while true; do - RESPONSE=$(create_ref "${NUMBER_NS}/${CANDIDATE}") && CLAIM_STATUS=0 || CLAIM_STATUS=$? - - if [[ "$CLAIM_STATUS" -eq 0 ]]; then - echo "Claimed build number ${CANDIDATE} (refs/${NUMBER_NS}/${CANDIDATE})" - break - fi - - if [[ "$RESPONSE" != *"Reference already exists"* ]]; then - fail_claim "$RESPONSE" - 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 +RESPONSE=$(create_ref "${NUMBER_NS}/${CANDIDATE}") && CLAIM_STATUS=0 || CLAIM_STATUS=$? +if [[ "$CLAIM_STATUS" -ne 0 ]]; then + if [[ "$RESPONSE" == *"Reference already exists"* ]]; then + echo "::error title=Build number claim failed::refs/${NUMBER_NS}/${CANDIDATE} already exists, which should be impossible" \ + "while holding refs/${NUMBER_LOCK_REF}. Check for another caller writing build-number refs without this lock (e.g. an" \ + "older, un-migrated version of this action) or a manual/external ref creation." >&2 exit 1 fi + fail_claim "$RESPONSE" +fi +echo "Claimed build number ${CANDIDATE} (refs/${NUMBER_NS}/${CANDIDATE})" - echo "::debug::Build number ${CANDIDATE} already claimed; trying $((CANDIDATE + 1)) (attempt $((attempt + 1))/${MAX_ATTEMPTS})" - CANDIDATE=$((CANDIDATE + 1)) - attempt=$((attempt + 1)) -done +if ((${#STALE_REFS[@]} > 0)); then + for ref in "${STALE_REFS[@]}"; do + delete_ref "$ref" || echo "::warning title=Stale build number ref not deleted::Failed to delete refs/${ref}; harmless, just" \ + "clutter - a future claim will retry." >&2 + done +fi -if ! create_ref "${RUN_NS}/${CANDIDATE}" >/dev/null; then - echo "::error title=Build number claim failed::Failed to record refs/${RUN_NS}/${CANDIDATE}; other jobs/reruns of this workflow" \ - "run waiting on refs/${RUN_LOCK_REF} would otherwise time out instead of reusing it." >&2 +# Retried rather than a single attempt: a same-run job waiting on the lock reuses this marker as soon as it appears, so a +# transient failure here - left unretried - would make it claim its own, second number for this run instead of waiting further. +MARKER_WRITTEN="" +for marker_attempt in 1 2 3; do + create_ref "${RUN_NS}/${CANDIDATE}" >/dev/null && { MARKER_WRITTEN=1; break; } + ((marker_attempt < 3)) && sleep "$RETRY_INTERVAL_SECONDS" +done +if [[ -z "$MARKER_WRITTEN" ]]; then + echo "::error title=Build number claim failed::Failed to record refs/${RUN_NS}/${CANDIDATE} after 3 attempts; other jobs/reruns" \ + "of this workflow run waiting on refs/${NUMBER_LOCK_REF} would otherwise time out instead of reusing it." >&2 exit 1 fi diff --git a/spec/get_build_number_spec.sh b/spec/get_build_number_spec.sh index 87ea73c2..8312a99c 100755 --- a/spec/get_build_number_spec.sh +++ b/spec/get_build_number_spec.sh @@ -5,6 +5,7 @@ export GITHUB_REPOSITORY="my org/my-repo" export GITHUB_SHA="deadbeefcafef00dfeed" export GITHUB_RUN_ID="123456789" export LOCK_POLL_INTERVAL_SECONDS=0 +export RETRY_INTERVAL_SECONDS=0 TEMP_DIR="${SHELLSPEC_TMPBASE:-/tmp}" export BUILD_NUMBER_FILE="${TEMP_DIR}/build_number.txt" @@ -26,7 +27,7 @@ Describe 'get_build_number.sh' Mock gh if [[ "$*" == *"matching-refs/build-runs/"* ]]; then echo "refs/build-runs/${GITHUB_RUN_ID}/5" - elif [[ "$*" == *"build-run-locks/"* ]]; then + elif [[ "$*" == *"build-number-lock"* ]]; then echo "1" >>"$GH_LOCK_CALLS_FILE" echo "gh $*" else @@ -97,7 +98,7 @@ Describe 'get_build_number.sh' The stderr should include "::error title=Invalid build number::Legacy build_number property 'notANumber'" End - It 'should claim the next number after the highest existing build-number ref, ignoring gaps, and never delete any of them' + It 'should claim the next number after the highest existing build-number ref, ignoring gaps, and delete the superseded ones' export GH_DELETE_CALLS_FILE="${TEMP_DIR}/gh_delete_calls_number.txt" rm -f "$GH_DELETE_CALLS_FILE" Mock gh @@ -116,46 +117,38 @@ Describe 'get_build_number.sh' The status should be success The output should include "Claimed build number 6" The contents of file "$BUILD_NUMBER_FILE" should equal "6" - The path "$GH_DELETE_CALLS_FILE" should not be file + The contents of file "$GH_DELETE_CALLS_FILE" should include "build-number/1" + The contents of file "$GH_DELETE_CALLS_FILE" should include "build-number/2" + The contents of file "$GH_DELETE_CALLS_FILE" should include "build-number/3" + The contents of file "$GH_DELETE_CALLS_FILE" should include "build-number/5" End - It 'should retry when a concurrent run claims the same candidate first, then succeed' - export GH_REFS_CALLS_FILE="${TEMP_DIR}/gh_refs_calls_retry.txt" - rm -f "$GH_REFS_CALLS_FILE" + It 'should warn but still succeed when a superseded build-number ref fails to delete' Mock gh if [[ "$*" == *"matching-refs/build-runs/"* ]]; then echo '' elif [[ "$*" == *"matching-refs/build-number/"* ]]; then - echo '' - elif [[ "$*" == *"ref=refs/build-number/"* ]]; 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 + echo 'refs/build-number/5' + elif [[ "$*" == *"--method DELETE"* && "$*" == *"build-number/5"* ]]; then + exit 1 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 1 already claimed; trying 2" - The output should include "Claimed build number 2" - The stderr should include "::warning title=Legacy build number not checked::" - The contents of file "$BUILD_NUMBER_FILE" should equal "2" + The output should include "Claimed build number 6" + The stderr should include "::warning title=Stale build number ref not deleted::Failed to delete refs/build-number/5" + The contents of file "$BUILD_NUMBER_FILE" should equal "6" End - It 'should fail after exhausting retries under permanent contention on the build-number namespace' - export MAX_ATTEMPTS=3 + It 'should fail immediately if the candidate build-number ref unexpectedly already exists while holding the lock' Mock gh if [[ "$*" == *"matching-refs/build-runs/"* ]]; then echo '' elif [[ "$*" == *"matching-refs/build-number/"* ]]; then echo '' - elif [[ "$*" == *"ref=refs/build-number/"* ]]; then + elif [[ "$*" == *"ref=refs/build-number/1"* ]]; then echo '{"message":"Reference already exists"}' >&2 exit 1 else @@ -164,8 +157,8 @@ Describe 'get_build_number.sh' 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" + The output should include "::group::Get build number" + The stderr should include "refs/build-number/1 already exists, which should be impossible while holding refs/build-number-lock" End It 'should fail immediately on an unexpected API error while claiming, not treat it as a collision' @@ -240,7 +233,7 @@ Describe 'get_build_number.sh' elif [[ "$*" == *"ref=refs/build-number/"* ]]; then echo '{"message":"Internal Server Error"}' >&2 exit 1 - elif [[ "$*" == *"--method DELETE"* && "$*" == *"build-run-locks/"* ]]; then + elif [[ "$*" == *"--method DELETE"* && "$*" == *"build-number-lock"* ]]; then echo "1" >>"$GH_UNLOCK_CALLS_FILE" echo "gh $*" else @@ -266,7 +259,7 @@ Describe 'get_build_number.sh' else echo "refs/build-runs/${GITHUB_RUN_ID}/9" fi - elif [[ "$*" == *"ref=refs/build-run-locks/"* ]]; then + elif [[ "$*" == *"ref=refs/build-number-lock"* ]]; then echo '{"message":"Reference already exists"}' >&2 exit 1 else @@ -279,7 +272,7 @@ Describe 'get_build_number.sh' The contents of file "$BUILD_NUMBER_FILE" should equal "9" End - It 'should tolerate a transient error while polling for the marker and retry instead of aborting the wait' + It 'should silently absorb a single transient error checking the marker via its internal retry' export GH_MARKER_CALLS_FILE="${TEMP_DIR}/gh_marker_calls_transient.txt" rm -f "$GH_MARKER_CALLS_FILE" Mock gh @@ -289,12 +282,10 @@ Describe 'get_build_number.sh' if [[ "$count" -eq 1 ]]; then echo '{"message":"Internal Server Error"}' >&2 exit 1 - elif [[ "$count" -lt 3 ]]; then - echo '' else echo "refs/build-runs/${GITHUB_RUN_ID}/11" fi - elif [[ "$*" == *"ref=refs/build-run-locks/"* ]]; then + elif [[ "$*" == *"ref=refs/build-number-lock"* ]]; then echo '{"message":"Reference already exists"}' >&2 exit 1 else @@ -304,16 +295,43 @@ Describe 'get_build_number.sh' When run script get-build-number/get_build_number.sh The status should be success The output should include "Reusing build number 11" - The stderr should include "Internal Server Error" + The stderr should not include "Marker check inconclusive" The contents of file "$BUILD_NUMBER_FILE" should equal "11" End + It 'should warn once its internal retry is exhausted, then succeed reusing the marker found on the next poll' + export GH_MARKER_CALLS_FILE="${TEMP_DIR}/gh_marker_calls_inconclusive.txt" + rm -f "$GH_MARKER_CALLS_FILE" + Mock gh + if [[ "$*" == *"matching-refs/build-runs/"* ]]; then + count=$(($(cat "$GH_MARKER_CALLS_FILE" 2>/dev/null || echo 0) + 1)) + echo "$count" >"$GH_MARKER_CALLS_FILE" + if [[ "$count" -le 2 ]]; then + echo '{"message":"Internal Server Error"}' >&2 + exit 1 + else + echo "refs/build-runs/${GITHUB_RUN_ID}/12" + fi + elif [[ "$*" == *"ref=refs/build-number-lock"* ]]; then + echo '{"message":"Reference already exists"}' >&2 + exit 1 + else + echo "gh $*" + fi + End + When run script get-build-number/get_build_number.sh + The status should be success + The output should include "Reusing build number 12" + The stderr should include "::warning title=Marker check inconclusive::" + The contents of file "$BUILD_NUMBER_FILE" should equal "12" + End + It 'should fail after exhausting the wait if the lock holder never publishes a marker' export LOCK_WAIT_MAX_ATTEMPTS=3 Mock gh if [[ "$*" == *"matching-refs/build-runs/"* ]]; then echo '' - elif [[ "$*" == *"ref=refs/build-run-locks/"* ]]; then + elif [[ "$*" == *"ref=refs/build-number-lock"* ]]; then echo '{"message":"Reference already exists"}' >&2 exit 1 else @@ -330,7 +348,7 @@ Describe 'get_build_number.sh' Mock gh if [[ "$*" == *"matching-refs/build-runs/"* ]]; then echo '' - elif [[ "$*" == *"ref=refs/build-run-locks/"* ]]; then + elif [[ "$*" == *"ref=refs/build-number-lock"* ]]; then echo '{"message":"Internal Server Error"}' >&2 exit 1 else