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