From 4e88c55a4faa0616a080150882086fd097283de8 Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Tue, 28 Jul 2026 00:15:15 +0200 Subject: [PATCH] Fail the job when altool rejects an upload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit distribute-beta and distribute-release reported green jobs on failed App Store uploads. On LeanBytes/framebison two consecutive betas never reached TestFlight while both runs went green; the only way to see it was reading the raw altool log. The benign-case guard was meant to tolerate a genuine redundant upload — same version and build already on ASC, which Apple reports as ITMS-90189. It matched on "already been (used|uploaded)". Apple phrases the build number collision as "an attribute with a value that has already been used (-19232) The bundle version must be higher than the previously uploaded version", so a real failure needing a higher CFBundleVersion hit the same branch and was treated as an idempotent re-run. RC was captured from altool and never checked, leaving the decision purely textual. Narrow the benign pattern to ITMS-90189 / "redundant binary upload", and accept RC == 0 up front so a success with unexpected wording is not misread as a failure. The logic was duplicated inline in both workflows, which is how one defect came to exist in two places, so extract it as classify_upload in .github/scripts/classify-upload.sh and source it from each. tests/run.sh covers the three outcomes against real altool output, sourcing the shipped script so the test cannot drift from what runs. The collision case is verbatim from the run that shipped green. Closes #2 --- .github/scripts/classify-upload.sh | 32 ++++++++++++++++++++++++ .github/workflows/distribute-beta.yml | 11 ++++---- .github/workflows/distribute-release.yml | 11 ++++---- tests/run.sh | 20 ++++++++++++++- 4 files changed, 61 insertions(+), 13 deletions(-) create mode 100755 .github/scripts/classify-upload.sh diff --git a/.github/scripts/classify-upload.sh b/.github/scripts/classify-upload.sh new file mode 100755 index 0000000..d4ffe84 --- /dev/null +++ b/.github/scripts/classify-upload.sh @@ -0,0 +1,32 @@ +#!/usr/bin/env bash +# Classify the result of `xcrun altool --upload-app`. +# +# Sourced by the publish steps of distribute-beta.yml and distribute-release.yml +# so both paths share one definition of "did this upload actually land?" — the +# logic used to be duplicated inline in each, and drifted into the same bug twice. +# +# classify_upload → one of: +# accepted ASC took the binary +# already-present the identical version+build is already there; safe to skip +# failed anything else; the caller must fail the job +classify_upload() { + local rc="$1" out="$2" + + if [ "$rc" -eq 0 ] || grep -qE "UPLOAD SUCCEEDED|No errors uploading" <<<"$out"; then + echo accepted; return + fi + + # Benign ONLY for a true redundant upload — same version AND same build already + # on ASC, which Apple reports as ITMS-90189 / "Redundant Binary Upload". + # + # Deliberately does NOT match the looser "already been used". Apple phrases the + # -19232 build-number collision as "an attribute with a value that has already + # been used", and that is a real failure needing a higher CFBundleVersion. The + # wider pattern swallowed it as an idempotent re-run and reported the job green, + # so failed uploads looked like successful ones. + if grep -qiE "ITMS-90189|redundant binary upload" <<<"$out"; then + echo already-present; return + fi + + echo failed +} diff --git a/.github/workflows/distribute-beta.yml b/.github/workflows/distribute-beta.yml index a1b9369..3ac2ba2 100644 --- a/.github/workflows/distribute-beta.yml +++ b/.github/workflows/distribute-beta.yml @@ -448,6 +448,7 @@ jobs: ASC_ISSUER_ID: ${{ secrets.ASC_ISSUER_ID }} run: | set -uo pipefail + source .shared-ci/.github/scripts/classify-upload.sh while IFS=$'\x1f' read -r id pname sub afile aseed bd bas das dap ver mkt label rtag; do { [ "$bas" = "True" ] && [ "$das" = "True" ]; } || continue echo "::group::TestFlight upload — product=$id" @@ -461,12 +462,10 @@ jobs: RC=$? set -e echo "$OUTPUT" - if grep -qE "UPLOAD SUCCEEDED|No errors uploading" <<<"$OUTPUT"; then - echo "TestFlight upload accepted for $id"; echo "::endgroup::"; continue - fi - if grep -qiE "already been (used|uploaded)|redundant binary upload|ITMS-90189" <<<"$OUTPUT"; then - echo "Build for $id already present — treating as success."; echo "::endgroup::"; continue - fi + case "$(classify_upload "$RC" "$OUTPUT")" in + accepted) echo "TestFlight upload accepted for $id"; echo "::endgroup::"; continue ;; + already-present) echo "Build for $id already present — treating as success."; echo "::endgroup::"; continue ;; + esac echo "::error::altool failed for $id (exit ${RC})"; echo "::endgroup::"; exit 1 done < /tmp/products.tsv diff --git a/.github/workflows/distribute-release.yml b/.github/workflows/distribute-release.yml index 0422a98..2f191f8 100644 --- a/.github/workflows/distribute-release.yml +++ b/.github/workflows/distribute-release.yml @@ -412,6 +412,7 @@ jobs: ASC_ISSUER_ID: ${{ secrets.ASC_ISSUER_ID }} run: | set -uo pipefail + source .shared-ci/.github/scripts/classify-upload.sh while IFS=$'\x1f' read -r id pname sub afile aseed clfile bd bas das dap; do { [ "$bas" = "True" ] && [ "$das" = "True" ]; } || continue echo "::group::App Store upload — product=$id" @@ -425,12 +426,10 @@ jobs: RC=$? set -e echo "$OUTPUT" - if grep -qE "UPLOAD SUCCEEDED|No errors uploading" <<<"$OUTPUT"; then - echo "App Store upload accepted for $id"; echo "::endgroup::"; continue - fi - if grep -qiE "already been (used|uploaded)|redundant binary upload|ITMS-90189" <<<"$OUTPUT"; then - echo "Build for $id already present — treating as success."; echo "::endgroup::"; continue - fi + case "$(classify_upload "$RC" "$OUTPUT")" in + accepted) echo "App Store upload accepted for $id"; echo "::endgroup::"; continue ;; + already-present) echo "Build for $id already present — treating as success."; echo "::endgroup::"; continue ;; + esac echo "::error::altool failed for $id (exit ${RC})"; echo "::endgroup::"; exit 1 done < /tmp/products.tsv diff --git a/tests/run.sh b/tests/run.sh index 5053f57..0777df3 100644 --- a/tests/run.sh +++ b/tests/run.sh @@ -99,5 +99,23 @@ echo "== validation: two empty-id products → hard error ==" CAP PRODUCTS_DIR="$DUAL" python3 "$PY" discover { [ $RC -ne 0 ] && grep -q "at most one product may omit" /tmp/pd.err; } && pass "dual-bare rejected" || bad "dual-bare should fail with the one-primary error (rc=$RC)" +echo "== classify_upload: altool outcome classification ==" +# Sourced from the shipped script rather than re-implemented, so this test cannot +# drift from what the publish steps actually run. +source "$ROOT/.github/scripts/classify-upload.sh" +cls() { GOT=$(classify_upload "$2" "$3"); [ "$GOT" = "$4" ] && pass "$1" || { echo " FAIL: $1 — got '$GOT', want '$4'"; FAIL=1; }; } + +cls "clean success → accepted" 0 \ + "UPLOAD SUCCEEDED with no errors +No errors uploading archive at './App.pkg'." accepted +cls "exit 0, quiet output → accepted" 0 "Uploading... done" accepted +# Verbatim from the FrameBison run that reported green while the upload failed. +cls "build-number collision (-19232) → failed" 31 \ + "ERROR: [ContentDelivery.Uploader.7814C25280] The provided entity includes an attribute with a value that has already been used (-19232) The bundle version must be higher than the previously uploaded version: '1'. +ERROR: [altool.main] ExitFailure (31)" failed +cls "true redundant upload (ITMS-90189) → already-present" 31 \ + "ERROR: [altool] Redundant Binary Upload. There already exists a binary upload with build version '42' (ITMS-90189)" already-present +cls "opaque altool error → failed" 1 "ERROR: [altool.main] network unreachable" failed + echo -[ $FAIL -eq 0 ] && echo "ALL products.py TESTS PASSED ✅" || { echo "SOME TESTS FAILED ❌"; exit 1; } +[ $FAIL -eq 0 ] && echo "ALL TESTS PASSED ✅" || { echo "SOME TESTS FAILED ❌"; exit 1; }