PREQ-7781 Detect and retry concurrent build_number increments

[julien-carsique-sonarsource]

Summary

• get-build-number/get_build_number.sh did a non-atomic read→increment→write on the repo's build_number custom property.
• Concurrent workflow runs (e.g. GitHub Stacked PRs, which can trigger several builds in parallel) could both read the same value, both compute the same "next" number, and both write it back — promote@v1 then fails with Multiple builds found for <repo> <number>.
• Fix: after writing the incremented value, read it back and verify it matches what we wrote. If a concurrent run raced ahead of us, retry with jittered backoff (up to MAX_ATTEMPTS, default 10) instead of trusting an unverified write.

Why not fully atomic?

GitHub's custom repository properties API has no documented conditional/atomic write support (no If-Match/ETag-based CAS, unlike the Contents API). A truly atomic counter would need a different backing store — that's the larger redesign already tracked in BUILD-11797 (migrate get-build-number from actions/cache to gh-action_cache). This PR closes the practical race window (GET→PATCH→GET-verify is much narrower than the original GET...PATCH span across two independent runs) without that bigger migration.

Root-caused and fixed for PREQ-7781 (https://sonarsource.atlassian.net/browse/PREQ-7781), reported by Ismail Cherri via stacked PRs on sonarqube-unification.

Test plan

• Added a test simulating a lost race (concurrent write detected, retries, then succeeds) and a test for exhausting all retries under permanent contention
• All 5 get_build_number_spec.sh examples pass (shellspec spec/get_build_number_spec.sh --shell bash)
• 100% line coverage on get_build_number.sh (kcov)

[hashicorp-vault-sonar-prod]

PREQ-7781

[gitar-bot]

⚠️ Bug: Verify-after-write misses symmetric same-value race

When two runs both read N and both PATCH N+1, each read-back returns N+1 and CONFIRMED == NEXT for both, so both pass verification and claim the same number — precisely the 'Multiple builds found' scenario the PR aims to fix. The check only catches a concurrent run that writes a different value between our PATCH and our read-back; it does not detect two runs writing the identical next value. This narrows but does not close the reported window. Consider adding random jitter before the initial read, or a post-write delay before verification so a competing writer's value can diverge, and document that this remains best-effort until the BUILD-11797 atomic-store migration.

Delay before verification so a same-value competing write is more likely to be overwritten and detected (still best-effort).:

NEXT_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]=${NEXT_BUILD_NUMBER}"

# Give a competing writer time to overwrite with a different value so the
# symmetric "both wrote N+1" collision becomes detectable on read-back.
sleep "0.$((RANDOM % 900 + 100))"
CONFIRMED_BUILD_NUMBER=$(get_property_value)

_{Was this helpful? React with 👍 / 👎}

[gitar-bot]

Code Review ⚠️ Changes requested 0 resolved / 1 findings

Adds verify-after-write logic to detect concurrent build_number increments, but the check still permits duplicate values when two runs write the same incremented number simultaneously.

⚠️ Bug: Verify-after-write misses symmetric same-value race

📄 get-build-number/get_build_number.sh:31-41

When two runs both read N and both PATCH N+1, each read-back returns N+1 and CONFIRMED == NEXT for both, so both pass verification and claim the same number — precisely the 'Multiple builds found' scenario the PR aims to fix. The check only catches a concurrent run that writes a different value between our PATCH and our read-back; it does not detect two runs writing the identical next value. This narrows but does not close the reported window. Consider adding random jitter before the initial read, or a post-write delay before verification so a competing writer's value can diverge, and document that this remains best-effort until the BUILD-11797 atomic-store migration.

Delay before verification so a same-value competing write is more likely to be overwritten and detected (still best-effort).

NEXT_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]=${NEXT_BUILD_NUMBER}"

# Give a competing writer time to overwrite with a different value so the
# symmetric "both wrote N+1" collision becomes detectable on read-back.
sleep "0.$((RANDOM % 900 + 100))"
CONFIRMED_BUILD_NUMBER=$(get_property_value)

🤖 Prompt for agents

Code Review: Adds verify-after-write logic to detect concurrent build_number increments, but the check still permits duplicate values when two runs write the same incremented number simultaneously.

1. ⚠️ Bug: Verify-after-write misses symmetric same-value race
   Files: get-build-number/get_build_number.sh:31-41

   When two runs both read N and both PATCH N+1, each read-back returns N+1 and CONFIRMED == NEXT for both, so both pass verification and claim the same number — precisely the 'Multiple builds found' scenario the PR aims to fix. The check only catches a concurrent run that writes a *different* value between our PATCH and our read-back; it does not detect two runs writing the identical next value. This narrows but does not close the reported window. Consider adding random jitter before the initial read, or a post-write delay before verification so a competing writer's value can diverge, and document that this remains best-effort until the BUILD-11797 atomic-store migration.

   Fix (Delay before verification so a same-value competing write is more likely to be overwritten and detected (still best-effort).):
   NEXT_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]=${NEXT_BUILD_NUMBER}"
   
   # Give a competing writer time to overwrite with a different value so the
   # symmetric "both wrote N+1" collision becomes detectable on read-back.
   sleep "0.$((RANDOM % 900 + 100))"
   CONFIRMED_BUILD_NUMBER=$(get_property_value)

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.
Unblock → Override a blocking verdict and allow merging.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact	Unblock
gitar auto-apply:on	gitar display:verbose	gitar unblock

_{Was this helpful? React with 👍 / 👎 | Gitar}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues
0 New dependency risks

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Copilot]

Pull request overview

This PR updates get-build-number to reduce build-number collisions across concurrent GitHub Actions runs by adding a write-verify-retry loop (with jittered backoff) when updating the repository’s build_number custom property.

Changes:

• Refactors get_build_number.sh to loop with a post-PATCH confirmation read and retry up to MAX_ATTEMPTS.
• Adds ShellSpec coverage for detected concurrent updates (retry-then-succeed) and for exhausting retries under sustained contention.
• Updates existing specs to account for the new “read twice” behavior (initial read + confirmation read).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
get-build-number/get_build_number.sh	Adds a retry loop and confirmation read after PATCH to detect certain concurrent updates.
spec/get_build_number_spec.sh	Extends mocks/tests to simulate races and retry exhaustion for the updated script behavior.

Suppressed comments (1)

spec/get_build_number_spec.sh:106

• This spec also triggers backoff retries and will run real sleep calls (up to ~1s per retry). Stubbing sleep to a no-op here keeps the test suite fast and avoids flakes due to timing.

  It 'should fail after exhausting retries if the race never resolves'
    export MAX_ATTEMPTS=3
    export GH_GET_CALLS_FILE="${TEMP_DIR}/gh_get_calls_5.txt"
    rm -f "$GH_GET_CALLS_FILE"
    Mock gh
      if [[ "$*" =~ "api --method PATCH" ]]; then

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[julien-carsique-sonarsource]

Not the correct approach

[julien-carsique-sonarsource]

Adding context on why: gitar-bot's review above is right — verify-after-write doesn't close the race. If two runs both read N and both PATCH N+1, both read back N+1 on verification and both think they won. That's a real gap, not just a narrower window.

Replacing this with a real atomic claim via GitHub's Git References API (ref creation is a genuine compare-and-swap — it fails with 422 if the ref already exists, no sleeping or probability involved). Continuing on PREQ-7781, new PR incoming.