From 17c1ea40a4c50cfd3eb79cdf216821a7fbef8c57 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Sun, 27 Sep 2026 11:12:05 +0000 Subject: [PATCH] test the bulk cancel race, and keep cancellation working Second review round on the bulk downloads: - The cancel-during-bulk fix had no test, so the invariant it exists for could regress silently. The fake engine can now delay an enqueue, which holds the race window open: the queue loop is kept busy, a cancel is fired mid-queue, and the test asserts the whole list ends up cancelled. Reverting to per-song locking fails it, which I checked. - The per-song catch around the queue loop swallowed CancellationException, so cancelling the caller did not stop the loop. It rethrows now. - The artwork pass skips songs whose row a cancel has already taken. - The lock-hold ceiling is recorded where the lock is taken: cancel latency and the poll tick both scale with the list length, and a per-list cancel generation would lift that if it ever bites. - Tests: an empty list says so, one song failing to enqueue leaves the rest of the list alone, the applyAction test covers Delete as well as Download and Cancel, and the two playlist fixtures share a helper. --- .../subtracks/data/repo/DownloadRepository.kt | 19 +++- .../data/download/FakeDownloadEngine.kt | 5 + .../data/repo/DownloadRepositoryTest.kt | 98 +++++++++++++++---- 3 files changed, 102 insertions(+), 20 deletions(-) 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,