fail a download whose payload is a server error page

A Subsonic server answers a refused download with HTTP 200 and an XML error
document, which the platform downloader saves like any payload, so the row went
Completed and playback then tried to decode the error page. No audio container
starts with markup, so the leading bytes tell them apart: a Completed download
that starts with < or { is failed instead, and a stored Completed row is
re-checked on reconcile so existing bad files get cleaned up too.

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

@@ -0,0 +1,22 @@
+package com.subtracks.data.download
+
+import java.io.File
+
+/**
+ * A Subsonic server answers a bad or refused download with HTTP 200 and an XML error document, which
+ * the platform downloader stores like any other payload, so a "successful" download can be an error
+ * page. No audio container starts with markup, so the leading bytes tell them apart.
+ */
+internal fun File.looksLikeAudio(): Boolean =
+    runCatching {
+        val head = ByteArray(HEAD_BYTES)
+        val read = inputStream().use { it.read(head) }
+        var start = 0
+        if (read >= 3 && head[0] == 0xEF.toByte() && head[1] == 0xBB.toByte() && head[2] == 0xBF.toByte()) start = 3
+        while (start < read && head[start].toInt().toChar().isWhitespace()) start++
+        start < read && head[start] != MARKUP_START && head[start] != JSON_START
+    }.getOrDefault(false)
+
+private const val HEAD_BYTES = 16
+private val MARKUP_START = '<'.code.toByte()
+private val JSON_START = '{'.code.toByte()

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

@@ -8,6 +8,7 @@ import com.subtracks.data.download.DownloadEngine
 import com.subtracks.data.download.EngineDownload
 import com.subtracks.data.download.EngineRequest
 import com.subtracks.data.download.EngineStatus
+import com.subtracks.data.download.looksLikeAudio
 import com.subtracks.data.model.BulkDownloadAction
 import com.subtracks.data.model.DownloadList
 import com.subtracks.data.model.DownloadStatus
@@ -425,7 +426,11 @@ class DownloadRepository(
     private suspend fun recover(row: SongDownload) {
         when (row.status) {
             DownloadStatus.Completed -> {
-                if (!file(row.sourceId, row.songId).exists()) markFailed(row, MISSING_FILE)
+                val file = file(row.sourceId, row.songId)
+                when {
+                    !file.exists() -> markFailed(row, MISSING_FILE)
+                    !file.looksLikeAudio() -> markFailed(row, SERVER_ERROR)
+                }
             }
 
             // Waiting rows have no engine id by design; the next promote hands them over.
@@ -510,20 +515,30 @@ class DownloadRepository(
         row: SongDownload,
         state: EngineDownload,
     ) {
+        val file = file(row.sourceId, row.songId)
+        // The engine reports success for any 200, including the XML error document a Subsonic server
+        // returns for a refused track; only the payload itself says whether it is audio.
+        val errorPage = state.status == EngineStatus.Completed && !file.looksLikeAudio()
         val status =
             when (state.status) {
                 EngineStatus.Pending -> DownloadStatus.Queued
                 EngineStatus.Running -> DownloadStatus.Running
-                EngineStatus.Completed -> if (file(row.sourceId, row.songId).exists()) DownloadStatus.Completed else DownloadStatus.Failed
+                EngineStatus.Completed -> if (errorPage) DownloadStatus.Failed else DownloadStatus.Completed
                 EngineStatus.Failed -> DownloadStatus.Failed
             }
-        if (status == DownloadStatus.Failed) file(row.sourceId, row.songId).delete()
+        if (status == DownloadStatus.Failed) file.delete()
         db.downloadDao().upsert(
             row.copy(
                 status = status,
                 bytes = state.bytes,
                 total = state.total,
-                error = state.error ?: if (status == DownloadStatus.Failed) "Download failed" else null,
+                error =
+                    when {
+                        state.error != null -> state.error
+                        errorPage -> SERVER_ERROR
+                        status == DownloadStatus.Failed -> "Download failed"
+                        else -> null
+                    },
             ),
         )
     }
@@ -603,6 +618,7 @@ class DownloadRepository(
         const val IN_FLIGHT_LIMIT = 8
         const val DELETE_CHUNK = 900
         const val MISSING_FILE = "The downloaded file is missing"
+        const val SERVER_ERROR = "The server sent an error instead of the track"
         const val NO_ADDRESS = "Can't download: the server address is unavailable"
         const val EMPTY_LIST = "Nothing left to download from this list"
         const val STOPPED = "The download stopped unexpectedly"

app/src/test/kotlin/com/subtracks/data/download/AudioFileTest.kt

@@ -0,0 +1,42 @@
+package com.subtracks.data.download
+
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Test
+import java.io.File
+
+class AudioFileTest {
+    @Test
+    fun anXmlErrorDocumentIsNotAudio() {
+        assertFalse(file("<?xml version=\"1.0\"?><subsonic-response status=\"failed\"/>").looksLikeAudio())
+    }
+
+    @Test
+    fun leadingWhitespaceAndABomDoNotHideMarkup() {
+        assertFalse(file("\uFEFF\n  <html><body>Gateway timeout</body></html>").looksLikeAudio())
+    }
+
+    @Test
+    fun aJsonErrorBodyIsNotAudio() {
+        assertFalse(file("{\"subsonic-response\":{\"status\":\"failed\"}}").looksLikeAudio())
+    }
+
+    @Test
+    fun anEmptyFileIsNotAudio() {
+        assertFalse(file("").looksLikeAudio())
+    }
+
+    @Test
+    fun aMissingFileIsNotAudio() {
+        assertFalse(File.createTempFile("absent", ".tmp").apply { delete() }.looksLikeAudio())
+    }
+
+    @Test
+    fun audioHeadersPass() {
+        assertTrue(file("audio").looksLikeAudio())
+        assertTrue(file("ID3\u0004\u0000audio").looksLikeAudio())
+        assertTrue(file("fLaC\u0000\u0000\u0000\u0022").looksLikeAudio())
+    }
+
+    private fun file(contents: String): File = File.createTempFile("audio", ".tmp").apply { writeText(contents) }.also { it.deleteOnExit() }
+}

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

@@ -920,6 +920,42 @@ class DownloadRepositoryTest {
             await { row(2, "o1")?.status == DownloadStatus.Completed }
         }
 
+    @Test
+    fun aServerErrorSavedAsThePayloadIsNotTreatedAsADownload() =
+        runTest {
+            seedLibrary()
+            repository.start()
+            runBlocking { repository.download(1, "s1") }
+            val id = engine.requests.single().first
+
+            engine.complete(id, total = 100)
+            writeFile(
+                1,
+                "s1",
+                "<?xml version=\"1.0\"?><subsonic-response status=\"failed\"><error code=\"70\"/></subsonic-response>",
+            )
+            runBlocking { repository.reconcile() }
+
+            assertEquals(DownloadStatus.Failed, row(1, "s1")?.status)
+            assertEquals("The server sent an error instead of the track", row(1, "s1")?.error)
+            assertFalse(file(1, "s1").exists())
+            assertNull(repository.localUri("s1"))
+        }
+
+    @Test
+    fun aCompletedRowLeftAsAnErrorPageIsFailedOnReconcile() =
+        runTest {
+            seedLibrary()
+            runBlocking { db.downloadDao().upsert(SongDownload(1, "s1", DownloadStatus.Completed, bytes = 10, total = 10)) }
+            writeFile(1, "s1", "<subsonic-response status=\"failed\"/>")
+
+            runBlocking { repository.reconcile() }
+
+            assertEquals(DownloadStatus.Failed, row(1, "s1")?.status)
+            assertFalse(file(1, "s1").exists())
+            assertNull(repository.localUri("s1"))
+        }
+
     private fun requestedSongIds(): List<String> = engine.requests.map { it.second.path.substringAfterLast('/') }
 
     private fun seedSongs(tracks: IntRange) {