fix: stop the sync feed hiding missing files from clients — #2704
test-go / test (push) Successful in 2m3s
android / Build + lint + test (push) Failing after 4m24s
test-go / integration (push) Successful in 5m8s

#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.
This commit is contained in:
2026-08-17 12:56:31 -04:00
parent 6d729d1512
commit 366692a1fc
14 changed files with 231 additions and 6 deletions
@@ -57,6 +57,14 @@ class ShuffleSource @Inject constructor(
private suspend fun materialize(orderedIds: List<String>): List<TrackRef> {
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)
}
}
}
@@ -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)
@@ -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(),
)
@@ -219,4 +219,5 @@ private fun SyncTrackWire.toEntity(): CachedTrackEntity = CachedTrackEntity(
filePath = filePath,
fileFormat = fileFormat,
genre = genre,
missing = missing,
)
@@ -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 =
@@ -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`
@@ -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,
)
/**
@@ -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,
)
@@ -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<TrackRef>, 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<TrackRef>, initialIndex: Int): PlayableQueue {
if (tracks.none { it.unavailable }) return PlayableQueue(tracks, initialIndex)
val kept = ArrayList<TrackRef>(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)))
}
@@ -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<String, Boolean>) =
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())
}
}
+1
View File
@@ -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)
+14
View File
@@ -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,
}
}
+18
View File
@@ -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{
+10
View File
@@ -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