From 26b8963205ea3203dcfc6d51b74caa1ca649d735 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Mon, 28 Sep 2026 08:11:33 +0000 Subject: [PATCH] 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. --- .../com/subtracks/data/download/AudioFile.kt | 22 ++++++++++ .../subtracks/data/repo/DownloadRepository.kt | 24 +++++++++-- .../subtracks/data/download/AudioFileTest.kt | 42 +++++++++++++++++++ .../data/repo/DownloadRepositoryTest.kt | 36 ++++++++++++++++ 4 files changed, 120 insertions(+), 4 deletions(-) create mode 100644 app/src/main/kotlin/com/subtracks/data/download/AudioFile.kt create mode 100644 app/src/test/kotlin/com/subtracks/data/download/AudioFileTest.kt 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) {