Missing-file lifecycle end to end, UPnP stall recovery, Android browse parity, Flutter client removed #126

Merged
bvandeusen merged 23 commits from dev into main 2026-08-17 16:28:14 -04:00
14 changed files with 231 additions and 6 deletions
Showing only changes of commit 366692a1fc - Show all commits
@@ -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