diff --git a/app/src/main/kotlin/com/subtracks/data/download/AudioFile.kt b/app/src/main/kotlin/com/subtracks/data/download/AudioFile.kt new file mode 100644 index 00000000..913e7e69 --- /dev/null +++ b/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() 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 478a826a..970e7fda 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt +++ b/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" diff --git a/app/src/test/kotlin/com/subtracks/data/download/AudioFileTest.kt b/app/src/test/kotlin/com/subtracks/data/download/AudioFileTest.kt new file mode 100644 index 00000000..8fa17026 --- /dev/null +++ b/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("").looksLikeAudio()) + } + + @Test + fun leadingWhitespaceAndABomDoNotHideMarkup() { + assertFalse(file("\uFEFF\n Gateway timeout").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() } +} 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 4d537d89..25d81b52 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt +++ b/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", + "", + ) + 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", "") + + runBlocking { repository.reconcile() } + + assertEquals(DownloadStatus.Failed, row(1, "s1")?.status) + assertFalse(file(1, "s1").exists()) + assertNull(repository.localUri("s1")) + } + private fun requestedSongIds(): List = engine.requests.map { it.second.path.substringAfterLast('/') } private fun seedSongs(tracks: IntRange) {