From 9f3e0b8cd34fbc15d4c38cccde1721db3bda5d15 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 9 Sep 2026 21:51:41 -0400 Subject: [PATCH] feat(version): sidecar and /api/client/version carry name, code and channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The client compares names while Android installs by versionCode, and the wire had no way to close that gap: the sidecar was one positional line and the endpoint returned a name only. This is the plumbing that makes the ordering key decidable by the client at all. The sidecar is now JSON rather than a grown positional string. That shape was chosen against a specific failure: the obvious growth path was " ", which a first-space split silently mangles the moment a third field appears — the code stops parsing as an integer and the reader falls back to name comparison WITHOUT erroring. JSON cannot mistake a new field for an old one. code is a POINTER on both sides, and omitempty on the wire. Absent has to stay distinguishable from zero: a build published before ordering keys were recorded genuinely has no code, and zero would claim it is infinitely old rather than unknown. A malformed sidecar now fails loudly instead of serving a blank version. If an unreadable file produced an empty name, every client would compare against nothing, conclude it was current, and go quiet — "I cannot read this" and "there is nothing newer" would return the same answer, which is the failure mode nobody reports because nobody is offered anything to report. The non-tag :latest path no longer RECONSTRUCTS the bundled APK's version. android-release now publishes the sidecar as a release asset beside the APK, and the image build downloads it. The old reconstruction duplicated a derivation formula across two files, and could only ever recover the name — the ordering key is build-time minutes and exists nowhere once that build ends. Releases predating the sidecar report their name with a null code, which is the honest answer rather than a guessed one. image-release also drops to a shallow checkout: it needed full history and tags only to re-derive versions from the tagged commit, and now touches git for nothing. MINSTREL_VERSION comes from GITHUB_REF. Two things checked rather than assumed. The Android Json sets ignoreUnknownKeys, so the added fields cannot break already-installed apps. It also sets coerceInputValues, which will silently turn a null code into 0 if step 4 declares the field non-nullable — recorded on task #3811, because reading the field declaration alone would never reveal it. Also fixes a stale comment block describing "the Flutter client", deleted in v2026.08.18. Step 3 of 5 — Scribe task #3810, milestone #390. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- .gitea/workflows/release.yml | 104 +++++++++++++++++++---------- internal/api/client_assets.go | 61 +++++++++++++++-- internal/api/client_assets_test.go | 81 ++++++++++++++++++++-- 3 files changed, 198 insertions(+), 48 deletions(-) diff --git a/.gitea/workflows/release.yml b/.gitea/workflows/release.yml index bd2418b2..c0bcdfaa 100644 --- a/.gitea/workflows/release.yml +++ b/.gitea/workflows/release.yml @@ -39,10 +39,9 @@ name: release # :latest (not just tags), a main build with no APK would silently strip # the in-app update channel off :latest until the next release. So on # non-tag builds image-release pulls the MOST RECENT release's signed APK -# and reconstructs its exact versionName from the tagged commit's timestamp -# (the same derivation android-release bakes in) for the version sidecar — -# no rebuild, just rebundle. Tag builds keep bundling their own -# freshly-built APK. +# AND the version sidecar published beside it — the recorded values, not +# recomputed ones — so no rebuild is needed, just a rebundle. Tag builds +# keep bundling their own freshly-built APK. # # Android testing (lint + detekt + unit tests, debug APK upload on main) # lives in android.yml and runs independently on every push. @@ -227,6 +226,8 @@ jobs: shell: bash env: CI_TOKEN: ${{ secrets.CI_TOKEN }} + VERSION_NAME: ${{ steps.ver.outputs.name }} + VERSION_CODE: ${{ steps.ver.outputs.code }} run: | set -euxo pipefail TAG="${GITHUB_REF#refs/tags/}" @@ -234,6 +235,20 @@ jobs: APK_PATH="app/build/outputs/apk/release/app-release.apk" ls -lh "${APK_PATH}" + # Publish the version sidecar as a release asset next to the APK. + # + # This is what lets a later :latest build stop RECONSTRUCTING the + # bundled APK's version and simply read what was recorded. The + # ordering key in particular cannot be re-derived after the fact — + # it is build-time minutes, so once this job ends the value exists + # nowhere else. Reconstruction could only ever recover the name, + # and only by duplicating a formula that then has to be kept in + # step across two files. + SIDECAR_PATH="/tmp/minstrel.apk.version" + printf '{"name":"%s","code":%s,"channel":"stable"}\n' \ + "${VERSION_NAME}" "${VERSION_CODE}" > "${SIDECAR_PATH}" + cat "${SIDECAR_PATH}" + RELEASE_JSON="$(curl -fsSL \ -H "Authorization: token ${CI_TOKEN}" \ "https://git.fabledsword.com/api/v1/repos/${REPO}/releases/tags/${TAG}")" @@ -255,6 +270,20 @@ jobs: exit 1 fi + # Same treatment for the sidecar. Named `.apk.version` so the + # downloader's `\.apk$` match cannot pick it up by mistake. + SIDECAR_HTTP=$(curl -sS -L -o /tmp/upload-sidecar.out -w '%{http_code}' \ + -H "Authorization: token ${CI_TOKEN}" \ + -F "attachment=@${SIDECAR_PATH}" \ + "https://git.fabledsword.com/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets?name=minstrel-${TAG}.apk.version") + echo "sidecar_upload_http=${SIDECAR_HTTP}" + cat /tmp/upload-sidecar.out || true + echo + if [ "${SIDECAR_HTTP}" -lt 200 ] || [ "${SIDECAR_HTTP}" -ge 300 ]; then + echo "::error::version sidecar upload returned HTTP ${SIDECAR_HTTP}" + exit 1 + fi + image-release: name: Build + push container image # `needs:` waits for android-release. For tag pushes android-release @@ -275,12 +304,11 @@ jobs: - name: Checkout uses: actions/checkout@v4 with: - # Full history + tags so non-tag :latest builds can resolve the - # latest release tag's commit and reconstruct the bundled APK's - # exact versionName from its timestamp (see "Bundle latest release - # APK" below). - fetch-depth: 0 - fetch-tags: true + # Shallow is fine here. This job used to need full history + tags to + # re-derive the bundled APK's version from the tagged commit; it now + # downloads the sidecar the release recorded, and touches git for + # nothing. MINSTREL_VERSION comes from GITHUB_REF, not from git. + fetch-depth: 1 - name: Detect buildable project id: guard @@ -346,29 +374,30 @@ jobs: if: steps.guard.outputs.ready == 'true' && startsWith(github.ref, 'refs/tags/v') shell: bash env: - # Pulled from android-release.outputs.version_name so the - # sidecar string the server hands clients matches the - # versionName baked into the APK they're comparing against. + # Both pulled from android-release's outputs so the sidecar the + # server hands clients matches exactly what is baked into the APK + # they are comparing against. APK_VERSION_NAME: ${{ needs.android-release.outputs.version_name }} + APK_VERSION_CODE: ${{ needs.android-release.outputs.version_code }} run: | set -euxo pipefail # The artifact lands as `app-release.apk` (the original Gradle # output name). The Dockerfile COPYs client/* into /app/client/ # and the server reads minstrel.apk + minstrel.apk.version. mv client/app-release.apk client/minstrel.apk - echo "${APK_VERSION_NAME}" > client/minstrel.apk.version + printf '{"name":"%s","code":%s,"channel":"stable"}\n' \ + "${APK_VERSION_NAME}" "${APK_VERSION_CODE}" > client/minstrel.apk.version + cat client/minstrel.apk.version ls -lh client/ - name: Bundle latest release APK (non-tag :latest builds) # Main pushes don't build an APK, but they DO move :latest — so # without this the in-app update channel would vanish from :latest # until the next tag. Pull the most-recent release's signed APK and - # reconstruct its exact versionName from the tagged commit's - # timestamp — the same derivation android-release uses — so the - # version sidecar the server hands clients matches the installed - # build. + # 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 / APK asset / tag-count can be resolved. + # version — if no release or APK asset can be resolved. if: steps.guard.outputs.ready == 'true' && !startsWith(github.ref, 'refs/tags/v') shell: bash env: @@ -386,23 +415,28 @@ jobs: if [ -z "${TAG}" ] || [ -z "${APK_URL}" ]; then echo "::notice::latest release '${TAG:-?}' has no APK asset — image ships without bundled APK"; exit 0 fi - # Reconstruct the bundled APK's name with the SAME derivation - # android-release uses — commit timestamp of the tagged commit. The - # two must agree exactly: this string is what the server hands - # clients to compare against what is installed, so a mismatch here - # is an update offered forever or never offered at all. - # - # This duplication is temporary. Once the tag itself becomes - # `v`, this whole block collapses to `${TAG#v}` with - # nothing to recompute and nothing to keep in step. - COMMIT_TS="$(git log --format=%ct -1 "${TAG}" 2>/dev/null || true)" - if [ -z "${COMMIT_TS}" ]; then - echo "::notice::could not resolve commit timestamp for ${TAG} (tag not fetched?) — skipping APK bundle"; exit 0 - fi - VERSION_NAME="$(date -u -d "@${COMMIT_TS}" +%Y.%m.%d.%H%M)" curl -fsSL -H "Authorization: token ${CI_TOKEN}" -o client/minstrel.apk "${APK_URL}" - echo "${VERSION_NAME}" > client/minstrel.apk.version - echo "::notice::bundled release APK ${TAG} as version ${VERSION_NAME}" + + # Take the version the release RECORDED rather than recomputing it. + # This used to re-derive the name from the tagged commit, which meant + # 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)" + if [ -n "${SIDECAR_URL}" ]; then + curl -fsSL -H "Authorization: token ${CI_TOKEN}" -o client/minstrel.apk.version "${SIDECAR_URL}" + cat client/minstrel.apk.version + else + # Releases published before sidecars were attached. Their name is + # still recoverable from the tag, but their ordering key genuinely + # is not — so it is reported ABSENT rather than guessed. A wrong + # key is an install the platform refuses; an absent one just tells + # the client to fall back to comparing names, which is exactly + # what those builds already do. + echo "::notice::release ${TAG} predates the version sidecar — bundling with name only, no ordering key" + printf '{"name":"%s","code":null,"channel":"stable"}\n' "${TAG#v}" > client/minstrel.apk.version + fi + echo "::notice::bundled release APK from ${TAG}" ls -lh client/ - name: Build and push diff --git a/internal/api/client_assets.go b/internal/api/client_assets.go index 1e25dc69..5f7e8c39 100644 --- a/internal/api/client_assets.go +++ b/internal/api/client_assets.go @@ -6,19 +6,21 @@ package api // /app/client/ at image build time. // // Both endpoints are authenticated — the bandwidth cost of the APK -// (~30-60 MB) makes anonymous access an abuse vector. The Flutter -// client's polling only fires after login (banner mounts in the post- -// login shell), so this gate is invisible to the actual update flow. +// (~30-60 MB) makes anonymous access an abuse vector. The client only +// polls after login, so this gate is invisible to the actual update flow. // // /api/client/apk additionally rate-limits per user to a single // download every 60s. Real install flows fire one download per // update; anything tighter is scripted/abusive. // // Returns 404 gracefully when the APK isn't present (dev environments, -// pre-CI-wiring); the Flutter client treats 404 as "no update channel -// available." +// pre-CI-wiring); the client treats 404 as "no update channel available." +// +// (These paragraphs said "the Flutter client" until 2026-09-10. That client +// was deleted in v2026.08.18 — the Android app is the only one now.) import ( + "encoding/json" "errors" "net/http" "os" @@ -84,8 +86,36 @@ func clientAPKAllowDownload(userID string, now time.Time) time.Duration { return 0 } +// clientVersionSidecar is the JSON written beside the bundled APK by +// release.yml. It carries three values that are deliberately separate: +// +// - Name is a LABEL for people, "YYYY.MM.DD.HHMM" from the commit's +// timestamp. Two channels carrying the same code report the same name. +// - Code is the ORDERING KEY, minutes since 2020-01-01 at build time, and +// is the value Android itself installs by. It answers "may this be +// installed over that?" — the name never does. +// - Channel is a SIBLING FIELD, never a suffix inside the name. +// +// JSON rather than a positional line on purpose. The obvious growth path for +// the old one-value file was " ", which a first-space split +// silently mangles the moment a third field appears: the code stops parsing, +// and the reader falls back to name comparison WITHOUT erroring. +type clientVersionSidecar struct { + Name string `json:"name"` + // Pointer, not int64: absent must stay distinguishable from zero. An + // artifact published before codes were recorded genuinely has no code — + // zero would claim it is infinitely old rather than unknown. + Code *int64 `json:"code"` + Channel string `json:"channel"` +} + type clientVersionResponse struct { - Version string `json:"version"` + Version string `json:"version"` + // omitempty on both: the client must be able to tell "this server does + // not report a code" from "this build's code is 0", because those call + // for different behaviour on the other end. + Code *int64 `json:"code,omitempty"` + Channel string `json:"channel,omitempty"` APKURL string `json:"apk_url"` SizeBytes int64 `json:"size_bytes"` } @@ -117,8 +147,25 @@ func (h *handlers) handleClientVersion(w http.ResponseWriter, _ *http.Request) { return } + var sidecar clientVersionSidecar + if err := json.Unmarshal(versionBytes, &sidecar); err != nil { + // Fail LOUDLY rather than serving a blank version. The failure mode + // this avoids is the one that never gets reported: if an unreadable + // sidecar produced an empty name, every client would compare against + // nothing, conclude it was current, and go quiet — "I cannot read + // this" and "there is nothing newer" would be the same answer. + writeErrWithLog(w, h.logger, "client_version: sidecar is not valid JSON", err) + return + } + if sidecar.Name == "" { + http.Error(w, `{"error":{"code":"bad_client_version","message":"version sidecar has no name"}}`, http.StatusInternalServerError) + return + } + writeJSON(w, http.StatusOK, clientVersionResponse{ - Version: strings.TrimSpace(string(versionBytes)), + Version: strings.TrimSpace(sidecar.Name), + Code: sidecar.Code, + Channel: strings.TrimSpace(sidecar.Channel), APKURL: "/api/client/apk", SizeBytes: stat.Size(), }) diff --git a/internal/api/client_assets_test.go b/internal/api/client_assets_test.go index f85f49eb..fdd735ec 100644 --- a/internal/api/client_assets_test.go +++ b/internal/api/client_assets_test.go @@ -77,18 +77,32 @@ func TestClientVersion_404WhenAPKButNoVersion(t *testing.T) { } } -func TestClientVersion_200WithBothFiles(t *testing.T) { +// writeClientAssets stages an APK plus a raw sidecar body, and returns the +// APK's size so callers can assert size_bytes without recomputing it. +func writeClientAssets(t *testing.T, sidecar string) int64 { + t.Helper() dir := withClientAPKDir(t) body := []byte("fake apk content") if err := os.WriteFile(filepath.Join(dir, clientAPKFilename), body, 0o644); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(dir, clientVersionFile), []byte("v2026.05.10\n"), 0o644); err != nil { + if err := os.WriteFile(filepath.Join(dir, clientVersionFile), []byte(sidecar), 0o644); err != nil { t.Fatal(err) } + return int64(len(body)) +} + +func getClientVersion(t *testing.T) *httptest.ResponseRecorder { + t.Helper() h := &handlers{logger: slog.New(slog.NewTextHandler(io.Discard, nil))} rr := httptest.NewRecorder() h.handleClientVersion(rr, httptest.NewRequest(http.MethodGet, "/api/client/version", nil)) + return rr +} + +func TestClientVersion_200WithBothFiles(t *testing.T) { + size := writeClientAssets(t, `{"name":"2026.09.10.1432","code":3523847,"channel":"stable"}`+"\n") + rr := getClientVersion(t) if rr.Code != http.StatusOK { t.Fatalf("want 200, got %d (body: %s)", rr.Code, rr.Body.String()) } @@ -96,14 +110,69 @@ func TestClientVersion_200WithBothFiles(t *testing.T) { if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { t.Fatal(err) } - if resp.Version != "v2026.05.10" { - t.Errorf("version: want trimmed v2026.05.10, got %q", resp.Version) + if resp.Version != "2026.09.10.1432" { + t.Errorf("version: want 2026.09.10.1432, got %q", resp.Version) + } + if resp.Code == nil { + t.Fatal("code: want 3523847, got absent — the client decides on this, so absent means it silently falls back to name comparison") + } + if *resp.Code != 3523847 { + t.Errorf("code: want 3523847, got %d", *resp.Code) + } + if resp.Channel != "stable" { + t.Errorf("channel: want stable, got %q", resp.Channel) } if resp.APKURL != "/api/client/apk" { t.Errorf("apk_url: want /api/client/apk, got %q", resp.APKURL) } - if resp.SizeBytes != int64(len(body)) { - t.Errorf("size_bytes: want %d, got %d", len(body), resp.SizeBytes) + if resp.SizeBytes != size { + t.Errorf("size_bytes: want %d, got %d", size, resp.SizeBytes) + } +} + +// A release published before ordering keys were recorded has a name and +// genuinely no code. That must arrive as ABSENT, not as 0 — zero would claim +// the build is infinitely old and offer an update to everyone forever. +func TestClientVersion_CodeAbsentIsOmittedNotZero(t *testing.T) { + writeClientAssets(t, `{"name":"2026.09.09","code":null,"channel":"stable"}`) + rr := getClientVersion(t) + if rr.Code != http.StatusOK { + t.Fatalf("want 200, got %d (body: %s)", rr.Code, rr.Body.String()) + } + var resp clientVersionResponse + if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { + t.Fatal(err) + } + if resp.Code != nil { + t.Errorf("code: want absent, got %d", *resp.Code) + } + // The wire must omit the key entirely, so a client can distinguish + // "this server reports no code" from "this build's code is 0". + var raw map[string]any + if err := json.Unmarshal(rr.Body.Bytes(), &raw); err != nil { + t.Fatal(err) + } + if _, present := raw["code"]; present { + t.Errorf("code key should be omitted entirely, body was %s", rr.Body.String()) + } +} + +// The failure this guards is the one nobody reports: if an unreadable sidecar +// produced an empty version, every client would compare against nothing, +// decide it was current, and go quiet. "I cannot read this" and "there is +// nothing newer" must not be the same answer. +func TestClientVersion_MalformedSidecarErrorsRatherThanReportingNothing(t *testing.T) { + for _, sidecar := range []string{ + "2026.09.10.1432", // the OLD plain-text format + `{"name":"x",`, // truncated JSON + `{"code":123,"channel":"dev"}`, // valid JSON, no name + "", + } { + writeClientAssets(t, sidecar) + rr := getClientVersion(t) + if rr.Code == http.StatusOK { + t.Errorf("sidecar %q: want an error status, got 200 with body %s", sidecar, rr.Body.String()) + } } }