address review: count a play even when its scrobble fails

A submitted scrobble now counts locally regardless of the HTTP result
(offline already did), so a transient server error no longer drops the
play from the sorts. Last-played uses MAX(existing, at) so a backwards
clock step cannot move an album or artist back down the list.

app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt

@@ -179,14 +179,20 @@ interface LibraryDao {
         recomputeArtistPlayData(sourceId)
     }
 
-    @Query("UPDATE songs SET playCount = playCount + 1, played = :at WHERE sourceId = :sourceId AND id = :songId")
+    @Query(
+        "UPDATE songs SET playCount = playCount + 1, played = MAX(COALESCE(played, :at), :at) " +
+            "WHERE sourceId = :sourceId AND id = :songId",
+    )
     suspend fun bumpSongPlay(
         sourceId: Long,
         songId: String,
         at: Long,
     )
 
-    @Query("UPDATE albums SET playCount = playCount + 1, played = :at WHERE sourceId = :sourceId AND id = :albumId")
+    @Query(
+        "UPDATE albums SET playCount = playCount + 1, played = MAX(COALESCE(played, :at), :at) " +
+            "WHERE sourceId = :sourceId AND id = :albumId",
+    )
     suspend fun bumpAlbumPlay(
         sourceId: Long,
         albumId: String,
@@ -194,8 +200,8 @@ interface LibraryDao {
     )
 
     @Query(
-        "UPDATE artists SET playCount = playCount + 1, played = :at WHERE sourceId = :sourceId " +
-            "AND id = (SELECT artistId FROM albums WHERE sourceId = :sourceId AND id = :albumId)",
+        "UPDATE artists SET playCount = playCount + 1, played = MAX(COALESCE(played, :at), :at) " +
+            "WHERE sourceId = :sourceId AND id = (SELECT artistId FROM albums WHERE sourceId = :sourceId AND id = :albumId)",
     )
     suspend fun bumpArtistPlay(
         sourceId: Long,

app/src/main/kotlin/com/subtracks/playback/Scrobbler.kt

@@ -126,17 +126,21 @@ class Scrobbler(
     }
 
     private suspend fun submit(event: Scrobble) {
-        try {
-            when (event) {
-                is Scrobble.NowPlaying -> {
-                    sink.nowPlaying(event.songId)
-                }
-
-                is Scrobble.Submission -> {
-                    sink.scrobble(event.songId, event.time)
-                    recordPlay(event.songId, event.time / 1000)
-                }
+        when (event) {
+            is Scrobble.NowPlaying -> {
+                attempt { sink.nowPlaying(event.songId) }
             }
+
+            is Scrobble.Submission -> {
+                attempt { recordPlay(event.songId, event.time / 1000) }
+                attempt { sink.scrobble(event.songId, event.time) }
+            }
+        }
+    }
+
+    private suspend fun attempt(block: suspend () -> Unit) {
+        try {
+            block()
         } catch (cancellation: CancellationException) {
             throw cancellation
         } catch (_: Exception) {

app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt

@@ -230,6 +230,13 @@ class LibraryDaoTest {
             assertEquals(500L, dao.albumOnce(sourceId, "al-1")!!.played)
             assertEquals(1L, dao.artistOnce(sourceId, "ar-1")!!.playCount)
             assertEquals(500L, dao.artistOnce(sourceId, "ar-1")!!.played)
+
+            dao.recordPlay(sourceId, "s1", 100)
+
+            assertEquals(2L, dao.songOnce(sourceId, "s1")!!.playCount)
+            assertEquals(500L, dao.songOnce(sourceId, "s1")!!.played)
+            assertEquals(500L, dao.albumOnce(sourceId, "al-1")!!.played)
+            assertEquals(500L, dao.artistOnce(sourceId, "ar-1")!!.played)
         }
 
     @Test

app/src/test/kotlin/com/subtracks/playback/ScrobblerTest.kt

@@ -12,6 +12,7 @@ import kotlinx.coroutines.test.UnconfinedTestDispatcher
 import kotlinx.coroutines.test.runTest
 import org.junit.Assert.assertEquals
 import org.junit.Test
+import java.io.IOException
 
 @OptIn(ExperimentalCoroutinesApi::class)
 class ScrobblerTest {
@@ -28,6 +29,7 @@ class ScrobblerTest {
 
     private class RecordingSink : ServerActionSink {
         val calls = mutableListOf<Call>()
+        var failScrobble = false
 
         override suspend fun nowPlaying(songId: String) {
             calls += Call.NowPlaying(songId)
@@ -37,6 +39,7 @@ class ScrobblerTest {
             songId: String,
             time: Long,
         ) {
+            if (failScrobble) throw IOException("scrobble failed")
             calls += Call.Scrobble(songId, time)
         }
 
@@ -136,6 +139,35 @@ class ScrobblerTest {
             assertEquals(listOf("s1" to 1L), recorded)
         }
 
+    @Test
+    fun aFailedSubmissionStillRecordsThePlayLocally() =
+        runTest {
+            val sink = RecordingSink().apply { failScrobble = true }
+            val recorded = mutableListOf<Pair<String, Long>>()
+            val scrobbler =
+                Scrobbler(
+                    sink = sink,
+                    enabled = flowOf(true),
+                    scope = testScope(),
+                    policy = ScrobblePolicy(now = { 1_000L }),
+                    recordPlay = { songId, at -> recorded += songId to at },
+                )
+            val state = MutableStateFlow(PlaybackState())
+            val position = MutableStateFlow(0L)
+            scrobbler.attach(state, position)
+            val item = QueueItem("s1", "Title", "Artist", "Album", null, durationMs = 30_000)
+
+            state.value = PlaybackState(item = item, isPlaying = true)
+            var tick = 500L
+            while (tick <= 15_000L) {
+                position.value = tick
+                tick += 500
+            }
+
+            assertEquals(emptyList<Call>(), sink.calls.filterIsInstance<Call.Scrobble>())
+            assertEquals(listOf("s1" to 1L), recorded)
+        }
+
     @Test
     fun disabledSendsNothing() =
         runTest {

docs/architecture.md

@@ -92,7 +92,7 @@ Stream URLs come from `SourceRepository.streamUri`, which asks the active `Subso
 
 The window is also the queue the media notification and Android Auto can browse — a deliberate trade-off, since the alternative is holding an entire large library in memory. The queue view resolves positions lazily too: it loads the queue around the cursor in chunks and extends by a chunk when you scroll near either end, so it never materializes the whole queue. Dragging uses `sh.calvin.reorderable`, which floats the lifted row and reorders the loaded list live (animating with `animateItem`), so editing is what a drag needs: the view reorders its loaded list while dragging and commits one move on drop. `queue_entries` is rewritten to edit: removing a position splits its entry into up to two ranges, and moving one turns the moved track into an explicit song entry inserted at the target index, so an edit costs a couple of rows rather than a copy of the queue. When the edit falls inside the player's window the controller mirrors it with `removeMediaItem`/`moveMediaItem` so playback is not interrupted; removing the playing track, or a move that crosses the window boundary, reloads the window around the cursor. The controller keeps the pre-edit entries and cursor, so the queue view can offer a one-step undo. The up-next block is fully editable in the same view (drag reorder and delete), while the context is read-only while shuffled. Timed lyrics and gapless format preferences are not wired yet.
 
-Scrobbling is a `Scrobbler` that collects the controller's `state` and `positionMs` and drives the Subsonic `scrobble` endpoint: a now-playing notification (`submission=false`) when a track starts playing, and one submission (`submission=true`, with the playback start time) when the track passes half its duration or four minutes, whichever comes first. That split is the endpoint's own `submission` flag, and the threshold is the Last.fm rule the server forwards to; servers such as Navidrome trust the client and apply no threshold themselves. Tracks under 30 seconds are never submitted, and only time actually played counts: forward and backward seeks are ignored, and a rewind only starts a fresh listen when it lands in the track's first few seconds (a repeat or an explicit restart). The duration is the player's resolved value, falling back to the library row when the player has not reported one. Sending is decoupled from the collector so a slow or offline write cannot stall position tracking, while a lock keeps the writes in order. The policy is a pure state machine with no Android dependencies. A Settings toggle (`UserPreferences.scrobbling`, on by default) gates both calls. A submission also bumps the played song's local `playCount`/`played` and its album and artist ranks, so the play sorts move before the next sync; the sync then overwrites them with the server's own numbers.
+Scrobbling is a `Scrobbler` that collects the controller's `state` and `positionMs` and drives the Subsonic `scrobble` endpoint: a now-playing notification (`submission=false`) when a track starts playing, and one submission (`submission=true`, with the playback start time) when the track passes half its duration or four minutes, whichever comes first. That split is the endpoint's own `submission` flag, and the threshold is the Last.fm rule the server forwards to; servers such as Navidrome trust the client and apply no threshold themselves. Tracks under 30 seconds are never submitted, and only time actually played counts: forward and backward seeks are ignored, and a rewind only starts a fresh listen when it lands in the track's first few seconds (a repeat or an explicit restart). The duration is the player's resolved value, falling back to the library row when the player has not reported one. Sending is decoupled from the collector so a slow or offline write cannot stall position tracking, while a lock keeps the writes in order. The policy is a pure state machine with no Android dependencies. A Settings toggle (`UserPreferences.scrobbling`, on by default) gates both calls. A submission also bumps the played song's local `playCount`/`played` and its album and artist ranks, so the play sorts move before the next sync; the sync then overwrites them with the server's own numbers, so a play the server never accepted is rolled back at that point.
 
 Server writes — now-playing, scrobbles and stars — go through a `ServerActionSink`. The only implementation today is `NetworkServerActionSink`, which resolves the active source and calls it; a persistent, ordered outbox can replace it later so plays and stars taken offline are queued and replayed when the server is reachable. Now-playing is deliberately not durable (Last.fm says failed now-playing requests must not be retried), while submissions and stars carry everything a queue would need (the song or entity id and, for a play, the time it was listened).