diff --git a/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt b/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt index 3e782b0a..e486f858 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt @@ -167,8 +167,18 @@ class DownloadRepository( } // Under one lock, so a cancel that lands mid-loop waits for the queue to be complete and // then removes all of it, rather than missing the songs enqueued after it looked. + // ponytail: the lock is held for the whole loop, so cancel latency and the poll tick scale + // with the list length; a per-list cancel generation would lift that if it ever bites. mutex.withLock { - songIds.forEach { songId -> runCatching { enqueueIfAbsentLocked(sourceId, songId) } } + songIds.forEach { songId -> + try { + enqueueIfAbsentLocked(sourceId, songId) + } catch (e: CancellationException) { + throw e + } catch (_: Exception) { + // One song failing to enqueue must not abandon the rest of the list. + } + } } fetchArtworkSafely(sourceId, songIds) } @@ -398,7 +408,12 @@ class DownloadRepository( songIds: List, ) { try { - withTimeoutOrNull(ART_TIMEOUT_MS) { songIds.forEach { fetchArtwork(sourceId, it) } } + withTimeoutOrNull(ART_TIMEOUT_MS) { + songIds.forEach { songId -> + // A cancel between the queue and this pass takes the row away; nothing to warm. + if (db.downloadDao().find(sourceId, songId) != null) fetchArtwork(sourceId, songId) + } + } } catch (e: CancellationException) { throw e } catch (_: Exception) { diff --git a/app/src/test/kotlin/com/subtracks/data/download/FakeDownloadEngine.kt b/app/src/test/kotlin/com/subtracks/data/download/FakeDownloadEngine.kt index 744d3dbb..cef35dbc 100644 --- a/app/src/test/kotlin/com/subtracks/data/download/FakeDownloadEngine.kt +++ b/app/src/test/kotlin/com/subtracks/data/download/FakeDownloadEngine.kt @@ -6,8 +6,13 @@ class FakeDownloadEngine : DownloadEngine { val requests = mutableListOf>() val cancelled = mutableListOf() + var enqueueDelayMs = 0L + var failEnqueueFor: String? = null override fun enqueue(request: EngineRequest): Long { + if (enqueueDelayMs > 0) Thread.sleep(enqueueDelayMs) + val failing = failEnqueueFor + if (failing != null && request.path.endsWith(failing)) throw IllegalStateException("enqueue failed") val id = nextId++ requests += id to request downloads[id] = EngineDownload(EngineStatus.Pending) diff --git a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt index 350925de..aab42a33 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt @@ -24,7 +24,9 @@ import com.subtracks.data.model.Source import com.subtracks.data.model.SubsonicSource import com.subtracks.data.model.coverArtKey import com.subtracks.data.prefs.fakeUserPreferences +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.first +import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.runTest import okhttp3.OkHttpClient @@ -501,15 +503,7 @@ class DownloadRepositoryTest { fun thePlaylistStatusCountsASongThatAppearsTwiceOnce() = runTest { seedLibrary() - runBlocking { - db.libraryDao().upsertPlaylists(listOf(Playlist(1, "pl1", "Playlist", null, null, 2, 0))) - db.libraryDao().upsertPlaylistSongs( - listOf( - PlaylistSong(1, "pl1", "s1", 0), - PlaylistSong(1, "pl1", "s1", 1), - ), - ) - } + seedPlaylist(listOf("s1", "s1")) assertEquals(ListDownloadStatus(total = 1, downloaded = 0, downloading = 0), statusOf(DownloadList.Playlist, "pl1")) } @@ -523,15 +517,7 @@ class DownloadRepositoryTest { fun downloadingAPlaylistQueuesEverySong() = runTest { seedLibrary() - runBlocking { - db.libraryDao().upsertPlaylists(listOf(Playlist(1, "pl1", "Playlist", null, null, 2, 0))) - db.libraryDao().upsertPlaylistSongs( - listOf( - PlaylistSong(1, "pl1", "s1", 0), - PlaylistSong(1, "pl1", "s2", 1), - ), - ) - } + seedPlaylist(listOf("s1", "s2")) runBlocking { repository.downloadAll(1, DownloadList.Playlist, "pl1") } @@ -570,6 +556,43 @@ class DownloadRepositoryTest { assertEquals(listOf("s1", "s2"), requestedSongIds()) } + @Test + fun cancellingWhileAListIsStillQueuingRemovesTheWholeList() = + runTest { + seedLibrary() + seedSongs(3..40) + engine.enqueueDelayMs = 1 + + val queuing = launch(Dispatchers.IO) { repository.downloadAll(1, DownloadList.Album, "al1") } + await { engine.requests.size >= 3 } + val cancelling = launch(Dispatchers.IO) { repository.cancelAll(1, DownloadList.Album, "al1") } + queuing.join() + cancelling.join() + + assertTrue("expected the whole list cancelled, ${db.downloadDao().all().size} rows left", db.downloadDao().all().isEmpty()) + } + + @Test + fun oneSongFailingToQueueDoesNotAbandonTheRest() = + runTest { + seedLibrary() + engine.failEnqueueFor = "s1" + + runBlocking { repository.downloadAll(1, DownloadList.Album, "al1") } + + assertEquals(listOf("s2"), requestedSongIds()) + } + + @Test + fun downloadingAListWithNothingInItSaysSo() = + runTest { + seedLibrary() + + runBlocking { repository.downloadAll(1, DownloadList.Album, "missing-album") } + + assertEquals(listOf("Nothing left to download from this list"), messages.toList()) + } + @Test fun applyActionRoutesToTheMatchingBulkAction() = runTest { @@ -581,10 +604,49 @@ class DownloadRepositoryTest { runBlocking { repository.applyAction(1, DownloadList.Album, "al1", BulkDownloadAction.Cancel) } assertEquals(ListDownloadStatus(total = 2), statusOf(DownloadList.Album, "al1")) + + runBlocking { repository.applyAction(1, DownloadList.Album, "al1", BulkDownloadAction.Download) } + runBlocking { db.downloadDao().upsert(SongDownload(1, "s2", DownloadStatus.Completed, engineId = 22)) } + runBlocking { repository.applyAction(1, DownloadList.Album, "al1", BulkDownloadAction.Delete) } + + assertNotNull(row(1, "s1")) + assertNull(row(1, "s2")) } private fun requestedSongIds(): List = engine.requests.map { it.second.path.substringAfterLast('/') } + private fun seedSongs(tracks: IntRange) { + runBlocking { + db.libraryDao().upsertSongs( + tracks.map { track -> + Song( + sourceId = 1, + id = "s$track", + albumId = "al1", + artistId = "ar1", + title = "Song $track", + album = "Album", + artist = "Artist", + duration = 100, + track = track.toLong(), + disc = 1, + starred = null, + genre = null, + ) + }, + ) + } + } + + private fun seedPlaylist(songIds: List) { + runBlocking { + db.libraryDao().upsertPlaylists(listOf(Playlist(1, "pl1", "Playlist", null, null, songIds.size.toLong(), 0))) + db.libraryDao().upsertPlaylistSongs( + songIds.mapIndexed { index, songId -> PlaylistSong(1, "pl1", songId, index.toLong()) }, + ) + } + } + private fun album(coverArt: String?) = Album( sourceId = 1,