diff --git a/.gitea/workflows/release.yml b/.gitea/workflows/release.yml index bb6d22d1..06bf08da 100644 --- a/.gitea/workflows/release.yml +++ b/.gitea/workflows/release.yml @@ -452,7 +452,7 @@ jobs: # 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)" + out="$(ci/version.sh --apk HEAD)" printf '%s\n' "${out}" >> "$GITHUB_OUTPUT" # The channel is a property of the LANE, not of the commit, which is diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt index e15ea87d..d93767c8 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt @@ -216,24 +216,56 @@ class MinstrelForwardingPlayer( // drops via isLoadingUpnp() = true; the OutputPickerController // observes the uiState.queue change and runs the resync (re-clears // Sonos's native queue + AddURIToQueue the new tracks + Play). + // + // The SAME tracks set again (play on the album already queued, from + // another track) is not a new queue: the renderer already holds it, and + // the uiState collector -- which keys on the track ids -- would never see + // a change to resync on, leaving the cast stuck "loading". It is a move + // within the queue, so the renderer is pointed at the new track and the + // play() that follows goes through. override fun setMediaItems(mediaItems: List) { + val sameQueue = holdsSameItems(mediaItems) super.setMediaItems(mediaItems) - markPendingResyncIfRemote() + afterQueueSet(sameQueue) } override fun setMediaItems(mediaItems: List, resetPosition: Boolean) { + val sameQueue = holdsSameItems(mediaItems) super.setMediaItems(mediaItems, resetPosition) - markPendingResyncIfRemote() + afterQueueSet(sameQueue) } override fun setMediaItems(mediaItems: List, startIndex: Int, startPositionMs: Long) { + val sameQueue = holdsSameItems(mediaItems) super.setMediaItems(mediaItems, startIndex, startPositionMs) - markPendingResyncIfRemote() + afterQueueSet(sameQueue) } - private fun markPendingResyncIfRemote() { - val wasActive = holder.active.value ?: return + private fun holdsSameItems(mediaItems: List): Boolean = + mediaItems.size == delegate.mediaItemCount && + mediaItems.indices.all { mediaItems[it].mediaId == delegate.getMediaItemAt(it).mediaId } + + private fun afterQueueSet(sameQueue: Boolean) { + val active = holder.active.value ?: return + if (!sameQueue) { + markPendingResync(active) + return + } + val index = delegate.currentMediaItemIndex + val positionMs = delegate.currentPosition.coerceAtLeast(0L) + Timber.w( + "setMediaItems with the queue %s already holds -- moving to track %d", + active.routeName, + index + 1, + ) + when (active.queueMode) { + QueueMode.SINGLE_TRACK -> startSingleTrack(active, index, positionMs) + QueueMode.NATIVE_QUEUE -> seekNativeQueue(active, index, positionMs) + } + } + + private fun markPendingResync(wasActive: ActiveUpnp) { Timber.w( "setMediaItems while UPnP active (%s) -- marking pending resync", wasActive.routeName, @@ -332,11 +364,15 @@ class MinstrelForwardingPlayer( super.seekTo(mediaItemIndex, positionMs) when (active.queueMode) { QueueMode.SINGLE_TRACK -> playSingleTrackAt(active, mediaItemIndex, positionMs) - QueueMode.NATIVE_QUEUE -> sendNativeQueueCommand(active) { - active.avTransport.seekToTrack(mediaItemIndex + 1) - if (positionMs > 0L) { - active.avTransport.seek(positionMs) - } + QueueMode.NATIVE_QUEUE -> seekNativeQueue(active, mediaItemIndex, positionMs) + } + } + + private fun seekNativeQueue(active: ActiveUpnp, index: Int, positionMs: Long) { + sendNativeQueueCommand(active) { + active.avTransport.seekToTrack(index + 1) + if (positionMs > 0L) { + active.avTransport.seek(positionMs) } } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt index d0d217d7..ecacb8ec 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt @@ -383,23 +383,27 @@ class OutputPickerController @Inject constructor( resyncSingleTrack(upnpRoute, transport, newQueue, newCurrentIndex) return@withLock } - val handledIncrementally = runCatching { + // The incremental diff is for a queue changed under a cast that kept + // running (play next, an append, a removal): it leaves the playing + // track alone. A queue REPLACED through setMediaItems has already + // cleared active, and the listener asked for its start track and for + // it to play -- both of which the diff would ignore, and with active + // never restored the app would think it had gone back to the phone + // while the speaker played on. That case reloads in full below. + val stillActive = activeUpnpHolder.active.value?.routeId == routeId + val handledIncrementally = stillActive && runCatching { sonosQueue.tryIncrementalResync(transport, oldIds, newQueue) }.getOrElse { e -> Timber.w(e, "Sonos incremental resync errored; falling back to full reload") false } if (handledIncrementally) { - // Active was never cleared on the incremental path; clear any - // target that markPendingResyncIfRemote set (it didn't, for - // incremental cases that don't go through setMediaItems, but - // belt-and-suspenders). activeUpnpHolder.setTarget(null) return@withLock } // Full rebuild: ensure active is cleared so transport calls drop - // (markPendingResyncIfRemote may already have done this on the - // setMediaItems path). + // (the forwarding player has already done this on the setMediaItems + // path). if (activeUpnpHolder.active.value != null) { activeUpnpHolder.set(null) activeUpnpHolder.setTarget(routeId) diff --git a/ci/version.sh b/ci/version.sh index b12a13d0..fb7f0ae8 100755 --- a/ci/version.sh +++ b/ci/version.sh @@ -3,11 +3,17 @@ # 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) +# commit that CHANGED SOMETHING SHIPPED (see SHIPPED, +# or SHIPPED_APK with --apk) # code= ordering key, minutes since 2020-01-01 at BUILD time # tag=v what a release of this commit must be called # -# Usage: ci/version.sh [] (default HEAD) +# Usage: ci/version.sh [--apk] [] (default HEAD) +# +# --apk names the Android app from android/ alone. The app compares this +# name with the one it is running, and a different name is what +# offers an update — so a name that moved for a server-only commit +# would offer every phone a rebuild of the app it already has. # # 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 @@ -30,6 +36,12 @@ set -euo pipefail readonly EPOCH_2020=1577836800 # 2020-01-01T00:00:00Z + +apk=false +if [ "${1:-}" = "--apk" ]; then + apk=true + shift +fi readonly REF="${1:-HEAD}" # Both clocks are overridable so a test can pin them. Nothing but tests should @@ -48,14 +60,17 @@ readonly REF="${1:-HEAD}" # 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. +# THIS LIST NAMES THE SERVER IMAGE, and it is shorter than it looks like it +# should be. The image ships cmd/, internal/, shared/, web/, +# config.example.yaml and client/ — and client/ holds the APK, so android/ +# stays in: an Android-only commit changes what the image carries. Exclude +# only what ships in neither artifact; over-inclusion is the harmless +# direction here. +# +# The APK itself is named from SHIPPED_APK (below), where over-inclusion is +# NOT harmless: the app offers an update whenever the name differs from its +# own, so a name moved by a server-only commit offers every phone a rebuild +# of the app it already has. # # The family's other repos (roundtable / roundtable-android) each keep a # tighter list because they are separate repos with one artifact apiece. Do @@ -100,9 +115,24 @@ readonly SHIPPED=( ':!web/vitest.setup.ts' ) +# What the APK is built from: android/ and nothing else (the Gradle build +# reads no path outside it). Still a denylist within it, for the same reason +# as above — a new directory under android/ counts until named here. +readonly SHIPPED_APK=( + android + ':!android/app/src/test' # JVM unit tests + ':!android/*.md' +) + +if [ "${apk}" = true ]; then + shipped=("${SHIPPED_APK[@]}") +else + shipped=("${SHIPPED[@]}") +fi + commit_epoch="${MINSTREL_COMMIT_EPOCH:-}" if [ -z "${commit_epoch}" ]; then - commit_epoch="$(git log --format=%ct -1 "${REF}" -- "${SHIPPED[@]}")" + 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 diff --git a/internal/server/release_version_test.go b/internal/server/release_version_test.go index 20407f30..5967cf30 100644 --- a/internal/server/release_version_test.go +++ b/internal/server/release_version_test.go @@ -455,7 +455,7 @@ func TestImageBuild_StampsADerivedVersionAndAChannel(t *testing.T) { 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") + t.Error("the image job no longer derives its version from ci/version.sh — the image's version has drifted out of test coverage") } } @@ -508,10 +508,11 @@ func commitFile(t *testing.T, dir, path, epoch string) { } // 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) { +// pinned-clock override, so the PATHSPEC is what is under test. flags go +// before the ref, as in `--apk`. +func versionIn(t *testing.T, dir string, flags ...string) (string, error) { t.Helper() - cmd := exec.Command(filepath.Join(repoRoot(t), "ci", "version.sh"), "HEAD") + cmd := exec.Command(filepath.Join(repoRoot(t), "ci", "version.sh"), append(flags, "HEAD")...) cmd.Dir = dir cmd.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null", @@ -558,11 +559,9 @@ func TestVersionName_IgnoresCommitsThatShipNothing(t *testing.T) { } } -// 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. +// android/ is deliberately NOT excluded from the image's name: the image +// bundles the APK under client/, so an Android-only commit changes what the +// image carries, and an image that changed must not keep its old name. func TestVersionName_AndroidSourcesCount(t *testing.T) { dir := gitRepo(t) commitFile(t, dir, "internal/server/thing.go", "1757443736") @@ -573,7 +572,71 @@ func TestVersionName_AndroidSourcesCount(t *testing.T) { 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) + t.Errorf("an Android commit did not move the image's version; it carries a new APK under its old name\n%s", out) + } +} + +// The app offers an update whenever the server's APK has a different name +// from its own, so the APK's name must move for Android commits and ONLY for +// them. Named from the whole repo, every server-only deploy offered each +// phone a rebuild of the app it already had. +func TestApkName_MovesOnlyForAndroidCommits(t *testing.T) { + const ( + app = "1757443736" // 2025.09.09.1848 + server = "1789000920" // 2026.09.10.0042, later + again = "1789600000" // 2026.09.16.2306, later still + ) + dir := gitRepo(t) + commitFile(t, dir, "android/app/src/main/Thing.kt", app) + commitFile(t, dir, "internal/server/thing.go", server) + commitFile(t, dir, "web/src/routes/page.svelte", server) + + out, err := versionIn(t, dir, "--apk") + if err != nil { + t.Fatalf("version.sh --apk failed: %v\n%s", err, out) + } + if !strings.Contains(out, "name=2025.09.09.1848") { + t.Errorf("a server-only commit moved the APK's name; every phone would be offered the app it already has\n%s", out) + } + // The image's name still follows the server commit, so the test above + // is not passing because the flag is ignored and everything is stale. + 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("without --apk the name should follow the server commit\n%s", out) + } + + // A test-only Android commit ships nothing either. + commitFile(t, dir, "android/app/src/test/java/A.kt", again) + out, err = versionIn(t, dir, "--apk") + if err != nil { + t.Fatalf("version.sh --apk failed: %v\n%s", err, out) + } + if !strings.Contains(out, "name=2025.09.09.1848") { + t.Errorf("an Android test-only commit moved the APK's name\n%s", out) + } + + // ...and a real Android commit must move it, or new app code would ship + // under the name of the old and never be offered. + commitFile(t, dir, "android/app/src/main/Thing.kt", again) + out, err = versionIn(t, dir, "--apk") + if err != nil { + t.Fatalf("version.sh --apk failed: %v\n%s", err, out) + } + if !strings.Contains(out, "name=2026.09.16.2306") { + t.Errorf("an Android commit did not move the APK's name; the new app would never be offered\n%s", out) + } +} + +// The APK lane must ask for the APK's name. Without the flag it would still +// build, sign and publish, and quietly go back to offering an update on every +// server deploy. +func TestReleaseWorkflow_NamesTheApkFromAndroidAlone(t *testing.T) { + step := stepBody(t, releaseYAML(t), "Compute release version") + if !strings.Contains(step, "ci/version.sh --apk HEAD") { + t.Errorf("the APK lane does not derive its version with --apk\n%s", step) } }