Compare commits
23
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
f70df9f827 | ||
|
|
4ce47397a9 | ||
|
|
721154847e | ||
|
|
633d4f591f | ||
|
|
f5dd4462de | ||
|
|
31190657d8 | ||
|
|
ecfa056d4d | ||
|
|
f367eeaa9d | ||
|
|
270ad7a71b | ||
|
|
17212e9eb4 | ||
|
|
8f4b76a638 | ||
|
|
aeb8781c4e | ||
|
|
439c8625d5 | ||
|
|
88508b536b | ||
|
|
1d67c160b2 | ||
|
|
90bb3538c6 | ||
|
|
a687ef439c | ||
|
|
eaf4654c0a | ||
|
|
ca1c18bbbb | ||
|
|
68136c64c0 | ||
|
|
9f3e0b8cd3 | ||
|
|
e46c6bcccf | ||
|
|
bfdaed9365 |
+19
-5
@@ -6,9 +6,20 @@
|
||||
**/build
|
||||
web/build
|
||||
|
||||
# Flutter mobile client — built separately on developer machines / Flutter CI.
|
||||
# Including it in the Go build context wastes ~70 files and invalidates the
|
||||
# `COPY . .` layer cache on every Flutter-only change.
|
||||
# The Android client — built by its own job, never from this context. The APK
|
||||
# reaches the image through client/, downloaded as a CI artifact, so nothing
|
||||
# here reads android/ sources.
|
||||
#
|
||||
# This block named `flutter_client/` until 2026-09-10 and lost its PATTERN when
|
||||
# that tree was deleted, leaving a comment describing an exclusion that was no
|
||||
# longer happening. android/ never took its place, so 4.1 MB of Gradle project
|
||||
# has been entering the context and busting the `COPY . .` layer on every
|
||||
# Android-only change.
|
||||
android/
|
||||
|
||||
# Local `make build` output — an 18 MB binary the image never uses, since the
|
||||
# builder stage compiles its own.
|
||||
bin/
|
||||
|
||||
# Docs and IDE noise
|
||||
docs/
|
||||
@@ -26,5 +37,8 @@ docs/
|
||||
!.env.example
|
||||
|
||||
# CI workflow files don't need to ship in the image.
|
||||
.forgejo/
|
||||
.github/
|
||||
#
|
||||
# This said `.forgejo/` and `.github/` — neither of which this repo has. Gitea
|
||||
# Actions reads `.gitea/`, so the one directory that actually exists was the
|
||||
# one not excluded, and every workflow edit invalidated the context.
|
||||
.gitea/
|
||||
|
||||
@@ -80,15 +80,12 @@ jobs:
|
||||
|
||||
- name: Upload debug APK
|
||||
if: github.event_name == 'push' && github.ref == 'refs/heads/main'
|
||||
# Mirrored action, never actions/upload-artifact. @v4+ throws
|
||||
# GHESNotSupportedError client-side on the hostname (no server setting
|
||||
# reaches that check), and @v3 is worse — it reports success while Gitea
|
||||
# serves artifacts back only through the v4 API, so the upload is stored
|
||||
# and invisible to every retrieval path. @v3 is what left 72 unreachable
|
||||
# artifacts on this repo. Pinned by SHA because the mirror auto-syncs;
|
||||
# full URL because DEFAULT_ACTIONS_URL sends bare owner/repo to github.com.
|
||||
# See Scribe issues 2255 / 2270.
|
||||
uses: https://git.fabledsword.com/bvandeusen/upload-artifact@cb8afe72b42edc798abfb8fcb556cf660d894245
|
||||
# Stock action: it works on this forge since the runner moved to
|
||||
# gitea/runner 3.x, which edits upload-artifact's client-side GHES refusal
|
||||
# out of the action bundle (Scribe snippet #2271). Never @v3 — it reports
|
||||
# success while Gitea serves artifacts back only through the v4 API, and
|
||||
# it is what left 72 unreachable artifacts on this repo (Scribe 2270).
|
||||
uses: actions/upload-artifact@v7
|
||||
with:
|
||||
name: minstrel-android-debug-${{ github.sha }}
|
||||
path: android/app/build/outputs/apk/debug/app-debug.apk
|
||||
|
||||
+291
-93
@@ -2,15 +2,71 @@ name: release
|
||||
|
||||
# Builds and pushes the minstrel container image to the Gitea registry.
|
||||
#
|
||||
# push to main → :main and :latest (latest-release APK bundled)
|
||||
# push tag vYYYY.MM.DD → :vYYYY.MM.DD and :latest (freshly-built APK bundled)
|
||||
# workflow_dispatch → manual trigger (same rules based on the ref)
|
||||
# push to dev → :dev (freshly-built dev APK bundled)
|
||||
# push to main → :latest + :<sha> (latest-release APK bundled)
|
||||
# push tag vYYYY.MM.DD.HHMM → :latest (fresh APK bundled)
|
||||
# workflow_dispatch → manual trigger (same rules based on the ref)
|
||||
#
|
||||
# Release model: per-day CalVer tags (no trailing patch digit). The day's
|
||||
# tag is intentionally mutable — if a second release happens the same day,
|
||||
# move the tag with `git push -f origin vYYYY.MM.DD` and the image tag of
|
||||
# the same name gets overwritten. :latest is updated by every main push
|
||||
# AND every tag push, so it always reflects the newest blessed image.
|
||||
# That is the whole tag map, and it is family rule 145 + 147 as written.
|
||||
#
|
||||
# :<sha> on main is the ROLLBACK UNIT — every production commit addressable
|
||||
# without a release ceremony. It is minted only on main, where rollback is
|
||||
# actually worth having: merges are gated (rule 2) so they number in the dozens
|
||||
# per year, while on dev they would be one per push, forever, for a channel
|
||||
# whose entire contract is that it moves.
|
||||
#
|
||||
# There are NO :<version> image tags. This repo published :vYYYY.MM.DD.HHMM
|
||||
# until 2026-09-10 and it was the inverse of the rule on both counts — minting
|
||||
# a version tag nobody pinned while the rollback unit the rule names did not
|
||||
# exist here at all. Git and the build's own self-reported version answer
|
||||
# "which build is this"; a third name for the same thing is upkeep for a model
|
||||
# we do not run. Operator, 2026-09-10: "only things like the APK need that kind
|
||||
# of versioning for their update process."
|
||||
#
|
||||
# There is no :main either. :latest tracks main's tip with no gate between them
|
||||
# (rule 147), so a second name for the same image sends readers looking for a
|
||||
# distinction that does not exist.
|
||||
#
|
||||
# The dev channel exists so testing a build does not require shipping one.
|
||||
# Before it, the only way to get an APK onto a phone was to cut a release,
|
||||
# which made `main` the staging area by default. `:dev` carries its own
|
||||
# freshly-built APK, signed with the SAME key as release builds — a different
|
||||
# key cannot install over the stable app, so anyone crossing channels would
|
||||
# have to uninstall and lose their data.
|
||||
#
|
||||
# :dev is published ALONE, with no per-commit tag. A rolling channel is
|
||||
# rolling by definition; a commit-addressable image for it would be a
|
||||
# rollback target nobody ever pulls, kept forever. Recovery on dev is to fix
|
||||
# forward.
|
||||
#
|
||||
# Note what this repo does NOT need: a cross-repo dispatch to refresh the
|
||||
# channel when its bundled APK is rebuilt. That mechanism exists elsewhere in
|
||||
# the family because the app and the server live in separate repos. Minstrel
|
||||
# is a monorepo — one push builds the APK and the image in the same run from
|
||||
# the same commit, so the channel cannot go stale against its own artifact.
|
||||
# The requirement is satisfied structurally; copying the mechanism would add
|
||||
# a moving part to fix a problem that does not exist here.
|
||||
#
|
||||
# Release model: the tag IS the artifact's version name with a `v` in front.
|
||||
# `v2026.09.10.1432` and `2026.09.10.1432` are the same string, derived from
|
||||
# the tagged commit's UTC timestamp — so there is no mismatch to reconcile
|
||||
# between what the tag says and what the APK reports, and nothing to look up
|
||||
# when minting one.
|
||||
#
|
||||
# TAGS ARE IMMUTABLE. Never move, retarget or delete a published tag. A
|
||||
# same-day second release is not a collision — HHMM makes every tag unique
|
||||
# by construction, so the answer is simply another tag.
|
||||
#
|
||||
# This block used to say the opposite: that the per-day tag was
|
||||
# "intentionally mutable" and that a same-day re-cut should
|
||||
# `git push -f origin vYYYY.MM.DD`. That instruction is what the family
|
||||
# rulebook now forbids outright, and it has incidents behind it — moving a
|
||||
# same-day tag forward once took a published release down with it. Anyone
|
||||
# installing from a tag is holding something the tag no longer points at,
|
||||
# which is a worse failure than an extra row in the tag list.
|
||||
#
|
||||
# :latest is updated by every main push AND every tag push, so it always
|
||||
# reflects the newest blessed image.
|
||||
#
|
||||
# APK pipeline: on tag pushes the android-release job builds + signs the
|
||||
# Android APK and uploads it as a workflow artifact. The image-release
|
||||
@@ -24,33 +80,37 @@ 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 (tag + commit-count, the same
|
||||
# formula 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.
|
||||
|
||||
on:
|
||||
push:
|
||||
branches: [main]
|
||||
branches: [main, dev]
|
||||
tags: ['v*']
|
||||
paths-ignore:
|
||||
- 'docs/**'
|
||||
- '**/*.md'
|
||||
workflow_dispatch:
|
||||
|
||||
# Force-moving the per-day tag (or rapidly re-pushing to main) should
|
||||
# supersede the in-flight build — the operator explicitly wants the
|
||||
# later commit to win.
|
||||
# A rapid re-push to main should supersede the in-flight build — the
|
||||
# operator explicitly wants the later commit to win. Tags no longer enter
|
||||
# into this: they are immutable and unique, so no tag build can ever be
|
||||
# superseded by another run on the same ref.
|
||||
concurrency:
|
||||
group: ${{ github.workflow }}-${{ github.ref }}
|
||||
cancel-in-progress: true
|
||||
|
||||
jobs:
|
||||
android-release:
|
||||
name: Build signed APK (tag releases only)
|
||||
if: startsWith(github.ref, 'refs/tags/v')
|
||||
name: Build signed APK (releases and dev)
|
||||
# Also builds on `dev`, which is what makes a test channel possible at
|
||||
# all. Without it the only way to get a build onto a phone was to cut a
|
||||
# release, which quietly turns `main` into the staging area.
|
||||
if: startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/dev'
|
||||
runs-on: flutter-ci
|
||||
container:
|
||||
image: git.fabledsword.com/bvandeusen/ci-android:36
|
||||
@@ -75,14 +135,18 @@ jobs:
|
||||
outputs:
|
||||
version_name: ${{ steps.ver.outputs.name }}
|
||||
version_code: ${{ steps.ver.outputs.code }}
|
||||
channel: ${{ steps.ver.outputs.channel }}
|
||||
|
||||
steps:
|
||||
- name: Checkout
|
||||
uses: actions/checkout@v4
|
||||
with:
|
||||
# fetch-depth: 0 retrieves full history; default shallow clone
|
||||
# would return 1 for `git rev-list --count HEAD`, breaking the
|
||||
# iteration suffix.
|
||||
# Full history. The version name now reads only the tip commit's
|
||||
# timestamp, so a shallow clone would technically serve — but this
|
||||
# job derives a value that ships to devices, and a shallow checkout
|
||||
# changes what git-derived values resolve to WITHOUT failing. The
|
||||
# whole failure class here is a green build carrying a wrong
|
||||
# version, so the cheap guarantee is worth keeping.
|
||||
fetch-depth: 0
|
||||
|
||||
- name: Compute release version
|
||||
@@ -91,12 +155,23 @@ jobs:
|
||||
working-directory: ${{ github.workspace }}
|
||||
run: |
|
||||
set -euo pipefail
|
||||
TAG="${GITHUB_REF#refs/tags/v}"
|
||||
COMMIT_COUNT=$(git rev-list --count HEAD)
|
||||
VERSION_NAME="${TAG}.${COMMIT_COUNT}"
|
||||
echo "name=${VERSION_NAME}" >> "$GITHUB_OUTPUT"
|
||||
echo "code=${COMMIT_COUNT}" >> "$GITHUB_OUTPUT"
|
||||
echo "::notice::APK version: ${VERSION_NAME} (code=${COMMIT_COUNT})"
|
||||
# The derivation lives in ci/version.sh, not here, so it can be
|
||||
# executed by a test on every push. Anything inline in this file is
|
||||
# unverifiable until a release is already running.
|
||||
out="$(ci/version.sh HEAD)"
|
||||
printf '%s\n' "${out}" >> "$GITHUB_OUTPUT"
|
||||
|
||||
# The channel is a property of the LANE, not of the commit, which is
|
||||
# why it is derived here rather than in version.sh. Same commit built
|
||||
# on dev and on main reports the same NAME and differs only here —
|
||||
# that is the whole point of separating the two values.
|
||||
if [ "${GITHUB_REF}" = "refs/heads/dev" ]; then
|
||||
channel=dev
|
||||
else
|
||||
channel=stable
|
||||
fi
|
||||
echo "channel=${channel}" >> "$GITHUB_OUTPUT"
|
||||
echo "::notice::APK $(printf '%s' "${out}" | tr '\n' ' ') channel=${channel}"
|
||||
|
||||
# Checked BEFORE the expensive work, not after it. "Attach APK to gitea
|
||||
# Release" below resolves the release by tag and fails if it is absent —
|
||||
@@ -108,6 +183,7 @@ jobs:
|
||||
# the release together, so this passes). A bare `git push origin vX` is the
|
||||
# case this catches.
|
||||
- name: Release must exist for this tag
|
||||
if: startsWith(github.ref, 'refs/tags/v')
|
||||
shell: bash
|
||||
working-directory: ${{ github.workspace }}
|
||||
env:
|
||||
@@ -156,13 +232,12 @@ jobs:
|
||||
-PMINSTREL_VERSION_CODE=${{ steps.ver.outputs.code }}
|
||||
|
||||
- name: Upload APK as workflow artifact
|
||||
# Mirrored action, never actions/upload-artifact — @v4+ refuses on the
|
||||
# hostname, @v3 uploads something Gitea will never serve back. This is
|
||||
# the producing half of a pair: image-release downloads `minstrel-apk`
|
||||
# below with the matching download-artifact mirror. Both must stay on
|
||||
# the v4 protocol — mixing a v3 upload with a v4 download (or the
|
||||
# reverse) yields an empty listing, not an error. See Scribe 2255 / 2270.
|
||||
uses: https://git.fabledsword.com/bvandeusen/upload-artifact@cb8afe72b42edc798abfb8fcb556cf660d894245
|
||||
# Stock action (snippet #2271) — never @v3, which uploads something Gitea
|
||||
# will never serve back. This is the producing half of a pair:
|
||||
# image-release downloads `minstrel-apk` below. Any upload v4+ pairs with
|
||||
# any download v4+ on this forge (every combination tested 2026-09-10,
|
||||
# Scribe spike #3843), so the two pins need not move together.
|
||||
uses: actions/upload-artifact@v7
|
||||
with:
|
||||
name: minstrel-apk
|
||||
path: android/app/build/outputs/apk/release/app-release.apk
|
||||
@@ -171,9 +246,15 @@ jobs:
|
||||
if-no-files-found: error
|
||||
|
||||
- name: Attach APK to gitea Release
|
||||
# Tag releases only. A dev build has no Release to hang assets on and
|
||||
# does not need one — the :dev image bundles the APK, and the server
|
||||
# serves it from /api/client/apk like any other.
|
||||
if: startsWith(github.ref, 'refs/tags/v')
|
||||
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/}"
|
||||
@@ -181,6 +262,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}")"
|
||||
@@ -202,6 +297,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
|
||||
@@ -222,11 +331,16 @@ jobs:
|
||||
- name: Checkout
|
||||
uses: actions/checkout@v4
|
||||
with:
|
||||
# Full history + tags so non-tag :latest builds can resolve the
|
||||
# latest release tag's commit count and reconstruct the bundled
|
||||
# APK's exact versionName (see "Bundle latest release APK" below).
|
||||
# Full history, and rule 149 names this specifically: any job that
|
||||
# DERIVES the version name needs it, because a shallow clone changes
|
||||
# what git-derived values resolve to WITHOUT failing — a too-low
|
||||
# value, silently, with every lane green.
|
||||
#
|
||||
# This job was depth-1 while it took the version from GITHUB_REF. It
|
||||
# now runs ci/version.sh itself, because with :<version> image tags
|
||||
# gone the server's self-reported version is the only thing that says
|
||||
# which build an image is.
|
||||
fetch-depth: 0
|
||||
fetch-tags: true
|
||||
|
||||
- name: Detect buildable project
|
||||
id: guard
|
||||
@@ -244,21 +358,68 @@ jobs:
|
||||
if: steps.guard.outputs.ready == 'true'
|
||||
shell: bash
|
||||
run: |
|
||||
if [[ "${GITHUB_REF}" == refs/tags/v* ]]; then
|
||||
VERSION="${GITHUB_REF#refs/tags/}"
|
||||
echo "args=-t ${IMAGE}:${VERSION} -t ${IMAGE}:latest" >> "$GITHUB_OUTPUT"
|
||||
echo "version=${VERSION}" >> "$GITHUB_OUTPUT"
|
||||
echo "::notice::Release build: ${VERSION} + latest"
|
||||
else
|
||||
# Main is the protected, post-PR-merge branch. Treat it as the
|
||||
# rolling stable channel — every main push moves :latest.
|
||||
# Pinned consumers can target :vYYYY.MM.DD; everyone else
|
||||
# gets the newest main.
|
||||
echo "args=-t ${IMAGE}:main -t ${IMAGE}:latest" >> "$GITHUB_OUTPUT"
|
||||
echo "version=main" >> "$GITHUB_OUTPUT"
|
||||
echo "::notice::Main-branch build: :main + :latest"
|
||||
set -euo pipefail
|
||||
|
||||
# THE VERSION, and it is derived the same way on every ref — the
|
||||
# branch decides the CHANNEL, never the version (family rule 149).
|
||||
#
|
||||
# This used to be three different things: the literal string "main"
|
||||
# on main, "dev" on dev, and the tag name on a tag. None of them
|
||||
# ordered, and the first two were the same string forever — two dev
|
||||
# images eight weeks apart were indistinguishable in the UI. That
|
||||
# mattered little while :vYYYY.MM.DD.HHMM existed to identify a
|
||||
# build; with version image tags gone, this IS how an operator tells
|
||||
# which build a container is running.
|
||||
#
|
||||
# `sed -n s///p` rather than `grep`: it exits 0 when nothing matches,
|
||||
# so the empty check below is actually reachable. A grep here would
|
||||
# kill the step at the assignment under the runner's pipefail — the
|
||||
# exact bug that took down the first main build after the version
|
||||
# rework.
|
||||
VERSION="$(ci/version.sh HEAD | sed -n 's/^name=//p')"
|
||||
if [ -z "${VERSION}" ]; then
|
||||
echo "::error::could not derive a build version from ci/version.sh"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
if [[ "${GITHUB_REF}" == refs/tags/v* ]]; then
|
||||
# A release refreshes the CHANNEL and mints nothing else.
|
||||
#
|
||||
# The tag build exists to produce the signed APK and attach it to
|
||||
# the release; the image it rebuilds is the SAME SOURCE as the main
|
||||
# build minutes earlier, differing only in which APK is baked in.
|
||||
# Rule 145 is explicit about that case: when the same source is
|
||||
# rebuilt with different contents, publish the moving channel tag
|
||||
# and never a commit-addressable one.
|
||||
#
|
||||
# :latest must move here rather than waiting for the next main
|
||||
# push, or the channel would carry the PREVIOUS release's APK
|
||||
# indefinitely — a channel that cannot refresh itself (rule 146).
|
||||
CHANNEL=stable
|
||||
echo "args=-t ${IMAGE}:latest" >> "$GITHUB_OUTPUT"
|
||||
echo "::notice::Release build ${VERSION}: refreshing :latest around the new APK"
|
||||
elif [[ "${GITHUB_REF}" == "refs/heads/dev" ]]; then
|
||||
# The rolling test channel, and :dev ALONE — deliberately no
|
||||
# per-commit tag. A rolling channel is rolling by definition, so a
|
||||
# commit-addressable image here would be a rollback target nobody
|
||||
# has ever pulled, accumulating in the registry forever. Recovery
|
||||
# on dev is to fix forward.
|
||||
CHANNEL=dev
|
||||
echo "args=-t ${IMAGE}:dev" >> "$GITHUB_OUTPUT"
|
||||
echo "::notice::Dev-branch build ${VERSION}: :dev"
|
||||
else
|
||||
# The production line: :latest tracks main's tip (rule 147) and
|
||||
# :<sha> is the rollback unit (rule 145). Full 40-char SHA, matching
|
||||
# the family's other repos, so a rollback target is addressable
|
||||
# straight from the commit anyone is reading.
|
||||
CHANNEL=stable
|
||||
echo "args=-t ${IMAGE}:latest -t ${IMAGE}:${GITHUB_SHA}" >> "$GITHUB_OUTPUT"
|
||||
echo "::notice::Main-branch build ${VERSION}: :latest + :${GITHUB_SHA}"
|
||||
fi
|
||||
|
||||
echo "version=${VERSION}" >> "$GITHUB_OUTPUT"
|
||||
echo "channel=${CHANNEL}" >> "$GITHUB_OUTPUT"
|
||||
|
||||
- name: Registry login
|
||||
if: steps.guard.outputs.ready == 'true'
|
||||
shell: bash
|
||||
@@ -267,54 +428,57 @@ jobs:
|
||||
| docker login git.fabledsword.com -u "${{ github.actor }}" --password-stdin
|
||||
|
||||
- name: Download signed APK artifact
|
||||
# Tag pushes only — android-release just produced this. Non-tag
|
||||
# builds take the "Bundle latest release APK" path below instead.
|
||||
if: steps.guard.outputs.ready == 'true' && startsWith(github.ref, 'refs/tags/v')
|
||||
# Consuming half of the pair — never actions/download-artifact. Same fork,
|
||||
# same reason: upstream's client-side GHES check rejects this hostname
|
||||
# before it connects. bvandeusen/download-artifact mirrors
|
||||
# code.forgejo.org/forgejo/download-artifact.
|
||||
#
|
||||
# SHA below is that fork's `v6` tag. Match on @actions/artifact, NOT on
|
||||
# the action's own version number — the two actions release on unrelated
|
||||
# cadences, and download v5 would pair a ^2.3.2 client with this file's
|
||||
# ^4.0.0 uploader. v6 is the tag whose bundled library major (^4.0.0) is
|
||||
# the same one proven against this instance by the upload side.
|
||||
# Deliberately NOT v7: it moves to node24 and upstream requires runner
|
||||
# >= 2.327.1 for it, which act_runner does not claim to satisfy.
|
||||
# Pinned, not tagged — the mirror auto-syncs every 8h.
|
||||
uses: https://git.fabledsword.com/bvandeusen/download-artifact@8d4e9521a5f7e5f8b6351f341f719f9f45a92a3a
|
||||
# Tag and dev pushes — android-release just produced this. Only `main`
|
||||
# takes the "Bundle latest release APK" path below, because it is the
|
||||
# one ref that moves a channel without building an APK of its own.
|
||||
if: >-
|
||||
steps.guard.outputs.ready == 'true' &&
|
||||
(startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/dev')
|
||||
# Consuming half of the pair: stock download-artifact, which works here for
|
||||
# the same reason as the upload (gitea/runner 3.x edits the GHES refusal
|
||||
# out of the bundle; snippet #2271). v8 runs on node24, which every
|
||||
# CI-runner image carries — the runner uses the image's own node.
|
||||
uses: actions/download-artifact@v8
|
||||
with:
|
||||
name: minstrel-apk
|
||||
path: client/
|
||||
|
||||
- name: Stage bundled APK + version sidecar
|
||||
if: steps.guard.outputs.ready == 'true' && startsWith(github.ref, 'refs/tags/v')
|
||||
if: >-
|
||||
steps.guard.outputs.ready == 'true' &&
|
||||
(startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/dev')
|
||||
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.
|
||||
# All three 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 }}
|
||||
APK_CHANNEL: ${{ needs.android-release.outputs.channel }}
|
||||
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":"%s"}\n' \
|
||||
"${APK_VERSION_NAME}" "${APK_VERSION_CODE}" "${APK_CHANNEL}" \
|
||||
> 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 (${TAG#v}.$(git rev-list --count
|
||||
# TAG) — identical to android-release's formula) 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.
|
||||
if: steps.guard.outputs.ready == 'true' && !startsWith(github.ref, 'refs/tags/v')
|
||||
# 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:
|
||||
CI_TOKEN: ${{ secrets.CI_TOKEN }}
|
||||
@@ -326,19 +490,40 @@ 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
|
||||
COUNT="$(git rev-list --count "${TAG}" 2>/dev/null || true)"
|
||||
if [ -z "${COUNT}" ]; then
|
||||
echo "::notice::could not resolve commit count for ${TAG} (tag not fetched?) — skipping APK bundle"; exit 0
|
||||
fi
|
||||
VERSION_NAME="${TAG#v}.${COUNT}"
|
||||
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)" || 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
|
||||
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
|
||||
@@ -346,6 +531,7 @@ jobs:
|
||||
run: |
|
||||
docker buildx build \
|
||||
--build-arg MINSTREL_VERSION="${{ steps.tags.outputs.version }}" \
|
||||
--build-arg MINSTREL_CHANNEL="${{ steps.tags.outputs.channel }}" \
|
||||
--push ${{ steps.tags.outputs.args }} .
|
||||
|
||||
# Verifies a tag release actually ended up complete, and names the specific
|
||||
@@ -356,8 +542,8 @@ jobs:
|
||||
# `failure` with none executed and image-release showed `skipped`. The run was
|
||||
# red, but the *release page rendered fine*, and `main`'s own push build had
|
||||
# already moved `:latest`, so the code was deployable and nothing looked
|
||||
# obviously wrong. The release was simply missing its APK and its immutable
|
||||
# `:vYYYY.MM.DD` image, which is easy to skim past.
|
||||
# obviously wrong. The release was simply missing its APK and its image,
|
||||
# which is easy to skim past.
|
||||
#
|
||||
# This job cannot prevent that (the cause was a runner failing to launch, not
|
||||
# anything in this file). What it does is turn an incomplete release into an
|
||||
@@ -408,18 +594,30 @@ jobs:
|
||||
# missing when v2026.08.07 had to be re-cut. `always()` on this job means
|
||||
# it runs even when image-release failed, so without this the guard would
|
||||
# cheerfully verify an incomplete release.
|
||||
- name: Immutable image tag must exist
|
||||
#
|
||||
# This asserted `:${TAG}` — the :vYYYY.MM.DD.HHMM image — until
|
||||
# 2026-09-10. Version image tags are no longer published (rule 145), so
|
||||
# that assertion would now fail every release for a tag nothing mints.
|
||||
# The rollback target it was really protecting is the :<sha> image, which
|
||||
# main's own build published for this same commit before the tag was cut.
|
||||
#
|
||||
# Checking it here earns its keep twice over: it still catches an image
|
||||
# push that silently did not happen, and it additionally proves the
|
||||
# ORDERING — a tag cut on a commit whose main build never completed has
|
||||
# no rollback target, and that is worth failing on rather than
|
||||
# discovering during an incident.
|
||||
- name: Rollback image must exist for the tagged commit
|
||||
shell: bash
|
||||
run: |
|
||||
set -euo pipefail
|
||||
TAG="${GITHUB_REF#refs/tags/}"
|
||||
IMAGE="git.fabledsword.com/bvandeusen/minstrel"
|
||||
|
||||
echo "${{ secrets.CI_TOKEN }}" \
|
||||
| docker login git.fabledsword.com -u "${{ github.actor }}" --password-stdin
|
||||
|
||||
if ! docker manifest inspect "${IMAGE}:${TAG}" > /dev/null 2>&1; then
|
||||
echo "::error::image ${IMAGE}:${TAG} was never pushed — the release tag has no immutable image, so there is nothing to pin or roll back to. Re-run this workflow run."
|
||||
if ! docker manifest inspect "${IMAGE}:${GITHUB_SHA}" > /dev/null 2>&1; then
|
||||
echo "::error::image ${IMAGE}:${GITHUB_SHA} does not exist — this commit has no rollback target."
|
||||
echo "::error::That image is published by the MAIN build of this commit, not by the tag build. If main's build never ran or failed, fix that first; a release whose commit cannot be rolled back to is the thing this check exists to refuse."
|
||||
exit 1
|
||||
fi
|
||||
echo "::notice::image verified: ${IMAGE}:${TAG}"
|
||||
echo "::notice::rollback target verified: ${IMAGE}:${GITHUB_SHA}"
|
||||
|
||||
@@ -32,6 +32,12 @@ on:
|
||||
- 'cmd/**'
|
||||
- '.golangci.yml'
|
||||
- '.gitea/workflows/test-go.yml'
|
||||
# The release lane's own trigger is `main` + tags, so nothing it
|
||||
# contains is exercised until a release is already running. These two
|
||||
# entries are what let internal/server/release_version_test.go guard
|
||||
# the version derivation on ordinary dev pushes instead.
|
||||
- 'ci/**'
|
||||
- '.gitea/workflows/release.yml'
|
||||
|
||||
# pull_request trigger intentionally omitted — see test-web.yml for
|
||||
# the rationale (single-author repo, push covers PR-merge equivalent).
|
||||
|
||||
@@ -12,6 +12,11 @@
|
||||
# Test binary, built with `go test -c`
|
||||
*.test
|
||||
|
||||
# `make build` output. bin/minstrel was tracked until 2026-09-10 — an 18 MB
|
||||
# binary committed by accident, last refreshed by a commit about web test
|
||||
# mocks, and re-dirtied by every local build since.
|
||||
bin/
|
||||
|
||||
# Bundled Android APK + version sidecar (#397). Populated by CI for
|
||||
# tag releases; never committed. README in client/ explains the flow.
|
||||
client/minstrel.apk
|
||||
|
||||
+13
-4
@@ -15,12 +15,21 @@ COPY . .
|
||||
# Overwrite the committed placeholder with the freshly-built SPA assets.
|
||||
COPY --from=web /web/build ./web/build
|
||||
ENV CGO_ENABLED=0
|
||||
# Version stamping: release.yml passes the git tag via MINSTREL_VERSION
|
||||
# build-arg; local `docker build` falls back to "dev". Surfaced at
|
||||
# /healthz for operator-side image-version verification.
|
||||
# Version stamping. release.yml passes the DERIVED version name
|
||||
# (YYYY.MM.DD.HHMM) and the lane's channel; a local `docker build` falls back
|
||||
# to "dev"/"local". Both are surfaced at /healthz.
|
||||
#
|
||||
# These are two values on purpose (family rule 149): the same commit built on
|
||||
# dev and on main reports the same NAME and differs only in CHANNEL. Folding
|
||||
# the channel into the version string is what the rule forbids — the version
|
||||
# used to BE the channel word here ("main"/"dev"), which meant two dev images
|
||||
# eight weeks apart were indistinguishable.
|
||||
ARG MINSTREL_VERSION=dev
|
||||
ARG MINSTREL_CHANNEL=local
|
||||
RUN go build -trimpath \
|
||||
-ldflags="-s -w -X 'git.fabledsword.com/bvandeusen/minstrel/internal/server.ServerVersion=${MINSTREL_VERSION}'" \
|
||||
-ldflags="-s -w \
|
||||
-X 'git.fabledsword.com/bvandeusen/minstrel/internal/server.ServerVersion=${MINSTREL_VERSION}' \
|
||||
-X 'git.fabledsword.com/bvandeusen/minstrel/internal/server.ServerChannel=${MINSTREL_CHANNEL}'" \
|
||||
-o /out/minstrel ./cmd/minstrel
|
||||
|
||||
FROM debian:bookworm-slim
|
||||
|
||||
@@ -112,11 +112,21 @@ Most operational keys have a `MINSTREL_<SECTION>_<FIELD>` env override. Recommen
|
||||
|
||||
Image tags (`git.fabledsword.com/bvandeusen/minstrel:<tag>`):
|
||||
|
||||
- `:latest` — the newest blessed image. Moves on every `main` push **and** every release. Recommended for most operators.
|
||||
- `:vYYYY.MM.DD` — immutable per-day release tags. Pin one of these for a deployment you don't want moving under you. (Per-day CalVer — no trailing patch digit; a same-day re-cut moves the tag forward.)
|
||||
- `:main` — the rolling post-merge tip. Same image as `:latest` at push time; choose it if you want to track `main` explicitly rather than the release line.
|
||||
- `:latest` — production. Tracks `main`'s tip and moves on every `main` push and every release. What most operators should run.
|
||||
- `:<commit-sha>` — the rollback unit. Every `main` push publishes one, so any production commit is addressable without a release ceremony. Immutable: a given SHA tag is never re-pushed. Pin one if you need a deployment that cannot change under you, and use it to roll back.
|
||||
- `:dev` — the rolling test channel, rebuilt on every push to `dev` and carrying its own freshly-built Android APK. Run this to try something before it ships. It moves constantly, has no per-commit tag, and its only recovery path is forward — if a `:dev` image is broken, the fix is the next push, not a rollback.
|
||||
|
||||
Every `:latest` and every `:vYYYY.MM.DD` bundles the current signed Android APK, so the in-app update channel is always live. Database migrations run automatically at startup; rollbacks require restoring a Postgres dump.
|
||||
That is the whole tag map. **There are no version-numbered image tags**, and no `:main`. Git and the build's own self-reported version answer "which build is this" — the Settings page shows it, and so does `/healthz`. Release *tags* in git are still `vYYYY.MM.DD.HHMM`; they name a changelog entry and the APK attached to it, not an image.
|
||||
|
||||
Rolling back to `:<commit-sha>` pins the **server code** at that commit — not the server-and-app pair. The Android APK is baked in at image build time, so a SHA image carries whichever app was current when that commit was built, which may be older than what `:latest` bundles now. If both halves matter, check what the image bundles rather than trusting the tag's name.
|
||||
|
||||
Every `:latest`, `:<commit-sha>` and `:dev` bundles a signed Android APK, so the in-app update channel is always live. All are signed with the same key, so a phone can move between the stable and dev channels without uninstalling — point it at a `:dev` server and the in-app updater offers that channel's build.
|
||||
|
||||
The app reports which channel it is on alongside its version, and decides whether an update is available using the build's ordering key rather than its displayed name — the same value Android installs by, so an offer it makes is one the platform will accept.
|
||||
|
||||
Database migrations run automatically at startup; rollbacks require restoring a Postgres dump.
|
||||
|
||||
Releases up to 2026-09-10 also published a `:vYYYY.MM.DD[.HHMM]` image tag. Those images still exist and still work — they are simply not extended.
|
||||
|
||||
## Specs
|
||||
|
||||
@@ -150,7 +160,7 @@ Two concurrent dev processes:
|
||||
|
||||
- Day-to-day work happens on `dev` (or feature branches merged into `dev`).
|
||||
- `main` is **protected** — changes land via PR from `dev`.
|
||||
- Releases are cut by tagging `v*` off `main`; the release workflow builds and pushes the container image to the Gitea registry.
|
||||
- Releases are cut by tagging `v*` off `main`; the release workflow builds the signed APK, attaches it to the release, and refreshes `:latest` around it.
|
||||
|
||||
Task and milestone tracking: Fable (`Minstrel` project, id 12).
|
||||
|
||||
|
||||
@@ -21,13 +21,24 @@ android {
|
||||
applicationId = "com.fabledsword.minstrel"
|
||||
minSdk = 26
|
||||
targetSdk = 36
|
||||
// versionName / versionCode are released-build values injected by
|
||||
// CI from the git tag + commit count. Local / debug builds fall
|
||||
// back to "dev" so the About card reads honestly. Releases ship
|
||||
// versionName="YYYY.MM.DD.<commits>" (e.g. "2026.06.02.142") and
|
||||
// versionCode=<commits>, which is monotonic forever and lets the
|
||||
// shared isVersionNewer comparator distinguish two same-day
|
||||
// re-cuts (the iteration suffix differs).
|
||||
// versionName / versionCode are released-build values injected by CI.
|
||||
// Local / debug builds fall back to "dev" so the About card reads
|
||||
// honestly.
|
||||
//
|
||||
// versionName is "YYYY.MM.DD.HHMM" from the COMMIT's timestamp, so
|
||||
// every lane building this source reports the same string and the
|
||||
// channel is the only thing that differs between them.
|
||||
//
|
||||
// versionCode is minutes since 2020-01-01 at BUILD time. It is the
|
||||
// value the platform decides installs by, so it must be monotonic by
|
||||
// construction.
|
||||
//
|
||||
// This comment used to say versionCode was a commit count and that it
|
||||
// was "monotonic forever". It was neither — a commit count runs ahead
|
||||
// on `dev`, so a dev build outranked the `main` release meant to
|
||||
// replace it and Android refused the install as a downgrade. Worth
|
||||
// knowing the claim was here, stated as a reassurance, while the bug
|
||||
// it denied was live.
|
||||
val versionNameOverride =
|
||||
(project.findProperty("MINSTREL_VERSION_NAME") as String?)?.takeIf { it.isNotBlank() }
|
||||
val versionCodeOverride =
|
||||
|
||||
@@ -1,15 +1,26 @@
|
||||
package com.fabledsword.minstrel.models
|
||||
|
||||
/**
|
||||
* Wire shape returned by `GET /api/client/version`. Mirrors
|
||||
* the Flutter client's `UpdateInfo`.
|
||||
* The server-bundled APK, as reported by `GET /api/client/version`.
|
||||
*
|
||||
* `version` is the server-bundled APK version (may have a leading
|
||||
* "v" from the git tag); `apkUrl` is server-relative (e.g.
|
||||
* `/api/client/apk`); `sizeBytes` is the download size.
|
||||
* Three values that are deliberately kept apart:
|
||||
*
|
||||
* - [version] is a LABEL for people — "YYYY.MM.DD.HHMM", derived from the
|
||||
* build's commit, so two channels carrying the same code read the same.
|
||||
* Display this; never decide on it when [code] is present.
|
||||
* - [code] is the ORDERING KEY, and is the same value Android itself
|
||||
* installs by. It answers "may this be installed over that?", which the
|
||||
* name cannot. Null when the server predates the field.
|
||||
* - [channel] is a SIBLING FIELD, never a suffix inside the name. Reported
|
||||
* verbatim rather than validated, so an unexpected value is shown rather
|
||||
* than dropped.
|
||||
*
|
||||
* [apkUrl] is server-relative (e.g. `/api/client/apk`).
|
||||
*/
|
||||
data class UpdateInfo(
|
||||
val version: String,
|
||||
val code: Long?,
|
||||
val channel: String?,
|
||||
val apkUrl: String,
|
||||
val sizeBytes: Long,
|
||||
)
|
||||
|
||||
@@ -4,12 +4,26 @@ import kotlinx.serialization.SerialName
|
||||
import kotlinx.serialization.Serializable
|
||||
|
||||
/**
|
||||
* Wire shape for `GET /api/client/version`. Defaults match Flutter:
|
||||
* apk_url falls back to `/api/client/apk` if the server omits it.
|
||||
* Wire shape for `GET /api/client/version`.
|
||||
*
|
||||
* `apkUrl` falls back to `/api/client/apk` if the server omits it.
|
||||
*
|
||||
* [code] MUST stay nullable, and this is not a style preference. The app's
|
||||
* Json is configured with `coerceInputValues = true`, which replaces a JSON
|
||||
* null with the declared default for a NON-nullable property — so writing
|
||||
* `val code: Long = 0` would turn "this server reports no ordering key" into
|
||||
* "this build's ordering key is 0", silently, with no error anywhere. A
|
||||
* nullable type is what keeps absent distinguishable from zero, and the
|
||||
* distinction is the whole reason the field exists.
|
||||
*
|
||||
* A server predating the ordering key sends neither [code] nor [channel];
|
||||
* both arrive null and the caller falls back to comparing names.
|
||||
*/
|
||||
@Serializable
|
||||
data class UpdateInfoWire(
|
||||
val version: String = "",
|
||||
val code: Long? = null,
|
||||
val channel: String? = null,
|
||||
@SerialName("apk_url") val apkUrl: String = "/api/client/apk",
|
||||
@SerialName("size_bytes") val sizeBytes: Long = 0,
|
||||
)
|
||||
|
||||
@@ -109,8 +109,11 @@ private fun MiniCover(coverUrl: String, contentDescription: String) {
|
||||
* NowPlayingScreen via [onExpandClick].
|
||||
*
|
||||
* Layout (Column):
|
||||
* - Slim seek slider at the top (4dp track)
|
||||
* - Row: cover | title/artist column | like | prev | play/pause | next
|
||||
* - Slim seek slider pinned at the top (4dp track)
|
||||
* - Row: cover | title/artist column | like | prev | play/pause | next.
|
||||
* Weighted so it fills the rest of the fixed-height bar and centres its
|
||||
* own content; otherwise the row keeps its intrinsic 48dp and the
|
||||
* leftover height collects at the bottom as dead surface.
|
||||
*
|
||||
* No kebab on the mini bar (operator 2026-06-01): the full kebab
|
||||
* surface lives on NowPlayingScreen, and dropping it from the mini
|
||||
@@ -164,6 +167,12 @@ fun MiniPlayer(
|
||||
durationMs = state.durationMs,
|
||||
)
|
||||
MiniRow(
|
||||
// Take whatever the progress fill leaves. Without this the
|
||||
// Column stacks 4dp + the row's intrinsic 48dp from the top
|
||||
// and the remaining 28dp of an 80dp bar sits empty
|
||||
// underneath — the content looked top-aligned rather than
|
||||
// centred, with a dead strip above the gesture bar.
|
||||
modifier = Modifier.weight(1f),
|
||||
track = track,
|
||||
isPlaying = state.isPlaying,
|
||||
isUpnpLoading = state.isUpnpLoading,
|
||||
@@ -205,6 +214,7 @@ private fun MiniProgressFill(positionMs: Long, durationMs: Long) {
|
||||
@Composable
|
||||
@Suppress("LongParameterList")
|
||||
private fun MiniRow(
|
||||
modifier: Modifier,
|
||||
track: TrackRef,
|
||||
isPlaying: Boolean,
|
||||
isUpnpLoading: Boolean,
|
||||
@@ -216,7 +226,7 @@ private fun MiniRow(
|
||||
onToggleLike: () -> Unit,
|
||||
) {
|
||||
Row(
|
||||
modifier = Modifier
|
||||
modifier = modifier
|
||||
.fillMaxWidth()
|
||||
.padding(horizontal = 12.dp),
|
||||
verticalAlignment = Alignment.CenterVertically,
|
||||
|
||||
+17
-4
@@ -9,7 +9,7 @@ import com.fabledsword.minstrel.update.data.ApkInstaller
|
||||
import com.fabledsword.minstrel.update.data.InstallStage
|
||||
import com.fabledsword.minstrel.update.data.UpdateRepository
|
||||
import com.fabledsword.minstrel.update.data.isBusy
|
||||
import com.fabledsword.minstrel.update.data.isVersionNewer
|
||||
import com.fabledsword.minstrel.update.data.isUpdateAvailable
|
||||
import com.fabledsword.minstrel.update.data.message
|
||||
import com.fabledsword.minstrel.update.data.stage
|
||||
import dagger.hilt.android.lifecycle.HiltViewModel
|
||||
@@ -37,6 +37,10 @@ sealed interface UpdateCheckResult {
|
||||
|
||||
data class AboutUiState(
|
||||
val installedVersion: String = BuildConfig.VERSION_NAME,
|
||||
// The value the platform installs by, and therefore the one the update
|
||||
// check must decide on. Held in state rather than read inline so a test
|
||||
// can drive the comparison without a BuildConfig.
|
||||
val installedCode: Long = BuildConfig.VERSION_CODE.toLong(),
|
||||
val isChecking: Boolean = false,
|
||||
val installStage: InstallStage = InstallStage.IDLE,
|
||||
val installMessage: String? = null,
|
||||
@@ -45,8 +49,9 @@ data class AboutUiState(
|
||||
|
||||
/**
|
||||
* Backs the About card's update controls. "Check for updates" calls
|
||||
* [UpdateRepository.getLatest], compares versus the build's
|
||||
* VERSION_NAME via [isVersionNewer], and reports the terminal state.
|
||||
* [UpdateRepository.getLatest], compares versus this build via
|
||||
* [isUpdateAvailable] — on the ordering key where the server reports one,
|
||||
* on the name otherwise — and reports the terminal state.
|
||||
* When an update is available, [install] downloads the APK via
|
||||
* [ApkInstaller] and installs it — routing the user to the "install
|
||||
* unknown apps" settings page first when that permission hasn't been
|
||||
@@ -66,9 +71,17 @@ class AboutCardViewModel @Inject constructor(
|
||||
viewModelScope.launch {
|
||||
internal.update { it.copy(isChecking = true, installMessage = null) }
|
||||
val installed = internal.value.installedVersion
|
||||
val installedCode = internal.value.installedCode
|
||||
val result = runCatching { repository.getLatest() }
|
||||
.map { latest ->
|
||||
if (isVersionNewer(latest.version, installed)) {
|
||||
if (
|
||||
isUpdateAvailable(
|
||||
serverCode = latest.code,
|
||||
serverName = latest.version,
|
||||
installedCode = installedCode,
|
||||
installedName = installed,
|
||||
)
|
||||
) {
|
||||
UpdateCheckResult.UpdateAvailable(latest)
|
||||
} else {
|
||||
UpdateCheckResult.Latest
|
||||
|
||||
+10
-2
@@ -19,7 +19,8 @@ private const val POLL_INTERVAL_MS = 24 * 60 * 60 * 1000L
|
||||
/**
|
||||
* Drives the shell's soft "update available" banner. Polls
|
||||
* `/api/client/version` at launch + every 24h and, when the bundled
|
||||
* APK is strictly newer than this build, exposes its [UpdateInfo] so
|
||||
* APK outranks this build — by ordering key where the server reports one,
|
||||
* by name otherwise — exposes its [UpdateInfo] so
|
||||
* [com.fabledsword.minstrel.update.ui.UpdateBanner] can nudge an
|
||||
* install. Mirrors Flutter's `ClientUpdateController`.
|
||||
*
|
||||
@@ -58,6 +59,13 @@ class UpdateBannerController @Inject constructor(
|
||||
|
||||
private suspend fun runOnce() {
|
||||
val info = runCatching { repository.getLatest() }.getOrNull() ?: return
|
||||
latest.value = info.takeIf { isVersionNewer(it.version, BuildConfig.VERSION_NAME) }
|
||||
latest.value = info.takeIf {
|
||||
isUpdateAvailable(
|
||||
serverCode = it.code,
|
||||
serverName = it.version,
|
||||
installedCode = BuildConfig.VERSION_CODE.toLong(),
|
||||
installedName = BuildConfig.VERSION_NAME,
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -21,10 +21,39 @@ class UpdateRepository @Inject constructor(retrofit: Retrofit) {
|
||||
|
||||
private fun UpdateInfoWire.toDomain(): UpdateInfo = UpdateInfo(
|
||||
version = version,
|
||||
code = code,
|
||||
channel = channel,
|
||||
apkUrl = apkUrl,
|
||||
sizeBytes = sizeBytes,
|
||||
)
|
||||
|
||||
/**
|
||||
* True when [server] should be offered over the installed build.
|
||||
*
|
||||
* **Decide on the ordering key whenever the server sends one.** That is the
|
||||
* same value Android's package installer compares, so an offer made this way
|
||||
* implies an install the platform will actually accept. The app used to
|
||||
* compare NAMES while the platform installed by `versionCode`, with nothing
|
||||
* keeping the two orderings consistent — so it could offer a build Android
|
||||
* then refused as a downgrade, or stay quiet about one it would have taken.
|
||||
*
|
||||
* Name comparison survives only as the fallback for a server that predates
|
||||
* the field. A null code means "this server cannot tell me" — never "zero" —
|
||||
* because treating absent as zero would rank every such server as infinitely
|
||||
* old and offer its build to everyone, forever.
|
||||
*/
|
||||
fun isUpdateAvailable(
|
||||
serverCode: Long?,
|
||||
serverName: String,
|
||||
installedCode: Long,
|
||||
installedName: String,
|
||||
): Boolean =
|
||||
if (serverCode != null) {
|
||||
serverCode > installedCode
|
||||
} else {
|
||||
isVersionNewer(serverName, installedName)
|
||||
}
|
||||
|
||||
/**
|
||||
* True when [server] is strictly newer than [installed]. Mirrors
|
||||
* Flutter's `isVersionNewer` — splits both strings on `.`, parses
|
||||
|
||||
+163
@@ -0,0 +1,163 @@
|
||||
package com.fabledsword.minstrel.update.data
|
||||
|
||||
import org.junit.jupiter.api.Test
|
||||
import kotlin.test.assertFalse
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
/**
|
||||
* The update channel had no tests at all before this. That is worth saying
|
||||
* out loud, because the thing it decides — whether anyone is ever offered an
|
||||
* update — fails silently in both directions: an update nobody is offered
|
||||
* looks exactly like being up to date, and nobody files a bug about a prompt
|
||||
* they never saw.
|
||||
*/
|
||||
class UpdateVersioningTest {
|
||||
@Test
|
||||
fun `decides on the ordering key when the server reports one`() {
|
||||
assertTrue(
|
||||
isUpdateAvailable(
|
||||
serverCode = 3523847, serverName = "2026.09.10.1432",
|
||||
installedCode = 3519456, installedName = "2026.09.09.1828",
|
||||
),
|
||||
)
|
||||
assertFalse(
|
||||
isUpdateAvailable(
|
||||
serverCode = 3519456, serverName = "2026.09.09.1828",
|
||||
installedCode = 3523847, installedName = "2026.09.10.1432",
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `an equal ordering key is not an update`() {
|
||||
assertFalse(
|
||||
isUpdateAvailable(
|
||||
serverCode = 3523847, serverName = "2026.09.10.1432",
|
||||
installedCode = 3523847, installedName = "2026.09.10.1432",
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* The property the whole rework exists for: the offer must agree with what
|
||||
* the platform will actually install. Where the two disagree, the ordering
|
||||
* key wins, because that is the value Android compares.
|
||||
*/
|
||||
@Test
|
||||
fun `the ordering key wins even when the name disagrees`() {
|
||||
// Name looks older, key is newer — e.g. an older commit rebuilt later.
|
||||
assertTrue(
|
||||
isUpdateAvailable(
|
||||
serverCode = 9_000_000, serverName = "2020.01.01.0000",
|
||||
installedCode = 1, installedName = "2099.12.31.2359",
|
||||
),
|
||||
)
|
||||
// Name looks newer, key is not. Offering this would be offering an
|
||||
// install the platform then refuses as a downgrade.
|
||||
assertFalse(
|
||||
isUpdateAvailable(
|
||||
serverCode = 1, serverName = "2099.12.31.2359",
|
||||
installedCode = 9_000_000, installedName = "2020.01.01.0000",
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `falls back to the name when the server reports no ordering key`() {
|
||||
assertTrue(
|
||||
isUpdateAvailable(
|
||||
serverCode = null, serverName = "2026.09.10.1432",
|
||||
installedCode = 3519456, installedName = "2026.09.09.1828",
|
||||
),
|
||||
)
|
||||
assertFalse(
|
||||
isUpdateAvailable(
|
||||
serverCode = null, serverName = "2026.09.09.1828",
|
||||
installedCode = 3519456, installedName = "2026.09.10.1432",
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* A null code must never be read as zero. Zero would rank every
|
||||
* older server as infinitely behind and offer its build to everyone,
|
||||
* forever — so this asserts the fallback runs instead of a comparison
|
||||
* against 0 succeeding by accident.
|
||||
*/
|
||||
@Test
|
||||
fun `a null ordering key is absent, not zero`() {
|
||||
// installedCode is 0 here: if null coerced to 0, "0 > 0" would be
|
||||
// false and this would wrongly report no update despite a newer name.
|
||||
assertTrue(
|
||||
isUpdateAvailable(
|
||||
serverCode = null, serverName = "2026.09.10.1432",
|
||||
installedCode = 0, installedName = "2026.09.09.1828",
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* The recorded migration constraint, pinned so it cannot be forgotten:
|
||||
* the old scheme's fourth segment was a commit count (~1895), the new
|
||||
* one is HHMM. Across a day boundary the date decides and all is well.
|
||||
*/
|
||||
@Test
|
||||
fun `new-scheme name outranks an old-scheme name on a later day`() {
|
||||
assertTrue(isVersionNewer("2026.09.10.1432", "2026.09.09.1895"))
|
||||
}
|
||||
|
||||
/**
|
||||
* ...but on the SAME day the comparison comes down to HHMM against a
|
||||
* commit count, and any build before ~19:00 UTC reads as older. This is
|
||||
* why the first new-scheme release had to be cut on a later calendar day.
|
||||
* Asserting the trap so nobody "fixes" it by accident.
|
||||
*/
|
||||
@Test
|
||||
fun `same-day new-scheme name can read older than an old-scheme name`() {
|
||||
assertFalse(isVersionNewer("2026.09.09.1828", "2026.09.09.1895"))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `name comparison degrades per segment rather than discarding`() {
|
||||
// The string is still compared rather than rejected outright: an
|
||||
// earlier segment decides and the unparseable tail never matters.
|
||||
assertTrue(isVersionNewer("2026.09.10.1432-dev", "2026.09.09.1828"))
|
||||
|
||||
// A shorter name pads with zeros instead of being refused.
|
||||
assertTrue(isVersionNewer("2026.09.10", "2026.09.09.9999"))
|
||||
assertFalse(isVersionNewer("2026.09.10", "2026.09.10.0"))
|
||||
}
|
||||
|
||||
/**
|
||||
* What "costs that segment's precision" actually means, and it is worth
|
||||
* pinning because it is a real edge rather than a nicety: when the
|
||||
* unparseable segment is the DECIDING one, it reads as 0 and loses. So a
|
||||
* `-dev` suffixed build compares as older than an unsuffixed one from the
|
||||
* same minute.
|
||||
*
|
||||
* That is the correct behaviour for a degrading parser — it is bounded
|
||||
* loss rather than a discarded string — but it is exactly why the channel
|
||||
* belongs in its own field and never in the name.
|
||||
*/
|
||||
@Test
|
||||
fun `an unparseable deciding segment reads as zero and loses`() {
|
||||
assertFalse(isVersionNewer("2026.09.10.1432-dev", "2026.09.10.1000"))
|
||||
}
|
||||
|
||||
/**
|
||||
* Both sides unparseable (branch-name builds) falls back to string
|
||||
* inequality, so a dev build still surfaces rather than comparing equal
|
||||
* and going silent.
|
||||
*/
|
||||
@Test
|
||||
fun `two unparseable names fall back to string inequality`() {
|
||||
assertTrue(isVersionNewer("main", "dev"))
|
||||
assertFalse(isVersionNewer("dev", "dev"))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a leading v is ignored on either side`() {
|
||||
assertTrue(isVersionNewer("v2026.09.10.1432", "2026.09.09.1828"))
|
||||
assertFalse(isVersionNewer("v2026.09.10.1432", "v2026.09.10.1432"))
|
||||
}
|
||||
}
|
||||
Binary file not shown.
+20
-47
@@ -62,57 +62,30 @@ None.
|
||||
- **Go toolchain pin.** `go.mod` is on `go 1.25.0` because `golang.org/x/crypto v0.51.0` declares 1.25 as its minimum. `ci-go:1.26` satisfies this with headroom. Future `x/crypto` bumps that move the Go floor should be paired with an image-tag bump in this file + the workflows.
|
||||
- **In-app update channel — `needs:`, not polling.** `release.yml`'s `image-release` job declares `needs: [android-release]`, so on tag pushes the signed APK is guaranteed present before the image build starts — no polling window, no race. (The old cross-workflow polling against `flutter.yml` is gone with that workflow.) On non-tag `main` pushes `android-release` is skipped and `image-release` instead pulls the most recent release's APK and reconstructs its exact `versionName`, so `:latest` never ships without an update channel. It degrades to an empty `client/` — never a wrong version — if no release, asset, or tag commit-count can be resolved.
|
||||
- **Cache server reachability.** `test-web.yml` does NOT use `cache: 'npm'` on `actions/setup-node` — the Gitea Actions cache server isn't reachable from this runner's container network and `setup-node` was burning ~4m41s on ETIMEDOUT before failing open. With the migration to `ci-go:1.26`, `setup-node` is removed entirely (Node is in the image). The cache concern reappears if a future change re-introduces a network-dependent action.
|
||||
- **Artifacts — use the mirrored actions, never `actions/{upload,download}-artifact`.**
|
||||
- **Artifacts — stock `actions/upload-artifact@v7` and `actions/download-artifact@v8`; never `@v3`.**
|
||||
```yaml
|
||||
uses: https://git.fabledsword.com/bvandeusen/upload-artifact@cb8afe72b42edc798abfb8fcb556cf660d894245
|
||||
uses: https://git.fabledsword.com/bvandeusen/download-artifact@8d4e9521a5f7e5f8b6351f341f719f9f45a92a3a
|
||||
uses: actions/upload-artifact@v7
|
||||
uses: actions/download-artifact@v8
|
||||
```
|
||||
Upstream's `@v4+` cannot work against this instance and no server-side change
|
||||
will help: `isGhes()` rejects any hostname that isn't `github.com` /
|
||||
`*.ghe.com` / `*.localhost` and throws before it opens a connection, so the
|
||||
server is never asked what it supports. `@v3` is worse — it reports success,
|
||||
and Gitea then serves artifacts back only through the v4 API
|
||||
(`content_encoding = application/zip`), so a v3 upload is stored but invisible
|
||||
to every retrieval path. A green job producing nothing retrievable; that is how
|
||||
72 unreachable artifacts accumulated on this repo. Scribe issues 2255 / 2270.
|
||||
Stock works on this forge since the runner moved to gitea/runner 3.x, which
|
||||
edits the actions' client-side `isGhes()` refusal out of their bundles. Proven
|
||||
on 2026-09-10 for upload v4–v7 and download v4–v8 (Scribe spike #3843). Until
|
||||
then this repo pinned SHA mirrors of the Forgejo project's forks, because
|
||||
upstream threw on the hostname before it opened a connection (Scribe 2255).
|
||||
|
||||
Both are pull mirrors of the Forgejo project's forks
|
||||
(`code.forgejo.org/forgejo/{upload,download}-artifact`, one commit on upstream
|
||||
disabling that check), mirrored so CI depends on commits we hold and pinned by
|
||||
SHA because the mirrors auto-sync every 8h — a moved upstream tag would
|
||||
otherwise silently change what runs.
|
||||
`@v3` is still broken: it reports success, and Gitea serves artifacts back only
|
||||
through the v4 API (`content_encoding = application/zip`), so a v3 upload is
|
||||
stored but invisible to every retrieval path. That is how 72 unreachable
|
||||
artifacts accumulated on this repo (Scribe 2270).
|
||||
|
||||
**Match the pins on `@actions/artifact`, not on the actions' own version
|
||||
numbers.** The two actions release on unrelated cadences, so equal version
|
||||
numbers do NOT mean a compatible pair — upload `v5` bundles `@actions/artifact`
|
||||
^4.0.0 while download `v5` bundles ^2.3.2. The pins above are upload **v5** and
|
||||
download **v6**, which is the pairing that puts ^4.0.0 on both sides. This
|
||||
matters because `release.yml` is a producer/consumer pair — `android-release`
|
||||
uploads `minstrel-apk`, `image-release` downloads it — and a protocol mismatch
|
||||
across it yields an empty listing rather than an error, exactly the silent
|
||||
failure this entry exists to prevent.
|
||||
|
||||
| tag | `@actions/artifact` | runtime |
|
||||
|---|---|---|
|
||||
| upload v4 | ^2.1.1 | node20 |
|
||||
| **upload v5** ← pinned | **^4.0.0** | node20 |
|
||||
| download v4 | ^2.1.1 | node20 |
|
||||
| download v5 | ^2.3.2 | node20 |
|
||||
| **download v6** ← pinned | **^4.0.0** | node20 |
|
||||
| download v7 | ^5.0.0 | **node24** |
|
||||
|
||||
The only true protocol break in this history was **v3 → v4** (upstream:
|
||||
"Downloading artifacts that were created from `actions/upload-artifact@v3` and
|
||||
below are not supported"); v4-and-up are one family. Later majors are mostly
|
||||
ergonomics and runtime — upload v4 forbids re-uploading a name and caps a job
|
||||
at 500 artifacts; download v5 made by-ID extraction match by-name.
|
||||
|
||||
**Do not jump the download pin to v7.** That major is a runner requirement, not
|
||||
a feature change: it moves to `runs.using: node24` and upstream states it
|
||||
"requires a minimum Actions Runner version of 2.327.1 … if you are using
|
||||
self-hosted runners, ensure they are updated before upgrading." act_runner is
|
||||
not GitHub's runner and makes no such version claim, so node24 is unverified
|
||||
here. Everything currently pinned is node20.
|
||||
**Pairing no longer needs managing.** This entry used to pin upload v5 against
|
||||
download v6 so both bundled `@actions/artifact` ^4.0.0, warning that a mismatch
|
||||
across `release.yml`'s producer/consumer pair would list empty. Tested, and not
|
||||
true on this instance: every download major v4–v8 read the artifacts of every
|
||||
upload major v4–v7, by name and by pattern (CI-runner run 6312). The only real
|
||||
protocol break is v3 → v4. node24 is no longer a concern either — every
|
||||
CI-runner image carries Node 24 and the runner runs actions with the image's
|
||||
`node`.
|
||||
|
||||
Upload steps set `if-no-files-found: error` rather than the default `warn`, so
|
||||
an upload that matches nothing fails its own job instead of failing the
|
||||
|
||||
Executable
+159
@@ -0,0 +1,159 @@
|
||||
#!/usr/bin/env bash
|
||||
#
|
||||
# Derives the three values a build is stamped with, and the tag that names it.
|
||||
#
|
||||
# name=YYYY.MM.DD.HHMM label for people, from the timestamp of the newest
|
||||
# commit that CHANGED SOMETHING SHIPPED (see SHIPPED)
|
||||
# code=<int> ordering key, minutes since 2020-01-01 at BUILD time
|
||||
# tag=v<name> what a release of this commit must be called
|
||||
#
|
||||
# Usage: ci/version.sh [<commit-ish>] (default HEAD)
|
||||
#
|
||||
# This exists as a script rather than inline workflow YAML for one reason:
|
||||
# release.yml only runs on `main` and on tags, so anything living inside it is
|
||||
# unverifiable until a release is already happening — which is the worst
|
||||
# possible moment to discover the version is wrong, because the failure mode
|
||||
# is silent (an update nobody is offered looks exactly like being current).
|
||||
# As a script it can be executed by a test on every push instead.
|
||||
#
|
||||
# The two clocks are deliberate and are NOT interchangeable:
|
||||
#
|
||||
# The NAME answers "is this the same code?" — so it must read identically on
|
||||
# every lane that builds this commit. Commit time does that; build time
|
||||
# prints two different strings for one thing.
|
||||
#
|
||||
# The CODE answers "may this be installed over that?" — so it must be
|
||||
# monotonic BY CONSTRUCTION. Build time is; commit time is not (rebuild an
|
||||
# older commit and it goes down, which on a phone is a refused install), and
|
||||
# a commit COUNT is worse still, because it runs ahead on `dev` and inverts
|
||||
# against `main`.
|
||||
set -euo pipefail
|
||||
|
||||
readonly EPOCH_2020=1577836800 # 2020-01-01T00:00:00Z
|
||||
readonly REF="${1:-HEAD}"
|
||||
|
||||
# Both clocks are overridable so a test can pin them. Nothing but tests should
|
||||
# set these — the defaults are the real derivation.
|
||||
# The paths that do NOT ship, in either artifact. Everything else counts.
|
||||
#
|
||||
# A DENYLIST, and the direction is the whole point. As an allowlist, the list
|
||||
# has to be updated by whoever adds a directory and nothing fails if they
|
||||
# don't — so the failure mode is a changed artifact keeping its old version,
|
||||
# silently, on a green run. That is a build lying about what it is. Inverted,
|
||||
# new content counts by default and the only way to wrongly EXCLUDE something
|
||||
# is to name it here deliberately.
|
||||
#
|
||||
# The two error directions are not symmetric, which is why this is not taste:
|
||||
# wrongly excluded → changed artifact, unchanged version. A silent lie.
|
||||
# wrongly included → version moves when nothing shipped. Cosmetic noise in
|
||||
# a string nobody sorts.
|
||||
#
|
||||
# THIS REPO SHIPS TWO ARTIFACTS FROM ONE DERIVATION, and that is why the list
|
||||
# is shorter than it looks like it should be. The server image ships cmd/,
|
||||
# internal/, shared/, web/, config.example.yaml and client/; the APK ships
|
||||
# android/. Neither ships the other's sources — but excluding android/ here
|
||||
# would stop an Android-only commit from moving the APK's OWN version, which
|
||||
# is the dangerous direction. So this is the union: exclude only what ships in
|
||||
# NEITHER, and accept that an Android commit also nudges the server's reported
|
||||
# version. Over-inclusion across the two, which is the harmless direction.
|
||||
#
|
||||
# The family's other repos (roundtable / roundtable-android) each keep a
|
||||
# tighter list because they are separate repos with one artifact apiece. Do
|
||||
# not copy theirs onto this one.
|
||||
readonly SHIPPED=(
|
||||
.
|
||||
':!.gitea' # CI workflows — including this script's own caller
|
||||
':!ci' # CI scripts — including this script
|
||||
':!docs'
|
||||
':!tools' # asset/font generators; their OUTPUT ships, they do not
|
||||
':!deploy' # test-database bootstrap SQL
|
||||
':!bin' # local `make build` output
|
||||
':!*.md'
|
||||
':!Makefile'
|
||||
':!docker-compose.yml'
|
||||
':!.env.example'
|
||||
':!.gitignore'
|
||||
':!.dockerignore'
|
||||
':!renovate.json'
|
||||
':!.golangci.yml'
|
||||
|
||||
# TESTS DO NOT SHIP, so they must not re-version an artifact.
|
||||
#
|
||||
# Named as globs rather than a directory because this repo has no tests/
|
||||
# tree to exclude: Go tests sit inline beside the code they cover, and the
|
||||
# web suite sits beside its modules. `go build` drops *_test.go outright and
|
||||
# the Vite build never imports a .test.ts, so neither reaches an artifact.
|
||||
#
|
||||
# A commit touching a test AND its source still moves the version — the
|
||||
# source path matches on its own. Only a test-ONLY commit is inert, which is
|
||||
# the whole intent.
|
||||
#
|
||||
# Patterns match what exists today and nothing speculative: there are no
|
||||
# .spec.* files, no __tests__/ directories and no androidTest/ tree. If any
|
||||
# appear they will re-version until named here, which is the harmless
|
||||
# direction and the reason this list is a denylist.
|
||||
':!*_test.go' # 158 files, inline beside the code
|
||||
':!*.test.ts' # 114 files
|
||||
':!*.test.js'
|
||||
':!android/app/src/test' # JVM unit tests; no androidTest tree exists
|
||||
':!web/vitest.config.ts' # test-harness config, not build config
|
||||
':!web/vitest.setup.ts'
|
||||
)
|
||||
|
||||
commit_epoch="${MINSTREL_COMMIT_EPOCH:-}"
|
||||
if [ -z "${commit_epoch}" ]; then
|
||||
commit_epoch="$(git log --format=%ct -1 "${REF}" -- "${SHIPPED[@]}")"
|
||||
# Loudly, on purpose. A silent fallback here is the landmine this whole
|
||||
# script exists to avoid: a plausible-looking version that is quietly wrong,
|
||||
# on a green run. Realistically this means a shallow clone (no commit in
|
||||
# range touches the shipped set) rather than a repo of pure CI config.
|
||||
if [ -z "${commit_epoch}" ]; then
|
||||
echo "version.sh: no commit under '${REF}' touches the shipped file set — shallow clone? (needs fetch-depth: 0)" >&2
|
||||
exit 1
|
||||
fi
|
||||
fi
|
||||
now_epoch="${MINSTREL_NOW_EPOCH:-$(date -u +%s)}"
|
||||
|
||||
if ! name="$(date -u -d "@${commit_epoch}" +%Y.%m.%d.%H%M 2>/dev/null)"; then
|
||||
echo "version.sh: could not read a commit timestamp from '${commit_epoch}'" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
if ! [ "${now_epoch}" -eq "${now_epoch}" ] 2>/dev/null; then
|
||||
echo "version.sh: build timestamp '${now_epoch}' is not a number" >&2
|
||||
exit 1
|
||||
fi
|
||||
code=$(( (now_epoch - EPOCH_2020) / 60 ))
|
||||
|
||||
# Assert the shape here, at the source. A malformed name builds, signs and
|
||||
# publishes perfectly happily; it only surfaces later as an update channel
|
||||
# that has quietly stopped offering anything.
|
||||
if [[ ! "${name}" =~ ^[0-9]{4}\.[0-9]{2}\.[0-9]{2}\.[0-9]{4}$ ]]; then
|
||||
echo "version.sh: name '${name}' is not YYYY.MM.DD.HHMM" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# A non-positive key means the build clock is set before 2020, and every
|
||||
# comparison downstream would be nonsense.
|
||||
if [ "${code}" -le 0 ]; then
|
||||
echo "version.sh: ordering key '${code}' is not positive — build clock wrong?" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Android's versionCode is a signed 32-bit int and the platform refuses an APK
|
||||
# whose code exceeds it. At ~525k minutes a year this is four thousand years
|
||||
# away in normal operation, so the realistic cause is a build machine with a
|
||||
# badly wrong clock — which produces a code that is not merely too large but
|
||||
# also unreachably high, permanently blocking every real build that follows
|
||||
# from ever outranking it. Cheaper to refuse the build than to discover that
|
||||
# from a phone that will not update.
|
||||
readonly VERSION_CODE_CEILING=2147483647
|
||||
if [ "${code}" -gt "${VERSION_CODE_CEILING}" ]; then
|
||||
echo "version.sh: ordering key '${code}' exceeds versionCode's int32 ceiling — build clock wrong?" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# KEY=VALUE, which is also exactly $GITHUB_OUTPUT's format.
|
||||
echo "name=${name}"
|
||||
echo "code=${code}"
|
||||
echo "tag=v${name}"
|
||||
@@ -97,14 +97,16 @@ type tuningSnapshot struct {
|
||||
func (h *handlers) tuningSnapshot() tuningSnapshot {
|
||||
var out tuningSnapshot
|
||||
out.Profiles = map[string]weightsResp{
|
||||
recsettings.ScopeRadio: weightsRespFrom(h.recSettings.Weights(recsettings.ScopeRadio)),
|
||||
recsettings.ScopeDailyMix: weightsRespFrom(h.recSettings.Weights(recsettings.ScopeDailyMix)),
|
||||
recsettings.ScopeRadio: weightsRespFrom(h.recSettings.Weights(recsettings.ScopeRadio)),
|
||||
recsettings.ScopeDailyMix: weightsRespFrom(h.recSettings.Weights(recsettings.ScopeDailyMix)),
|
||||
recsettings.ScopeSongsLike: weightsRespFrom(h.recSettings.Weights(recsettings.ScopeSongsLike)),
|
||||
}
|
||||
out.Taste = tasteRespFrom(h.recSettings.Taste())
|
||||
out.Discover = discoverRespFrom(h.recSettings.Discover())
|
||||
out.Shipped.Profiles = map[string]weightsResp{
|
||||
recsettings.ScopeRadio: weightsRespFrom(recsettings.ShippedRadioWeights()),
|
||||
recsettings.ScopeDailyMix: weightsRespFrom(recsettings.ShippedDailyMixWeights()),
|
||||
recsettings.ScopeRadio: weightsRespFrom(recsettings.ShippedRadioWeights()),
|
||||
recsettings.ScopeDailyMix: weightsRespFrom(recsettings.ShippedDailyMixWeights()),
|
||||
recsettings.ScopeSongsLike: weightsRespFrom(recsettings.ShippedSongsLikeWeights()),
|
||||
}
|
||||
out.Shipped.Taste = tasteRespFrom(recsettings.ShippedTasteTuning())
|
||||
out.Shipped.Discover = discoverRespFrom(recsettings.ShippedDiscoverTuning())
|
||||
|
||||
+12
-6
@@ -24,6 +24,7 @@ import (
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/playevents"
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/playlists"
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/reacquisition"
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/recommendation"
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/recsettings"
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/tags"
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/tracks"
|
||||
@@ -55,6 +56,7 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev
|
||||
streamSecret: streamSecret,
|
||||
netSettings: netSettings,
|
||||
reacqSettings: reacqSettings,
|
||||
librarySize: recommendation.NewLibrarySize(nil),
|
||||
}
|
||||
|
||||
r.Route("/api", func(api chi.Router) {
|
||||
@@ -268,12 +270,16 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev
|
||||
}
|
||||
|
||||
type handlers struct {
|
||||
pool *pgxpool.Pool
|
||||
logger *slog.Logger
|
||||
events *playevents.Writer
|
||||
recCfg config.RecommendationConfig
|
||||
recSettings *recsettings.Service
|
||||
rng func() float64
|
||||
pool *pgxpool.Pool
|
||||
logger *slog.Logger
|
||||
events *playevents.Writer
|
||||
recCfg config.RecommendationConfig
|
||||
recSettings *recsettings.Service
|
||||
rng func() float64
|
||||
// librarySize memoises the track count that sizes the candidate pool
|
||||
// (#3880). Held here rather than counted per request: the count is a
|
||||
// full table scan, and library size only moves when a scan runs.
|
||||
librarySize *recommendation.LibrarySize
|
||||
lidarrCfg *lidarrconfig.Service
|
||||
lidarrRequests *lidarrrequests.Service
|
||||
lidarrQuarantine *lidarrquarantine.Service
|
||||
|
||||
@@ -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 "<name> <code>", 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(),
|
||||
})
|
||||
|
||||
@@ -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())
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+20
-2
@@ -87,7 +87,17 @@ func (h *handlers) handleRadio(w http.ResponseWriter, r *http.Request) {
|
||||
currentVec.DeviceClass = latestDeviceClass(r.Context(), q, user.ID, h.logger)
|
||||
|
||||
exclude := parseExcludeParam(r.URL.Query().Get("exclude"))
|
||||
limits := recommendation.DefaultCandidateSourceLimits()
|
||||
// Size the pool to the library (#3880). A fixed ~170 candidates samples a
|
||||
// shrinking fraction of a growing collection, which is what made the
|
||||
// recommendations feel less relevant as the library grew. Degrades to the
|
||||
// base limits if the count is unavailable — never fails the request over a
|
||||
// sizing hint.
|
||||
librarySize := h.librarySize.Get(r.Context(), func(ctx context.Context) (int64, error) {
|
||||
return recommendation.CountLibraryTracks(ctx, q)
|
||||
})
|
||||
limits := recommendation.ScaleForLibrary(
|
||||
recommendation.DefaultCandidateSourceLimits(), librarySize,
|
||||
)
|
||||
candidates, err := recommendation.LoadCandidatesFromSimilarity(
|
||||
r.Context(), q, user.ID, seedID,
|
||||
h.recCfg.RecentlyPlayedHours, currentVec, exclude, limits,
|
||||
@@ -108,7 +118,15 @@ func (h *handlers) handleRadio(w http.ResponseWriter, r *http.Request) {
|
||||
// Scoring weights come from the DB-backed tuning lab (#1250) —
|
||||
// read per request so an admin change takes effect live.
|
||||
weights := h.recSettings.Weights(recsettings.ScopeRadio)
|
||||
picks := recommendation.Shuffle(candidates, weights, time.Now().UTC(), h.rng, limit-1)
|
||||
// Diversity caps (#3882). Radio had none while every sibling surface did,
|
||||
// which is how a whole session could come back from one artist. Scaled to
|
||||
// the requested length so a 20-track radio and a 200-track one are capped
|
||||
// alike; Shuffle relaxes them rather than returning a short radio.
|
||||
//
|
||||
// limit-1 because the seed track occupies the first slot and is prepended
|
||||
// below — the caps govern the tracks that FOLLOW it.
|
||||
caps := recommendation.RadioDiversityCaps(limit - 1)
|
||||
picks := recommendation.Shuffle(candidates, weights, time.Now().UTC(), h.rng, limit-1, caps)
|
||||
|
||||
out := make([]TrackRef, 0, len(picks)+1)
|
||||
out = append(out, trackRefFrom(track, album.Title, artist.Name))
|
||||
|
||||
@@ -0,0 +1,16 @@
|
||||
-- Drop the rows the narrower constraints are about to forbid, or re-adding
|
||||
-- them fails against existing data (the 0051 down-migration pattern).
|
||||
DELETE FROM recommendation_weight_profiles WHERE profile = 'songs_like';
|
||||
DELETE FROM recommendation_tuning_audit WHERE scope = 'songs_like';
|
||||
|
||||
ALTER TABLE recommendation_tuning_audit
|
||||
DROP CONSTRAINT recommendation_tuning_audit_scope_check;
|
||||
ALTER TABLE recommendation_tuning_audit
|
||||
ADD CONSTRAINT recommendation_tuning_audit_scope_check
|
||||
CHECK (scope IN ('radio', 'daily_mix', 'taste', 'discover'));
|
||||
|
||||
ALTER TABLE recommendation_weight_profiles
|
||||
DROP CONSTRAINT recommendation_weight_profiles_profile_check;
|
||||
ALTER TABLE recommendation_weight_profiles
|
||||
ADD CONSTRAINT recommendation_weight_profiles_profile_check
|
||||
CHECK (profile IN ('radio', 'daily_mix'));
|
||||
@@ -0,0 +1,37 @@
|
||||
-- 0057_songs_like_tuning.up.sql — a THIRD weight profile, for Songs-like
|
||||
-- (Scribe #3881, milestone #398).
|
||||
--
|
||||
-- Songs-like shared the `daily_mix` profile with For-You, and that is the bug.
|
||||
-- The two surfaces want opposite things: For-You is a broad "what will they
|
||||
-- enjoy today", Songs-like answers "what sounds like THIS", and under one set
|
||||
-- of weights the broad answer wins. Operator, 2026-09-10: "I'm expecting to
|
||||
-- get a consistent sound and style from the experience... I was getting a
|
||||
-- seeming wide variety of music from each one when I was hoping to stay in a
|
||||
-- certain neighborhood."
|
||||
--
|
||||
-- Under the shared daily_mix weights, an UNRELATED track the user had liked
|
||||
-- and not played recently scored 1.0 + 2.0 + 1.0 = 4.0 before taste, while a
|
||||
-- PERFECT similarity match they had not liked scored 1.0 + 1.5 = 2.5. Liking
|
||||
-- something outranked sounding like the seed. Splitting the profile is what
|
||||
-- lets similarity dominate here without making For-You narrow.
|
||||
--
|
||||
-- Rows are seeded by the recsettings boot reconcile, not here, so shipped
|
||||
-- defaults live in exactly one place (Go) — same as 0040.
|
||||
|
||||
-- Rule #36: a new value for a CHECK-gated column needs the constraint
|
||||
-- rewritten in the SAME change, or the first row written under the new
|
||||
-- profile fails at runtime rather than at migrate time.
|
||||
ALTER TABLE recommendation_weight_profiles
|
||||
DROP CONSTRAINT recommendation_weight_profiles_profile_check;
|
||||
ALTER TABLE recommendation_weight_profiles
|
||||
ADD CONSTRAINT recommendation_weight_profiles_profile_check
|
||||
CHECK (profile IN ('radio', 'daily_mix', 'songs_like'));
|
||||
|
||||
-- The audit table gates the same name on a separate constraint. Missing this
|
||||
-- one would let the profile be seeded and then fail on the first knob turn —
|
||||
-- green at boot, 500 on first use.
|
||||
ALTER TABLE recommendation_tuning_audit
|
||||
DROP CONSTRAINT recommendation_tuning_audit_scope_check;
|
||||
ALTER TABLE recommendation_tuning_audit
|
||||
ADD CONSTRAINT recommendation_tuning_audit_scope_check
|
||||
CHECK (scope IN ('radio', 'daily_mix', 'taste', 'discover', 'songs_like'));
|
||||
+101
-15
@@ -219,7 +219,30 @@ var (
|
||||
// uniform with radio pending trend data.
|
||||
ContextTimeWeight: 1.0,
|
||||
}
|
||||
// Songs-like's own profile (#3881). Pre-push literal only; shipped
|
||||
// defaults live in recsettings.ShippedSongsLikeWeights and must stay in
|
||||
// sync with it, exactly as systemMixWeights does above.
|
||||
//
|
||||
// SimilarityWeight dominates here and every seed-INDEPENDENT term is
|
||||
// demoted, which is the whole difference between this surface and For-You.
|
||||
// See ShippedSongsLikeWeights for the property the numbers encode.
|
||||
songsLikeWeights = recommendation.ScoringWeights{
|
||||
BaseWeight: 1.0,
|
||||
LikeBoost: 0.5,
|
||||
RecencyWeight: 0.25,
|
||||
SkipPenalty: 2.0,
|
||||
JitterMagnitude: 0.05,
|
||||
ContextWeight: 0.5,
|
||||
SimilarityWeight: 4.0,
|
||||
TasteWeight: 0.25,
|
||||
ContextTimeWeight: 0.5,
|
||||
}
|
||||
systemTasteConfig = taste.DefaultConfig()
|
||||
|
||||
// Sizes the candidate pool to the library (#3880). Cached because the
|
||||
// count is a full table scan and the daily build runs it once per user;
|
||||
// within the TTL every user in a build shares one count.
|
||||
systemLibrarySize = recommendation.NewLibrarySize(nil)
|
||||
)
|
||||
|
||||
// SetSystemMixWeights installs the current daily_mix scoring weights.
|
||||
@@ -230,6 +253,20 @@ func SetSystemMixWeights(w recommendation.ScoringWeights) {
|
||||
systemMixWeights = w
|
||||
}
|
||||
|
||||
// SetSongsLikeWeights installs the songs_like scoring profile (#3881).
|
||||
// Same push model as SetSystemMixWeights — recsettings calls it on boot and
|
||||
// after every knob turn, so a tuning change takes effect on the next daily
|
||||
// build with no restart.
|
||||
//
|
||||
// Separate from systemMixWeights because Songs-like and For-You want opposite
|
||||
// things: For-You roams, Songs-like must not. Sharing one profile is what made
|
||||
// "Songs like X" wander.
|
||||
func SetSongsLikeWeights(w recommendation.ScoringWeights) {
|
||||
systemTuningMu.Lock()
|
||||
defer systemTuningMu.Unlock()
|
||||
songsLikeWeights = w
|
||||
}
|
||||
|
||||
// SetTasteConfig installs the taste-profile build configuration
|
||||
// (half-life + engagement curve, #1250). Same push model as
|
||||
// SetSystemMixWeights.
|
||||
@@ -239,6 +276,12 @@ func SetTasteConfig(c taste.Config) {
|
||||
systemTasteConfig = c
|
||||
}
|
||||
|
||||
func currentSongsLikeWeights() recommendation.ScoringWeights {
|
||||
systemTuningMu.RLock()
|
||||
defer systemTuningMu.RUnlock()
|
||||
return songsLikeWeights
|
||||
}
|
||||
|
||||
func currentSystemMixWeights() recommendation.ScoringWeights {
|
||||
systemTuningMu.RLock()
|
||||
defer systemTuningMu.RUnlock()
|
||||
@@ -392,7 +435,13 @@ func pickWeightedTail(tailPool []recommendation.Candidate, dateStr string, tailN
|
||||
// tieBreakHash). The scoring RNG is seeded by userIDHash so jitter is
|
||||
// deterministic per (user, day) but rotates across days. Pure — no
|
||||
// truncation, no cap.
|
||||
func scoreAndSortCandidates(cands []recommendation.Candidate, userID pgtype.UUID, dateStr string, now time.Time) []recommendation.Candidate {
|
||||
// weights is a parameter rather than a read of currentSystemMixWeights()
|
||||
// because this sort IS the selection: the caller caps and truncates in the
|
||||
// order this returns, so whatever profile ranks here decides which tracks
|
||||
// reach the playlist. Scoring with daily_mix here and re-scoring with
|
||||
// songs_like afterwards would have let the new profile relabel tracks it had
|
||||
// no part in choosing — inert where it matters (#3881).
|
||||
func scoreAndSortCandidates(cands []recommendation.Candidate, userID pgtype.UUID, dateStr string, now time.Time, weights recommendation.ScoringWeights) []recommendation.Candidate {
|
||||
rng := rand.New(rand.NewSource(int64(userIDHash(userID, dateStr))))
|
||||
type scored struct {
|
||||
c recommendation.Candidate
|
||||
@@ -411,7 +460,6 @@ func scoreAndSortCandidates(cands []recommendation.Candidate, userID pgtype.UUID
|
||||
sort.SliceStable(ordered, func(i, j int) bool {
|
||||
return uuidLessPL(ordered[i].Track.ID, ordered[j].Track.ID)
|
||||
})
|
||||
weights := currentSystemMixWeights()
|
||||
pairs := make([]scored, len(ordered))
|
||||
for i, c := range ordered {
|
||||
pairs[i] = scored{c: c, score: recommendation.Score(c.Inputs, weights, now, rng.Float64)}
|
||||
@@ -632,6 +680,13 @@ func produceSeedMixes(
|
||||
seedPool := pickSeedArtistsFromRows(seedRowsLocal)
|
||||
seeds := pickSeedArtistsForDay(seedPool, userID, dateStr)
|
||||
|
||||
// Once per build rather than once per seed artist — six mixes would
|
||||
// otherwise mean six full-table counts for a number that cannot have
|
||||
// changed between them.
|
||||
librarySize := systemLibrarySize.Get(ctx, func(c context.Context) (int64, error) {
|
||||
return recommendation.CountLibraryTracks(c, q)
|
||||
})
|
||||
|
||||
out := make([]builtPlaylist, 0, len(seeds))
|
||||
for _, artistID := range seeds {
|
||||
artistRow, aerr := q.GetArtistByID(ctx, artistID)
|
||||
@@ -647,23 +702,46 @@ func produceSeedMixes(
|
||||
continue
|
||||
}
|
||||
zeroVec := recommendation.SessionVector{Seed: true}
|
||||
// Songs-like's own pool shape, not the default (#3881). Same total
|
||||
// size; the composition shifts toward arms that actually measure
|
||||
// distance from the seed. The default gave ~29% of candidates a
|
||||
// sim_score of literally 0.
|
||||
//
|
||||
// Then scaled to the library (#3880): a bigger collection should put
|
||||
// more genuinely-similar candidates in reach, not the same ~170
|
||||
// regardless. The weights still rank sim_score-0 arms last, so the
|
||||
// growth reaches coherence rather than working against it.
|
||||
cands, cerr := recommendation.LoadCandidatesFromSimilarity(
|
||||
ctx, q, userID, seedTrack, 1, zeroVec, []pgtype.UUID{seedTrack},
|
||||
recommendation.DefaultCandidateSourceLimits(),
|
||||
recommendation.ScaleForLibrary(
|
||||
recommendation.SongsLikeCandidateSourceLimits(), librarySize,
|
||||
),
|
||||
)
|
||||
if cerr != nil {
|
||||
logger.Warn("system playlist: seed candidates load failed; skipping",
|
||||
"artist_id", uuidStringPL(artistID), "err", cerr)
|
||||
continue
|
||||
}
|
||||
// "Songs like X" excludes X's own songs.
|
||||
filtered := make([]recommendation.Candidate, 0, len(cands))
|
||||
for _, c := range cands {
|
||||
if !pgtypeUUIDEqual(c.Track.ArtistID, artistID) {
|
||||
filtered = append(filtered, c)
|
||||
}
|
||||
}
|
||||
tracks := pickTopN(filtered, userID, dateStr, now, systemMixLength)
|
||||
// The seed artist's own songs are ELIGIBLE here, deliberately.
|
||||
//
|
||||
// This used to filter them out — "Songs like X excludes X's own
|
||||
// songs" — which reads as obviously right and is not. The seed is a
|
||||
// TRACK, and the tracks most likely to sound like it are usually the
|
||||
// rest of that artist's catalogue; excluding them threw away the
|
||||
// nearest neighbours of the very thing the mix is built around, and
|
||||
// then reached further out to replace them. On a surface whose whole
|
||||
// job is staying in one neighbourhood, that is backwards.
|
||||
//
|
||||
// Operator, 2026-09-10: "it should also be able to include music from
|
||||
// the same artist."
|
||||
//
|
||||
// Domination is bounded by the cap rather than by exclusion, which is
|
||||
// the distinction that makes this safe: capCandidatesByAlbumAndArtist
|
||||
// inside pickTopN allows at most discoverMaxTracksPerArtist (3) of a
|
||||
// 25-track mix — 12%, a presence rather than a takeover. The seed
|
||||
// track itself still cannot appear; it is passed as an exclusion to
|
||||
// LoadCandidatesFromSimilarity above.
|
||||
tracks := pickTopN(cands, userID, dateStr, now, systemMixLength)
|
||||
if len(tracks) == 0 {
|
||||
continue
|
||||
}
|
||||
@@ -838,13 +916,20 @@ func BuildSystemPlaylists(ctx context.Context, pool *pgxpool.Pool, logger *slog.
|
||||
// truncates to n. Used by Songs-like-X (and as the fallback inside
|
||||
// pickHeadAndTail for small pools).
|
||||
func pickTopN(cands []recommendation.Candidate, userID pgtype.UUID, dateStr string, now time.Time, n int) []rankedCandidate {
|
||||
sorted := scoreAndSortCandidates(cands, userID, dateStr, now)
|
||||
// songs_like, not daily_mix (#3881). produceSeedMixes is this function's
|
||||
// only caller, so the switch moves exactly one surface — For-You ranks
|
||||
// through pickHeadAndTail and keeps the broader daily_mix profile.
|
||||
//
|
||||
// The SAME profile does the selection sort and the final score. Passing
|
||||
// one and using the other is the subtle version of this bug: the playlist
|
||||
// would still be chosen by daily_mix and merely wear songs_like numbers.
|
||||
weights := currentSongsLikeWeights()
|
||||
sorted := scoreAndSortCandidates(cands, userID, dateStr, now, weights)
|
||||
capped := capCandidatesByAlbumAndArtist(sorted)
|
||||
if len(capped) > n {
|
||||
capped = capped[:n]
|
||||
}
|
||||
rng := rand.New(rand.NewSource(int64(userIDHash(userID, dateStr))))
|
||||
weights := currentSystemMixWeights()
|
||||
out := make([]rankedCandidate, len(capped))
|
||||
for i, c := range capped {
|
||||
out[i] = rankedCandidate{
|
||||
@@ -873,10 +958,11 @@ func pickHeadAndTail(
|
||||
cands []recommendation.Candidate, seedOf map[pgtype.UUID]int, numSeeds int,
|
||||
userID pgtype.UUID, dateStr string, now time.Time, headN, tailN int,
|
||||
) []rankedCandidate {
|
||||
sorted := scoreAndSortCandidates(cands, userID, dateStr, now)
|
||||
// daily_mix — For-You is the broad surface and keeps the roaming profile.
|
||||
weights := currentSystemMixWeights()
|
||||
sorted := scoreAndSortCandidates(cands, userID, dateStr, now, weights)
|
||||
capped := capCandidatesByAlbumAndArtist(sorted)
|
||||
rng := rand.New(rand.NewSource(int64(userIDHash(userID, dateStr))))
|
||||
weights := currentSystemMixWeights()
|
||||
|
||||
total := headN + tailN
|
||||
if len(capped) <= total {
|
||||
|
||||
@@ -2,8 +2,10 @@ package playlists_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"io"
|
||||
"log/slog"
|
||||
"path/filepath"
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
@@ -287,6 +289,141 @@ func TestBuildSystemPlaylists_Concurrency(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// seedSharedArtistLibrary seeds artists that genuinely OWN several tracks.
|
||||
//
|
||||
// seedActiveLibrary cannot be used for this: its helper documents that
|
||||
// "artist and album are not deduplicated across calls (mbid-less upsert)",
|
||||
// so every track gets its own artist row and a seed artist always has
|
||||
// exactly one track — the seed itself, which is excluded. A same-artist
|
||||
// assertion against that fixture can never pass no matter what the code
|
||||
// does, which is how the first version of this test failed.
|
||||
//
|
||||
// Albums are not deduplicated either, for the same mbid-less reason, so each
|
||||
// track ends up under its own album row however the titles are written. That
|
||||
// is convenient here rather than a problem: it means the per-ALBUM cap (2)
|
||||
// never binds, and the per-ARTIST cap (3) is unambiguously the thing under
|
||||
// test. Do not "fix" the album titles into something shared without checking
|
||||
// which cap you are then measuring.
|
||||
func seedSharedArtistLibrary(
|
||||
t *testing.T, pool *pgxpool.Pool, name string, numArtists, tracksPerArtist int,
|
||||
) (dbq.User, []pgtype.UUID) {
|
||||
t.Helper()
|
||||
q := dbq.New(pool)
|
||||
ctx := context.Background()
|
||||
u := seedUser(t, pool, name)
|
||||
now := time.Now().UTC()
|
||||
dir := t.TempDir()
|
||||
|
||||
artistIDs := make([]pgtype.UUID, 0, numArtists)
|
||||
for a := 0; a < numArtists; a++ {
|
||||
artistName := name + "-shared-" + string(rune('A'+a))
|
||||
ar, err := q.UpsertArtist(ctx, dbq.UpsertArtistParams{Name: artistName, SortName: artistName})
|
||||
if err != nil {
|
||||
t.Fatalf("seed artist: %v", err)
|
||||
}
|
||||
artistIDs = append(artistIDs, ar.ID)
|
||||
|
||||
for k := 0; k < tracksPerArtist; k++ {
|
||||
albumTitle := fmt.Sprintf("%s - Album %d", artistName, k/2)
|
||||
al, err := q.UpsertAlbum(ctx, dbq.UpsertAlbumParams{
|
||||
Title: albumTitle, SortTitle: albumTitle, ArtistID: ar.ID,
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("seed album: %v", err)
|
||||
}
|
||||
tk, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{
|
||||
Title: fmt.Sprintf("%s-t%d", artistName, k), AlbumID: al.ID, ArtistID: ar.ID,
|
||||
DurationMs: 1000,
|
||||
FilePath: filepath.Join(dir, fmt.Sprintf("%s-%d-%d.mp3", name, a, k)),
|
||||
FileSize: 100, FileFormat: "mp3",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("seed track: %v", err)
|
||||
}
|
||||
// Well clear of the recently-played exclusion window, which uses
|
||||
// the DATABASE clock rather than the build's `now`.
|
||||
for pl := 0; pl < 3; pl++ {
|
||||
seedPlayEvent(t, pool, u.ID, tk.ID,
|
||||
now.Add(-time.Duration(24+a*10+k+pl)*time.Hour), false)
|
||||
}
|
||||
}
|
||||
}
|
||||
return u, artistIDs
|
||||
}
|
||||
|
||||
// The seed artist's own tracks are ELIGIBLE for its "Songs like" mix, and are
|
||||
// bounded by the diversity cap rather than excluded outright (#3881).
|
||||
//
|
||||
// produceSeedMixes used to filter them out — "Songs like X excludes X's own
|
||||
// songs" — which threw away the nearest neighbours of the seed TRACK and then
|
||||
// reached further out to replace them. Operator, 2026-09-10: "it should also
|
||||
// be able to include music from the same artist."
|
||||
//
|
||||
// Asserted end-to-end rather than by reading the source, because the guard has
|
||||
// to survive the filter coming back in a different shape — and because an
|
||||
// absence check would now match the comment explaining why the filter is gone.
|
||||
func TestBuildSystemPlaylists_SongsLikeIncludesItsSeedArtist(t *testing.T) {
|
||||
pool := newPool(t)
|
||||
logger := discardLogger()
|
||||
u, _ := seedSharedArtistLibrary(t, pool, "seedartist", 4, 6)
|
||||
ctx := context.Background()
|
||||
now := time.Date(2026, 5, 4, 12, 0, 0, 0, time.UTC)
|
||||
|
||||
if err := playlists.BuildSystemPlaylists(ctx, pool, logger, u.ID, now, t.TempDir()); err != nil {
|
||||
t.Fatalf("build: %v", err)
|
||||
}
|
||||
|
||||
rows, err := pool.Query(ctx, `
|
||||
SELECT count(*) FILTER (WHERE t.artist_id = p.seed_artist_id) AS own,
|
||||
count(*) AS total
|
||||
FROM playlists p
|
||||
JOIN playlist_tracks pt ON pt.playlist_id = p.id
|
||||
JOIN tracks t ON t.id = pt.track_id
|
||||
WHERE p.user_id = $1 AND p.system_variant = 'songs_like_artist'
|
||||
GROUP BY p.id
|
||||
`, u.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("query: %v", err)
|
||||
}
|
||||
defer rows.Close()
|
||||
|
||||
// discoverMaxTracksPerArtist, which this package_test cannot reference.
|
||||
// Duplicated deliberately: if the cap moves, this failing is the point.
|
||||
const maxPerArtist = 3
|
||||
|
||||
mixes, withOwn := 0, 0
|
||||
for rows.Next() {
|
||||
var own, total int
|
||||
if err := rows.Scan(&own, &total); err != nil {
|
||||
t.Fatalf("scan: %v", err)
|
||||
}
|
||||
mixes++
|
||||
if own > 0 {
|
||||
withOwn++
|
||||
}
|
||||
// The bound is what makes inclusion safe. Without it, "include the
|
||||
// seed artist" becomes "the mix is mostly the seed artist", which is
|
||||
// the radio failure (#3882) arriving on a different surface.
|
||||
if own > maxPerArtist {
|
||||
t.Errorf("a songs_like mix carries %d tracks by its own seed artist "+
|
||||
"out of %d; the per-artist cap (%d) is not being applied",
|
||||
own, total, maxPerArtist)
|
||||
}
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
t.Fatalf("rows: %v", err)
|
||||
}
|
||||
|
||||
if mixes == 0 {
|
||||
t.Fatal("no songs_like_artist mixes were built, so this test asserts nothing")
|
||||
}
|
||||
if withOwn == 0 {
|
||||
t.Errorf("none of the %d songs_like mixes contains a single track by its own "+
|
||||
"seed artist — the tracks most likely to sound like the seed are being "+
|
||||
"excluded from the surface whose job is sounding like the seed", mixes)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBuildSystemPlaylists_DailyNonceDeterminism(t *testing.T) {
|
||||
pool := newPool(t)
|
||||
logger := discardLogger()
|
||||
|
||||
@@ -105,6 +105,83 @@ func DefaultCandidateSourceLimits() CandidateSourceLimits {
|
||||
}
|
||||
}
|
||||
|
||||
// SongsLikeCandidateSourceLimits is the pool shape for "Songs like {X}"
|
||||
// (#3881). Same total size as the default (~170) — the composition is what
|
||||
// changes, shifted hard toward arms that actually measure distance from the
|
||||
// seed.
|
||||
//
|
||||
// The surface answers "what sounds like THIS", and it shared the default
|
||||
// pool with For-You, which answers the much broader "what will they enjoy
|
||||
// today". Under the default, 50 of ~170 candidates carried sim_score = 0 by
|
||||
// construction — `taste_overlap` (tracks by the user's top taste artists) and
|
||||
// `random_fill` (literally any track not already in the pool), both of which
|
||||
// are seed-INDEPENDENT. Nearly a third of the pool had no relationship to the
|
||||
// seed at all, and the operator saw it: "I was getting a seeming wide variety
|
||||
// of music from each one when I was hoping to stay in a certain neighborhood."
|
||||
//
|
||||
// TIERED, per rule 131 — a system mix degrades, it never vanishes:
|
||||
//
|
||||
// tier 1 lb_similar real track-level similarity. The exact promise.
|
||||
// tier 2 similar_artist, tag_overlap, coplay, likes_overlap
|
||||
// seed-RELATED but weaker signal.
|
||||
// tier 3 taste_overlap, random_fill
|
||||
// seed-independent. The floor, and nothing more.
|
||||
//
|
||||
// The tiering is enforced by SCORE rather than by a fallback ladder: tier-3
|
||||
// arms carry sim_score 0, so under SongsLike weights (SimilarityWeight 4.0,
|
||||
// everything seed-independent demoted) they rank below any real match and
|
||||
// surface only when tiers 1–2 cannot fill the mix. That is the rule's
|
||||
// "fill from tier 1 first, reach down only when a tier cannot fill".
|
||||
//
|
||||
// Which is exactly why tier 3 is REDUCED rather than removed. Zeroing those
|
||||
// two arms was the first instinct and it is the vanish-or-nothing shape rule
|
||||
// 131 exists to forbid: a seed whose artist has thin ListenBrainz coverage
|
||||
// would produce a short mix or none at all, and "no playlist" is a worse
|
||||
// answer than "a few tracks further from the seed than we would like".
|
||||
//
|
||||
// DO NOT SHRINK AN ARM ORDERED BY UNSEEDED random(). This is the constraint
|
||||
// that shapes the numbers below, and it is not obvious from reading them.
|
||||
//
|
||||
// `likes_overlap` and `random_fill` both end in a bare `ORDER BY random()`
|
||||
// (recommendation.sql:118, :161) with no daily seed. Such an arm returns a
|
||||
// STABLE set only while its LIMIT exceeds the rows eligible for it — at that
|
||||
// point it returns all of them and the random order is irrelevant, because
|
||||
// the caller sorts by id before scoring. Drop the limit below the eligible
|
||||
// count and the arm starts returning a random SUBSET, which differs between
|
||||
// two builds on the same day.
|
||||
//
|
||||
// That is a real defect (#3889) rather than a quirk of this function, and it
|
||||
// bit here: cutting RandomFill to 10 broke
|
||||
// TestBuildSystemPlaylists_DailyNonceDeterminism, whose library is smaller
|
||||
// than the default limit and whose determinism was therefore accidental.
|
||||
// Growing an arm is always safe; only shrinking one is.
|
||||
//
|
||||
// So the seed-independent arms are trimmed only where the ordering is
|
||||
// deterministic: `taste_overlap` sorts by `tpa.weight DESC, t.id` and can be
|
||||
// cut, `random_fill` cannot. The reduction is consequently modest — and it
|
||||
// matters less than it looks, because the WEIGHTS are what demote sim_score-0
|
||||
// candidates now. The pool change biases the draw; the songs_like profile is
|
||||
// what actually keeps unrelated tracks out of the result.
|
||||
//
|
||||
// One arm is left alone that arguably should not be: `likes_overlap` assigns
|
||||
// a FLAT 0.6 sim_score (recommendation.sql:108) rather than measuring
|
||||
// anything — a collaborative signal wearing similarity's clothes, which a
|
||||
// raised SimilarityWeight amplifies. If real ListenBrainz scores commonly
|
||||
// land below 0.6 it will outrank genuine matches. It cannot be trimmed here
|
||||
// without the determinism fix landing first; the honest repair is to stop it
|
||||
// claiming a similarity score it never computed (#3879).
|
||||
func SongsLikeCandidateSourceLimits() CandidateSourceLimits {
|
||||
return CandidateSourceLimits{
|
||||
LBSimilar: 60, // tier 1 — doubled; the only arm that measures the seed
|
||||
SimilarArtist: 40, // tier 2 — raised; growing is always safe
|
||||
TagOverlap: 20, // tier 2
|
||||
UserCoplay: 20, // tier 2
|
||||
LikesOverlap: 20, // tier 2 — NOT trimmed: unseeded random(), see above
|
||||
TasteOverlap: 10, // tier 3 floor — halved; deterministic ordering, safe
|
||||
RandomFill: 30, // tier 3 floor — NOT trimmed: unseeded random(), see above
|
||||
}
|
||||
}
|
||||
|
||||
// LoadCandidatesFromSimilarity is M4c's primary candidate-pool loader.
|
||||
// 5-way SQL UNION (LB-similar / similar-artist tracks / MB-tag overlap /
|
||||
// likes-overlap / random fill) + dedup-by-max sim_score. Returns
|
||||
|
||||
@@ -0,0 +1,188 @@
|
||||
package recommendation
|
||||
|
||||
import (
|
||||
"context"
|
||||
"math"
|
||||
"sync"
|
||||
"time"
|
||||
|
||||
"github.com/jackc/pgx/v5/pgtype"
|
||||
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
|
||||
)
|
||||
|
||||
// Scaling the candidate pool to the library (#3880).
|
||||
//
|
||||
// DefaultCandidateSourceLimits returns what its own comment calls "the v1
|
||||
// hardcoded constants per spec" — ~170 candidates, identical for a 500-track
|
||||
// library and a 100,000-track one. Operator, 2026-09-10: "is the pool that we
|
||||
// draw from somehow scaled to the amount of music in the library... my earlier
|
||||
// understanding of the tuning and work may have been skewed by what was in my
|
||||
// library."
|
||||
//
|
||||
// It was not, and the consequence compounds. The pool samples a shrinking
|
||||
// FRACTION of the library as it grows — 17% of 1,000 tracks, 1.7% of 10,000,
|
||||
// 0.17% of 100,000 — so the ceiling on how much of a collection can ever
|
||||
// surface stays flat while the collection does not. RandomFill, whose entire
|
||||
// job is exploration, becomes a thinner and noisier slice of a more diverse
|
||||
// corpus at exactly the moment diversity rises.
|
||||
|
||||
const (
|
||||
// libraryScaleReference is the library size at which the base limits
|
||||
// apply unchanged. Below it nothing scales, so small libraries keep
|
||||
// today's behaviour exactly.
|
||||
//
|
||||
// ASSUMED, NOT MEASURED. The size the v1 constants were actually tuned
|
||||
// against is unrecorded; 5,000 is a plausible mid-size library and a
|
||||
// deliberately conservative place to start growing. #3879 should replace
|
||||
// this with the real number, and until it does, this constant is the
|
||||
// single thing to change.
|
||||
libraryScaleReference = 5000
|
||||
|
||||
// maxLibraryScale bounds growth so a very large library does not turn
|
||||
// every recommendation query into a huge scan. At 4x the pool tops out
|
||||
// around 500 candidates, which is still cheap to score in memory.
|
||||
maxLibraryScale = 4.0
|
||||
)
|
||||
|
||||
// libraryScale is sqrt rather than linear on purpose. Linear growth would put
|
||||
// a 100,000-track library at a 3,400-candidate pool — a slow query, slow
|
||||
// scoring, and far past the point where more candidates improve the answer.
|
||||
// Square-root growth keeps the pool meaningfully proportional while staying
|
||||
// bounded: 1x at the reference, 2x at four times it, 4x at sixteen times.
|
||||
//
|
||||
// Never returns below 1: the base limits are a floor, not a midpoint. That
|
||||
// also keeps this safe against #3889 — growing an arm ordered by unseeded
|
||||
// random() is fine, shrinking one is what breaks same-day determinism.
|
||||
func libraryScale(libraryTracks int64) float64 {
|
||||
if libraryTracks <= libraryScaleReference {
|
||||
return 1.0
|
||||
}
|
||||
f := math.Sqrt(float64(libraryTracks) / float64(libraryScaleReference))
|
||||
return math.Min(f, maxLibraryScale)
|
||||
}
|
||||
|
||||
// ScaleForLibrary grows the library-bounded arms of a limit set for a library
|
||||
// of the given size, and leaves the rest alone.
|
||||
//
|
||||
// THE SCALING IS NOT UNIFORM, and that is the substance of it rather than a
|
||||
// refinement. An arm's limit only matters if there are rows for it to cut off,
|
||||
// so what an arm is BOUNDED BY decides whether library size can help it:
|
||||
//
|
||||
// scaled LBSimilar, SimilarArtist, TagOverlap — bounded by similarity
|
||||
// and tag data, which grows as the library does. A bigger library
|
||||
// means more of ListenBrainz's returned MBIDs survive the
|
||||
// local-library filter in similarity/worker.go.
|
||||
// scaled RandomFill — the exploration arm, and the one the complaint is
|
||||
// really about. This is where "fraction of the library" lives.
|
||||
// unscaled LikesOverlap — bounded by the USER's likes.
|
||||
// unscaled UserCoplay — bounded by the INSTANCE's co-play graph.
|
||||
// unscaled TasteOverlap — bounded by the taste profile's artists.
|
||||
//
|
||||
// Raising the last three would not sample more of the library; it would
|
||||
// sample more of a set that did not grow, which is churn rather than reach.
|
||||
// Leaving TasteOverlap alone has a second benefit: it and RandomFill are the
|
||||
// two arms carrying sim_score 0, so this does not inflate the
|
||||
// seed-independent share as fast as a uniform scale would.
|
||||
func ScaleForLibrary(base CandidateSourceLimits, libraryTracks int64) CandidateSourceLimits {
|
||||
f := libraryScale(libraryTracks)
|
||||
grow := func(n int) int { return int(float64(n) * f) } // f >= 1, so never shrinks
|
||||
return CandidateSourceLimits{
|
||||
LBSimilar: grow(base.LBSimilar),
|
||||
SimilarArtist: grow(base.SimilarArtist),
|
||||
TagOverlap: grow(base.TagOverlap),
|
||||
RandomFill: grow(base.RandomFill),
|
||||
LikesOverlap: base.LikesOverlap,
|
||||
UserCoplay: base.UserCoplay,
|
||||
TasteOverlap: base.TasteOverlap,
|
||||
}
|
||||
}
|
||||
|
||||
// CountLibraryTracks returns the whole library's track count.
|
||||
//
|
||||
// Reuses CountTracksMatching with an empty pattern — `title ILIKE '%%'`
|
||||
// matches every row — rather than adding a query, because a new one would
|
||||
// need sqlc regeneration (rule 28, and see #3889 for where that blocks).
|
||||
// The user id is left invalid so the quarantine anti-join is skipped: this
|
||||
// number sizes a pool, and a per-user view of it would be false precision.
|
||||
func CountLibraryTracks(ctx context.Context, q *dbq.Queries) (int64, error) {
|
||||
return q.CountTracksMatching(ctx, dbq.CountTracksMatchingParams{
|
||||
Column1: "",
|
||||
UserID: pgtype.UUID{}, // invalid → NULL → no quarantine filter
|
||||
})
|
||||
}
|
||||
|
||||
// librarySizeTTL is how long a counted size is reused. Library size changes
|
||||
// only when a scan runs, so minutes are plenty — and the count is a full
|
||||
// table scan (the ILIKE defeats every index), which is why it is not done
|
||||
// per request.
|
||||
const librarySizeTTL = 5 * time.Minute
|
||||
|
||||
// librarySizeTimeout bounds the count itself — see loadWithDeadline.
|
||||
const librarySizeTimeout = 3 * time.Second
|
||||
|
||||
// LibrarySize memoises the library track count.
|
||||
//
|
||||
// Degrades rather than fails: a count that errors or times out leaves the
|
||||
// previous value in place, and a zero (never yet counted) scales to the base
|
||||
// limits — which is exactly today's behaviour. Nothing about sizing a
|
||||
// candidate pool justifies failing the request it is sizing.
|
||||
type LibrarySize struct {
|
||||
mu sync.Mutex
|
||||
now func() time.Time // injectable for tests
|
||||
at time.Time
|
||||
size int64
|
||||
}
|
||||
|
||||
// NewLibrarySize returns an empty cache. The zero value works too; this
|
||||
// exists so tests can pin the clock.
|
||||
func NewLibrarySize(now func() time.Time) *LibrarySize {
|
||||
return &LibrarySize{now: now}
|
||||
}
|
||||
|
||||
// Get returns the cached size, refreshing through load when stale.
|
||||
//
|
||||
// A NIL RECEIVER IS VALID and means "no cache": the count still runs, it is
|
||||
// just not memoised. That is deliberate rather than defensive habit. The api
|
||||
// handlers struct is built directly by a dozen tests that cannot know about
|
||||
// every field, and a nil here previously panicked inside a radio request —
|
||||
// turning a missing pool-sizing HINT into a 500. Uncached-but-correct is the
|
||||
// right failure for something whose whole contract is that it degrades.
|
||||
func (l *LibrarySize) Get(ctx context.Context, load func(context.Context) (int64, error)) int64 {
|
||||
if l == nil {
|
||||
n, err := loadWithDeadline(ctx, load)
|
||||
if err != nil {
|
||||
return 0 // scales to the base limits
|
||||
}
|
||||
return n
|
||||
}
|
||||
|
||||
l.mu.Lock()
|
||||
defer l.mu.Unlock()
|
||||
|
||||
now := time.Now
|
||||
if l.now != nil {
|
||||
now = l.now
|
||||
}
|
||||
if !l.at.IsZero() && now().Sub(l.at) < librarySizeTTL {
|
||||
return l.size
|
||||
}
|
||||
|
||||
n, err := loadWithDeadline(ctx, load)
|
||||
if err != nil {
|
||||
// Keep the last known value and re-try at the next call rather than
|
||||
// stamping `at`, so a transient failure does not pin a stale number
|
||||
// for the whole TTL.
|
||||
return l.size
|
||||
}
|
||||
l.size, l.at = n, now()
|
||||
return l.size
|
||||
}
|
||||
|
||||
// loadWithDeadline bounds the count. Rule 156: the caller is a user waiting
|
||||
// on a radio, and a pool-sizing hint is never worth hanging for.
|
||||
func loadWithDeadline(ctx context.Context, load func(context.Context) (int64, error)) (int64, error) {
|
||||
cctx, cancel := context.WithTimeout(ctx, librarySizeTimeout)
|
||||
defer cancel()
|
||||
return load(cctx)
|
||||
}
|
||||
@@ -0,0 +1,216 @@
|
||||
package recommendation
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// Small libraries must behave exactly as before. Scaling is meant to help a
|
||||
// growing collection, not to change what every existing install already does.
|
||||
func TestScaleForLibrary_SmallLibrariesAreUntouched(t *testing.T) {
|
||||
base := DefaultCandidateSourceLimits()
|
||||
for _, size := range []int64{0, 1, 500, libraryScaleReference} {
|
||||
if got := ScaleForLibrary(base, size); got != base {
|
||||
t.Errorf("library of %d changed the limits: %+v, want %+v", size, got, base)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// The complaint, restated as a property: a bigger library must reach further
|
||||
// into itself. Operator, 2026-09-10 — "is the pool that we draw from somehow
|
||||
// scaled to the amount of music in the library".
|
||||
func TestScaleForLibrary_BigLibrariesGetABiggerPool(t *testing.T) {
|
||||
base := DefaultCandidateSourceLimits()
|
||||
small := ScaleForLibrary(base, libraryScaleReference)
|
||||
big := ScaleForLibrary(base, libraryScaleReference*16)
|
||||
|
||||
if big.RandomFill <= small.RandomFill {
|
||||
t.Errorf("RandomFill %d did not grow for a 16x library (was %d); that arm "+
|
||||
"IS the fraction-of-library problem", big.RandomFill, small.RandomFill)
|
||||
}
|
||||
if big.LBSimilar <= small.LBSimilar {
|
||||
t.Errorf("LBSimilar %d did not grow for a 16x library (was %d)",
|
||||
big.LBSimilar, small.LBSimilar)
|
||||
}
|
||||
}
|
||||
|
||||
// THE SUBSTANCE: scaling is per-arm, decided by what each arm is BOUNDED BY.
|
||||
// Raising a limit only helps if there are rows for it to cut off, so arms
|
||||
// bounded by the user's likes, the instance's co-play graph or the taste
|
||||
// profile gain nothing from a bigger library — raising them would sample more
|
||||
// of a set that did not grow.
|
||||
//
|
||||
// A uniform scale would look right and be wrong, which is why this is pinned
|
||||
// separately from "the pool got bigger".
|
||||
func TestScaleForLibrary_OnlyLibraryBoundedArmsGrow(t *testing.T) {
|
||||
base := DefaultCandidateSourceLimits()
|
||||
big := ScaleForLibrary(base, libraryScaleReference*16)
|
||||
|
||||
for _, tc := range []struct {
|
||||
arm string
|
||||
got, want int
|
||||
why string
|
||||
}{
|
||||
{"LikesOverlap", big.LikesOverlap, base.LikesOverlap, "bounded by the user's likes"},
|
||||
{"UserCoplay", big.UserCoplay, base.UserCoplay, "bounded by the instance's co-play graph"},
|
||||
{"TasteOverlap", big.TasteOverlap, base.TasteOverlap, "bounded by the taste profile's artists"},
|
||||
} {
|
||||
if tc.got != tc.want {
|
||||
t.Errorf("%s scaled to %d (base %d) but is %s — a bigger library gives it "+
|
||||
"nothing more to return", tc.arm, tc.got, tc.want, tc.why)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Growth is bounded, or a huge library turns every recommendation query into
|
||||
// a huge scan. sqrt keeps it proportional; the ceiling keeps it affordable.
|
||||
func TestScaleForLibrary_GrowthIsBounded(t *testing.T) {
|
||||
base := DefaultCandidateSourceLimits()
|
||||
huge := ScaleForLibrary(base, 100_000_000)
|
||||
|
||||
if huge.RandomFill > int(float64(base.RandomFill)*maxLibraryScale) {
|
||||
t.Errorf("RandomFill %d exceeds the %.0fx ceiling on base %d",
|
||||
huge.RandomFill, maxLibraryScale, base.RandomFill)
|
||||
}
|
||||
// sqrt, not linear — and the test point matters. At SIXTEEN times the
|
||||
// reference both curves land on 4x, because linear has already been
|
||||
// clamped by the ceiling; asserting there proves nothing. Four times the
|
||||
// reference is below the ceiling for both, so the curves separate: sqrt
|
||||
// gives 2x, linear would give 4x.
|
||||
four := ScaleForLibrary(base, libraryScaleReference*4)
|
||||
if four.RandomFill != base.RandomFill*2 {
|
||||
t.Errorf("4x library gave RandomFill %d, want %d — sqrt growth (linear "+
|
||||
"would give %d)", four.RandomFill, base.RandomFill*2, base.RandomFill*4)
|
||||
}
|
||||
}
|
||||
|
||||
// Never below the base. The base limits are a floor, not a midpoint — and
|
||||
// #3889 makes this load-bearing rather than tidy: shrinking an arm ordered by
|
||||
// unseeded random() changes pool membership between same-day rebuilds.
|
||||
func TestScaleForLibrary_NeverShrinksAnArm(t *testing.T) {
|
||||
base := DefaultCandidateSourceLimits()
|
||||
for _, size := range []int64{0, 1, 100, 4999, 5001, 1_000_000} {
|
||||
got := ScaleForLibrary(base, size)
|
||||
for _, tc := range []struct {
|
||||
arm string
|
||||
got, want int
|
||||
}{
|
||||
{"LBSimilar", got.LBSimilar, base.LBSimilar},
|
||||
{"SimilarArtist", got.SimilarArtist, base.SimilarArtist},
|
||||
{"TagOverlap", got.TagOverlap, base.TagOverlap},
|
||||
{"RandomFill", got.RandomFill, base.RandomFill},
|
||||
{"LikesOverlap", got.LikesOverlap, base.LikesOverlap},
|
||||
{"UserCoplay", got.UserCoplay, base.UserCoplay},
|
||||
{"TasteOverlap", got.TasteOverlap, base.TasteOverlap},
|
||||
} {
|
||||
if tc.got < tc.want {
|
||||
t.Errorf("library %d shrank %s to %d (base %d)", size, tc.arm, tc.got, tc.want)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// A pool-sizing hint must never fail the request it is sizing. A count that
|
||||
// errors leaves the previous value in place, and a never-counted cache
|
||||
// returns 0 — which scales to the base limits, i.e. exactly today's
|
||||
// behaviour.
|
||||
func TestLibrarySize_DegradesOnFailure(t *testing.T) {
|
||||
clock := time.Now()
|
||||
c := NewLibrarySize(func() time.Time { return clock })
|
||||
|
||||
boom := func(context.Context) (int64, error) { return 0, errors.New("db is down") }
|
||||
if got := c.Get(context.Background(), boom); got != 0 {
|
||||
t.Errorf("first failure returned %d, want 0 (which scales to the base limits)", got)
|
||||
}
|
||||
if got := ScaleForLibrary(DefaultCandidateSourceLimits(), 0); got != DefaultCandidateSourceLimits() {
|
||||
t.Error("a zero library size did not scale to the base limits")
|
||||
}
|
||||
|
||||
// A good count, then a failure: the last known value survives.
|
||||
if got := c.Get(context.Background(), func(context.Context) (int64, error) { return 40_000, nil }); got != 40_000 {
|
||||
t.Fatalf("got %d, want 40000", got)
|
||||
}
|
||||
clock = clock.Add(librarySizeTTL + time.Second)
|
||||
if got := c.Get(context.Background(), boom); got != 40_000 {
|
||||
t.Errorf("a failed refresh returned %d, discarding the last known 40000", got)
|
||||
}
|
||||
}
|
||||
|
||||
// The count is a full table scan, so it must not run per request.
|
||||
func TestLibrarySize_CachesWithinTheTTL(t *testing.T) {
|
||||
clock := time.Now()
|
||||
c := NewLibrarySize(func() time.Time { return clock })
|
||||
calls := 0
|
||||
load := func(context.Context) (int64, error) { calls++; return 1234, nil }
|
||||
|
||||
for i := 0; i < 5; i++ {
|
||||
c.Get(context.Background(), load)
|
||||
}
|
||||
if calls != 1 {
|
||||
t.Errorf("counted %d times within the TTL, want 1 — this is a full table scan", calls)
|
||||
}
|
||||
|
||||
clock = clock.Add(librarySizeTTL + time.Second)
|
||||
c.Get(context.Background(), load)
|
||||
if calls != 2 {
|
||||
t.Errorf("counted %d times after the TTL expired, want 2 — the size never refreshes", calls)
|
||||
}
|
||||
}
|
||||
|
||||
// A transient failure must not pin a stale value for the whole TTL: the
|
||||
// failed refresh does not stamp the clock, so the next call retries.
|
||||
func TestLibrarySize_RetriesAfterAFailedRefresh(t *testing.T) {
|
||||
clock := time.Now()
|
||||
c := NewLibrarySize(func() time.Time { return clock })
|
||||
|
||||
c.Get(context.Background(), func(context.Context) (int64, error) { return 100, nil })
|
||||
clock = clock.Add(librarySizeTTL + time.Second)
|
||||
c.Get(context.Background(), func(context.Context) (int64, error) { return 0, errors.New("blip") })
|
||||
|
||||
// Immediately after, with no clock movement — a stamped failure would
|
||||
// serve the stale 100 until the TTL expired again.
|
||||
if got := c.Get(context.Background(), func(context.Context) (int64, error) { return 900, nil }); got != 900 {
|
||||
t.Errorf("got %d after a failed refresh, want 900 — the failure pinned a stale value", got)
|
||||
}
|
||||
}
|
||||
|
||||
// A nil cache must not panic, and must still be CORRECT — uncached, not
|
||||
// broken.
|
||||
//
|
||||
// This is not a hypothetical hardening. internal/api builds its handlers
|
||||
// struct directly in a dozen tests, none of which know about every field, so
|
||||
// librarySize arrives nil there. The first version of this panicked inside
|
||||
// handleRadio and took down TestHandleRadio_ColdStart_OnlySeedReturned with a
|
||||
// SIGSEGV — turning a missing pool-sizing HINT into a request-killing crash,
|
||||
// which is the opposite of what a value that "degrades rather than fails" is
|
||||
// supposed to do.
|
||||
func TestLibrarySize_NilReceiverStillCounts(t *testing.T) {
|
||||
var c *LibrarySize // deliberately not constructed
|
||||
|
||||
calls := 0
|
||||
got := c.Get(context.Background(), func(context.Context) (int64, error) {
|
||||
calls++
|
||||
return 40_000, nil
|
||||
})
|
||||
if got != 40_000 {
|
||||
t.Errorf("nil cache returned %d, want 40000 — it should still count, just not memoise", got)
|
||||
}
|
||||
if calls != 1 {
|
||||
t.Errorf("nil cache called the loader %d times, want 1", calls)
|
||||
}
|
||||
|
||||
// Uncached: a second call counts again rather than reusing anything.
|
||||
c.Get(context.Background(), func(context.Context) (int64, error) { calls++; return 40_000, nil })
|
||||
if calls != 2 {
|
||||
t.Errorf("nil cache memoised across calls (%d loads); it has nowhere to store a value", calls)
|
||||
}
|
||||
|
||||
// And it still degrades on error rather than panicking.
|
||||
if got := c.Get(context.Background(), func(context.Context) (int64, error) {
|
||||
return 0, errors.New("db is down")
|
||||
}); got != 0 {
|
||||
t.Errorf("nil cache returned %d on a failed count, want 0 (base limits)", got)
|
||||
}
|
||||
}
|
||||
@@ -4,6 +4,8 @@ import (
|
||||
"sort"
|
||||
"time"
|
||||
|
||||
"github.com/jackc/pgx/v5/pgtype"
|
||||
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
|
||||
)
|
||||
|
||||
@@ -13,15 +15,72 @@ type Candidate struct {
|
||||
Inputs ScoringInputs
|
||||
}
|
||||
|
||||
// Shuffle scores each candidate, sorts descending by score, and returns
|
||||
// the top `limit` candidates. limit <= 0 returns nil; nil input returns
|
||||
// nil. Pure — no IO, no global state beyond the rng callback.
|
||||
// DiversityCaps bounds how much of one selection a single artist or album
|
||||
// may occupy. A zero value means "no cap", which is what Shuffle did
|
||||
// unconditionally before #3882.
|
||||
type DiversityCaps struct {
|
||||
MaxPerArtist int
|
||||
MaxPerAlbum int
|
||||
}
|
||||
|
||||
// radioCapArtistPer25 / radioCapAlbumPer25 hold the caps at the same
|
||||
// PROPORTION the system mixes already use — 3 artist / 2 album tracks in a
|
||||
// 25-track mix — so a 20-track radio and a 200-track one feel alike rather
|
||||
// than one of them being effectively uncapped.
|
||||
//
|
||||
// A fixed count cannot do that. Three-per-artist is a reasonable 12% of a
|
||||
// 25-track mix and an absurd 1.5% of a 200-track radio, where it would put
|
||||
// the selection permanently in the relaxation path below and quietly undo
|
||||
// the cap it was meant to enforce.
|
||||
const (
|
||||
radioCapArtistPer25 = 3
|
||||
radioCapAlbumPer25 = 2
|
||||
)
|
||||
|
||||
// RadioDiversityCaps scales the diversity caps to the requested radio
|
||||
// length. Floors of 2 and 1 keep a very short radio from being capped into
|
||||
// a single track per artist, which would be its own kind of wrong.
|
||||
func RadioDiversityCaps(limit int) DiversityCaps {
|
||||
artist := limit * radioCapArtistPer25 / 25
|
||||
if artist < 2 {
|
||||
artist = 2
|
||||
}
|
||||
album := limit * radioCapAlbumPer25 / 25
|
||||
if album < 1 {
|
||||
album = 1
|
||||
}
|
||||
return DiversityCaps{MaxPerArtist: artist, MaxPerAlbum: album}
|
||||
}
|
||||
|
||||
// Shuffle scores each candidate, sorts descending by score, and returns the
|
||||
// top `limit` candidates, preferring artist/album diversity. limit <= 0
|
||||
// returns nil; nil input returns nil. Pure — no IO, no global state beyond
|
||||
// the rng callback.
|
||||
//
|
||||
// THE COUNT IS NEVER REDUCED BY THE CAPS. Selection runs in two passes: the
|
||||
// first takes candidates that fit under the caps, and the second fills any
|
||||
// remaining slots from those the first pass skipped, still in score order.
|
||||
// So the result holds min(limit, len(candidates)) either way — the caps
|
||||
// change WHICH tracks are chosen, never HOW MANY.
|
||||
//
|
||||
// That two-pass shape is the whole design, and a hard cap would have been
|
||||
// the easy mistake. Radio asks for 50 tracks by default and 200 at most; a
|
||||
// pool concentrated on a few artists would return six tracks and call it a
|
||||
// radio. Rule 131's principle — degrade, never vanish — applies past the
|
||||
// system mixes it was written for.
|
||||
//
|
||||
// Before #3882 there was no cap here at all, while discover.go,
|
||||
// you_might_like.go and home.go all had one. That asymmetry is what let a
|
||||
// radio session come back entirely from a single artist: nothing between
|
||||
// the pool and the output bounded any artist's share, so a pool dominated
|
||||
// by one artist produced an output dominated by it too.
|
||||
func Shuffle(
|
||||
candidates []Candidate,
|
||||
weights ScoringWeights,
|
||||
now time.Time,
|
||||
rng func() float64,
|
||||
limit int,
|
||||
caps DiversityCaps,
|
||||
) []Candidate {
|
||||
if len(candidates) == 0 || limit <= 0 {
|
||||
return nil
|
||||
@@ -40,9 +99,36 @@ func Shuffle(
|
||||
if limit > len(scored) {
|
||||
limit = len(scored)
|
||||
}
|
||||
out := make([]Candidate, limit)
|
||||
for i := 0; i < limit; i++ {
|
||||
out[i] = scored[i].c
|
||||
|
||||
out := make([]Candidate, 0, limit)
|
||||
deferred := make([]Candidate, 0, len(scored)-limit)
|
||||
artistCount := map[pgtype.UUID]int{}
|
||||
albumCount := map[pgtype.UUID]int{}
|
||||
|
||||
for _, s := range scored {
|
||||
if len(out) == limit {
|
||||
break
|
||||
}
|
||||
overArtist := caps.MaxPerArtist > 0 && artistCount[s.c.Track.ArtistID] >= caps.MaxPerArtist
|
||||
overAlbum := caps.MaxPerAlbum > 0 && albumCount[s.c.Track.AlbumID] >= caps.MaxPerAlbum
|
||||
if overArtist || overAlbum {
|
||||
// Held back, not discarded — pass two may still need it.
|
||||
deferred = append(deferred, s.c)
|
||||
continue
|
||||
}
|
||||
artistCount[s.c.Track.ArtistID]++
|
||||
albumCount[s.c.Track.AlbumID]++
|
||||
out = append(out, s.c)
|
||||
}
|
||||
|
||||
// Pass two: the caps could not fill the request, so relax them rather
|
||||
// than hand back a short radio. Still score order, so the best of the
|
||||
// held-back candidates go first.
|
||||
for _, c := range deferred {
|
||||
if len(out) == limit {
|
||||
break
|
||||
}
|
||||
out = append(out, c)
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
@@ -0,0 +1,197 @@
|
||||
package recommendation
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/jackc/pgx/v5/pgtype"
|
||||
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
|
||||
)
|
||||
|
||||
// candBy builds a candidate with a real artist and album identity, which
|
||||
// `cand` deliberately leaves zero.
|
||||
func candBy(t *testing.T, id, artist, album string, in ScoringInputs) Candidate {
|
||||
t.Helper()
|
||||
tr := dbq.Track{Title: id}
|
||||
if err := tr.ID.Scan("00000000-0000-0000-0000-" + id); err != nil {
|
||||
t.Fatalf("track id %q: %v", id, err)
|
||||
}
|
||||
if err := tr.ArtistID.Scan("00000000-0000-0000-0001-" + artist); err != nil {
|
||||
t.Fatalf("artist id %q: %v", artist, err)
|
||||
}
|
||||
if err := tr.AlbumID.Scan("00000000-0000-0000-0002-" + album); err != nil {
|
||||
t.Fatalf("album id %q: %v", album, err)
|
||||
}
|
||||
return Candidate{Track: tr, Inputs: in}
|
||||
}
|
||||
|
||||
// artistKey is the map key artistsOf produces for a given fixture artist,
|
||||
// derived the same way the fixture builds the UUID. Hand-writing the hex is
|
||||
// how the first version of this test went wrong: the artist id is not all
|
||||
// zeros — it carries 0001 in its fourth group — so the literal did not match
|
||||
// and the assertion measured nothing.
|
||||
func artistKey(t *testing.T, artist string) string {
|
||||
t.Helper()
|
||||
var id pgtype.UUID
|
||||
if err := id.Scan("00000000-0000-0000-0001-" + artist); err != nil {
|
||||
t.Fatalf("artist id %q: %v", artist, err)
|
||||
}
|
||||
return fmt.Sprintf("%x", id.Bytes)
|
||||
}
|
||||
|
||||
// artistsOf counts how many picks each artist contributed.
|
||||
func artistsOf(picks []Candidate) map[string]int {
|
||||
out := map[string]int{}
|
||||
for _, p := range picks {
|
||||
out[fmt.Sprintf("%x", p.Track.ArtistID.Bytes)]++
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// THE BUG. Operator, 2026-09-10: started radio from a song and "literally
|
||||
// all of the songs in the playlist after that were from a single artist".
|
||||
//
|
||||
// One artist's tracks all outscore everything else, and there are more of
|
||||
// them than the radio has room for. Without a cap the output is entirely
|
||||
// that artist — nothing between the pool and the result bounded its share.
|
||||
func TestShuffle_CapStopsOneArtistTakingTheWholeRadio(t *testing.T) {
|
||||
var cs []Candidate
|
||||
// 20 tracks by artist A, all liked so they sort to the top.
|
||||
for i := 0; i < 20; i++ {
|
||||
cs = append(cs, candBy(t, fmt.Sprintf("%012d", i), "00000000000a",
|
||||
fmt.Sprintf("%012d", i), ScoringInputs{IsGeneralLiked: true}))
|
||||
}
|
||||
// 10 tracks by 10 other artists, none liked, so they all rank below.
|
||||
for i := 0; i < 10; i++ {
|
||||
cs = append(cs, candBy(t, fmt.Sprintf("%012d", 100+i),
|
||||
fmt.Sprintf("%012d", 200+i), fmt.Sprintf("%012d", 100+i),
|
||||
ScoringInputs{IsGeneralLiked: false}))
|
||||
}
|
||||
|
||||
const limit = 10
|
||||
caps := DiversityCaps{MaxPerArtist: 3}
|
||||
picks := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), limit, caps)
|
||||
|
||||
if len(picks) != limit {
|
||||
t.Fatalf("len = %d, want %d", len(picks), limit)
|
||||
}
|
||||
byArtist := artistsOf(picks)
|
||||
artistA := artistKey(t, "00000000000a")
|
||||
if got := byArtist[artistA]; got != 3 {
|
||||
t.Errorf("artist A contributed %d of %d picks, cap is 3", got, limit)
|
||||
}
|
||||
if len(byArtist) < 8 {
|
||||
t.Errorf("only %d distinct artists in a %d-track radio; the cap is not "+
|
||||
"spreading the selection", len(byArtist), limit)
|
||||
}
|
||||
|
||||
// The same pool with NO cap is the regression this exists to catch. If
|
||||
// this stops holding, the test above is no longer proving anything.
|
||||
uncapped := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), limit, DiversityCaps{})
|
||||
if artistsOf(uncapped)[artistA] != limit {
|
||||
t.Errorf("uncapped selection was not single-artist, so this fixture no " +
|
||||
"longer reproduces the bug being fixed")
|
||||
}
|
||||
}
|
||||
|
||||
// A CAP IS A BOUND, NOT AN EXCLUSION. The operator asked for the opposite of
|
||||
// removal: "again it should be able to add songs from the same artist."
|
||||
func TestShuffle_CappedArtistIsStillRepresented(t *testing.T) {
|
||||
var cs []Candidate
|
||||
for i := 0; i < 20; i++ {
|
||||
cs = append(cs, candBy(t, fmt.Sprintf("%012d", i), "00000000000a",
|
||||
fmt.Sprintf("%012d", i), ScoringInputs{IsGeneralLiked: true}))
|
||||
}
|
||||
for i := 0; i < 10; i++ {
|
||||
cs = append(cs, candBy(t, fmt.Sprintf("%012d", 100+i),
|
||||
fmt.Sprintf("%012d", 200+i), fmt.Sprintf("%012d", 100+i),
|
||||
ScoringInputs{IsGeneralLiked: false}))
|
||||
}
|
||||
picks := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10,
|
||||
DiversityCaps{MaxPerArtist: 3})
|
||||
if artistsOf(picks)[artistKey(t, "00000000000a")] == 0 {
|
||||
t.Error("the dominant artist was excluded entirely; the cap should bound " +
|
||||
"its share, not remove it")
|
||||
}
|
||||
}
|
||||
|
||||
// RULE 131, applied past the system mixes it was written for: degrade, never
|
||||
// vanish. A hard cap over a pool with few artists would hand back a six-track
|
||||
// "radio" for a fifty-track request. The caps must change WHICH tracks are
|
||||
// picked, never HOW MANY.
|
||||
func TestShuffle_CapsNeverShortenTheResult(t *testing.T) {
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
artists int
|
||||
perArt int
|
||||
limit int
|
||||
}{
|
||||
{"one artist owns the entire pool", 1, 30, 10},
|
||||
{"two artists, tight cap", 2, 15, 20},
|
||||
{"pool smaller than the request", 3, 2, 50},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
var cs []Candidate
|
||||
n := 0
|
||||
for a := 0; a < tc.artists; a++ {
|
||||
for k := 0; k < tc.perArt; k++ {
|
||||
cs = append(cs, candBy(t, fmt.Sprintf("%012d", n),
|
||||
fmt.Sprintf("%012d", 300+a), fmt.Sprintf("%012d", n),
|
||||
ScoringInputs{}))
|
||||
n++
|
||||
}
|
||||
}
|
||||
want := tc.limit
|
||||
if len(cs) < want {
|
||||
want = len(cs)
|
||||
}
|
||||
picks := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), tc.limit,
|
||||
DiversityCaps{MaxPerArtist: 3, MaxPerAlbum: 2})
|
||||
if len(picks) != want {
|
||||
t.Errorf("len = %d, want %d — the caps shortened the result instead "+
|
||||
"of relaxing to fill it", len(picks), want)
|
||||
}
|
||||
// No duplicates: a candidate deferred in pass one must not also be
|
||||
// taken in pass two.
|
||||
seen := map[[16]byte]bool{}
|
||||
for _, p := range picks {
|
||||
if seen[p.Track.ID.Bytes] {
|
||||
t.Fatalf("track %x appears twice; pass two re-added a pick", p.Track.ID.Bytes)
|
||||
}
|
||||
seen[p.Track.ID.Bytes] = true
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// A fixed count cannot serve both a 20-track radio and a 200-track one: three
|
||||
// per artist is 12% of the first and 1.5% of the second, which would leave the
|
||||
// long radio permanently in the relaxation path and effectively uncapped.
|
||||
func TestRadioDiversityCaps_ScaleWithTheRequestedLength(t *testing.T) {
|
||||
short := RadioDiversityCaps(25)
|
||||
long := RadioDiversityCaps(200)
|
||||
|
||||
if short.MaxPerArtist != 3 {
|
||||
t.Errorf("a 25-track radio caps artists at %d, want 3 — the same "+
|
||||
"proportion the system mixes use", short.MaxPerArtist)
|
||||
}
|
||||
if long.MaxPerArtist <= short.MaxPerArtist {
|
||||
t.Errorf("a 200-track radio caps artists at %d, no higher than a 25-track "+
|
||||
"one at %d; the cap is not scaling", long.MaxPerArtist, short.MaxPerArtist)
|
||||
}
|
||||
// Proportion held, not just "bigger".
|
||||
if long.MaxPerArtist != 24 {
|
||||
t.Errorf("200-track artist cap = %d, want 24 (3 per 25)", long.MaxPerArtist)
|
||||
}
|
||||
|
||||
// Floors: a tiny radio must not be capped down to one track per artist.
|
||||
tiny := RadioDiversityCaps(1)
|
||||
if tiny.MaxPerArtist < 2 {
|
||||
t.Errorf("tiny radio artist cap = %d, want at least 2", tiny.MaxPerArtist)
|
||||
}
|
||||
if tiny.MaxPerAlbum < 1 {
|
||||
t.Errorf("tiny radio album cap = %d, want at least 1", tiny.MaxPerAlbum)
|
||||
}
|
||||
}
|
||||
@@ -21,7 +21,7 @@ func TestShuffle_LikedRanksAboveUnliked(t *testing.T) {
|
||||
cand("000000000001", ScoringInputs{IsGeneralLiked: false}),
|
||||
cand("000000000002", ScoringInputs{IsGeneralLiked: true}),
|
||||
}
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10)
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10, DiversityCaps{})
|
||||
if out[0].Track.Title != "000000000002" {
|
||||
t.Errorf("liked track did not rank first: %+v", out)
|
||||
}
|
||||
@@ -33,7 +33,7 @@ func TestShuffle_HighSkipRanksLast(t *testing.T) {
|
||||
cand("000000000002", ScoringInputs{PlayCount: 10, SkipCount: 0}), // ratio 0
|
||||
cand("000000000003", ScoringInputs{PlayCount: 10, SkipCount: 5}), // ratio 0.5
|
||||
}
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10)
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10, DiversityCaps{})
|
||||
if out[0].Track.Title != "000000000002" || out[2].Track.Title != "000000000001" {
|
||||
t.Errorf("skip-ratio ordering broken: %v", titles(out))
|
||||
}
|
||||
@@ -44,7 +44,7 @@ func TestShuffle_LimitTruncates(t *testing.T) {
|
||||
for i := range cs {
|
||||
cs[i] = cand("00000000000"+string(rune('a'+i%26)), ScoringInputs{})
|
||||
}
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10)
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10, DiversityCaps{})
|
||||
if len(out) != 10 {
|
||||
t.Errorf("len = %d, want 10", len(out))
|
||||
}
|
||||
@@ -58,7 +58,7 @@ func TestShuffle_JitterDoesNotFlipStructuralWinner(t *testing.T) {
|
||||
cand("000000000001", ScoringInputs{IsGeneralLiked: false}),
|
||||
cand("000000000002", ScoringInputs{IsGeneralLiked: true}),
|
||||
}
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), r.Float64, 10)
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), r.Float64, 10, DiversityCaps{})
|
||||
if out[0].Track.Title != "000000000002" {
|
||||
t.Fatalf("iter %d: liked did not rank first; out=%v", i, titles(out))
|
||||
}
|
||||
@@ -66,7 +66,7 @@ func TestShuffle_JitterDoesNotFlipStructuralWinner(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestShuffle_Empty_ReturnsEmpty(t *testing.T) {
|
||||
out := Shuffle(nil, defaultWeights(), time.Now(), fixedRNG(0.5), 10)
|
||||
out := Shuffle(nil, defaultWeights(), time.Now(), fixedRNG(0.5), 10, DiversityCaps{})
|
||||
if len(out) != 0 {
|
||||
t.Errorf("len = %d, want 0", len(out))
|
||||
}
|
||||
|
||||
@@ -0,0 +1,99 @@
|
||||
package recommendation
|
||||
|
||||
import "testing"
|
||||
|
||||
// Songs-like's pool must lean on arms that MEASURE distance from the seed.
|
||||
// The default gave ~29% of candidates a sim_score of literally 0
|
||||
// (taste_overlap and random_fill are both `0.0::float8` in
|
||||
// recommendation.sql), which is what let "Songs like X" wander.
|
||||
func TestSongsLikeLimits_FavourTheArmsThatMeasureTheSeed(t *testing.T) {
|
||||
d := DefaultCandidateSourceLimits()
|
||||
s := SongsLikeCandidateSourceLimits()
|
||||
|
||||
if s.LBSimilar <= d.LBSimilar {
|
||||
t.Errorf("LBSimilar %d is not above the default %d — the only arm that "+
|
||||
"measures track-level distance from the seed should be favoured here",
|
||||
s.LBSimilar, d.LBSimilar)
|
||||
}
|
||||
// The two seed-INDEPENDENT arms, which is the whole complaint.
|
||||
zeroSimDefault := d.TasteOverlap + d.RandomFill
|
||||
zeroSimSongsLike := s.TasteOverlap + s.RandomFill
|
||||
if zeroSimSongsLike >= zeroSimDefault {
|
||||
t.Errorf("seed-independent arms total %d, not reduced from the default %d; "+
|
||||
"these carry sim_score 0 by construction", zeroSimSongsLike, zeroSimDefault)
|
||||
}
|
||||
}
|
||||
|
||||
// RULE 131: a system playlist degrades, it never vanishes.
|
||||
//
|
||||
// Zeroing the seed-independent arms was the first instinct and is exactly the
|
||||
// vanish-or-nothing shape that rule forbids: a seed whose artist has thin
|
||||
// ListenBrainz coverage would yield a short mix or none at all. They are the
|
||||
// tier-3 FLOOR — reduced hard, never removed — and the songs_like weights are
|
||||
// what keep them at the bottom of the ranking rather than out of the pool.
|
||||
//
|
||||
// "A few tracks further from the seed than we would like" beats "no playlist".
|
||||
func TestSongsLikeLimits_KeepATierThreeFloor(t *testing.T) {
|
||||
s := SongsLikeCandidateSourceLimits()
|
||||
if s.RandomFill <= 0 {
|
||||
t.Error("RandomFill is zero: a seed with thin similarity coverage now produces " +
|
||||
"a short or empty mix instead of degrading (rule 131)")
|
||||
}
|
||||
if s.TasteOverlap <= 0 {
|
||||
t.Error("TasteOverlap is zero: the graded floor is gone, leaving only random " +
|
||||
"fill between a sparse seed and an empty playlist (rule 131)")
|
||||
}
|
||||
}
|
||||
|
||||
// The pool should stay roughly the size it was — this change is about
|
||||
// COMPOSITION, not about starving the surface. A much smaller pool would also
|
||||
// shrink what the per-artist cap has to work with.
|
||||
func TestSongsLikeLimits_KeepThePoolRoughlyTheSameSize(t *testing.T) {
|
||||
total := func(l CandidateSourceLimits) int {
|
||||
return l.LBSimilar + l.SimilarArtist + l.TagOverlap + l.LikesOverlap +
|
||||
l.RandomFill + l.TasteOverlap + l.UserCoplay
|
||||
}
|
||||
d, s := total(DefaultCandidateSourceLimits()), total(SongsLikeCandidateSourceLimits())
|
||||
if s < d/2 {
|
||||
t.Errorf("songs_like pool is %d against the default %d — less than half; "+
|
||||
"this was meant to re-weight the pool, not starve it", s, d)
|
||||
}
|
||||
}
|
||||
|
||||
// The constraint that is invisible in the numbers, and that this file exists
|
||||
// to keep visible.
|
||||
//
|
||||
// `likes_overlap` and `random_fill` end in a bare `ORDER BY random()` with no
|
||||
// daily seed (recommendation.sql:118, :161). Such an arm returns a stable set
|
||||
// only while its LIMIT exceeds the eligible rows; below that it returns a
|
||||
// random SUBSET that differs between two builds on the same day, and the
|
||||
// daily-determinism promise quietly stops holding.
|
||||
//
|
||||
// This is not hypothetical — it is how this change first failed CI. Cutting
|
||||
// RandomFill to 10 broke TestBuildSystemPlaylists_DailyNonceDeterminism,
|
||||
// whose library is smaller than the default limit and whose determinism was
|
||||
// therefore an accident of the limit exceeding the library.
|
||||
//
|
||||
// Growing these arms is always safe. Only shrinking is, and the fix that
|
||||
// would make shrinking safe is a seeded ordering (#3889), not a smaller
|
||||
// number here.
|
||||
func TestSongsLikeLimits_DoNotShrinkTheUnseededRandomArms(t *testing.T) {
|
||||
d := DefaultCandidateSourceLimits()
|
||||
s := SongsLikeCandidateSourceLimits()
|
||||
|
||||
for _, tc := range []struct {
|
||||
arm string
|
||||
songsLike, dflt int
|
||||
}{
|
||||
{"RandomFill", s.RandomFill, d.RandomFill},
|
||||
{"LikesOverlap", s.LikesOverlap, d.LikesOverlap},
|
||||
} {
|
||||
if tc.songsLike < tc.dflt {
|
||||
t.Errorf("%s cut from %d to %d. That arm is ordered by unseeded random(), "+
|
||||
"so a smaller limit makes pool membership vary between same-day "+
|
||||
"rebuilds — it breaks daily determinism rather than merely narrowing "+
|
||||
"the mix. Fix the ordering (#3889) before trimming this.",
|
||||
tc.arm, tc.dflt, tc.songsLike)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -40,6 +40,13 @@ const (
|
||||
// never be read as taste signal (#2374) — filing it under taste would put
|
||||
// it one careless join from the leak that design forbids.
|
||||
ScopeDiscover = "discover"
|
||||
// ScopeSongsLike is the "Songs like {X}" surface (#3881). It shared
|
||||
// daily_mix with For-You until 2026-09-10, and that sharing WAS the bug:
|
||||
// the two surfaces want opposite things. For-You answers "what will they
|
||||
// enjoy today" and is supposed to roam; Songs-like answers "what sounds
|
||||
// like THIS" and is the tightest surface in the product. One set of
|
||||
// weights cannot serve both, and the broad answer was winning.
|
||||
ScopeSongsLike = "songs_like"
|
||||
)
|
||||
|
||||
// TasteTuning is the tunable subset of taste.Config: the engagement
|
||||
@@ -92,6 +99,76 @@ func ShippedDailyMixWeights() recommendation.ScoringWeights {
|
||||
}
|
||||
}
|
||||
|
||||
// ShippedSongsLikeWeights are the shipped songs_like-profile defaults
|
||||
// (#3881). The whole point is that SIMILARITY DOMINATES; every other
|
||||
// profile balances it against taste and engagement, and this one must not.
|
||||
//
|
||||
// The failure being corrected, arithmetic from the daily_mix profile that
|
||||
// this surface used to share:
|
||||
//
|
||||
// unrelated track, liked, not played recently → 1.0 + 2.0 + 1.0 = 4.0
|
||||
// PERFECT similarity match, not liked → 1.0 + 1.5 = 2.5
|
||||
//
|
||||
// Liking something outranked sounding like the seed, because LikeBoost (2.0)
|
||||
// exceeded SimilarityWeight's entire range (1.5) and TasteWeight (1.5, and
|
||||
// seed-independent) matched it outright.
|
||||
//
|
||||
// THE PROPERTY THESE NUMBERS ENCODE, which is what to preserve if they are
|
||||
// retuned: the similarity term's range must exceed the combined range of
|
||||
// every seed-INDEPENDENT differentiator, so that a closer match cannot be
|
||||
// beaten on the strength of likes, freshness and taste alone.
|
||||
//
|
||||
// seed-independent spread = LikeBoost 0.5 + Recency 0.25
|
||||
// + Taste 0.25 + ContextTime 0.5
|
||||
// + jitter 0.05 = 1.55
|
||||
// similarity spread = 0 → 4.0
|
||||
//
|
||||
// So a similarity advantage of ~0.39 (1.55/4.0) wins outright regardless of
|
||||
// everything else, while tracks within that band still get ordered by what
|
||||
// the user likes and has not heard lately. Tight, not deaf.
|
||||
//
|
||||
// BaseWeight stays 1.0: it is identical for every candidate and so
|
||||
// differentiates nothing — it sets the floor, not the shape. SkipPenalty
|
||||
// stays 2.0 because a track the user skips is still unwanted no matter how
|
||||
// similar it is.
|
||||
//
|
||||
// These are DEFAULTS, not settings (rule 25) — the operator turns them in the
|
||||
// admin tuning card and good values get baked back here. They are a
|
||||
// defensible starting point rather than a measured optimum: the per-arm fill
|
||||
// rates and the real sim_score distribution are still unknown (#3879), and
|
||||
// `likes_overlap` contributes a flat 0.6 that a high SimilarityWeight
|
||||
// amplifies. Expect to move these once that lands.
|
||||
func ShippedSongsLikeWeights() recommendation.ScoringWeights {
|
||||
return recommendation.ScoringWeights{
|
||||
BaseWeight: 1.0, // same for all candidates; differentiates nothing
|
||||
LikeBoost: 0.5, // was 2.0 — a tie-break among similar tracks, not an override
|
||||
RecencyWeight: 0.25, // was 1.0 — freshness must not outrank sounding right
|
||||
SkipPenalty: 2.0, // unchanged — a skipped track stays unwanted
|
||||
JitterMagnitude: 0.05, // was 0.1 — less shuffle on a coherence surface
|
||||
ContextWeight: 0.5,
|
||||
SimilarityWeight: 4.0, // was 1.5 — dominant, by design
|
||||
TasteWeight: 0.25, // was 1.5 — seed-INDEPENDENT, so demoted hard
|
||||
ContextTimeWeight: 0.5, // was 1.0
|
||||
}
|
||||
}
|
||||
|
||||
// shippedWeightsFor returns the shipped defaults for a weight-profile scope,
|
||||
// or ok=false if the scope is not a weight profile. Single source for the
|
||||
// three call sites (seed, update-validation, reset) so adding a fourth
|
||||
// profile cannot be half-wired — which is how a scope ends up seedable but
|
||||
// not resettable.
|
||||
func shippedWeightsFor(scope string) (recommendation.ScoringWeights, bool) {
|
||||
switch scope {
|
||||
case ScopeRadio:
|
||||
return ShippedRadioWeights(), true
|
||||
case ScopeDailyMix:
|
||||
return ShippedDailyMixWeights(), true
|
||||
case ScopeSongsLike:
|
||||
return ShippedSongsLikeWeights(), true
|
||||
}
|
||||
return recommendation.ScoringWeights{}, false
|
||||
}
|
||||
|
||||
// DiscoverTuning is the tunable set for the Discover request surface (#2377).
|
||||
type DiscoverTuning struct {
|
||||
// TagOverlapWeight scales the taste-tag term: score × (1 + w × overlap).
|
||||
@@ -165,8 +242,9 @@ func New(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger) (*Service
|
||||
func (s *Service) reconcile(ctx context.Context) error {
|
||||
q := dbq.New(s.pool)
|
||||
for profile, w := range map[string]recommendation.ScoringWeights{
|
||||
ScopeRadio: ShippedRadioWeights(),
|
||||
ScopeDailyMix: ShippedDailyMixWeights(),
|
||||
ScopeRadio: ShippedRadioWeights(),
|
||||
ScopeDailyMix: ShippedDailyMixWeights(),
|
||||
ScopeSongsLike: ShippedSongsLikeWeights(),
|
||||
} {
|
||||
if err := q.UpsertWeightProfileDefaults(ctx, upsertParams(profile, w)); err != nil {
|
||||
return fmt.Errorf("seed profile %q: %w", profile, err)
|
||||
@@ -234,6 +312,7 @@ func (s *Service) reconcile(ctx context.Context) error {
|
||||
// reads Weights(ScopeRadio) per request.
|
||||
func (s *Service) push() {
|
||||
playlists.SetSystemMixWeights(s.Weights(ScopeDailyMix))
|
||||
playlists.SetSongsLikeWeights(s.Weights(ScopeSongsLike))
|
||||
playlists.SetTasteConfig(s.TasteConfig())
|
||||
}
|
||||
|
||||
@@ -297,7 +376,7 @@ type fieldChange struct {
|
||||
// Unknown fields and out-of-range values reject the whole patch. A
|
||||
// no-op patch (all values equal to current) writes no audit row.
|
||||
func (s *Service) UpdateProfile(ctx context.Context, profile string, patch map[string]float64) error {
|
||||
if profile != ScopeRadio && profile != ScopeDailyMix {
|
||||
if _, ok := shippedWeightsFor(profile); !ok {
|
||||
return fmt.Errorf("%w: %q", ErrUnknownScope, profile)
|
||||
}
|
||||
current := s.Weights(profile)
|
||||
@@ -340,17 +419,18 @@ func (s *Service) UpdateDiscover(ctx context.Context, patch map[string]float64)
|
||||
// Reset restores a scope to its shipped defaults, with one audit row
|
||||
// carrying the full diff. A scope already at defaults is a no-op.
|
||||
func (s *Service) Reset(ctx context.Context, scope string) error {
|
||||
switch scope {
|
||||
case ScopeRadio, ScopeDailyMix:
|
||||
shipped := ShippedRadioWeights()
|
||||
if scope == ScopeDailyMix {
|
||||
shipped = ShippedDailyMixWeights()
|
||||
}
|
||||
if shipped, ok := shippedWeightsFor(scope); ok {
|
||||
// Every weight profile resets the same way; the per-scope defaults
|
||||
// come from one place so a new profile cannot be seedable but not
|
||||
// resettable. This was an if/else over two hard-coded scopes until
|
||||
// songs_like made it three.
|
||||
changes := diffWeights(s.Weights(scope), shipped)
|
||||
if len(changes) == 0 {
|
||||
return nil
|
||||
}
|
||||
return s.persistProfile(ctx, scope, shipped, "reset", changes)
|
||||
}
|
||||
switch scope {
|
||||
case ScopeTaste:
|
||||
shipped := ShippedTasteTuning()
|
||||
changes := diffTaste(s.Taste(), shipped)
|
||||
|
||||
@@ -0,0 +1,130 @@
|
||||
package recsettings
|
||||
|
||||
import (
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/recommendation"
|
||||
)
|
||||
|
||||
// The bug, reproduced as a ranking: a track that sounds nothing like the seed
|
||||
// but which the user liked and has not played lately used to OUTRANK a perfect
|
||||
// similarity match. Operator, 2026-09-10: "I was getting a seeming wide variety
|
||||
// of music from each one when I was hoping to stay in a certain neighborhood."
|
||||
//
|
||||
// This is the whole point of the songs_like profile, so it is asserted as
|
||||
// BEHAVIOUR — two candidates, which one wins — rather than by checking the
|
||||
// weight numbers. Numbers get retuned; this property must survive that.
|
||||
//
|
||||
// It also pins the contrast: daily_mix is EXPECTED to fail this. If both
|
||||
// profiles started ranking the same way, the split would have quietly become
|
||||
// pointless and nothing else would notice.
|
||||
func TestSongsLikeWeights_SimilarityBeatsAnUnrelatedLikedTrack(t *testing.T) {
|
||||
now := time.Now().UTC()
|
||||
stale := now.Add(-365 * 24 * time.Hour)
|
||||
|
||||
// A perfect similarity match the user has never liked and played recently.
|
||||
// Everything except similarity is working against it.
|
||||
perfectMatch := recommendation.ScoringInputs{
|
||||
SimilarityScore: 1.0,
|
||||
IsGeneralLiked: false,
|
||||
LastPlayedAt: &now,
|
||||
}
|
||||
// Nothing to do with the seed, but liked and long unplayed — every
|
||||
// seed-independent term in its favour.
|
||||
unrelatedFavourite := recommendation.ScoringInputs{
|
||||
SimilarityScore: 0.0,
|
||||
IsGeneralLiked: true,
|
||||
LastPlayedAt: &stale,
|
||||
TasteMatchScore: 1.0,
|
||||
}
|
||||
|
||||
// Jitter fixed at its midpoint so the comparison is about the weights.
|
||||
noJitter := func() float64 { return 0.5 }
|
||||
|
||||
songsLike := ShippedSongsLikeWeights()
|
||||
matchScore := recommendation.Score(perfectMatch, songsLike, now, noJitter)
|
||||
favScore := recommendation.Score(unrelatedFavourite, songsLike, now, noJitter)
|
||||
if matchScore <= favScore {
|
||||
t.Errorf("songs_like ranks an unrelated liked track (%.3f) at or above a "+
|
||||
"perfect similarity match (%.3f) — the mix will wander", favScore, matchScore)
|
||||
}
|
||||
|
||||
// The contrast that makes the split worth having. If this ever passes,
|
||||
// daily_mix has been tightened into songs_like and one of them is redundant.
|
||||
daily := ShippedDailyMixWeights()
|
||||
dMatch := recommendation.Score(perfectMatch, daily, now, noJitter)
|
||||
dFav := recommendation.Score(unrelatedFavourite, daily, now, noJitter)
|
||||
if dMatch > dFav {
|
||||
t.Errorf("daily_mix now also puts similarity first (%.3f vs %.3f); the two "+
|
||||
"profiles no longer differ, so songs_like is buying nothing", dMatch, dFav)
|
||||
}
|
||||
}
|
||||
|
||||
// The property the songs_like numbers encode, stated independently of them:
|
||||
// the similarity term's range must exceed the combined range of every
|
||||
// seed-INDEPENDENT differentiator, so a closer match cannot be beaten on
|
||||
// likes, freshness and taste alone.
|
||||
//
|
||||
// BaseWeight is excluded deliberately — it is identical for every candidate
|
||||
// and so differentiates nothing. SkipPenalty is excluded because it only ever
|
||||
// pushes a candidate DOWN, and a skipped track should lose however similar.
|
||||
func TestSongsLikeWeights_SimilarityOutrangesEverySeedIndependentTerm(t *testing.T) {
|
||||
w := ShippedSongsLikeWeights()
|
||||
|
||||
// TasteMatchScore and ContextAffinityScore are in [-1,+1]; recencyDecay is
|
||||
// in [0,1]; LikeBoost is all-or-nothing.
|
||||
seedIndependent := w.LikeBoost + w.RecencyWeight + w.TasteWeight +
|
||||
w.ContextTimeWeight + w.JitterMagnitude
|
||||
|
||||
if w.SimilarityWeight <= seedIndependent {
|
||||
t.Errorf("SimilarityWeight %.2f does not outrange the seed-independent "+
|
||||
"terms (%.2f) — likes/recency/taste can outvote sounding like the seed",
|
||||
w.SimilarityWeight, seedIndependent)
|
||||
}
|
||||
}
|
||||
|
||||
// A scope that is seedable but not resettable is the half-wired shape this
|
||||
// guards: the profile appears in the admin card, the operator turns a knob,
|
||||
// and Reset then 404s on a scope the rest of the service knows about.
|
||||
func TestShippedWeightsFor_CoversEveryWeightProfile(t *testing.T) {
|
||||
for _, scope := range []string{ScopeRadio, ScopeDailyMix, ScopeSongsLike} {
|
||||
if _, ok := shippedWeightsFor(scope); !ok {
|
||||
t.Errorf("scope %q has no shipped defaults; it cannot be seeded or reset", scope)
|
||||
}
|
||||
}
|
||||
// Non-weight scopes must NOT resolve here, or Reset would treat the taste
|
||||
// singleton as a weight profile and write nonsense.
|
||||
for _, scope := range []string{ScopeTaste, ScopeDiscover, "nonsense"} {
|
||||
if _, ok := shippedWeightsFor(scope); ok {
|
||||
t.Errorf("scope %q resolved as a weight profile and is not one", scope)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Guards the sync the comment in playlists/system.go asks for: the pre-push
|
||||
// literal there must match the shipped defaults here, or a build that has not
|
||||
// yet been reconciled ranks differently from one that has.
|
||||
func TestShippedSongsLikeWeights_AreDominatedBySimilarity(t *testing.T) {
|
||||
w := ShippedSongsLikeWeights()
|
||||
daily := ShippedDailyMixWeights()
|
||||
|
||||
if w.SimilarityWeight <= daily.SimilarityWeight {
|
||||
t.Errorf("songs_like SimilarityWeight %.2f is not above daily_mix's %.2f",
|
||||
w.SimilarityWeight, daily.SimilarityWeight)
|
||||
}
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
songs, day float64
|
||||
}{
|
||||
{"LikeBoost", w.LikeBoost, daily.LikeBoost},
|
||||
{"TasteWeight", w.TasteWeight, daily.TasteWeight},
|
||||
{"RecencyWeight", w.RecencyWeight, daily.RecencyWeight},
|
||||
} {
|
||||
if tc.songs >= tc.day {
|
||||
t.Errorf("songs_like %s (%.2f) is not demoted below daily_mix (%.2f); "+
|
||||
"these are the seed-independent terms that made the mix wander",
|
||||
tc.name, tc.songs, tc.day)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,718 @@
|
||||
package server
|
||||
|
||||
import (
|
||||
"os"
|
||||
"os/exec"
|
||||
"path/filepath"
|
||||
"regexp"
|
||||
"strconv"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// Guards the version derivation that stamps every build.
|
||||
//
|
||||
// These assertions used to be impossible to run. The derivation lived inline
|
||||
// in release.yml, which triggers only on `main` and on tags — so a mistake in
|
||||
// it could not surface until a release was already under way, and its failure
|
||||
// mode is silence: a version nobody can compare looks exactly like being up to
|
||||
// date, and nobody reports an update they were never offered.
|
||||
//
|
||||
// Moving it to ci/version.sh made it executable, so this runs on every push
|
||||
// that touches the release machinery. That is the whole point of the file; the
|
||||
// specific assertions below matter less than the fact that they run at all.
|
||||
//
|
||||
// The tests EXECUTE the script rather than asserting on its text, so they
|
||||
// break when the behaviour changes rather than when the wording does.
|
||||
|
||||
// highestOldSchemeCode is the largest versionCode ever shipped under the
|
||||
// retired commit-count scheme (v2026.09.09 shipped 1895). Every code the new
|
||||
// scheme emits must clear it, or Android would refuse the upgrade as a
|
||||
// downgrade and the update channel would be a one-way door.
|
||||
const highestOldSchemeCode = 1895
|
||||
|
||||
func repoRoot(t *testing.T) string {
|
||||
t.Helper()
|
||||
dir, err := os.Getwd()
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
for {
|
||||
if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil {
|
||||
return dir
|
||||
}
|
||||
parent := filepath.Dir(dir)
|
||||
if parent == dir {
|
||||
t.Fatalf("no go.mod above %s", dir)
|
||||
}
|
||||
dir = parent
|
||||
}
|
||||
}
|
||||
|
||||
// runVersion executes ci/version.sh with both clocks pinned, so the result is
|
||||
// deterministic. Returns the parsed KEY=VALUE output.
|
||||
func runVersion(t *testing.T, commitEpoch, nowEpoch string) map[string]string {
|
||||
t.Helper()
|
||||
out, err := versionScript(t, commitEpoch, nowEpoch)
|
||||
if err != nil {
|
||||
t.Fatalf("ci/version.sh failed: %v\n%s", err, out)
|
||||
}
|
||||
parsed := map[string]string{}
|
||||
for _, line := range strings.Split(strings.TrimSpace(out), "\n") {
|
||||
if k, v, ok := strings.Cut(line, "="); ok {
|
||||
parsed[k] = v
|
||||
}
|
||||
}
|
||||
return parsed
|
||||
}
|
||||
|
||||
func versionScript(t *testing.T, commitEpoch, nowEpoch string) (string, error) {
|
||||
t.Helper()
|
||||
root := repoRoot(t)
|
||||
cmd := exec.Command(filepath.Join(root, "ci", "version.sh"))
|
||||
cmd.Dir = root
|
||||
cmd.Env = append(os.Environ(),
|
||||
"MINSTREL_COMMIT_EPOCH="+commitEpoch,
|
||||
"MINSTREL_NOW_EPOCH="+nowEpoch,
|
||||
)
|
||||
out, err := cmd.CombinedOutput()
|
||||
return string(out), err
|
||||
}
|
||||
|
||||
func TestVersionName_IsCommitTimeToTheMinute(t *testing.T) {
|
||||
// 2025-09-09T18:48:56Z
|
||||
got := runVersion(t, "1757443736", "1789000920")
|
||||
if want := "2025.09.09.1848"; got["name"] != want {
|
||||
t.Errorf("name = %q, want %q", got["name"], want)
|
||||
}
|
||||
}
|
||||
|
||||
// HHMM is the segment most likely to be silently mangled, and it only bites
|
||||
// for about a tenth of the day — a build just after midnight must emit "0042",
|
||||
// never "42". A stripped leading zero shifts the segment by two orders of
|
||||
// magnitude and reverses comparisons against every other build that day.
|
||||
func TestVersionName_PadsTheMinuteSegment(t *testing.T) {
|
||||
// 2026-09-10T00:42:00Z
|
||||
got := runVersion(t, "1789000920", "1789000920")
|
||||
if want := "2026.09.10.0042"; got["name"] != want {
|
||||
t.Errorf("name = %q, want %q — leading zero lost?", got["name"], want)
|
||||
}
|
||||
}
|
||||
|
||||
func TestVersionName_DerivesFromCommitNotBuildClock(t *testing.T) {
|
||||
// Same commit, two different build clocks: the NAME must not move, or a
|
||||
// dev build and a main build of one commit would report different strings
|
||||
// and the channel field would stop being the only thing separating them.
|
||||
a := runVersion(t, "1757443736", "1789000920")
|
||||
b := runVersion(t, "1757443736", "1789500000")
|
||||
if a["name"] != b["name"] {
|
||||
t.Errorf("name moved with the build clock: %q vs %q", a["name"], b["name"])
|
||||
}
|
||||
if a["code"] == b["code"] {
|
||||
t.Errorf("code did NOT move with the build clock (%q) — it is not build-derived", a["code"])
|
||||
}
|
||||
}
|
||||
|
||||
func TestVersionCode_IsMinutesSince2020AndClearsTheOldScheme(t *testing.T) {
|
||||
got := runVersion(t, "1789000920", "1789000920")
|
||||
code, err := strconv.Atoi(got["code"])
|
||||
if err != nil {
|
||||
t.Fatalf("code %q is not an integer: %v", got["code"], err)
|
||||
}
|
||||
if want := (1789000920 - 1577836800) / 60; code != want {
|
||||
t.Errorf("code = %d, want %d", code, want)
|
||||
}
|
||||
if code <= highestOldSchemeCode {
|
||||
t.Errorf("code %d does not clear the retired commit-count scheme (%d) — "+
|
||||
"Android would refuse the upgrade as a downgrade", code, highestOldSchemeCode)
|
||||
}
|
||||
if int64(code) > 2147483647 {
|
||||
t.Errorf("code %d overflows versionCode's int32 ceiling", code)
|
||||
}
|
||||
}
|
||||
|
||||
// The tag is not chosen, it is the name with a `v`. Anything else reintroduces
|
||||
// the mismatch between what a tag claims and what the artifact reports.
|
||||
func TestTag_IsTheNameWithAPrefix(t *testing.T) {
|
||||
got := runVersion(t, "1757443736", "1789000920")
|
||||
if want := "v" + got["name"]; got["tag"] != want {
|
||||
t.Errorf("tag = %q, want %q", got["tag"], want)
|
||||
}
|
||||
}
|
||||
|
||||
// A guard that cannot fail is worse than no guard. These prove the script
|
||||
// rejects the shapes it claims to reject, rather than emitting something
|
||||
// plausible and letting it ship.
|
||||
func TestVersionScript_RejectsUnusableClocks(t *testing.T) {
|
||||
for _, tc := range []struct{ name, commit, now string }{
|
||||
{"unreadable commit timestamp", "notanumber", "1789000920"},
|
||||
{"non-numeric build clock", "1789000920", "abc"},
|
||||
{"build clock before 2020", "1789000920", "1000000000"},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
out, err := versionScript(t, tc.commit, tc.now)
|
||||
if err == nil {
|
||||
t.Errorf("script succeeded on %s, output: %s", tc.name, out)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// Pins the wiring, not the formula: if release.yml stops calling the script,
|
||||
// every assertion above keeps passing while the thing that actually ships goes
|
||||
// unguarded again. That silent decoupling is the specific regression here.
|
||||
func TestReleaseWorkflow_UsesTheSharedDerivation(t *testing.T) {
|
||||
body, err := os.ReadFile(filepath.Join(repoRoot(t), ".gitea", "workflows", "release.yml"))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
yaml := string(body)
|
||||
|
||||
if !strings.Contains(yaml, "ci/version.sh") {
|
||||
t.Error("release.yml no longer calls ci/version.sh — the derivation has drifted out of test coverage")
|
||||
}
|
||||
// The retired scheme, which must not come back. A presence check is safe
|
||||
// against prose; the historical note in that file names the old formula
|
||||
// only in comments, so match the executable form.
|
||||
if strings.Contains(yaml, "$(git rev-list --count") {
|
||||
t.Error("release.yml derives a commit count again — that is not monotonic across branches")
|
||||
}
|
||||
}
|
||||
|
||||
// jobBoundary matches a blank line followed by job-level (two-space)
|
||||
// indentation — the end of the last step in a job.
|
||||
var jobBoundary = regexp.MustCompile(`\n\n [^ \n]`)
|
||||
|
||||
// stepBody returns one workflow step's text, from its `- name:` line to the
|
||||
// start of the next step or the next job.
|
||||
//
|
||||
// The naive cut — "up to the next `- name:`" — silently returns an EMPTY body
|
||||
// for the last step in a job, and every assertion over it then passes
|
||||
// vacuously. That is the failure rule 167 names: a check that reads as
|
||||
// coverage while asserting on nothing. Cutting at a blank line followed by
|
||||
// job-level indentation handles the last-step case, which is exactly where
|
||||
// `Build and push` sits.
|
||||
func stepBody(t *testing.T, yaml, name string) string {
|
||||
t.Helper()
|
||||
i := strings.Index(yaml, "- name: "+name)
|
||||
if i < 0 {
|
||||
t.Fatalf("no %q step in release.yml", name)
|
||||
}
|
||||
body := yaml[i:]
|
||||
end := len(body)
|
||||
if j := strings.Index(body, "\n - name:"); j >= 0 {
|
||||
end = j
|
||||
}
|
||||
// A blank line followed by EXACTLY two spaces and then content starts a
|
||||
// new job or job-level comment. The "exactly" matters: a blank line inside
|
||||
// a `run:` block is followed by ten-space indentation and would otherwise
|
||||
// match, truncating the step mid-body — which is how this helper first
|
||||
// sliced the verify step down to its first two lines.
|
||||
if loc := jobBoundary.FindStringIndex(body); loc != nil && loc[0] < end {
|
||||
end = loc[0]
|
||||
}
|
||||
body = body[:end]
|
||||
if strings.TrimSpace(strings.TrimPrefix(body, "- name: "+name)) == "" {
|
||||
t.Fatalf("step %q sliced to an empty body — the assertions below would pass vacuously", name)
|
||||
}
|
||||
return body
|
||||
}
|
||||
|
||||
// releaseYAML reads the workflow once per test.
|
||||
func releaseYAML(t *testing.T) string {
|
||||
t.Helper()
|
||||
body, err := os.ReadFile(filepath.Join(repoRoot(t), ".gitea", "workflows", "release.yml"))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
return string(body)
|
||||
}
|
||||
|
||||
// imageTagArms splits "Compute image tags" into its three ref branches. The
|
||||
// whole tag policy lives in that if/elif/else, so the arms are the unit worth
|
||||
// asserting on — a tag published from the wrong arm reaches the wrong
|
||||
// audience, and every such mistake still builds and still pushes a valid
|
||||
// image.
|
||||
func imageTagArms(t *testing.T, yaml string) (tagArm, devArm, mainArm string) {
|
||||
t.Helper()
|
||||
const (
|
||||
tagMarker = `if [[ "${GITHUB_REF}" == refs/tags/v* ]]; then`
|
||||
devMarker = `elif [[ "${GITHUB_REF}" == "refs/heads/dev" ]]; then`
|
||||
mainMarker = "\n else"
|
||||
endMarker = "\n fi"
|
||||
)
|
||||
iTag := strings.Index(yaml, tagMarker)
|
||||
iDev := strings.Index(yaml, devMarker)
|
||||
iMain := -1
|
||||
if iDev >= 0 {
|
||||
if k := strings.Index(yaml[iDev:], mainMarker); k >= 0 {
|
||||
iMain = iDev + k
|
||||
}
|
||||
}
|
||||
if iTag < 0 || iDev < 0 || iMain < 0 {
|
||||
t.Fatal("Compute image tags no longer has a tag/dev/main arm — the tag policy has been restructured, so these guards are pinning nothing")
|
||||
}
|
||||
iEnd := iMain
|
||||
if k := strings.Index(yaml[iMain:], endMarker); k >= 0 {
|
||||
iEnd = iMain + k
|
||||
} else {
|
||||
t.Fatal("no closing fi after the main arm")
|
||||
}
|
||||
return yaml[iTag:iDev], yaml[iDev:iMain], yaml[iMain:iEnd]
|
||||
}
|
||||
|
||||
// The worst regression this wiring can produce: a dev push that also moves
|
||||
// :latest would ship untested code to every stable operator, silently, on the
|
||||
// next pull. Nothing else in the suite would notice — the build stays green
|
||||
// and the image is valid, it is simply the wrong audience.
|
||||
func TestDevChannel_PublishesDevAloneAndNeverLatest(t *testing.T) {
|
||||
_, arm, _ := imageTagArms(t, releaseYAML(t))
|
||||
|
||||
if !strings.Contains(arm, "${IMAGE}:dev") {
|
||||
t.Errorf("dev arm does not publish :dev\n%s", arm)
|
||||
}
|
||||
if strings.Contains(arm, ":latest") {
|
||||
t.Errorf("dev arm moves :latest — that ships dev code to every stable operator\n%s", arm)
|
||||
}
|
||||
// Rule 145: a rolling channel gets no commit-addressable tag.
|
||||
if strings.Contains(arm, "GITHUB_SHA") {
|
||||
t.Errorf("dev arm publishes a per-commit tag; a rolling channel should not\n%s", arm)
|
||||
}
|
||||
}
|
||||
|
||||
// The two bundling paths must stay mutually exclusive. If the rebundle step's
|
||||
// condition were relaxed back to "not a tag", a dev push would run BOTH: stage
|
||||
// its freshly-built APK, then overwrite it with the previous release's. The
|
||||
// image would still build and the sidecar would still parse — it would just
|
||||
// quietly serve stale art to the channel whose whole job is being current.
|
||||
func TestDevChannel_RebundlePathIsMainOnly(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]
|
||||
}
|
||||
if !strings.Contains(step, "github.ref == 'refs/heads/main'") {
|
||||
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)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// The rollback unit, and the reason it is worth a guard: it is invisible until
|
||||
// the moment it is needed. Nothing pulls :<sha> during normal operation, so if
|
||||
// this arm stopped minting one, every build would stay green and every image
|
||||
// would be valid — and the absence would surface only during an incident, as
|
||||
// "there is nothing to roll back to."
|
||||
//
|
||||
// Rules 145 and 147: main push → :latest + :<sha>.
|
||||
func TestMainChannel_PublishesLatestAndTheRollbackUnit(t *testing.T) {
|
||||
_, _, arm := imageTagArms(t, releaseYAML(t))
|
||||
|
||||
if !strings.Contains(arm, "${IMAGE}:latest") {
|
||||
t.Errorf("main arm does not move :latest — production would stop tracking main's tip\n%s", arm)
|
||||
}
|
||||
if !strings.Contains(arm, "${IMAGE}:${GITHUB_SHA}") {
|
||||
t.Errorf("main arm publishes no commit-addressable image; there is no rollback target for production commits\n%s", arm)
|
||||
}
|
||||
// Rule 147: :latest tracks main's tip, and a second name for the same
|
||||
// image sends readers looking for a distinction that does not exist.
|
||||
if strings.Contains(arm, "${IMAGE}:main") {
|
||||
t.Errorf("main arm publishes :main — rule 147 says that tag should not exist\n%s", arm)
|
||||
}
|
||||
}
|
||||
|
||||
// A release refreshes the CHANNEL and mints nothing else (rules 145 + 146).
|
||||
//
|
||||
// The specific regression: re-adding :<sha> here. The tag build rebuilds the
|
||||
// SAME SOURCE as main's build minutes earlier, differing only in which APK is
|
||||
// baked in — so a :<sha> minted here would overwrite main's immutable rollback
|
||||
// target with different contents, under the same name. That is the exact thing
|
||||
// rule 145's immutability clause exists to prevent, and it is the half with
|
||||
// the incidents behind it.
|
||||
func TestReleaseBuild_RefreshesTheChannelAndMintsNothingElse(t *testing.T) {
|
||||
arm, _, _ := imageTagArms(t, releaseYAML(t))
|
||||
|
||||
if !strings.Contains(arm, "${IMAGE}:latest") {
|
||||
t.Errorf("tag arm does not refresh :latest — the channel would keep serving the PREVIOUS release's APK until someone pushed to main\n%s", arm)
|
||||
}
|
||||
if strings.Contains(arm, "GITHUB_SHA") {
|
||||
t.Errorf("tag arm mints a :<sha> image; that would re-push main's immutable rollback target with different bundled contents\n%s", arm)
|
||||
}
|
||||
}
|
||||
|
||||
// No version-numbered image tags anywhere, on any arm (rule 145, and the
|
||||
// operator's 2026-09-10 decision to drop them across every project).
|
||||
//
|
||||
// Asserted across the whole step rather than per-arm because the mistake this
|
||||
// catches is re-adding one ANYWHERE, and the tag arm is only the likeliest
|
||||
// spot. `${VERSION}` still legitimately appears in the step as the build's
|
||||
// self-reported version, so the assertion has to name the image-tag form
|
||||
// specifically rather than the variable — otherwise it would fire on correct
|
||||
// code and get "fixed" by deleting the guard.
|
||||
func TestNoVersionNumberedImageTags(t *testing.T) {
|
||||
yaml := releaseYAML(t)
|
||||
tagArm, devArm, mainArm := imageTagArms(t, yaml)
|
||||
|
||||
for _, tc := range []struct{ name, arm string }{
|
||||
{"tag", tagArm}, {"dev", devArm}, {"main", mainArm},
|
||||
} {
|
||||
for _, forbidden := range []string{
|
||||
"${IMAGE}:${VERSION}",
|
||||
"${IMAGE}:v",
|
||||
"${IMAGE}:${GITHUB_REF#refs/tags/}",
|
||||
} {
|
||||
if strings.Contains(tc.arm, forbidden) {
|
||||
t.Errorf("%s arm publishes a version-numbered image tag (%q); git and the build's self-reported version answer \"which build is this\"\n%s",
|
||||
tc.name, forbidden, tc.arm)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// The verify job asserted the :<version> image existed. With version tags
|
||||
// gone that assertion would fail every release for a tag nothing mints — so
|
||||
// this pins that it was re-pointed rather than deleted, since deleting it is
|
||||
// the tempting way to make a failing guard go green.
|
||||
func TestVerifyJob_ChecksTheRollbackImageNotAVersionTag(t *testing.T) {
|
||||
yaml := releaseYAML(t)
|
||||
|
||||
if !strings.Contains(yaml, "- name: Rollback image must exist for the tagged commit") {
|
||||
t.Fatal("the release-verification step that checks an image exists is gone; an image push that silently did not happen would now pass verification")
|
||||
}
|
||||
step := stepBody(t, yaml, "Rollback image must exist for the tagged commit")
|
||||
if !strings.Contains(step, "${IMAGE}:${GITHUB_SHA}") {
|
||||
t.Errorf("the verify step does not inspect the commit's rollback image\n%s", step)
|
||||
}
|
||||
if strings.Contains(step, "${IMAGE}:${TAG}") {
|
||||
t.Errorf("the verify step still inspects a version-numbered image, which is no longer published — this would fail every release\n%s", step)
|
||||
}
|
||||
}
|
||||
|
||||
// The server's self-reported version is now the ONLY thing that identifies a
|
||||
// build, so a lane that stamps a channel word instead of a version silently
|
||||
// removes that ability. It used to stamp the literal "main"/"dev".
|
||||
func TestImageBuild_StampsADerivedVersionAndAChannel(t *testing.T) {
|
||||
yaml := releaseYAML(t)
|
||||
|
||||
step := stepBody(t, yaml, "Build and push")
|
||||
for _, want := range []string{
|
||||
"MINSTREL_VERSION=",
|
||||
"MINSTREL_CHANNEL=",
|
||||
} {
|
||||
if !strings.Contains(step, want) {
|
||||
t.Errorf("Build and push does not pass %s — the image cannot report which build it is\n%s", want, step)
|
||||
}
|
||||
}
|
||||
|
||||
// The version must come from the shared derivation, not from the ref.
|
||||
// Reading it off GITHUB_REF is what produced "main" and "dev" as version
|
||||
// strings, which is the regression this pins.
|
||||
_, _, mainArm := imageTagArms(t, yaml)
|
||||
if strings.Contains(mainArm, `version=main`) {
|
||||
t.Errorf("the main arm stamps the literal string \"main\" as a version; two images months apart would be indistinguishable\n%s", mainArm)
|
||||
}
|
||||
if !strings.Contains(yaml, "ci/version.sh HEAD | sed") {
|
||||
t.Error("the image job no longer derives its version from ci/version.sh — the version and the APK's version can now drift apart")
|
||||
}
|
||||
}
|
||||
|
||||
// gitRepo builds a throwaway repo and returns its path. Commit timestamps are
|
||||
// pinned so the derivation is deterministic.
|
||||
func gitRepo(t *testing.T) string {
|
||||
t.Helper()
|
||||
dir := t.TempDir()
|
||||
run := func(args ...string) {
|
||||
t.Helper()
|
||||
cmd := exec.Command("git", args...)
|
||||
cmd.Dir = dir
|
||||
cmd.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null")
|
||||
if out, err := cmd.CombinedOutput(); err != nil {
|
||||
t.Fatalf("git %v: %v\n%s", args, err, out)
|
||||
}
|
||||
}
|
||||
run("-c", "init.defaultBranch=main", "init", "-q")
|
||||
run("config", "user.email", "t@example.invalid")
|
||||
run("config", "user.name", "t")
|
||||
return dir
|
||||
}
|
||||
|
||||
// commitFile writes path and commits it with a pinned committer timestamp.
|
||||
func commitFile(t *testing.T, dir, path, epoch string) {
|
||||
t.Helper()
|
||||
full := filepath.Join(dir, path)
|
||||
if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
// Content must DIFFER from any earlier write to the same path, or git
|
||||
// records nothing and the commit silently covers fewer files than the
|
||||
// test believes. That is not hypothetical: it made the
|
||||
// source-alongside-test case pass vacuously.
|
||||
if err := os.WriteFile(full, []byte("content @"+epoch+"\n"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
for _, args := range [][]string{{"add", "-A"}, {"commit", "-q", "-m", path}} {
|
||||
cmd := exec.Command("git", args...)
|
||||
cmd.Dir = dir
|
||||
cmd.Env = append(os.Environ(),
|
||||
"GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null",
|
||||
"GIT_AUTHOR_DATE=@"+epoch+" +0000",
|
||||
"GIT_COMMITTER_DATE=@"+epoch+" +0000",
|
||||
)
|
||||
if out, err := cmd.CombinedOutput(); err != nil {
|
||||
t.Fatalf("git %v: %v\n%s", args, err, out)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// versionIn runs ci/version.sh inside dir, reading real git rather than the
|
||||
// pinned-clock override, so the PATHSPEC is what is under test.
|
||||
func versionIn(t *testing.T, dir string) (string, error) {
|
||||
t.Helper()
|
||||
cmd := exec.Command(filepath.Join(repoRoot(t), "ci", "version.sh"), "HEAD")
|
||||
cmd.Dir = dir
|
||||
cmd.Env = append(os.Environ(),
|
||||
"GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null",
|
||||
"MINSTREL_NOW_EPOCH=1789000920",
|
||||
)
|
||||
out, err := cmd.CombinedOutput()
|
||||
return string(out), err
|
||||
}
|
||||
|
||||
// The version names what SHIPPED, so a commit that changes nothing shippable
|
||||
// must not move it.
|
||||
//
|
||||
// The failure this prevents is not the cosmetic one. The derivation is a
|
||||
// denylist precisely so that new content counts by default: the direction that
|
||||
// matters is a changed artifact keeping its OLD version, silently, on a green
|
||||
// run. This test pins the cheap half of that (CI-only commits are inert) and,
|
||||
// in the same breath, that a source commit still moves it — because a pathspec
|
||||
// typo that excluded everything would satisfy the first assertion alone.
|
||||
func TestVersionName_IgnoresCommitsThatShipNothing(t *testing.T) {
|
||||
const (
|
||||
shipped = "1757443736" // 2025-09-09T18:48:56Z
|
||||
ciOnly = "1789000920" // 2026-09-10T00:42:00Z, later
|
||||
)
|
||||
dir := gitRepo(t)
|
||||
commitFile(t, dir, "internal/server/thing.go", shipped)
|
||||
commitFile(t, dir, ".gitea/workflows/release.yml", ciOnly)
|
||||
|
||||
out, err := versionIn(t, dir)
|
||||
if err != nil {
|
||||
t.Fatalf("version.sh failed: %v\n%s", err, out)
|
||||
}
|
||||
if !strings.Contains(out, "name=2025.09.09.1848") {
|
||||
t.Errorf("a CI-only commit moved the version — the pathspec is not excluding it\n%s", out)
|
||||
}
|
||||
|
||||
// ...and the pathspec must not be so broad it excludes everything.
|
||||
commitFile(t, dir, "internal/server/other.go", ciOnly)
|
||||
out, err = versionIn(t, dir)
|
||||
if err != nil {
|
||||
t.Fatalf("version.sh failed: %v\n%s", err, out)
|
||||
}
|
||||
if !strings.Contains(out, "name=2026.09.10.0042") {
|
||||
t.Errorf("a source commit did NOT move the version — the pathspec excludes too much, which is the silent-lie direction\n%s", out)
|
||||
}
|
||||
}
|
||||
|
||||
// android/ is deliberately NOT excluded, and that is the subtle half of the
|
||||
// list. It ships in no server image — but it is the APK's entire source, and
|
||||
// ONE script derives the version for both artifacts. Excluding it would stop
|
||||
// an Android-only commit from moving the APK's own version, which is exactly
|
||||
// the silent downgrade the versioning rework exists to prevent.
|
||||
func TestVersionName_AndroidSourcesCount(t *testing.T) {
|
||||
dir := gitRepo(t)
|
||||
commitFile(t, dir, "internal/server/thing.go", "1757443736")
|
||||
commitFile(t, dir, "android/app/src/main/Thing.kt", "1789000920")
|
||||
|
||||
out, err := versionIn(t, dir)
|
||||
if err != nil {
|
||||
t.Fatalf("version.sh failed: %v\n%s", err, out)
|
||||
}
|
||||
if !strings.Contains(out, "name=2026.09.10.0042") {
|
||||
t.Errorf("an Android commit did not move the version; the APK would ship new code under its old version name\n%s", out)
|
||||
}
|
||||
}
|
||||
|
||||
// No shipped commit in range means a shallow clone, and the script must refuse
|
||||
// rather than emit something plausible. A wrong version builds, signs and
|
||||
// publishes perfectly happily; it surfaces later as an update channel that has
|
||||
// quietly stopped offering anything.
|
||||
func TestVersionScript_RefusesWhenNothingShippedIsInRange(t *testing.T) {
|
||||
dir := gitRepo(t)
|
||||
commitFile(t, dir, "ci/version.sh", "1789000920")
|
||||
|
||||
out, err := versionIn(t, dir)
|
||||
if err == nil {
|
||||
t.Fatalf("script succeeded with no shipped commit in range; it should refuse\n%s", out)
|
||||
}
|
||||
if strings.Contains(out, "name=") {
|
||||
t.Errorf("script emitted a version name while refusing — that value could still be consumed\n%s", out)
|
||||
}
|
||||
}
|
||||
|
||||
// commitFiles is commitFile for more than one path in a single commit.
|
||||
func commitFiles(t *testing.T, dir, epoch string, paths ...string) {
|
||||
t.Helper()
|
||||
for _, rel := range paths {
|
||||
full := filepath.Join(dir, rel)
|
||||
if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := os.WriteFile(full, []byte("content @"+epoch+"\n"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}
|
||||
for _, args := range [][]string{{"add", "-A"}, {"commit", "-q", "-m", strings.Join(paths, " ")}} {
|
||||
cmd := exec.Command("git", args...)
|
||||
cmd.Dir = dir
|
||||
cmd.Env = append(os.Environ(),
|
||||
"GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null",
|
||||
"GIT_AUTHOR_DATE=@"+epoch+" +0000",
|
||||
"GIT_COMMITTER_DATE=@"+epoch+" +0000",
|
||||
)
|
||||
if out, err := cmd.CombinedOutput(); err != nil {
|
||||
t.Fatalf("git %v: %v\n%s", args, err, out)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// gitOutput runs a git command in dir and returns its combined output.
|
||||
func gitOutput(t *testing.T, dir string, args ...string) string {
|
||||
t.Helper()
|
||||
cmd := exec.Command("git", args...)
|
||||
cmd.Dir = dir
|
||||
cmd.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null")
|
||||
out, err := cmd.CombinedOutput()
|
||||
if err != nil {
|
||||
t.Fatalf("git %v: %v\n%s", args, err, out)
|
||||
}
|
||||
return string(out)
|
||||
}
|
||||
|
||||
// Tests do not ship, so a test-only commit must not re-version an artifact.
|
||||
//
|
||||
// `go build` drops *_test.go outright and the Vite build never imports a
|
||||
// .test.ts, so neither reaches an image or an APK. This repo has no tests/
|
||||
// tree — Go tests sit inline beside the code — so the exclusions are globs,
|
||||
// and a glob is easy to get subtly wrong in a way that still looks right.
|
||||
func TestVersionName_TestsDoNotReVersionTheArtifact(t *testing.T) {
|
||||
const (
|
||||
shipped = "1757443736" // 2025.09.09.1848
|
||||
later = "1789000920" // 2026.09.10.0042
|
||||
)
|
||||
for _, tc := range []struct{ name, path string }{
|
||||
{"go test beside its source", "internal/server/thing_test.go"},
|
||||
{"web unit test", "web/src/lib/api/admin.test.ts"},
|
||||
{"web script test", "web/scripts/tokens-to-css.test.js"},
|
||||
{"android JVM unit test", "android/app/src/test/java/A.kt"},
|
||||
{"vitest harness config", "web/vitest.config.ts"},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
dir := gitRepo(t)
|
||||
commitFile(t, dir, "internal/server/thing.go", shipped)
|
||||
commitFile(t, dir, tc.path, later)
|
||||
|
||||
out, err := versionIn(t, dir)
|
||||
if err != nil {
|
||||
t.Fatalf("version.sh failed: %v\n%s", err, out)
|
||||
}
|
||||
if !strings.Contains(out, "name=2025.09.09.1848") {
|
||||
t.Errorf("a commit touching only %s moved the version; tests do not ship\n%s", tc.path, out)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// The direction that actually costs something, and the reason the exclusions
|
||||
// above are globs over FILES rather than over their directories.
|
||||
//
|
||||
// `':!internal'` would satisfy every assertion in the test above while
|
||||
// silently excluding the entire server. This pins the opposite: a commit that
|
||||
// changes a test AND the source under it must still move the version, because
|
||||
// the source path matches on its own. Without this, a too-broad exclusion
|
||||
// reads as a passing test suite and ships an artifact under a stale version.
|
||||
func TestVersionName_ATestAlongsideItsSourceStillCounts(t *testing.T) {
|
||||
const (
|
||||
shipped = "1757443736"
|
||||
later = "1789000920"
|
||||
)
|
||||
dir := gitRepo(t)
|
||||
commitFile(t, dir, "internal/server/thing.go", shipped)
|
||||
commitFiles(t, dir, later,
|
||||
"internal/server/thing.go",
|
||||
"internal/server/thing_test.go",
|
||||
)
|
||||
|
||||
// The commit must actually contain BOTH paths. Rewriting a file with
|
||||
// identical bytes records nothing, and this assertion would then be
|
||||
// checking a test-only commit while appearing to check a mixed one.
|
||||
touched := gitOutput(t, dir, "show", "--name-only", "--format=", "HEAD")
|
||||
for _, want := range []string{"internal/server/thing.go", "internal/server/thing_test.go"} {
|
||||
if !strings.Contains(touched, want) {
|
||||
t.Fatalf("fixture is wrong: HEAD does not contain %s\n%s", want, touched)
|
||||
}
|
||||
}
|
||||
|
||||
out, err := versionIn(t, dir)
|
||||
if err != nil {
|
||||
t.Fatalf("version.sh failed: %v\n%s", err, out)
|
||||
}
|
||||
if !strings.Contains(out, "name=2026.09.10.0042") {
|
||||
t.Errorf("a source change accompanied by a test change did NOT move the version — "+
|
||||
"the exclusions are matching directories rather than test files\n%s", out)
|
||||
}
|
||||
}
|
||||
@@ -218,6 +218,7 @@ func (s *Server) handleHealthz(w http.ResponseWriter, _ *http.Request) {
|
||||
_ = json.NewEncoder(w).Encode(map[string]string{
|
||||
"status": "ok",
|
||||
"version": ServerVersion,
|
||||
"channel": ServerChannel,
|
||||
"min_client_version": MinClientVersion,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -5,12 +5,27 @@ package server
|
||||
// older clients see version_too_old at /healthz and refuse to operate.
|
||||
const MinClientVersion = "0.1.0"
|
||||
|
||||
// ServerVersion is the deployed server image's version tag. Defaults to
|
||||
// "dev" for local builds; overridden at link time via:
|
||||
// ServerVersion is the build's own version name — YYYY.MM.DD.HHMM, derived
|
||||
// from the commit it was built from by ci/version.sh. Defaults to "dev" for
|
||||
// local builds; overridden at link time via:
|
||||
//
|
||||
// -ldflags="-X 'git.fabledsword.com/bvandeusen/minstrel/internal/server.ServerVersion=v2026.05.10.2'"
|
||||
// -ldflags="-X 'git.fabledsword.com/bvandeusen/minstrel/internal/server.ServerVersion=2026.09.10.1449'"
|
||||
//
|
||||
// release.yml passes the git tag through MINSTREL_VERSION build-arg →
|
||||
// Dockerfile ldflag. Surfaced at /healthz so operators can verify which
|
||||
// image their container is running without exec'ing into it.
|
||||
// release.yml passes it through the MINSTREL_VERSION build-arg → Dockerfile
|
||||
// ldflag. Surfaced at /healthz so operators can verify which image their
|
||||
// container is running without exec'ing into it.
|
||||
//
|
||||
// This carried the literal strings "main" and "dev" until 2026-09-10, which
|
||||
// made every image on a channel report the same thing forever. It stopped
|
||||
// being cosmetic when :vYYYY.MM.DD.HHMM image tags were retired (family rule
|
||||
// 145): this is now the ONLY thing that says which build is running.
|
||||
var ServerVersion = "dev"
|
||||
|
||||
// ServerChannel is which line this build came off — "stable" or "dev", or
|
||||
// "local" for a plain `docker build`.
|
||||
//
|
||||
// A SIBLING FIELD, never a suffix inside ServerVersion (family rule 149). The
|
||||
// same commit built on both lanes reports the same version and differs only
|
||||
// here; folding the two together is what makes a version string stop being
|
||||
// comparable.
|
||||
var ServerChannel = "local"
|
||||
|
||||
@@ -34,14 +34,17 @@ export type DiscoverTuning = {
|
||||
snooze_days: number;
|
||||
};
|
||||
|
||||
export type TuningScope = 'radio' | 'daily_mix' | 'taste' | 'discover';
|
||||
export type TuningScope = 'radio' | 'daily_mix' | 'songs_like' | 'taste' | 'discover';
|
||||
|
||||
/** The weight-profile scopes, as distinct from the singleton-settings scopes. */
|
||||
export type WeightProfileScope = 'radio' | 'daily_mix' | 'songs_like';
|
||||
|
||||
export type TuningSnapshot = {
|
||||
profiles: Record<'radio' | 'daily_mix', WeightProfile>;
|
||||
profiles: Record<WeightProfileScope, WeightProfile>;
|
||||
taste: TasteTuning;
|
||||
discover: DiscoverTuning;
|
||||
shipped: {
|
||||
profiles: Record<'radio' | 'daily_mix', WeightProfile>;
|
||||
profiles: Record<WeightProfileScope, WeightProfile>;
|
||||
taste: TasteTuning;
|
||||
discover: DiscoverTuning;
|
||||
};
|
||||
|
||||
@@ -6,26 +6,36 @@
|
||||
// is actually running (came up debugging the in-app update flow when
|
||||
// it wasn't obvious whether v2026.05.10.0 or .1 was deployed).
|
||||
//
|
||||
// This is now the ONLY place an operator can see which build they are on.
|
||||
// Image tags stopped carrying the version on 2026-09-10 — :latest and :dev
|
||||
// are rolling names and :<sha> answers "which commit", not "which build" —
|
||||
// so the server's self-report is the answer.
|
||||
//
|
||||
// The channel is shown BESIDE the version, never spliced into it: the same
|
||||
// commit built on both lanes reports an identical version and differs only
|
||||
// in channel, so "2026.09.10.1449 · dev" and "2026.09.10.1449 · stable" are
|
||||
// the same code on two lines. Suppressed for stable, which is the
|
||||
// unremarkable case and would just be noise on every install.
|
||||
//
|
||||
// /healthz is unauthenticated, so the bare fetch works without
|
||||
// credentials. Renders nothing on parse failure or pre-version
|
||||
// images that don't include the field — graceful degradation.
|
||||
|
||||
type Health = { status: string; version?: string };
|
||||
type Health = { status: string; version?: string; channel?: string };
|
||||
|
||||
let version = $state<string | null>(null);
|
||||
let channel = $state<string | null>(null);
|
||||
|
||||
onMount(async () => {
|
||||
try {
|
||||
const res = await fetch('/healthz');
|
||||
if (!res.ok) return;
|
||||
const body = (await res.json()) as Partial<Health>;
|
||||
if (body.version && body.version !== 'dev') {
|
||||
version = body.version;
|
||||
} else if (body.version === 'dev') {
|
||||
// Local dev images report "dev" — show it so the operator
|
||||
// can tell they're not on a release tag.
|
||||
version = 'dev';
|
||||
}
|
||||
if (!body.version) return;
|
||||
version = body.version;
|
||||
// Reported verbatim rather than validated against an enum — a build
|
||||
// claiming something unexpected is better shown than dropped.
|
||||
channel = body.channel && body.channel !== 'stable' ? body.channel : null;
|
||||
} catch {
|
||||
// network / parse error — silent.
|
||||
}
|
||||
@@ -33,5 +43,7 @@
|
||||
</script>
|
||||
|
||||
{#if version}
|
||||
<p class="text-xs text-text-secondary">Server {version}</p>
|
||||
<p class="text-xs text-text-secondary">
|
||||
Server {version}{#if channel} · {channel}{/if}
|
||||
</p>
|
||||
{/if}
|
||||
|
||||
@@ -6,6 +6,7 @@
|
||||
resetTuning,
|
||||
getTrends,
|
||||
type TuningScope,
|
||||
type WeightProfileScope,
|
||||
type TuningSnapshot,
|
||||
type WeightProfile,
|
||||
type TasteTuning,
|
||||
@@ -49,9 +50,15 @@
|
||||
{ key: 'snooze_days', label: 'Snooze length (days)', hint: 'How long "not right now" parks a suggestion before it returns on its own. Records no opinion about the artist and never feeds the taste profile.' }
|
||||
];
|
||||
|
||||
const profileScopes: { scope: 'radio' | 'daily_mix'; label: string; blurb: string }[] = [
|
||||
const profileScopes: { scope: WeightProfileScope; label: string; blurb: string }[] = [
|
||||
{ scope: 'radio', label: 'Radio', blurb: 'Seed-directed listening — the user picked a direction.' },
|
||||
{ scope: 'daily_mix', label: 'Daily mixes', blurb: 'For You, Songs like…, and the discovery mixes.' }
|
||||
{ scope: 'daily_mix', label: 'Daily mixes', blurb: 'For You, the discovery mixes, and "You might like".' },
|
||||
{
|
||||
scope: 'songs_like',
|
||||
label: 'Songs like…',
|
||||
blurb:
|
||||
'The tightest surface: everything here should sound like the seed track. Similarity dominates on purpose — raising like/taste/recency here is what makes these mixes wander.'
|
||||
}
|
||||
];
|
||||
|
||||
let snapshot = $state<TuningSnapshot | null>(null);
|
||||
@@ -64,10 +71,11 @@
|
||||
const f: Record<string, Record<string, string>> = {
|
||||
radio: {},
|
||||
daily_mix: {},
|
||||
songs_like: {},
|
||||
taste: {},
|
||||
discover: {}
|
||||
};
|
||||
for (const p of ['radio', 'daily_mix'] as const) {
|
||||
for (const p of profileScopes.map((s) => s.scope)) {
|
||||
for (const { key } of weightFields) f[p][key] = String(snap.profiles[p][key]);
|
||||
}
|
||||
for (const { key } of tasteFields) f.taste[key] = String(snap.taste[key]);
|
||||
|
||||
@@ -55,11 +55,19 @@ const discover = (over: Partial<Record<string, number>> = {}) => ({
|
||||
// new scopes to BOTH this fixture and `shipped`.
|
||||
function snapshot(over: Partial<TuningSnapshot> = {}): TuningSnapshot {
|
||||
return {
|
||||
profiles: { radio: weights({ taste_weight: 1 }), daily_mix: weights() },
|
||||
profiles: {
|
||||
radio: weights({ taste_weight: 1 }),
|
||||
daily_mix: weights(),
|
||||
songs_like: weights({ similarity_weight: 4, like_boost: 0.5, taste_weight: 0.25 })
|
||||
},
|
||||
taste: taste(),
|
||||
discover: discover(),
|
||||
shipped: {
|
||||
profiles: { radio: weights({ taste_weight: 1 }), daily_mix: weights() },
|
||||
profiles: {
|
||||
radio: weights({ taste_weight: 1 }),
|
||||
daily_mix: weights(),
|
||||
songs_like: weights({ similarity_weight: 4, like_boost: 0.5, taste_weight: 0.25 })
|
||||
},
|
||||
taste: taste(),
|
||||
discover: discover()
|
||||
},
|
||||
@@ -75,11 +83,12 @@ beforeEach(() => {
|
||||
});
|
||||
|
||||
describe('Admin tuning page', () => {
|
||||
test('renders both profiles and the taste card with current values', async () => {
|
||||
test('renders every weight profile and the taste card with current values', async () => {
|
||||
(getTuning as ReturnType<typeof vi.fn>).mockResolvedValue(snapshot());
|
||||
render(TuningPage);
|
||||
await waitFor(() => expect(screen.getByText('Radio')).toBeInTheDocument());
|
||||
expect(screen.getByText('Daily mixes')).toBeInTheDocument();
|
||||
expect(screen.getByText('Songs like…')).toBeInTheDocument();
|
||||
expect(screen.getByText('Taste profile build')).toBeInTheDocument();
|
||||
const radioTaste = screen.getByLabelText(/taste weight/i, {
|
||||
selector: '#radio-taste_weight'
|
||||
@@ -89,6 +98,27 @@ describe('Admin tuning page', () => {
|
||||
expect(halfLife.value).toBe('75');
|
||||
});
|
||||
|
||||
// Songs-like is a separate weight profile precisely so it can be tuned
|
||||
// apart from For-You (#3881). If its card stopped rendering its OWN values
|
||||
// — or quietly fell back to daily_mix's — the split would exist in the
|
||||
// backend and be unreachable in the UI, which is the same as not shipping
|
||||
// it (rule 27).
|
||||
test('the songs-like card carries its own weights, not daily_mix\'s', async () => {
|
||||
(getTuning as ReturnType<typeof vi.fn>).mockResolvedValue(snapshot());
|
||||
render(TuningPage);
|
||||
await waitFor(() => expect(screen.getByText('Songs like…')).toBeInTheDocument());
|
||||
|
||||
const sim = document.getElementById('songs_like-similarity_weight') as HTMLInputElement;
|
||||
const like = document.getElementById('songs_like-like_boost') as HTMLInputElement;
|
||||
expect(sim.value).toBe('4');
|
||||
expect(like.value).toBe('0.5');
|
||||
|
||||
// The contrast that makes the card worth having: daily_mix must still show
|
||||
// its own, different numbers on the same page.
|
||||
const dailySim = document.getElementById('daily_mix-similarity_weight') as HTMLInputElement;
|
||||
expect(dailySim.value).not.toBe(sim.value);
|
||||
});
|
||||
|
||||
test('save sends only the changed fields for the scope', async () => {
|
||||
(getTuning as ReturnType<typeof vi.fn>).mockResolvedValue(snapshot());
|
||||
(patchTuning as ReturnType<typeof vi.fn>).mockResolvedValue(snapshot());
|
||||
|
||||
Reference in New Issue
Block a user