From 0fb2ce2918edecc215f1ef2f09e5c08ecd62edb5 Mon Sep 17 00:00:00 2001
From: austinried <4966622+austinried@users.noreply.github.com>
Date: Sat, 26 Sep 2026 01:55:35 +0000
Subject: [PATCH] address review: drop the dead disc query, pin the fetch
 bound, cover multi-page disc prune

---
 .../com/subtracks/data/db/LibraryDao.kt       |  3 --
 .../data/source/subsonic/SubsonicSource.kt    |  3 +-
 .../source/subsonic/SubsonicSourceTest.kt     |  4 +--
 .../subtracks/data/sync/SyncServiceTest.kt    | 34 +++++++++++++++++++
 4 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt b/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt
index 532b9e73..3e17a550 100644
--- a/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt
+++ b/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt
@@ -82,9 +82,6 @@ interface LibraryDao {
         limit: Int,
     ): List<String>
 
-    @Query("SELECT albumId, disc FROM discs WHERE sourceId = :sourceId")
-    suspend fun discKeys(sourceId: Long): List<DiscKey>
-
     @Query(
         "SELECT albumId, disc FROM discs WHERE sourceId = :sourceId " +
             "AND (albumId > :afterAlbumId OR (albumId = :afterAlbumId AND disc > :afterDisc)) " +
diff --git a/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicSource.kt b/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicSource.kt
index 6a1f8086..8dd79656 100644
--- a/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicSource.kt
+++ b/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicSource.kt
@@ -282,11 +282,12 @@ class SubsonicSource(
     private companion object {
         const val PAGE_SIZE = 500
         const val MAX_PAGES = 20_000
-        const val MAX_CONCURRENT_FETCHES = 4
         const val THUMBNAIL_SIZE = 256
     }
 }
 
+internal const val MAX_CONCURRENT_FETCHES = 4
+
 private class PageCapExceeded(
     message: String,
 ) : SubsonicException(-1, message)
diff --git a/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicSourceTest.kt b/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicSourceTest.kt
index 0ce3dbfe..d3d3aca8 100644
--- a/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicSourceTest.kt
+++ b/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicSourceTest.kt
@@ -312,7 +312,7 @@ class SubsonicSourceTest {
                 assertEquals(listOf(0L, 1L, 2L), batch.sortedBy { it.position }.map { it.position })
                 assertEquals((1..3).map { "$playlistId-s$it" }, batch.sortedBy { it.position }.map { it.songId })
             }
-            assertTrue(maxInFlight.get() >= 2)
+            assertTrue(maxInFlight.get() in 2..MAX_CONCURRENT_FETCHES)
         }
 
     @Test
@@ -389,7 +389,7 @@ class SubsonicSourceTest {
             val songs = SubsonicSource(1, client()).songs().toList().flatten()
 
             assertEquals(4, songs.size)
-            assertTrue(maxInFlight.get() >= 2)
+            assertTrue(maxInFlight.get() in 2..MAX_CONCURRENT_FETCHES)
         }
 
     @Test
diff --git a/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt b/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt
index f848c8cd..7bc8e629 100644
--- a/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt
+++ b/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt
@@ -10,6 +10,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
 import com.subtracks.data.db.SubtracksDatabase
 import com.subtracks.data.model.Album
 import com.subtracks.data.model.Artist
+import com.subtracks.data.model.DiscKey
 import com.subtracks.data.model.Playlist
 import com.subtracks.data.model.PlaylistSong
 import com.subtracks.data.model.Song
@@ -542,6 +543,34 @@ class SyncServiceTest {
             assertEquals(survivors.sorted(), db.libraryDao().artistIds(1).sorted())
         }
 
+    @Test
+    fun partialDiscPruneAcrossManyPagesKeepsTheSurvivors() =
+        runTest {
+            insertSource()
+            val total = 1100
+
+            fun albumsWithDiscTitles(predicate: (Int) -> Boolean) =
+                (1..total).map { index ->
+                    if (predicate(index)) {
+                        album("al$index").copy(discTitles = mapOf(1L to "Disc $index"))
+                    } else {
+                        album("al$index")
+                    }
+                }
+            val source = FakeMusicSource(albums = albumsWithDiscTitles { true })
+
+            SyncService(db, source).sync()
+            assertEquals(total, storedDiscKeys().size)
+
+            source.albums = albumsWithDiscTitles { it % 2 == 0 }
+
+            SyncService(db, source).sync()
+
+            val survivors = (1..total).filter { it % 2 == 0 }.map { "al$it" }.toSet()
+            assertEquals(survivors.size, storedDiscKeys().size)
+            assertEquals(survivors, storedDiscKeys().map { it.albumId }.toSet())
+        }
+
     @Test
     fun anIdenticalSyncDoesNotRewriteRows() =
         runTest {
@@ -571,6 +600,11 @@ class SyncServiceTest {
             }
         }
 
+    private suspend fun storedDiscKeys(): List<DiscKey> =
+        db
+            .libraryDao()
+            .discKeysAfter(1, "", 0L, 100_000)
+
     private suspend fun storedDiscs(albumId: String): Map<Long, String> =
         db
             .libraryDao()