Fail the Artifact Registry image cleanup when gcloud fails

.github/scripts/cleanup_cloud_run_images.sh

@@ -18,6 +18,11 @@
 # The target repo (region/project/repository) comes from cloud_run_env.sh — the
 # same config the deploy scripts source — so this can never prune a different
 # repo than the pipeline actually pushes to.
+#
+# A failed listing or deletion fails the script, and each failed deletion is
+# logged with gcloud's error. Deleting an image needs
+# artifactregistry.versions.delete on the repo, and artifactregistry.tags.delete
+# too when it is tagged; roles/artifactregistry.writer has neither.
 
 set -euo pipefail
 
@@ -33,15 +38,22 @@ AR_REPO="${AR_REPO:-${CLOUD_RUN_ARTIFACT_REPOSITORY}}"
 image_repo="${AR_LOCATION}-docker.pkg.dev/${AR_PROJECT}/${AR_REPO}"
 
 # Deletable refs (<image>@<digest>), newest first — sorted here by createTime
-# (ISO timestamps sort chronologically), one row per image version.
+# (ISO timestamps sort chronologically), one row per image version. The listing
+# is read into a variable first because bash ignores the exit status of a
+# process substitution, so a failed listing would otherwise look like 0 images.
+if ! listing="$(
+  gcloud artifacts docker images list "${image_repo}" \
+    --format="value[separator='|'](createTime,package,version)"
+)"; then
+  echo "::error::Could not list images in ${image_repo}."
+  exit 1
+fi
 refs=()
 while IFS='|' read -r _ctime pkg ver; do
-  [[ -n "${ver}" ]] && refs+=("${pkg}@${ver}")
-done < <(
-  gcloud artifacts docker images list "${image_repo}" \
-    --format="value[separator='|'](createTime,package,version)" \
-    | sort -r
-)
+  if [[ -n "${ver}" ]]; then
+    refs+=("${pkg}@${ver}")
+  fi
+done < <(printf '%s\n' "${listing}" | sort -r)
 
 total="${#refs[@]}"
 to_delete=()
@@ -54,13 +66,26 @@ echo "Images:    ${total} (keeping newest ${KEEP})"
 echo "Deleting:  ${#to_delete[@]}"
 
 deleted=0
+failed=0
 for ref in "${to_delete[@]:-}"; do
   [[ -z "${ref}" ]] && continue
   if [[ "${DRY_RUN}" == "1" ]]; then
     echo "[dry-run] would delete ${ref}"
-  elif gcloud artifacts docker images delete "${ref}" --delete-tags --quiet \
-    >/dev/null 2>&1; then
+  elif output="$(gcloud artifacts docker images delete "${ref}" --delete-tags \
+    --quiet 2>&1)"; then
     deleted=$((deleted + 1))
+  else
+    failed=$((failed + 1))
+    echo "Failed to delete ${ref}:"
+    while IFS= read -r error_line; do
+      echo "  ${error_line}"
+    done <<< "${output}"
   fi
 done
-[[ "${DRY_RUN}" == "1" ]] || echo "Deleted ${deleted}/${#to_delete[@]} image(s)."
+if [[ "${DRY_RUN}" != "1" ]]; then
+  echo "Deleted ${deleted}/${#to_delete[@]} image(s)."
+fi
+if [[ "${failed}" -gt 0 ]]; then
+  echo "::error::Failed to delete ${failed}/${#to_delete[@]} image(s); the gcloud errors are above."
+  exit 1
+fi

changelog.d/ar-image-cleanup-errors.fixed.md

@@ -0,0 +1 @@
+The Artifact Registry image cleanup that runs after a production deploy now fails, and logs the gcloud error for each image it could not delete, instead of reporting success when it lists or deletes nothing.

tests/unit/test_workflow_guard_scripts.py

@@ -51,8 +51,18 @@ def test_require_git_ref_fails_with_an_annotation_on_another_ref() -> None:
     assert result.stdout == "::error::Cloud Run secret sync must run from master.\n"
 
 
-def _fake_gcloud(tmp_path: Path, rows: list[str], *, delete_exit: int = 0) -> Path:
-    """Return a bin dir whose fake gcloud lists ``rows`` and logs every other call."""
+def _fake_gcloud(
+    tmp_path: Path,
+    rows: list[str],
+    *,
+    failing_refs: tuple[str, ...] = (),
+    list_exit: int = 0,
+) -> Path:
+    """Return a bin dir whose fake gcloud lists ``rows`` and logs every other call.
+
+    Deleting a ref in ``failing_refs`` prints an error and exits 1, and the
+    listing exits with ``list_exit``.
+    """
     bin_dir = tmp_path / "bin"
     bin_dir.mkdir()
     listing = tmp_path / "listing.txt"
@@ -63,10 +73,16 @@ def _fake_gcloud(tmp_path: Path, rows: list[str], *, delete_exit: int = 0) -> Pa
         "#!/usr/bin/env bash\n"
         'if [[ "$1 $2 $3 $4" == "artifacts docker images list" ]]; then\n'
         f'  cat "{listing}"\n'
-        "  exit 0\n"
+        f"  exit {list_exit}\n"
         "fi\n"
         f'echo "$*" >> "{calls}"\n'
-        f"exit {delete_exit}\n"
+        f"for failing_ref in {' '.join(failing_refs)}; do\n"
+        '  if [[ "$5" == "${failing_ref}" ]]; then\n'
+        '    echo "ERROR: PERMISSION_DENIED on $5" >&2\n'
+        "    exit 1\n"
+        "  fi\n"
+        "done\n"
+        'echo "Delete request issued for $5" >&2\n'
     )
     gcloud.chmod(0o755)
     return bin_dir
@@ -80,9 +96,16 @@ def _fake_gcloud(tmp_path: Path, rows: list[str], *, delete_exit: int = 0) -> Pa
 
 
 def _cleanup(
-    tmp_path: Path, rows: list[str], **env: str
+    tmp_path: Path,
+    rows: list[str],
+    *,
+    failing_refs: tuple[str, ...] = (),
+    list_exit: int = 0,
+    **env: str,
 ) -> tuple[subprocess.CompletedProcess[str], list[str]]:
-    bin_dir = _fake_gcloud(tmp_path, rows, delete_exit=int(env.pop("DELETE_EXIT", "0")))
+    bin_dir = _fake_gcloud(
+        tmp_path, rows, failing_refs=failing_refs, list_exit=list_exit
+    )
     result = _run(
         CLEANUP_IMAGES,
         {"PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", **env},
@@ -120,9 +143,25 @@ def test_cleanup_keeps_everything_within_the_limit(tmp_path: Path) -> None:
     assert "Deleting:  0" in result.stdout
 
 
-def test_cleanup_counts_only_successful_deletions(tmp_path: Path) -> None:
-    result, calls = _cleanup(tmp_path, IMAGES, KEEP="1", DELETE_EXIT="1")
+def test_cleanup_reports_each_failed_deletion_and_fails(tmp_path: Path) -> None:
+    result, calls = _cleanup(
+        tmp_path, IMAGES, failing_refs=("repo/api@sha256:oldest",), KEEP="1"
+    )
 
-    assert result.returncode == 0, result.stderr
+    assert result.returncode == 1
     assert len(calls) == 2
-    assert "Deleted 0/2 image(s)." in result.stdout
+    assert "Failed to delete repo/api@sha256:oldest:\n" in result.stdout
+    assert "  ERROR: PERMISSION_DENIED on repo/api@sha256:oldest\n" in result.stdout
+    assert "Failed to delete repo/api@sha256:middle" not in result.stdout
+    assert "Delete request issued" not in result.stdout
+    assert "Deleted 1/2 image(s)." in result.stdout
+    assert "::error::Failed to delete 1/2 image(s)" in result.stdout
+
+
+def test_cleanup_fails_when_the_listing_fails(tmp_path: Path) -> None:
+    result, calls = _cleanup(tmp_path, IMAGES, list_exit=1, KEEP="1")
+
+    assert result.returncode == 1
+    assert calls == []
+    assert "::error::Could not list images in " in result.stdout
+    assert "Images:" not in result.stdout