From c2ba4785c86125dde679e5d928cf181ce065232c Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Fri, 30 Oct 2026 20:11:57 +0400 Subject: [PATCH] Fail the Artifact Registry image cleanup when gcloud fails The cleanup job sent gcloud's output to /dev/null and exited 0 whether or not any deletion worked. Run 40968846055 logged "Deleted 0/108 image(s)" and passed: the deploy service account has roles/artifactregistry.writer, which lacks artifactregistry.tags.delete and artifactregistry.versions.delete. Each failed deletion now prints gcloud's error, and the script exits 1 when any deletion fails. A failed listing also exits 1 instead of being read as an empty repository, since bash ignores a process substitution's exit status. The newest-15 retention is unchanged. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/cleanup_cloud_run_images.sh | 45 +++++++++++---- changelog.d/ar-image-cleanup-errors.fixed.md | 1 + tests/unit/test_workflow_guard_scripts.py | 59 ++++++++++++++++---- 3 files changed, 85 insertions(+), 20 deletions(-) create mode 100644 changelog.d/ar-image-cleanup-errors.fixed.md diff --git a/.github/scripts/cleanup_cloud_run_images.sh b/.github/scripts/cleanup_cloud_run_images.sh index 0f9156e8c..4266c0b88 100755 --- a/.github/scripts/cleanup_cloud_run_images.sh +++ b/.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 (@), 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 diff --git a/changelog.d/ar-image-cleanup-errors.fixed.md b/changelog.d/ar-image-cleanup-errors.fixed.md new file mode 100644 index 000000000..ae7035be6 --- /dev/null +++ b/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. diff --git a/tests/unit/test_workflow_guard_scripts.py b/tests/unit/test_workflow_guard_scripts.py index b43585f09..c3c1ecad1 100644 --- a/tests/unit/test_workflow_guard_scripts.py +++ b/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