start the download before fetching its artwork

- The artwork step no longer holds up the media: the song goes to the platform
  first and the art is fetched afterwards, in the same foreground window. A
  stalled artwork server can no longer delay a download, and a process death
  during the fetch can no longer lose the download entirely.
- Resuming a queued download after a restart fetches its artwork now too; only a
  fresh download did before.
- The artwork budget is a minute rather than ten seconds, which costs nothing but
  a live coroutine now that nothing waits on it for the media.
- OkHttpArtworkFetcher derives a client with a per-call timeout. A blocking
  OkHttp call is not interrupted by a coroutine timeout, so without it the outer
  budget bounded our wait but not the socket.

app/src/main/kotlin/com/subtracks/data/download/ArtworkStore.kt

@@ -8,6 +8,7 @@ import okhttp3.Request
 import java.io.File
 import java.io.IOException
 import java.util.Base64
+import java.util.concurrent.TimeUnit
 
 /**
  * Cover art downloaded alongside the media, stored per source under `downloads/<sourceId>/art`.
@@ -68,8 +69,12 @@ fun interface ArtworkFetcher {
 }
 
 class OkHttpArtworkFetcher(
-    private val http: OkHttpClient,
+    http: OkHttpClient,
 ) : ArtworkFetcher {
+    // A blocking OkHttp call is not interrupted when the coroutine around it is cancelled, so the
+    // per-call timeout is what actually bounds a stuck artwork request.
+    private val http = http.newBuilder().callTimeout(CALL_TIMEOUT_SECONDS, TimeUnit.SECONDS).build()
+
     override suspend fun fetch(url: String): ByteArray =
         withContext(Dispatchers.IO) {
             val request = Request.Builder().url(url).build()
@@ -78,4 +83,8 @@ class OkHttpArtworkFetcher(
                 response.body?.bytes() ?: throw IOException("Artwork response had no body")
             }
         }
+
+    private companion object {
+        const val CALL_TIMEOUT_SECONDS = 15L
+    }
 }

app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt

@@ -99,17 +99,24 @@ class DownloadRepository(
             showMessage(NO_ADDRESS)
             return@withContext
         }
-        // Artwork runs in our process and is fetched while the app is in the foreground; the
+        if (!enqueueIfAbsent(sourceId, songId)) return@withContext
-        // media itself is handed to the platform, which keeps going without us.
+        // The media is already with the platform, so this only has to happen while we are still
-        runCatching { withTimeoutOrNull(ART_TIMEOUT_MS) { fetchArtwork(sourceId, songId) } }
+        // in the foreground: artwork is fetched by us, and it must not hold up the download.
+        fetchArtworkSafely(sourceId, songId)
+    }
+
+    private suspend fun enqueueIfAbsent(
+        sourceId: Long,
+        songId: String,
+    ): Boolean =
         mutex.withLock {
-            if (alreadyDownloadingOrDownloaded(sourceId, songId)) return@withLock
+            if (alreadyDownloadingOrDownloaded(sourceId, songId)) return@withLock false
             val existing = db.downloadDao().find(sourceId, songId)
             existing?.engineId?.let(engine::cancel)
-            val url = sourceRepository.downloadUri(sourceId, songId) ?: return@withLock
+            val url = sourceRepository.downloadUri(sourceId, songId) ?: return@withLock false
             enqueue(sourceId, songId, url)
+            true
         }
-    }
 
     suspend fun remove(
         sourceId: Long,
@@ -139,14 +146,14 @@ class DownloadRepository(
 
     suspend fun reconcile() =
         withContext(dispatcher) {
-            mutex.withLock {
+            val resumed =
-                db.downloadDao().all().forEach { row ->
+                mutex.withLock {
-                    val state = row.engineId?.let(engine::download)
+                    val resumed = reconcileRows(db.downloadDao().all())
-                    if (state == null) recover(row) else apply(row, state)
+                    sweep(db.downloadDao().all())
+                    ensurePolling()
+                    resumed
                 }
-                sweep(db.downloadDao().all())
+            resumed.forEach { fetchArtworkSafely(it.sourceId, it.songId) }
-                ensurePolling()
-            }
         }
 
     private fun downloadStates(): Flow<Map<String, SongDownload>> =
@@ -224,26 +231,48 @@ class DownloadRepository(
         ensurePolling()
     }
 
-    private suspend fun recover(row: SongDownload) {
+    private suspend fun recover(row: SongDownload): Boolean =
         when (row.status) {
             DownloadStatus.Completed -> {
                 if (!file(row.sourceId, row.songId).exists()) markFailed(row, MISSING_FILE)
+                false
             }
 
             DownloadStatus.Queued -> {
-                if (sourceRepository.activeSourceIdOnce() != row.sourceId) return
+                if (sourceRepository.activeSourceIdOnce() != row.sourceId) return false
-                val url = sourceRepository.downloadUri(row.sourceId, row.songId) ?: return
+                val url = sourceRepository.downloadUri(row.sourceId, row.songId) ?: return false
                 enqueue(row.sourceId, row.songId, url)
+                true
             }
 
             DownloadStatus.Running -> {
                 markFailed(row, STOPPED)
+                false
             }
 
             DownloadStatus.Failed -> {
-                Unit
+                false
             }
         }
+
+    private suspend fun reconcileRows(rows: List<SongDownload>): List<SongDownload> {
+        val resumed = ArrayList<SongDownload>()
+        rows.forEach { row ->
+            val state = row.engineId?.let(engine::download)
+            if (state == null) {
+                if (recover(row)) resumed += row
+            } else {
+                apply(row, state)
+            }
+        }
+        return resumed
+    }
+
+    private suspend fun fetchArtworkSafely(
+        sourceId: Long,
+        songId: String,
+    ) {
+        runCatching { withTimeoutOrNull(ART_TIMEOUT_MS) { fetchArtwork(sourceId, songId) } }
     }
 
     private suspend fun apply(
@@ -283,15 +312,14 @@ class DownloadRepository(
             scope.launch {
                 while (true) {
                     delay(POLL_MS)
-                    mutex.withLock {
+                    val resumed =
-                        val sourceId = sourceRepository.activeSourceIdOnce()
+                        mutex.withLock {
-                        val active = db.downloadDao().all().filter { it.status.isActive && it.sourceId == sourceId }
+                            val sourceId = sourceRepository.activeSourceIdOnce()
-                        if (active.isEmpty()) return@launch
+                            val active = db.downloadDao().all().filter { it.status.isActive && it.sourceId == sourceId }
-                        active.forEach { row ->
+                            if (active.isEmpty()) return@launch
-                            val state = row.engineId?.let(engine::download)
+                            reconcileRows(active)
-                            if (state == null) recover(row) else apply(row, state)
                         }
-                    }
+                    resumed.forEach { fetchArtworkSafely(it.sourceId, it.songId) }
                 }
             }
     }
@@ -331,6 +359,6 @@ class DownloadRepository(
         const val NO_ADDRESS = "Can't download: the server address is unavailable"
         const val STOPPED = "The download stopped unexpectedly"
         const val NOT_ACTIVE = "Can't download from a source that isn't active"
-        const val ART_TIMEOUT_MS = 10_000L
+        const val ART_TIMEOUT_MS = 60_000L
     }
 }

app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt

@@ -46,10 +46,12 @@ class DownloadRepositoryTest {
     private lateinit var dir: File
     private val messages = CopyOnWriteArrayList<String>()
     private val requestedArt = CopyOnWriteArrayList<String>()
+    private val artFetchedAfterTheEngineRequest = CopyOnWriteArrayList<Boolean>()
     private var failArtworkFetch = false
     private val fetcher =
         ArtworkFetcher { url ->
             requestedArt += url
+            artFetchedAfterTheEngineRequest += engine.requests.isNotEmpty()
             if (failArtworkFetch) throw IOException("boom")
             byteArrayOf(1, 2, 3)
         }
@@ -312,6 +314,29 @@ class DownloadRepositoryTest {
             }
         }
 
+    @Test
+    fun theDownloadStartsBeforeItsArtworkIsFetched() =
+        runTest {
+            seedLibrary()
+
+            runBlocking { repository.download(1, "s1") }
+
+            assertTrue(artFetchedAfterTheEngineRequest.isNotEmpty())
+            assertTrue(artFetchedAfterTheEngineRequest.all { it })
+        }
+
+    @Test
+    fun resumingAQueuedDownloadAlsoStoresItsArtwork() =
+        runTest {
+            seedLibrary()
+            db.downloadDao().upsert(SongDownload(1, "s1", DownloadStatus.Queued, engineId = 7))
+
+            runBlocking { repository.reconcile() }
+
+            assertEquals(1, engine.requests.size)
+            assertNotNull(artwork.uri(1, coverArtKey(1, ALBUM_ART, false)))
+        }
+
     @Test
     fun aSongWithoutArtworkStillDownloads() =
         runTest {