PREQ-7781 Claim build numbers atomically via Git refs instead of verify-and-retry

[julien-carsique-sonarsource]

Summary

Replaces the verify-after-write + retry approach from #335 (closed) with a real atomic claim,
coordinated purely through Git references - no repository custom property, no actions/cache, no
Vault-issued write-capable credential for the claim itself.

gitar-bot's review on #335 correctly found verify-after-write doesn't close the race: two runs
that both read N and both PATCH N+1 both pass verification and claim the same number. Git ref
creation is a genuine compare-and-swap (POST .../git/refs fails with 422 if the ref already
exists) - get_build_number.sh claims a number by creating refs/build-number/<N>, exclusively
and deterministically.

⚠️ Breaking change: contents: write now required

Every caller of get-build-number needs to change contents: read to contents: write in its
permissions: block. Claiming a number now writes Git references directly with the calling
workflow's own ambient token (previously the write happened via a Vault-issued credential, and the
ambient token only needed read access).

This is not a soft/optional upgrade: a caller still on contents: read gets a 403 on every claim
except the narrow case where the current workflow run already has a marker to reuse (that path is
read-only, so it happens to work by accident, not by design). There is no working
"read-only/rerun-only mode" - any run needing a genuinely new number fails until this is updated.
The error message now calls out the missing permission by name instead of only dumping the raw API
response, to make this discoverable without reading this PR.

Please flag this in the next release notes.

Design

Three ref namespaces, all created via the same atomic POST .../git/refs (see README's
Git References section for the full table):

• refs/build-number/<N> - the claim itself, sole source of truth for uniqueness. Deleted once
  superseded by the next claim - safe only because that deletion happens while holding the
  exclusive build-number-lock below. An earlier revision deleted superseded refs without that
  lock, reasoning the claim loop only ever scans forward from the current max; gitar-bot's review
  caught the actual race: 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 - a real,
  confirmed bug, not a hypothetical. The lock closes that: nothing else can be scanning, claiming,
  or deleting while one claim holds it, so there's no external process left to race.
• refs/build-runs/<run_id>/<N> - 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. Never automatically deleted (see "Known follow-ups" below); the
  README's new Inspecting current refs section documents
  git ls-remote to check what currently exists.
• refs/build-number-lock/global - exclusive, repository-wide, transient lock serializing
  every new claim (not scoped to a run - a single global lock also serializes same-run job races,
  so a separate per-run lock is no longer needed). Created, used, and deleted again within a single
  claim cycle (via a trap ... EXIT inside the script, so release runs on every exit path,
  including cancellation) - not held for a run's lifetime. This trades away claiming in parallel
  for the ability to safely delete superseded build-number refs; it assumes claim volume stays
  low enough, relative to how fast one claim completes, that the resulting wait queue stays short.
  MAX_ATTEMPTS/collision-retry is gone along with it - under exclusive access a collision on the
  candidate is no longer an expected race, so it's now treated as a fatal, actionable error instead
  of something to retry past. The /global segment isn't decorative: POST .../git/refs rejects a
  ref with fewer than three slash-separated components - caught by real CI against this branch
  (both here and via a throwaway PR in sonar-dummy), not by the mocked unit tests.

As a side effect, serializing every claim (including the migration-seed branch below) through one
lock also closes a second, unrelated bug this same review surfaced: two different concurrent
workflow 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 tested and expected via spec/get_build_number_spec.sh's
"ignoring gaps" case) but ruled out ever finding the max more cheaply than a full scan. With every
claim now serialized, that gap can no longer form, and since superseded refs are deleted, the scan
stays effectively O(1) regardless of the repository's total history - no separate optimization
needed.

All ref reads/writes use the calling workflow's own ambient token (contents: write, see the
breaking-change note above) - not a Vault-issued credential. Vault is only used to read the legacy
build_number repository property once, as a one-time migration seed for repositories with
existing history (seeded with a safety buffer above the legacy value, to allow for it advancing
elsewhere during the transition); this is skipped entirely, with a ::warning::, if no migration
token is available. The whole Vault dependency is temporary and will be dropped once every
repository has migrated (has ≥1 build-number ref).

acquire_run_lock.sh and get_build_number.sh started as two files (split by token model: the
lock check needed only the ambient token, the claim needed Vault too) and were later consolidated
into one script once Vault became an unconditional fetch - there was no longer a structural reason
to keep them separate.

Vault dependency - no permission change needed

The claim no longer needs contents: write on a Vault-issued token at all - that moved to each
calling workflow's own permissions: block (a per-repo, per-run, auto-revoked token, not a
standing shared credential; see the breaking-change note above). The only remaining Vault use
(reading the legacy property) is already covered by the existing, unchanged
repository_custom_properties: write permission (write implies read). Companion PR
SonarSource/re-terraform-aws-vault#9518 is closed - it's no longer needed.

Known follow-ups (not blocking this fix)

• Every claim now serializes through build-number-lock/global (see Design above), on the assumed
  tradeoff that claim volume stays low enough, relative to how fast one claim completes, that the
  resulting wait queue stays short. Revisit only if real usage data after rollout shows meaningful
  contention.
• Once an audit confirms no repository is still pinned to a pre-ref-only version of
  get-build-number, drop the Vault dependency (and the build-number Vault preset) entirely.
• refs/build-runs/<run_id>/<N> markers are never automatically deleted and accumulate for the
  lifetime of the repository. An earlier revision of this commit built and real-CI-tested a
  prune-markers maintenance mode for this, removed before merge: it doesn't scale as written -
  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 (e.g. a coarse time-bucket prefix), not a small fix. Left as a real,
  undocumented-until-now follow-up instead of shipped half-solved. git ls-remote (see the
  README's new Inspecting current refs section) covers the
  "what's the current state" need in the meantime.
• Idea raised, not implemented: embed a date in the build-runs marker name for readability when
  browsing, e.g. refs/build-runs/<YYYYMMDD>/<run_id>/<N>. Rejected in that exact form - leading
  with the date breaks correctness, not just style: git/matching-refs is prefix-only (no
  wildcard for a middle/leading segment, so a lookup can't ask "any date, this run_id" in one
  call), reruns can happen on a different day than the original claim, and a claim/reuse-check
  straddling midnight within the same run would compute two different "today" values and break the
  same-run reuse guarantee. If revisited, the date would need to be a suffix after run_id
  (e.g. refs/build-runs/<run_id>/<N>-<YYYYMMDD>), where it's purely decorative and never
  looked up.

Real-repo verification

In addition to unit tests, this branch (not @master) is exercised by a throwaway draft PR in
sonar-dummy - SonarSource/sonar-dummy#642.
Every SonarSource/ci-github-actions/*@master call across that repository's workflows is
redirected to this branch for the PR's duration - not just an isolated test workflow, but the real
build.yml pipeline (build-maven x4, report-ci-metrics, promote), integration-test.yml,
unified-dogfooding.yml, and pre-commit.yml's config-npm call - plus a dedicated workflow
calling get-build-number directly with a 3-way matrix (real lock contention). The first attempt
(before redirecting everything) is what caught the "at least three slash-separated components" API
constraint fixed above; a later attempt is what gitar-bot's review of the sonar-dummy PR itself
caught the smoke test not actually exercising contention or asserting reuse, both since fixed
there.

Two runs' worth of real evidence, not a mock, of the exact cross-job reuse property this design
exists to guarantee:

• 3-way matrix, run 32875091444 -
  one job wins the race (Claimed build number 12100), the other two reuse it
  (Reusing build number 12100, already claimed by this workflow run), each job's own internal
  reuse assertion confirms all three end up with 12100.
• Real build pipeline, run 32875091336 -
  Build claims 12102; Build Windows, Build on WarpBuild Runner, Build macOS, and Promote
  all reuse it via refs/build-runs/<run_id>/12102 without ever touching
  refs/build-number-lock/global themselves.

Test plan

• spec/get_build_number_spec.sh rewritten: fresh claim, migration seed (with and without a
  token), gap-scan and delete-the-superseded-refs, delete-failure warning (non-fatal), fatal
  "already exists while holding the lock" anomaly, fatal API errors (including the new
  contents:write-specific message), marker-write failure, lock-wait poll/timeout/race-after-
  acquire, transient poll-error tolerance, malformed marker format.
• 436 examples, 0 failures (./run_shell_tests.sh)
• 100% line coverage on get_build_number.sh (kcov)
• shellcheck, actionlint, yamllint, markdownlint clean
• Real CI green on test-build-number-generation, -reuse, -reuse-from-env,
  -reuse-same-run[-windows] (against this repository's own real, accumulated build-runs
  markers - see the duration note in the README)
• Real-repo smoke test in sonar-dummy (see above)
• SonarCloud Quality Gate passed

[hashicorp-vault-sonar-prod]

PREQ-7781

[gitar-bot]

Confirmed against the current get_build_number.sh (the two-script design with acquire_run_lock.sh is gone, consolidated into one script with trap release_lock EXIT at get-build-number/get_build_number.sh:66):

• Lock release on cancellation: correct, resolved by the redesign. The trap ... EXIT at line 66 fires on any script termination, so it isn't tied to Actions' success/failure/cancelled distinction the way the old YAML if: always() && steps.claim.outcome == 'failure' step was.
• New leader on freed lock: correct, resolved by the redesign. The loop at get_build_number.sh:71-92 re-checks try_reuse_existing_claim and retries create_ref on the lock ref every iteration, so a waiter picks up the lock as soon as it's freed rather than being stuck polling for a marker that belongs to a phase that no longer exists.

The other two, however, don't match what's in the branch right now:

• Empty migration token: get_build_number.sh:110 only enters the legacy-seed branch if [[ -n "${LEGACY_PROPERTY_TOKEN:-}" ]]. When the token is empty/missing, that block is skipped silently — there's no ::warning:: anywhere in the file referencing a missing/empty migration token. MAX_CLAIMED just stays 0 with only the generic ::debug::Highest known build number: 0 at line 123.
• find_run_marker swallowing errors: find_run_marker() at get_build_number.sh:42-44 is still gh api ... --jq '.[0].ref // empty' || true — the || true discards any API error without capturing or logging it. There's no ::debug:: emitting the failure detail.

Could you double check these last two — are they on a different commit/branch than what's currently pushed, or still pending?

✅ All blocking issues resolved.

_{Configure merge blocking}

[Copilot]

Pull request overview

Replaces property-based build numbering with atomic Git-ref claims, run markers, global locking, and bounded marker pruning.

Changes:

• Adds lock-protected claims, migration seeding, marker reuse, and cleanup.
• Adds pruning for markers belonging to deleted workflow runs.
• Updates action metadata, documentation, tests, and CI permissions.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Final review findings
spec/prune_build_run_markers_spec.sh	No final comments.
spec/get_build_number_spec.sh	No final comments.
README.md	Nit, 3 votes: Update config-maven, config-pip, and config-uv examples from contents: read to contents: write.
get-build-number/prune_build_run_markers.sh	No final comments.
get-build-number/get_build_number.sh	Critical, 2 votes: Failed lock deletion can permanently block claims; add safe stale-lock recovery. Critical, 2 votes: Marker lookup errors can cause duplicate claims; retry or fail on inconclusive reads. Critical, 1 vote: Missing migration credentials can incorrectly restart numbering at 1; fail closed or require opt-in. Critical, 1 vote: Leading-zero refs can be parsed as invalid octal; use explicit base-10 parsing. Critical, 1 vote: Leading-zero legacy values have the same octal issue. Critical, 2 votes: Marker-write failure can allow another same-run job to claim a second number; make publication recoverable or fail subsequent callers.
get-build-number/action.yml	Moderate, 2 votes: Vault lookup failure prevents the documented no-token path; make the lookup optional. Nit, 3 votes: Align the maintenance-mode description with README support for scheduled workflows.
.github/workflows/test-update-release-channel.yml	Critical, 1 vote: pull_request code receives contents: write; keep write-capable claiming in a trusted workflow or separate it from validation.
.github/workflows/test-shell-scripts.yml	Critical, 1 vote: pull_request code receives contents: write; keep write-capable claiming in a trusted workflow or separate it from validation.
.github/workflows/test-build-number.yml	Critical, 1 vote: pull_request code receives contents: write; keep write-capable claiming in a trusted workflow or separate it from validation.
.github/workflows/pre-commit.yml	Critical, 1 vote: pull_request code receives contents: write; keep write-capable claiming in a trusted workflow or separate it from validation.

Suppressed comments (3)

get-build-number/action.yml:6

• The base action output description now correctly describes ref-based reuse, but the published output descriptions of build-maven, build-gradle, build-yarn, and build-poetry still say the number is reused from cache. Those wrappers expose this output, so update their metadata to remove the behavior that this change deleted.

    description: The build number, newly claimed or reused if this workflow run already claimed one

get-build-number/get_build_number.sh:85

• An EXIT trap is not guaranteed to run if runner cancellation escalates to SIGTERM or kills the process. A cancellation at this point can therefore leave refs/build-number-lock/global behind and make later claims wait until timeout, despite the design claiming cleanup on cancellation. Add INT/TERM handlers that exit through the existing EXIT cleanup before the runner's hard kill.

trap release_lock EXIT

get-build-number/prune_build_run_markers.sh:12

• This comment says the build-number namespace is never deleted, but get_build_number.sh explicitly deletes superseded refs at lines 161-164 while holding the global lock. Only this pruning script excludes that namespace; please correct the comment so maintainers do not infer that stale claim refs persist indefinitely.

# refs/build-number/<number> is intentionally not handled here - see get_build_number.sh for why that namespace is never deleted.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[julien-carsique-sonarsource]

@gitar-bot Both are addressed in the current commit (3329814 as of this reply) - this comment looks like it's checking a commit from before the repository-wide-lock redesign landed, which restructured the whole file (it's ~180 lines now, so the line numbers you cite don't correspond to anything current):

• Empty migration token: get_build_number.sh:139 - ::warning title=Legacy build number not checked::No refs/${NUMBER_NS}/* exist yet and no migration token is available; starting from 1. ... This fires whenever LEGACY_PROPERTY_TOKEN is empty.
• find_run_marker swallowing errors: no || true anywhere in the current file. find_run_marker() (lines 57-68) retries the check once, and on exhausting both attempts emits ::warning title=Marker check inconclusive::Could not confirm whether refs/${RUN_NS}/* exists after 2 attempts (${output}); proceeding as if not yet published. with the actual API error captured in ${output}.

Let me know if you're seeing something different on your end - happy to dig further, but I've re-checked the pushed file directly and both are there.

[gitar-bot]

@gitar-bot Both are addressed in the current commit (3329814 as of this reply) - this comment looks like it's checking a commit from before the repository-wide-lock redesign landed, which restructured the whole file (it's ~180 lines now, so the line numbers you cite don't correspond to anything current):

• Empty migration token: get_build_number.sh:139 - ::warning title=Legacy build number not checked::No refs/${NUMBER_NS}/* exist yet and no migration token is available; starting from 1. ... This fires whenever LEGACY_PROPERTY_TOKEN is empty.
• find_run_marker swallowing errors: no || true anywhere in the current file. find_run_marker() (lines 57-68) retries the check once, and on exhausting both attempts emits ::warning title=Marker check inconclusive::Could not confirm whether refs/${RUN_NS}/* exists after 2 attempts (${output}); proceeding as if not yet published. with the actual API error captured in ${output}.

Let me know if you're seeing something different on your end - happy to dig further, but I've re-checked the pushed file directly and both are there.

Confirmed — re-checked get-build-number/get_build_number.sh at 3329814 directly and both are there as described:

• Line 138-139: the ::warning title=Legacy build number not checked::... fires whenever LEGACY_PROPERTY_TOKEN is empty.
• find_run_marker() (lines 58-68): no || true in the file; it retries once and on exhausting both attempts emits ::warning title=Marker check inconclusive::... with the real API error in ${output}.

My earlier replies were checking a stale commit before the repository-wide-lock redesign restructured this file. Thanks for the correction — closing these out.

[gitar-bot]

Code Review ✅ Approved 20 resolved / 20 findings

Replaces the verify-after-write build number claim with an atomic Git ref create-and-lock mechanism, addressing 17 findings including race conditions, lock leaks, and permission errors.

✅ 20 resolved

✅ Edge Case: Stale build_number hint can spuriously exhaust MAX_ATTEMPTS

📄 get-build-number/get_build_number.sh:34-48 📄 get-build-number/get_build_number.sh:61-64
The claim loop scans linearly from BUILD_NUMBER+1 and gives up after MAX_ATTEMPTS (default 50) consecutive collisions (get_build_number.sh:35-57). Since refs/build-locks/* are never pruned and the property PATCH is best-effort, if the hint lags the true frontier by more than MAX_ATTEMPTS (e.g. after repeated PATCH failures or a burst of >50 concurrent claims), every candidate in the window collides and the run fails even though a free number exists just beyond the window. Consider seeding the starting candidate from the highest existing build-lock ref, or exponentially jumping the candidate on repeated collisions instead of a fixed +1 linear scan.

✅ Edge Case: Empty LEGACY_PROPERTY_TOKEN silently changes token for legacy read

📄 get-build-number/get_build_number.sh:40-41 📄 get-build-number/action.yml:70
On the migration path (no locks yet) the legacy property is read with GH_TOKEN="${LEGACY_PROPERTY_TOKEN:-}". If the Vault fetch yields an empty string, the request is made with an empty/unauthenticated token (or falls back to the ambient github.token, which may lack repository_custom_properties: read); either way the gh api call can fail and, being in a command substitution under set -e, aborts the whole build with a confusing error on a fresh repo. Guard the migration read: only attempt it when LEGACY_PROPERTY_TOKEN is non-empty, otherwise skip seeding (candidate starts at 1) and/or emit a clear message.

✅ Edge Case: Re-run after pre-marker failure deadlocks and can never claim a number

📄 get-build-number/acquire_run_lock.sh:20 📄 get-build-number/acquire_run_lock.sh:29 📄 get-build-number/acquire_run_lock.sh:34-48 📄 get-build-number/get_build_number.sh:80-84 📄 get-build-number/action.yml:48-62
GITHUB_RUN_ID is stable across workflow re-runs (only run_attempt changes) and refs/build-run-locks/<run_id> is created atomically and never deleted. If the winning job acquires the lock but dies (cancelled, or get_build_number.sh hits a fatal API error / marker-record failure at lines 66-67 or 80-84) before publishing refs/build-runs/<run_id>/<N>, then re-running the workflow re-enters acquire_run_lock.sh with the same run_id, finds the lock already held, waits LOCK_WAIT_MAX_ATTEMPTS for a marker that will never appear, and fails. Every subsequent re-run fails identically, so the run can only succeed by pushing a new commit. This is a regression from the previous read-only check_existing_claim.sh, where a re-run would simply claim a fresh number. Consider cleaning up refs/build-run-locks/<run_id> in an always() step when the claiming job fails, or letting a re-run (GITHUB_RUN_ATTEMPT > 1) fall through to claim a new number when no marker appears before timeout.

✅ Edge Case: Transient gh error during marker polling aborts loser job

📄 get-build-number/acquire_run_lock.sh:36-50
In the wait loop, MARKER=$(gh api ... matching-refs ...) at acquire_run_lock.sh:37 runs under set -euo pipefail. A single transient/network failure of this gh call makes the command substitution fail and immediately aborts the loser job with a non-descriptive error, instead of being retried on the next poll iteration like a genuine 'marker not yet present' case. Consider tolerating a failed poll (e.g. MARKER=$(gh api ... || true)) so transient errors are retried within the bounded wait rather than failing the job outright.

✅ Bug: Lock release never runs on cancellation, contradicting its comment

📄 get-build-number/action.yml:75-88 📄 get-build-number/acquire_run_lock.sh:20-32 📄 get-build-number/acquire_run_lock.sh:53-57
The step's comment says always() is used "so this still runs on cancellation", but the guard is steps.claim.outcome == 'failure' — a step interrupted by a job/workflow cancellation has outcome cancelled, not failure, so the DELETE is skipped and refs/build-run-locks/<run_id> stays held with no marker. Any rerun of that run then takes the loser path in acquire_run_lock.sh (ref already exists), polls for a marker that will never appear, and fails after LOCK_WAIT_MAX_ATTEMPTS — permanently, until the ref is deleted by hand. Note the condition must also exclude the loser case (run-lock succeeded with skip=true), where the lock belongs to another job and must not be deleted.

...and 15 more resolved from earlier reviews

Implementation Status ◻️ 0 of 1 objectives covered

◻️ PREQ-7781 - 0 of 1 objectives covered

This PR does not implement any changes related to claiming build numbers atomically via Git refs.

Other objectives on this issue, possibly covered elsewhere:

• ◻️ Claim build numbers atomically via Git refs instead of verify-and-retry to prevent race conditions in GitHub Stacked PRs

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

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

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

_{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