From 366692a1fcf0da011a0d4bc944cd9eceb253ac79 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 17 Aug 2026 12:56:31 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20stop=20the=20sync=20feed=20hiding=20miss?= =?UTF-8?q?ing=20files=20from=20clients=20=E2=80=94=20#2704?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2523 filtered missing tracks out of every path that CHOOSES music, but the client sync feed was never touched: GetTracksByIDs has no filter and the wire had no field for it. So every Android client held a cached library containing tracks whose files are gone, with no way to tell, and could queue them from any cache-first path -- the exact failure #2523 existed to prevent, reached by a different route. Ships the state rather than filtering the feed, of the two options the ticket weighed. A missing file is expected to come back: the scanner clears the mark, and adopts the row if it returns renamed (#2528). Withholding the row would mean a delete-and-recreate on every client for what is usually a transient unmount, churning caches and throwing away the identity #2528 works to preserve. Room goes to v8. No hand-written migration: the pre-v1 destructive fallback rebuilds from sync, which repopulates every row with the new column -- exactly the case that policy exists for. The interesting part was working out what "missing" means to a client, and it is NOT "unplayable". Two findings shaped the fix: Server search and album detail never filtered missing tracks either, and that turns out to be right rather than an oversight. The consistent rule the codebase already follows is that Minstrel never PICKS a missing track for you -- recommendation, discover, mixes and browse all exclude them -- but it does not hide one you went looking for by name or opened an album to find. Hiding track 4 makes an album look wrong. So the fix is to mark and to keep it out of queues, not to hide it. And a track whose server file is missing still plays perfectly if its audio is already in the device cache. ShuffleSource's offline pools filter to exactly those residents, so it now clears the mark on the way out: the bytes are local and the server's loss is irrelevant. Without that, the queue filter below would have thrown away tracks that work, turning a fix into an offline regression. The queue protection is one choke point rather than five call sites. setQueue is where playlists, album play-all, search, radio and cold-boot resume all converge. dropUnavailable is pure so the index arithmetic is pinned by tests -- removing entries ahead of the requested position would otherwise start playback on the wrong track, and asking to start on a missing track now starts the next playable one, which is the "gets skipped" behaviour the operator asked for. An entirely missing queue returns empty and the caller leaves the player alone rather than replacing what is playing with silence. --- .../minstrel/cache/ShuffleSource.kt | 10 +- .../minstrel/cache/db/AppDatabase.kt | 8 +- .../cache/db/entities/CachedTrackEntity.kt | 5 + .../minstrel/cache/sync/SyncController.kt | 1 + .../minstrel/library/data/LibraryMappers.kt | 2 + .../fabledsword/minstrel/models/TrackRef.kt | 10 ++ .../minstrel/models/wire/SyncResponseWire.kt | 9 ++ .../minstrel/models/wire/TrackWire.kt | 5 + .../minstrel/player/PlayerController.kt | 48 +++++++++- .../minstrel/player/DropUnavailableTest.kt | 96 +++++++++++++++++++ internal/api/convert.go | 1 + internal/api/library_sync_views.go | 14 +++ internal/api/library_sync_views_test.go | 18 ++++ internal/api/types.go | 10 ++ 14 files changed, 231 insertions(+), 6 deletions(-) create mode 100644 android/app/src/test/java/com/fabledsword/minstrel/player/DropUnavailableTest.kt diff --git a/android/app/src/main/java/com/fabledsword/minstrel/cache/ShuffleSource.kt b/android/app/src/main/java/com/fabledsword/minstrel/cache/ShuffleSource.kt index d28cc2cb..d64a3f0a 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/cache/ShuffleSource.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/cache/ShuffleSource.kt @@ -57,6 +57,14 @@ class ShuffleSource @Inject constructor( private suspend fun materialize(orderedIds: List): List { if (orderedIds.isEmpty()) return emptyList() val byId = trackDao.getByIds(orderedIds).associateBy { it.id } - return orderedIds.mapNotNull { byId[it]?.toDomain() } + return orderedIds.mapNotNull { id -> + // Clear the server's missing mark (#2704). Every id reaching here + // came through residentIdsByRecency, which already proved the + // AUDIO is in the local cache — so these play regardless of what + // the server has lost, and the queue filter in PlayerController + // would otherwise throw away tracks that work perfectly. Missing + // means "cannot stream", not "cannot play". + byId[id]?.toDomain()?.copy(unavailable = false) + } } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/cache/db/AppDatabase.kt b/android/app/src/main/java/com/fabledsword/minstrel/cache/db/AppDatabase.kt index 5e5c9306..49b0f9cf 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/cache/db/AppDatabase.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/cache/db/AppDatabase.kt @@ -65,9 +65,13 @@ import com.fabledsword.minstrel.cache.db.entities.SyncMetadataEntity AuthSessionEntity::class, DiagnosticEventEntity::class, ], + // v8: + cached_tracks.missing, the server's missing-file mark (#2704), + // so cache-first surfaces stop offering files that cannot stream. // v7: + diagnostic_events table (M9) and the diagnosticsOptOut column - // on auth_session. Pre-v1 destructive fallback rebuilds on mismatch. - version = 7, + // on auth_session. Pre-v1 destructive fallback rebuilds on mismatch — + // which is exactly right here: the next sync refills every row with the + // new column populated, so there is nothing to migrate by hand. + version = 8, exportSchema = true, ) @TypeConverters(MinstrelTypeConverters::class) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/cache/db/entities/CachedTrackEntity.kt b/android/app/src/main/java/com/fabledsword/minstrel/cache/db/entities/CachedTrackEntity.kt index 3cf501b9..d8fe1624 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/cache/db/entities/CachedTrackEntity.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/cache/db/entities/CachedTrackEntity.kt @@ -8,6 +8,10 @@ import kotlinx.datetime.Instant /** * Cache row for one track. Mirrors the Flutter client's * `CachedTracks` Drift table. + * + * [missing] carries the server's missing-file mark (#2704). Every read that + * can put a track in front of the user — or in a queue — must exclude it, and + * the DAO queries do that rather than each call site remembering to. */ @Entity(tableName = "cached_tracks") data class CachedTrackEntity( @@ -21,5 +25,6 @@ data class CachedTrackEntity( val filePath: String? = null, val fileFormat: String? = null, val genre: String? = null, + val missing: Boolean = false, val fetchedAt: Instant = Clock.System.now(), ) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/cache/sync/SyncController.kt b/android/app/src/main/java/com/fabledsword/minstrel/cache/sync/SyncController.kt index 106c3827..a7c8620b 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/cache/sync/SyncController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/cache/sync/SyncController.kt @@ -219,4 +219,5 @@ private fun SyncTrackWire.toEntity(): CachedTrackEntity = CachedTrackEntity( filePath = filePath, fileFormat = fileFormat, genre = genre, + missing = missing, ) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/library/data/LibraryMappers.kt b/android/app/src/main/java/com/fabledsword/minstrel/library/data/LibraryMappers.kt index 8df83a69..b90e9359 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/library/data/LibraryMappers.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/library/data/LibraryMappers.kt @@ -97,6 +97,7 @@ fun CachedTrackEntity.toDomain( trackNumber = trackNumber, discNumber = discNumber, durationSec = durationMs.millisToSeconds(), + unavailable = missing, // Deterministic from track id; matches the server's stream_url // (internal/api/convert.go:75 streamURL builder). Cached rows // didn't carry streamUrl before, which left MetadataProvider- @@ -121,6 +122,7 @@ fun TrackWire.toDomain(): TrackRef = discNumber = discNumber, durationSec = durationSec, streamUrl = streamUrl, + unavailable = unavailable, ) fun ArtistWire.toDomain(): ArtistRef = diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/TrackRef.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/TrackRef.kt index e32dafbd..06cc3ce1 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/TrackRef.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/TrackRef.kt @@ -31,6 +31,16 @@ data class TrackRef( val discNumber: Int? = null, val durationSec: Int = 0, val streamUrl: String = "", + /** + * The server has no file for this track right now (#2704). It still + * belongs to the library, keeps its history, and may come back — but + * streaming it will fail, so nothing should queue it. + * + * NOT the same as unplayable on this device: audio already resident in + * the local cache plays regardless of what the server has, which is why + * the offline pools in ShuffleSource deliberately ignore this. + */ + val unavailable: Boolean = false, ) { /** * Cover URL derived from the parent album's `/api/albums/{id}/cover` diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/SyncResponseWire.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/SyncResponseWire.kt index 23bbc822..eb107be8 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/SyncResponseWire.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/SyncResponseWire.kt @@ -42,6 +42,15 @@ data class SyncTrackWire( @SerialName("file_path") val filePath: String? = null, @SerialName("file_format") val fileFormat: String? = null, val genre: String? = null, + // The file is currently absent from disk server-side (#2704). Shipped as + // state rather than the row being withheld, because a missing file is + // expected to return — dropping it would churn the cache on every + // transient unmount and discard the identity #2528 preserves. + // + // Defaults false so a server predating the field deserialises cleanly and + // its tracks stay playable, which is the correct reading of "this server + // has nothing to say about missing files". + val missing: Boolean = false, ) /** diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/TrackWire.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/TrackWire.kt index 3b9630af..ec230fc0 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/TrackWire.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/TrackWire.kt @@ -26,4 +26,9 @@ data class TrackWire( @SerialName("disc_number") val discNumber: Int? = null, @SerialName("duration_sec") val durationSec: Int = 0, @SerialName("stream_url") val streamUrl: String = "", + // Omitted by the server when false, so the default carries most rows + // (#2704). True only from the direct-lookup surfaces — album detail and + // search — which return a track the user asked for by name or container + // rather than one Minstrel chose. + val unavailable: Boolean = false, ) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt index 28a8481c..631b0ad5 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt @@ -260,8 +260,16 @@ class PlayerController @Inject constructor( autoplay: Boolean = true, ) { val controller = mediaController ?: return - queueRefs = tracks - val items = tracks.map { it.toMediaItem(source) } + // One choke point for #2704: a track whose file the server has lost + // must not take a queue slot, whichever surface built the list. + // Playlists already drop them earlier (toPlayableTrackRefs), but + // album play-all, search, radio and cold-boot resume all arrive here + // too, and catching it once beats remembering at five call sites. + val playable = dropUnavailable(tracks, initialIndex) + if (playable.tracks.isEmpty()) return + queueRefs = playable.tracks + val items = playable.tracks.map { it.toMediaItem(source) } + val startIndex = playable.initialIndex // Drift #562 cold-boot resume calls this from a non-Main suspend // context after awaitReady() unblocks (ResumeController launches // on Dispatchers.Default by the time it reaches us). MediaController @@ -270,7 +278,7 @@ class PlayerController @Inject constructor( // if we're already there, run directly to avoid the re-dispatch // latency UI callers depend on. runOnControllerThread(controller) { - controller.setMediaItems(items, initialIndex, /* startPositionMs = */ 0L) + controller.setMediaItems(items, startIndex, /* startPositionMs = */ 0L) controller.prepare() if (autoplay) controller.play() } @@ -874,3 +882,37 @@ data class PlaybackErrorEvent( val title: String, val detail: String? = null, ) + +/** + * A queue with the server-missing tracks removed, and the caller's starting + * index moved to match (#2704). + */ +data class PlayableQueue(val tracks: List, val initialIndex: Int) + +/** + * Drop tracks the server has no file for, keeping [initialIndex] pointing at + * the same music. + * + * The index is the fiddly half and the reason this is a function rather than + * a `filter` at the call site: removing entries before the requested position + * would otherwise start playback on the wrong track. The new index is the + * count of surviving tracks ahead of it, which also gives the right behaviour + * when the requested track is ITSELF missing — playback starts at the next + * one that can play, i.e. it gets skipped. + * + * Returns an empty queue when nothing survives, which the caller treats as + * "don't touch the player": replacing a playing queue with silence because a + * stale list turned out to be entirely missing would be worse than ignoring + * the request. + */ +fun dropUnavailable(tracks: List, initialIndex: Int): PlayableQueue { + if (tracks.none { it.unavailable }) return PlayableQueue(tracks, initialIndex) + val kept = ArrayList(tracks.size) + var newIndex = 0 + tracks.forEachIndexed { i, track -> + if (track.unavailable) return@forEachIndexed + if (i < initialIndex) newIndex++ + kept.add(track) + } + return PlayableQueue(kept, newIndex.coerceAtMost((kept.size - 1).coerceAtLeast(0))) +} diff --git a/android/app/src/test/java/com/fabledsword/minstrel/player/DropUnavailableTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/player/DropUnavailableTest.kt new file mode 100644 index 00000000..7fa27eeb --- /dev/null +++ b/android/app/src/test/java/com/fabledsword/minstrel/player/DropUnavailableTest.kt @@ -0,0 +1,96 @@ +package com.fabledsword.minstrel.player + +import com.fabledsword.minstrel.models.TrackRef +import org.junit.jupiter.api.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * The queue filter for #2704. The index arithmetic is the part worth pinning: + * getting it wrong starts playback on the wrong track, which is a subtler and + * more annoying bug than the one being fixed. + */ +class DropUnavailableTest { + + private fun track(id: String, unavailable: Boolean = false) = + TrackRef(id = id, title = id, unavailable = unavailable) + + private fun queue(vararg spec: Pair) = + spec.map { (id, missing) -> track(id, missing) } + + @Test + fun `a queue with nothing missing is returned untouched`() { + val tracks = queue("a" to false, "b" to false, "c" to false) + val out = dropUnavailable(tracks, initialIndex = 1) + assertEquals(tracks, out.tracks) + assertEquals(1, out.initialIndex) + } + + @Test + fun `missing tracks are dropped`() { + val out = dropUnavailable(queue("a" to false, "b" to true, "c" to false), 0) + assertEquals(listOf("a", "c"), out.tracks.map { it.id }) + } + + /** + * The whole reason for the index math: two removals ahead of the target + * would otherwise start playback two tracks early. + */ + @Test + fun `the starting index follows its track past earlier removals`() { + val out = dropUnavailable( + queue("a" to true, "b" to true, "c" to false, "d" to false), + initialIndex = 2, + ) + assertEquals(listOf("c", "d"), out.tracks.map { it.id }) + assertEquals(0, out.initialIndex) + assertEquals("c", out.tracks[out.initialIndex].id) + } + + @Test + fun `removals after the starting index leave it alone`() { + val out = dropUnavailable( + queue("a" to false, "b" to false, "c" to true), + initialIndex = 1, + ) + assertEquals("b", out.tracks[out.initialIndex].id) + } + + /** Tapping a missing track starts the next playable one — it gets skipped. */ + @Test + fun `asking to start on a missing track starts on the next playable one`() { + val out = dropUnavailable( + queue("a" to false, "b" to true, "c" to false), + initialIndex = 1, + ) + assertEquals("c", out.tracks[out.initialIndex].id) + } + + @Test + fun `a missing track at the end cannot push the index out of bounds`() { + val out = dropUnavailable( + queue("a" to false, "b" to false, "c" to true), + initialIndex = 2, + ) + assertTrue(out.initialIndex in out.tracks.indices) + assertEquals("b", out.tracks[out.initialIndex].id) + } + + /** + * The caller treats this as "don't touch the player". Replacing what is + * currently playing with silence, because a stale list turned out to be + * entirely missing, would be worse than ignoring the request. + */ + @Test + fun `an entirely missing queue comes back empty`() { + val out = dropUnavailable(queue("a" to true, "b" to true), 0) + assertTrue(out.tracks.isEmpty()) + assertEquals(0, out.initialIndex) + } + + @Test + fun `an empty queue stays empty`() { + val out = dropUnavailable(emptyList(), 0) + assertTrue(out.tracks.isEmpty()) + } +} diff --git a/internal/api/convert.go b/internal/api/convert.go index a7148825..2ea8b1da 100644 --- a/internal/api/convert.go +++ b/internal/api/convert.go @@ -139,6 +139,7 @@ func trackRefFrom(t dbq.Track, albumTitle, artistName string) TrackRef { ArtistName: artistName, DurationSec: durationMsToSec(t.DurationMs), StreamURL: streamURL(t.ID), + Unavailable: t.MissingSince.Valid, } if t.TrackNumber != nil { ref.TrackNumber = int(*t.TrackNumber) diff --git a/internal/api/library_sync_views.go b/internal/api/library_sync_views.go index 0454149c..1e8520f9 100644 --- a/internal/api/library_sync_views.go +++ b/internal/api/library_sync_views.go @@ -79,6 +79,19 @@ type trackSyncView struct { FilePath string `json:"file_path"` FileFormat string `json:"file_format"` Genre *string `json:"genre"` + // Missing reports that the file is currently absent from disk (#2704). + // + // Shipped as state rather than filtered out of the feed, because a + // missing file is expected to come back: the scanner clears the mark + // when it does, and adopts the row if it returns under a new name + // (#2528). Dropping the row instead would mean a delete-and-recreate on + // every client for what is often a transient unmount, churning caches + // and discarding the identity #2528 works to preserve. + // + // A bool rather than the timestamp: clients need it to decide whether a + // track is playable, which is a yes/no. The "gone since" clock is an + // operator concern and lives on the admin surface. + Missing bool `json:"missing"` } func toTrackSyncView(t dbq.Track) trackSyncView { @@ -93,6 +106,7 @@ func toTrackSyncView(t dbq.Track) trackSyncView { FilePath: t.FilePath, FileFormat: t.FileFormat, Genre: t.Genre, + Missing: t.MissingSince.Valid, } } diff --git a/internal/api/library_sync_views_test.go b/internal/api/library_sync_views_test.go index 146ad665..bd322d23 100644 --- a/internal/api/library_sync_views_test.go +++ b/internal/api/library_sync_views_test.go @@ -3,6 +3,7 @@ package api import ( "encoding/json" "testing" + "time" "github.com/jackc/pgx/v5/pgtype" @@ -75,9 +76,26 @@ func TestTrackSyncView_WireKeys(t *testing.T) { assertJSONKeys(t, "track", b, []string{ "id", "album_id", "artist_id", "title", "duration_ms", "track_number", "disc_number", "file_path", "file_format", "genre", + "missing", }) } +// The mark is what the client filters on, so a wrong value here silently +// re-introduces #2704: a present track marked missing vanishes from the +// client's library, a missing one stays playable and fails at the speaker. +func TestTrackSyncView_MissingReflectsTheMark(t *testing.T) { + present := dbq.Track{ID: validUUID, AlbumID: validUUID, ArtistID: validUUID} + if toTrackSyncView(present).Missing { + t.Error("a track with no missing_since must not be marked missing") + } + + gone := present + gone.MissingSince = pgtype.Timestamptz{Time: time.Now(), Valid: true} + if !toTrackSyncView(gone).Missing { + t.Error("a track with missing_since must be marked missing") + } +} + func TestPlaylistSyncView_WireKeys(t *testing.T) { variant := "discover" p := dbq.Playlist{ diff --git a/internal/api/types.go b/internal/api/types.go index 53e872f6..bcab22cf 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -82,6 +82,16 @@ type TrackRef struct { DiscNumber int `json:"disc_number,omitempty"` DurationSec int `json:"duration_sec"` StreamURL string `json:"stream_url"` + // Unavailable reports that the file is missing from disk (#2704). + // + // Present on every TrackRef rather than only where it can be true: the + // selection surfaces (recommendation, discover, mixes, browse) filter + // missing tracks out entirely, so it is always false there. The surfaces + // that CAN return one are the direct lookups — album detail and search — + // where the user asked for that specific thing by name or container and + // hiding it would be the wrong answer. Marking it lets the client grey + // it and keep it out of a queue. + Unavailable bool `json:"unavailable,omitempty"` } // ArtistDetail is the response body of GET /api/artists/{id}. Embeds the ref