Address post-merge review feedback on immutable per-arch tags

.github/workflows/publish.yml

@@ -44,8 +44,8 @@ jobs:
           # auto-picked by the release workflow. Strip leading "v" and the
           # trailing "-<N>" to derive the stellar-cli version; the refresh
           # index N (0 when there's no suffix, i.e. the first release) names
-          # the immutable :<version>-<N> Docker tag published by the aliases
-          # job (see issue #38).
+          # the immutable :<cli>-rust<key>-<arch>-<N> Docker tags minted by the
+          # manifest job (see issue #38).
           no_prefix="${RELEASE_TAG#v}"
           version="${no_prefix%%-*}"
           test -n "$version" || { echo "::error::could not determine stellar_cli_version from release tag '$RELEASE_TAG'"; exit 1; }

RELEASE.md

@@ -56,7 +56,7 @@ Every release gets a unique tag. Tags are never reused or updated in place.
 - **First release of a stellar-cli version**: `v<version>-0` (e.g. `v26.0.0-0`).
 - **Refresh of the same stellar-cli version**: `v<version>-<N>` with `N` incrementing per refresh (e.g. `v26.0.0-1`, `v26.0.0-2`).
 
-The `-N` index lines up one-to-one with the immutable `:<cli>-rust<key>-<arch>-<N>` Docker tags, starting at `-0`. The `release` workflow picks the next available `-N` automatically from **both** existing releases and existing `release/*` branches — so an iteration that's been prepared (branch/PR merged) but whose GitHub Release hasn't been published yet never gets its number reused. Reuse would republish those immutable tags over different digests and defeat their immutability. Each release page is the snapshot of `builds.json` at that iteration; the historical `v26.0.0-0` page stays intact when `v26.0.0-1` is later published.
+The `-N` index lines up one-to-one with the immutable `:<cli>-rust<key>-<arch>-<N>` Docker tags, starting at `-0`. The `release` workflow picks the next available `-N` automatically from **both** existing releases and open `release/*` branches — so an iteration that's been prepared (branch/PR open) but not yet released never gets its number reused while it's in review. Reuse would republish those immutable tags over different digests and defeat their immutability. (The branch is auto-deleted on merge; publishing the GitHub Release follows merge immediately, so there's no practical window to reuse a merged-but-unpublished iteration's number.) Each release page is the snapshot of `builds.json` at that iteration; the historical `v26.0.0-0` page stays intact when `v26.0.0-1` is later published.
 
 > A handful of early releases predate this scheme and use a suffixless `v<version>` tag (e.g. `v25.1.0`); those count as iteration 0, so the next refresh of such a version is `-1`.
 
@@ -143,9 +143,11 @@ Triggered exclusively by the `release: published` event — when a maintainer cl
 
 Per-architecture tags (`:<cli>-rust<key>-<arch>`) and multi-arch manifest lists (`:<cli>-rust<key>`) on Docker Hub are **mutable** — re-publishing a `(cli, rust base)` pair overwrites the tag in place. Reproducibility is anchored by the per-arch image content digest and by the `builds.json` pins, not by tag stability.
 
-Moving aliases (`:<cli>`, `:latest`) re-point each release. The immutable `:<cli>-rust<key>-<arch>-<N>` snapshots are the exception — they're keyed by the release's refresh index, so a re-run recreates the same tags at the same digests rather than moving them.
+Moving aliases (`:<cli>`, `:latest`) re-point each release. The immutable `:<cli>-rust<key>-<arch>-<N>` snapshots are the exception — they're keyed by the release's refresh index and, by design, never move: the `manifest` job leaves an existing `:…-<N>` tag alone when it already pins the same digest and **fails loudly** if a re-run built a different digest, rather than clobbering an on-chain `bldimg` anchor.
 
-To recover from a failed run, use **Re-run failed jobs** from the GitHub Actions UI; re-runs simply rebuild and overwrite. Recovering from a corrupt push is the same — just re-run, no manual tag deletion needed.
+To recover from a failed run, use **Re-run failed jobs** from the GitHub Actions UI. This re-runs against the same release event, so the tag and its refresh index `N` are unchanged — no new GitHub Release is created. Re-running only the failed downstream jobs (`manifest`, `aliases`, `release`) reuses the per-arch images already pushed by `build` and just overwrites the mutable tags; no manual tag deletion is needed.
+
+Re-running the `build` job itself is different: builds are not byte-reproducible (`BUILD_DATE` is the run's wall-clock time), so a rebuild generally produces a **new** per-arch digest. The mutable tags overwrite fine, but the `manifest` job will then refuse to re-point the already-created immutable `:…-<N>` snapshot and fail. That guard is intentional — it protects the digest a contract may already pin. If you truly need to replace a published iteration's content, cut a **new** refresh iteration (`v<cli>-<N+1>`) instead of rebuilding an existing one.
 
 ## Backfilling immutable per-arch tags for older releases
 

scripts/backfill_iteration_tags.py

@@ -26,17 +26,23 @@
      digests are recoverable here.
 
 For the given cli:
-  1. Resolve iteration `N` — the highest `v<cli>[-N]` release tag. The mutable
-     per-arch tags reflect that newest iteration's content, which is all the
-     registry still exposes (superseded iterations were orphaned when overwritten
-     and cannot be recovered).
+  1. Resolve iteration `N` — the highest `v<cli>[-N]` release tag, or an explicit
+     `--iteration`. The mutable per-arch tags reflect that newest iteration's
+     content, which is all the registry still exposes (superseded iterations were
+     orphaned when overwritten and cannot be recovered). Auto-resolving assumes
+     the newest release's publish reached the build+push step; if it failed
+     *before* pushing images the live tags still hold an earlier iteration's
+     content, so pass `--iteration <N>` to label it correctly instead of
+     mislabeling it as the newest N.
   2. Read the index digest each current `:<cli>-rust<key>-<arch>` tag exposes
      (the tag's own top-level digest — the same `bldimg` anchor the publish
      workflow records, not the child per-platform submanifest).
   3. `docker buildx imagetools create` an immutable `:<cli>-rust<key>-<arch>-<N>`
      tag for each digest, re-referencing it so it can no longer become untagged.
 
-Per-arch tags that already exist are skipped, so the script is safe to re-run.
+A snapshot tag that already pins the same digest is skipped, so the script is
+safe to re-run; one that exists pinning a *different* digest fails loudly rather
+than being silently clobbered — that would be an immutability violation.
 """
 
 import argparse
@@ -140,6 +146,17 @@ def build_parser() -> argparse.ArgumentParser:
     parser.add_argument("--stellar-cli-version", required=True, metavar="V")
     parser.add_argument("--registry", default="docker.io/stellar/stellar-cli", metavar="REF")
     parser.add_argument("--repo", default="stellar/stellar-cli-docker", metavar="SLUG")
+    parser.add_argument(
+        "--iteration",
+        type=int,
+        metavar="N",
+        help=(
+            "Iteration index to label the recovered snapshots with. Defaults to "
+            "the highest v<cli>[-N] release tag. Override when the newest "
+            "release's publish failed before pushing images, so the live per-arch "
+            "tags still hold an earlier iteration's content."
+        ),
+    )
     parser.add_argument(
         "--dry-run",
         action="store_true",
@@ -148,16 +165,40 @@ def build_parser() -> argparse.ArgumentParser:
     return parser
 
 
+def resolve_iteration(args: argparse.Namespace, cli: str) -> int:
+    """The iteration index to label recovered snapshots with.
+
+    An explicit `--iteration` wins. Otherwise it's the newest `v<cli>[-N]`
+    release, which assumes that release's publish reached build+push so the live
+    per-arch tags hold its content — a loud warning flags the assumption so an
+    operator recovering from a publish that failed before pushing knows to pass
+    `--iteration <N>` instead of mislabeling an earlier iteration as the newest.
+    """
+    if args.iteration is not None:
+        return args.iteration
+    iteration = latest_iteration(gh_cli.list_release_tags(args.repo), cli)
+    if iteration is None:
+        common.die(f"no published releases found for stellar-cli {cli}")
+    common.log(
+        f"labeling recovered snapshots as iteration {iteration} (newest "
+        f"v{cli}[-N] release); this assumes that release's publish pushed its "
+        f"per-arch images. If it failed before the build/push step, the live "
+        f"tags still hold an earlier iteration — re-run with --iteration <N> to "
+        f"pin the correct one."
+    )
+    return iteration
+
+
 def main(argv: list[str] | None = None) -> int:
     args = build_parser().parse_args(argv)
+    if args.iteration is not None and args.iteration < 0:
+        common.die(f"--iteration must be non-negative, got {args.iteration}")
     common.preflight_checks(["buildx", "gh"])
 
     cli = args.stellar_cli_version
     registry = args.registry
 
-    iteration = latest_iteration(gh_cli.list_release_tags(args.repo), cli)
-    if iteration is None:
-        common.die(f"no published releases found for stellar-cli {cli}")
+    iteration = resolve_iteration(args, cli)
 
     repo_path = dockerhub.repo_path(registry)
     pairs = current_pairs(dockerhub.list_tags(repo_path), cli)
@@ -168,11 +209,19 @@ def main(argv: list[str] | None = None) -> int:
     skipped = 0
     for (key, arch), digest in sorted(pairs.items()):
         target = f"{registry}:{cli}-rust{key}-{arch}-{iteration}"
-        if docker_inspect.exists(target):
-            common.log(f"skip {target}: already tagged")
-            skipped += 1
-            continue
         source = f"{registry}@{digest}"
+        if docker_inspect.exists(target):
+            existing = docker_inspect.index_digest(target)
+            if existing == digest:
+                common.log(f"skip {target}: already pins {digest}")
+                skipped += 1
+                continue
+            common.die(
+                f"{target} already exists pinning {existing}, but the live "
+                f"per-arch tag now exposes {digest}; refusing to re-point an "
+                f"immutable tag. If a newer iteration has since published, pass "
+                f"--iteration for the correct index."
+            )
         common.log(f"::group::backfill {target} -> {source}")
         if args.dry_run:
             common.log(f"docker buildx imagetools create --tag {target} {source}")

scripts/lib/gh_cli.py

@@ -30,11 +30,15 @@ def list_release_tags(repo: str) -> list[str]:
 def list_release_branch_tags(repo: str) -> list[str]:
     """Release tags of the `release/<tag>` branches that exist on the repo.
 
-    A release branch is created at prepare time and persists across the
-    merge -> publish gap (merging the PR doesn't publish the GitHub
-    Release). Consulting it stops the tag picker from reusing an iteration
-    that's already been prepared but not yet published — which would let a
-    later publish overwrite the immutable `:<cli>-rust<key>-<arch>-<N>` tags.
+    A release branch is created at prepare time and exists until its release PR
+    is merged. Consulting it stops the tag picker from reusing an iteration
+    that's been prepared (branch pushed, PR not yet merged) but not yet released
+    — which would let a later publish overwrite the immutable
+    `:<cli>-rust<key>-<arch>-<N>` tags.
+
+    The repo auto-deletes the branch on merge, so this covers the review window
+    (prepare -> merge); the normal flow publishes the GitHub Release right after
+    merge, so the brief merge -> publish gap isn't separately guarded here.
     """
     out = runner.capture(
         [

tests/unit/test_backfill_iteration_tags.py

@@ -125,6 +125,14 @@ def _wire_main(monkeypatch: pytest.MonkeyPatch, *, existing: set[str], releases=
     monkeypatch.setattr(backfill.gh_cli, "list_release_tags", lambda repo: releases or ["v25.1.0"])
     monkeypatch.setattr(backfill.dockerhub, "list_tags", lambda repo_path: _hub_tags())
     monkeypatch.setattr(backfill.docker_inspect, "exists", lambda ref: ref in existing)
+
+    # An already-existing snapshot pins the same index digest its live per-arch
+    # tag exposes — the safe, re-runnable case, so `main` skips it. Tests that
+    # want a re-point conflict patch index_digest to return something else.
+    def _index_digest(ref: str) -> str:
+        return ARM64_INDEX if ref.rsplit("-", 1)[0].endswith("arm64") else AMD64_INDEX
+
+    monkeypatch.setattr(backfill.docker_inspect, "index_digest", _index_digest)
     created = MagicMock()
     monkeypatch.setattr(backfill.docker_inspect, "create_manifest", created)
     return created
@@ -160,6 +168,8 @@ def test_main_uses_highest_release_iteration(monkeypatch: pytest.MonkeyPatch) ->
 
 
 def test_main_skips_already_tagged_arches(monkeypatch: pytest.MonkeyPatch) -> None:
+    # amd64's snapshot already exists pinning the same digest → skip; arm64's is
+    # created.
     created = _wire_main(monkeypatch, existing={_arch_tag("amd64")})
 
     rc = backfill.main(["--stellar-cli-version", "25.1.0", "--registry", "reg/img"])
@@ -170,6 +180,41 @@ def test_main_skips_already_tagged_arches(monkeypatch: pytest.MonkeyPatch) -> No
     assert _arch_tag("arm64") in tags
 
 
+def test_main_refuses_to_repoint_existing_snapshot(monkeypatch: pytest.MonkeyPatch) -> None:
+    # A snapshot that already exists pinning a *different* digest than the live
+    # per-arch tag is an immutability violation — fail loudly, don't clobber.
+    _wire_main(monkeypatch, existing={_arch_tag("amd64")})
+    monkeypatch.setattr(backfill.docker_inspect, "index_digest", lambda ref: "sha256:" + "0" * 64)
+
+    with pytest.raises(SystemExit):
+        backfill.main(["--stellar-cli-version", "25.1.0", "--registry", "reg/img"])
+
+
+def test_main_accepts_explicit_iteration(monkeypatch: pytest.MonkeyPatch) -> None:
+    # Newest release is -1, but --iteration pins the live content to 0 (e.g. the
+    # -1 publish failed before pushing, so the live tags still hold -0's images).
+    created = _wire_main(monkeypatch, existing=set(), releases=["v25.1.0", "v25.1.0-1"])
+
+    rc = backfill.main(
+        ["--stellar-cli-version", "25.1.0", "--registry", "reg/img", "--iteration", "0"]
+    )
+
+    assert rc == 0
+    tags = [call.args[0] for call in created.call_args_list]
+    assert _arch_tag("amd64", 0) in tags
+    assert _arch_tag("arm64", 0) in tags
+    assert _arch_tag("amd64", 1) not in tags
+
+
+def test_main_rejects_negative_iteration(monkeypatch: pytest.MonkeyPatch) -> None:
+    _wire_main(monkeypatch, existing=set())
+
+    with pytest.raises(SystemExit):
+        backfill.main(
+            ["--stellar-cli-version", "25.1.0", "--registry", "reg/img", "--iteration", "-1"]
+        )
+
+
 def test_main_dry_run_creates_nothing(monkeypatch: pytest.MonkeyPatch) -> None:
     created = _wire_main(monkeypatch, existing=set())