From 88508b536b54d614131927e399a7af4eb41dd338 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 10 Sep 2026 08:37:23 -0400 Subject: [PATCH] fix(release): a non-matching grep must not kill the rebundle step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first `main` build after the version rework failed, and the bug was mine. :latest was never moved — "Build and push" was skipped — so nothing reached production, but every subsequent main push would have failed the same way. The runner invokes `shell: bash` as `bash -e -o pipefail`. Under pipefail a command substitution reports the FIRST non-zero status in its pipeline, not the last, so VAR="$(printf ... | grep -oP ... | grep -E '\.apk\.version$' | head -1)" exits non-zero when that grep matches nothing, even though `head` succeeded. With -e the step dies AT THE ASSIGNMENT — before reaching the `if` written to handle exactly the empty case. Which is what happened: v2026.09.09 predates sidecar assets, so its `.apk.version` grep matched nothing and the step aborted instead of falling through to the name-only branch I added in 9f3e0b8c for precisely that release. The transition case was described correctly in that commit message and then not handled in code. The other two assignments carried the same latent hazard and had simply never fired, because a release always has a tag_name and an .apk asset. So the step's documented promise — "degrades to an empty client/ (404 update channel) — never a wrong version — if no release or APK asset can be resolved" — was never actually reachable under pipefail. All three now carry `|| true`. Reproduced under the runner's exact shell before fixing: without `|| true` the script exits 1 with no output at all, proving it never reaches the branch; with it, the fallback runs and emits the name-only sidecar. Guarded, since the graceful degradation depends on this and the failure is invisible until the one release that triggers it: the new test asserts every command-substitution grep in that step ends with `|| true`, and was falsified by removing it from the sidecar line. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- .gitea/workflows/release.yml | 18 +++++++--- internal/server/release_version_test.go | 46 +++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/.gitea/workflows/release.yml b/.gitea/workflows/release.yml index 4c7f7f88..bb0ac992 100644 --- a/.gitea/workflows/release.yml +++ b/.gitea/workflows/release.yml @@ -421,7 +421,10 @@ jobs: # the sidecar published beside it, so what the server reports is what # that build actually recorded rather than something re-derived here. # Degrades to an empty client/ (404 update channel) — never a wrong - # version — if no release or APK asset can be resolved. + # version — if no release or APK asset can be resolved. That + # degradation only actually works because the greps below carry + # `|| true`; under the runner's default pipefail a non-matching grep + # kills the step instead of falling through to the empty-case branch. if: steps.guard.outputs.ready == 'true' && github.ref == 'refs/heads/main' shell: bash env: @@ -434,8 +437,15 @@ jobs: if [ -z "${REL_JSON}" ]; then echo "::notice::no published release — image ships without bundled APK"; exit 0 fi - TAG="$(printf '%s' "${REL_JSON}" | grep -oP '"tag_name":\s*"\K[^"]+' | head -1)" - APK_URL="$(printf '%s' "${REL_JSON}" | grep -oP '"browser_download_url":\s*"\K[^"]+' | grep -E '\.apk$' | head -1)" + # `|| true` on every one of these, and it is load-bearing rather + # than defensive habit. The runner already invokes this shell as + # `bash -e -o pipefail`, so a pipeline whose grep matches NOTHING + # exits non-zero even though `head` succeeded — and the step dies at + # the assignment, before ever reaching the `if` written to handle the + # empty case. Every "degrades gracefully" branch below is unreachable + # without this. + TAG="$(printf '%s' "${REL_JSON}" | grep -oP '"tag_name":\s*"\K[^"]+' | head -1)" || true + APK_URL="$(printf '%s' "${REL_JSON}" | grep -oP '"browser_download_url":\s*"\K[^"]+' | grep -E '\.apk$' | head -1)" || true if [ -z "${TAG}" ] || [ -z "${APK_URL}" ]; then echo "::notice::latest release '${TAG:-?}' has no APK asset — image ships without bundled APK"; exit 0 fi @@ -446,7 +456,7 @@ jobs: # the formula lived in two files that had to be kept in step, and it # could only ever recover the name — the ordering key is build-time # minutes and does not exist anywhere after that build ends. - SIDECAR_URL="$(printf '%s' "${REL_JSON}" | grep -oP '"browser_download_url":\s*"\K[^"]+' | grep -E '\.apk\.version$' | head -1)" + SIDECAR_URL="$(printf '%s' "${REL_JSON}" | grep -oP '"browser_download_url":\s*"\K[^"]+' | grep -E '\.apk\.version$' | head -1)" || true if [ -n "${SIDECAR_URL}" ]; then curl -fsSL -H "Authorization: token ${CI_TOKEN}" -o client/minstrel.apk.version "${SIDECAR_URL}" cat client/minstrel.apk.version diff --git a/internal/server/release_version_test.go b/internal/server/release_version_test.go index 892103ff..4dbf2bef 100644 --- a/internal/server/release_version_test.go +++ b/internal/server/release_version_test.go @@ -240,3 +240,49 @@ func TestDevChannel_RebundlePathIsMainOnly(t *testing.T) { t.Errorf("the rebundle step is not gated to main; a dev push would overwrite its own APK\n%s", step) } } + +// The runner invokes `shell: bash` as `bash -e -o pipefail`. That makes any +// command substitution whose grep matches NOTHING fatal at the assignment — +// pipefail reports the grep's non-zero status even though `head` succeeded — +// so the step dies before reaching the `if` written to handle the empty case. +// +// This is not hypothetical. It took down the first `main` build after the +// version rework: the newest release predated sidecar assets, its +// `.apk.version` grep matched nothing, and the step aborted instead of +// falling through to the name-only branch that exists precisely for it. The +// same latent hazard sat on the other two assignments and had simply never +// fired, because their greps always matched. +// +// Pins the property that makes every "degrades gracefully" branch in that +// step reachable at all. +func TestBundleStep_GrepsCannotKillTheStep(t *testing.T) { + body, err := os.ReadFile(filepath.Join(repoRoot(t), ".gitea", "workflows", "release.yml")) + if err != nil { + t.Fatal(err) + } + yaml := string(body) + + i := strings.Index(yaml, "- name: Bundle latest release APK") + if i < 0 { + t.Fatal("no 'Bundle latest release APK' step") + } + step := yaml[i:] + if j := strings.Index(step, "\n - name:"); j >= 0 { + step = step[:j] + } + + for _, line := range strings.Split(step, "\n") { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "#") || !strings.Contains(trimmed, "grep") { + continue + } + // Only assignments from a command substitution can abort the step. + if !strings.Contains(trimmed, `="$(`) { + continue + } + if !strings.HasSuffix(trimmed, "|| true") { + t.Errorf("this assignment dies under pipefail when its grep matches nothing, "+ + "skipping the empty-case branch below it:\n %s", trimmed) + } + } +}