From 6c4d40ef9b0556a9071a3cfbcfa58878433e8996 Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Fri, 16 Oct 2026 08:11:41 +0000 Subject: Refresh skips on feed refresh, without stamping server offsets onto publisher audio A feed refresh never re-asked the skip server for already-downloaded episodes, so detection improvements only ever reached a show by deleting and redownloading it. Fixing that safely needs to know which episodes are safe to re-query: a show using dynamic ad insertion serves a different file on every request, so a server-measured offset only applies to the server's own copy, not whatever the publisher fallback left on disk. - EpisodeEntity.audioFromServer (schema 13->14, MIGRATION_13_14) records which source actually landed; EpisodeDownloader sets it, defaulting to false everywhere else so a requeue, retention reclaim or failed re-download clears a stale claim the same way they already clear localPath. - New SkipRefresher re-queries downloaded, server-sourced episodes via the existing ServerSkips.resolve/record pair, one episode's failure not blocking the rest. Wired into SubscriptionRepository.refreshAll via a new refreshSkips collaborator -- the choke point every refresh route passes through, including the library's refresh button. - SkipSummary's episode screen readout gets a distinct line for a download that can never be refreshed in place, instead of the misleading "Not checked yet". Migration verified on-device (gradle-verify connectedDebugAndroidTest, MigrationTest: 14/14). Full connected suite: 31/31. Unit suite: 682/682. detekt/lint clean; coverage 93.86% against a 93.85% baseline. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B --- .../java/org/terst/listen/data/MigrationTest.kt | 38 +++++ .../main/java/org/terst/listen/data/AppDatabase.kt | 23 ++- app/src/main/java/org/terst/listen/data/Daos.kt | 22 ++- .../main/java/org/terst/listen/data/Entities.kt | 14 ++ .../java/org/terst/listen/data/Repositories.kt | 17 +- .../terst/listen/data/SubscriptionRepository.kt | 19 ++- .../org/terst/listen/download/EpisodeDownloader.kt | 30 ++-- .../org/terst/listen/fingerprint/SkipSummary.kt | 17 ++ .../java/org/terst/listen/server/SkipRefresher.kt | 39 +++++ .../test/java/org/terst/listen/data/FakeDaos.kt | 13 +- .../listen/data/SubscriptionRepositoryTest.kt | 24 +++ .../terst/listen/download/EpisodeDownloaderTest.kt | 87 ++++++++++ .../terst/listen/fingerprint/SkipSummaryTest.kt | 48 ++++++ .../org/terst/listen/server/SkipRefresherTest.kt | 176 +++++++++++++++++++++ 14 files changed, 551 insertions(+), 16 deletions(-) create mode 100644 app/src/main/java/org/terst/listen/server/SkipRefresher.kt create mode 100644 app/src/test/java/org/terst/listen/server/SkipRefresherTest.kt (limited to 'app/src') diff --git a/app/src/androidTest/java/org/terst/listen/data/MigrationTest.kt b/app/src/androidTest/java/org/terst/listen/data/MigrationTest.kt index fa070de..5352db8 100644 --- a/app/src/androidTest/java/org/terst/listen/data/MigrationTest.kt +++ b/app/src/androidTest/java/org/terst/listen/data/MigrationTest.kt @@ -557,4 +557,42 @@ class MigrationTest { assertEquals("ad", it.getString(2)) } } + + /** + * The new column has to arrive as "not from the server" for every existing download. A + * download from before this column existed could have come from either source, and the + * conservative answer for "we don't know" is the one that refuses to refresh it against + * the server's timestamps -- the opposite default would risk stamping a server-measured + * offset onto a publisher's copy of a dynamically-ad-inserted show. + */ + @Test + fun migrate13To14DefaultsExistingDownloadsToNotFromTheServer() { + helper.createDatabase(name, 13).apply { + execSQL( + """INSERT INTO subscriptions + (id, feedUrl, kind, title, addedAtMs, skipIntroMs, skipOutroMs, dripEnabled, + dripIntervalMs, dripCount, dripOldestFirst, skipAdChapters, bedtime) + VALUES ('s1', 'https://example.com/feed.xml', 'PODCAST', 'A Show', 1000, 0, 0, + 0, 86400000, 1, 1, 1, 0)""" + ) + execSQL( + """INSERT INTO episodes + (id, subscriptionId, guid, title, mediaUrl, mediaType, discoveredAtMs, + positionMs, downloadState, localPath) + VALUES ('e1', 's1', 'g1', 'One', 'https://cdn/1.mp3', 'audio/mpeg', 1000, + 271828, 'DOWNLOADED', '/audio/e1.mp3')""" + ) + close() + } + + val db = helper.runMigrationsAndValidate(name, 14, true, AppDatabase.MIGRATION_13_14) + + db.query("SELECT audioFromServer, positionMs, localPath FROM episodes WHERE id = 'e1'").use { + assertEquals(1, it.count) + it.moveToFirst() + assertEquals("an existing download must not be assumed to be the server's", 0, it.getInt(0)) + assertEquals("the crown jewel must survive", 271828L, it.getLong(1)) + assertEquals("/audio/e1.mp3", it.getString(2)) + } + } } diff --git a/app/src/main/java/org/terst/listen/data/AppDatabase.kt b/app/src/main/java/org/terst/listen/data/AppDatabase.kt index 6bc7df4..73fa38c 100644 --- a/app/src/main/java/org/terst/listen/data/AppDatabase.kt +++ b/app/src/main/java/org/terst/listen/data/AppDatabase.kt @@ -30,7 +30,7 @@ class Converters { ShowMidrollEntity::class, EpisodeMidrollEntity::class, ], - version = 13, + version = 14, exportSchema = true, ) @TypeConverters(Converters::class) @@ -359,6 +359,25 @@ abstract class AppDatabase : RoomDatabase() { } } + /** + * Records whether a downloaded episode's audio came from the skip server, as opposed to + * the publisher. + * + * Load-bearing: a show using dynamic ad insertion serves a different file on every + * request, so an offset the server measured only means anything against its own copy. + * Existing rows default to 0 -- not the server's -- because a download from before this + * column existed could have come from either source and "we don't know" has to resolve + * to the conservative answer, not an optimistic one that would let a refresh stamp a + * server offset onto a publisher's file. + */ + val MIGRATION_13_14 = object : Migration(13, 14) { + override fun migrate(db: SupportSQLiteDatabase) { + db.execSQL( + "ALTER TABLE `episodes` ADD COLUMN `audioFromServer` INTEGER NOT NULL DEFAULT 0" + ) + } + } + fun get(context: Context): AppDatabase = instance ?: synchronized(this) { instance ?: build(context.applicationContext).also { instance = it } } @@ -370,7 +389,7 @@ abstract class AppDatabase : RoomDatabase() { .addMigrations( MIGRATION_1_2, MIGRATION_2_3, MIGRATION_3_4, MIGRATION_4_5, MIGRATION_5_6, MIGRATION_6_7, MIGRATION_7_8, MIGRATION_8_9, MIGRATION_9_10, - MIGRATION_10_11, MIGRATION_11_12, MIGRATION_12_13, + MIGRATION_10_11, MIGRATION_11_12, MIGRATION_12_13, MIGRATION_13_14, ) .addCallback(SEED_DEFAULT_FILTERS) // No fallbackToDestructiveMigration. A listener's playback positions and play diff --git a/app/src/main/java/org/terst/listen/data/Daos.kt b/app/src/main/java/org/terst/listen/data/Daos.kt index b971aac..6a67184 100644 --- a/app/src/main/java/org/terst/listen/data/Daos.kt +++ b/app/src/main/java/org/terst/listen/data/Daos.kt @@ -238,11 +238,29 @@ interface EpisodeDao { @Query("UPDATE episodes SET playedAtMs = NULL WHERE id = :id") suspend fun markUnplayed(id: String) + /** + * [audioFromServer] defaults to false, matching every caller except a successful podcast + * download that actually landed the skip server's audio. That default is also what clears + * a stale flag: a requeue, a retention reclaim or a failed re-download all call this with + * the default, the same way they already clear [localPath] -- a provenance claim from a + * file that is no longer on disk must not survive it. + */ @Query( """UPDATE episodes SET downloadState = :state, localPath = :localPath, - downloadedAtMs = :downloadedAtMs WHERE id = :id""" + downloadedAtMs = :downloadedAtMs, audioFromServer = :audioFromServer WHERE id = :id""" ) - suspend fun updateDownload(id: String, state: DownloadState, localPath: String?, downloadedAtMs: Long?) + suspend fun updateDownload( + id: String, + state: DownloadState, + localPath: String?, + downloadedAtMs: Long?, + audioFromServer: Boolean = false, + ) + + /** Downloaded episodes whose audio came from the skip server, and can safely be re-queried + * against it: the bytes on disk are the bytes the server measured. */ + @Query("SELECT * FROM episodes WHERE downloadState = :downloaded AND audioFromServer = 1") + suspend fun downloadedFromServer(downloaded: DownloadState = DownloadState.DOWNLOADED): List /** * Marks an episode and everything older in the same podcast as played. diff --git a/app/src/main/java/org/terst/listen/data/Entities.kt b/app/src/main/java/org/terst/listen/data/Entities.kt index 7480101..ce160b5 100644 --- a/app/src/main/java/org/terst/listen/data/Entities.kt +++ b/app/src/main/java/org/terst/listen/data/Entities.kt @@ -199,6 +199,20 @@ data class EpisodeEntity( val localPath: String? = null, val downloadedAtMs: Long? = null, + /** + * Whether [localPath] is the skip server's copy of the audio, as opposed to the publisher's. + * + * Load-bearing, not informational: a show using dynamic ad insertion serves a different + * file on every request -- Radiolab's Grass Apes measured 22:05 from the publisher and + * 28:18 from the server -- so an offset the server measured only applies to its own copy. + * `EpisodeDownloader` sets this; nothing else may, because nothing else knows which file + * actually landed. Defaults to false, which is the conservative direction for the existing + * rows this column arrives on: every download before this column existed might have come + * from either source, and the honest answer for "we don't know" is "treat it as not the + * server's", not "assume the best". + */ + @ColumnInfo(defaultValue = "0") val audioFromServer: Boolean = false, + /** * When slow-drip released this episode, or null while it is still held back. * diff --git a/app/src/main/java/org/terst/listen/data/Repositories.kt b/app/src/main/java/org/terst/listen/data/Repositories.kt index d064bb9..ad1cfd7 100644 --- a/app/src/main/java/org/terst/listen/data/Repositories.kt +++ b/app/src/main/java/org/terst/listen/data/Repositories.kt @@ -8,6 +8,7 @@ import kotlinx.coroutines.flow.first import okhttp3.OkHttpClient import org.terst.listen.server.OkHttpSkipTransport import org.terst.listen.server.ServerSkips +import org.terst.listen.server.SkipRefresher import org.terst.listen.server.SkipServer import org.terst.listen.server.SkipServerConfig import org.terst.listen.download.ChaquopyAudioSource @@ -151,6 +152,7 @@ class AppGraph(private val context: Context) { onDiscovered = { ruleApplier.apply(it) }, probe = { url -> ChaquopyAudioSource(context).probe(url) }, onRefreshed = { feedUrls -> skipServer.pushSubscriptions(feedUrls) }, + refreshSkips = { skipRefresher.refreshAll() }, ), ) } @@ -174,13 +176,26 @@ class AppGraph(private val context: Context) { ) } + /** Shared between the download path and [skipRefresher], which write the server's answer + * into exactly the same columns. */ + val serverSkips: ServerSkips by lazy { + ServerSkips(skipServer, subscriptions, fingerprints, playbackState, midrolls) + } + + /** + * Re-queries the server for episodes a refresh can safely re-query: downloaded, and already + * known to be the server's own copy of the audio. See [SkipRefresher]'s own documentation + * for why that second condition is load-bearing, not an optimisation. + */ + val skipRefresher: SkipRefresher by lazy { SkipRefresher(episodes, serverSkips) } + val downloader: EpisodeDownloader by lazy { EpisodeDownloader( client, episodes, libraryDir, ChaquopyAudioSource(context), - serverAudio = ServerSkips(skipServer, subscriptions, fingerprints, playbackState, midrolls), + serverAudio = serverSkips, ) } diff --git a/app/src/main/java/org/terst/listen/data/SubscriptionRepository.kt b/app/src/main/java/org/terst/listen/data/SubscriptionRepository.kt index a66441a..1281b43 100644 --- a/app/src/main/java/org/terst/listen/data/SubscriptionRepository.kt +++ b/app/src/main/java/org/terst/listen/data/SubscriptionRepository.kt @@ -60,6 +60,18 @@ data class SubscriptionCollaborators( * the library's own refresh button, which does not go via a worker. */ val onRefreshed: suspend (List) -> Unit = {}, + /** + * Re-queries the skip server for episodes it is safe to re-query -- downloaded, and already + * known to be the server's own copy of the audio. + * + * Fired from `refreshAll` for the same reason [onRefreshed] is: it is the one choke point + * every refresh route passes through, including the library's own refresh button, which a + * worker-only hook would miss. Unlike [onRefreshed] this does not depend on what this + * particular refresh found -- an already-downloaded episode's offsets can improve with no + * new episodes at all -- so it is a plain trigger rather than something handed this + * refresh's results. + */ + val refreshSkips: suspend () -> Unit = {}, ) class SubscriptionRepository( @@ -132,7 +144,12 @@ class SubscriptionRepository( // would otherwise never announce a new subscription. Failure is ignored on purpose -- // the skip server improves playback rather than being a precondition for it. collaborators.onRefreshed(shows.map { it.feedUrl }) - return shows.associate { it.id to refresh(it) } + val outcomes = shows.associate { it.id to refresh(it) } + // Also unconditional on what this refresh found: a downloaded episode's offsets can + // improve with no new episodes published at all, since the server keeps re-running + // detection on its own schedule. + collaborators.refreshSkips() + return outcomes } /** diff --git a/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt b/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt index 32180c4..1479812 100644 --- a/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt +++ b/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt @@ -58,12 +58,13 @@ class EpisodeDownloader( episodes.updateDownload(episode.id, DownloadState.DOWNLOADING, null, null) - val result = when (kind) { + val result: Result = when (kind) { // Both extracted kinds take the same route: the source is yt-dlp, and which site - // the URL points at is yt-dlp's problem rather than this app's. + // the URL points at is yt-dlp's problem rather than this app's. Neither ever goes + // near the skip server, so the file is never the server's. SubscriptionKind.YOUTUBE_CHANNEL, SubscriptionKind.FACEBOOK_VIDEO, - -> downloadViaExtractor(episode) + -> downloadViaExtractor(episode).map { AudioResult(it, fromServer = false) } SubscriptionKind.PODCAST -> downloadPodcast(episode) } @@ -72,16 +73,27 @@ class EpisodeDownloader( // Nothing is triggered by a completed download any more. It used to start Tier 3 // fingerprinting, which has moved to the server -- where comparing several // episodes at once is possible at all. + // + // fromServer records which file actually landed, not merely which one was asked + // for: a show using dynamic ad insertion serves a different file on every request, + // so this is the only way a later refresh can tell whether re-querying the server + // for this episode's offsets would even mean anything for the bytes on disk. onSuccess = { - episodes.updateDownload(episode.id, DownloadState.DOWNLOADED, it.absolutePath, now()) + episodes.updateDownload( + episode.id, DownloadState.DOWNLOADED, it.file.absolutePath, now(), it.fromServer, + ) }, // localPath is cleared: a FAILED row pointing at a path invites something downstream - // to try playing a file that is not there. + // to try playing a file that is not there. audioFromServer defaults to false on this + // call, clearing any stale claim from an earlier download the same way. onFailure = { episodes.updateDownload(episode.id, DownloadState.FAILED, null, null) }, ) - return result + return result.map { it.file } } + /** What actually landed on disk, and whether it was the skip server's own copy of it. */ + private data class AudioResult(val file: File, val fromServer: Boolean) + /** * A recorded download whose file has since been deleted has to be fetched again, not treated * as present -- otherwise playback fails on a file the database insists exists. @@ -100,7 +112,7 @@ class EpisodeDownloader( * episode into a failed download. Offsets are recorded only when the server's audio is * actually what landed on disk. */ - private suspend fun downloadPodcast(episode: EpisodeEntity): Result { + private suspend fun downloadPodcast(episode: EpisodeEntity): Result { val fromServer = serverAudio?.resolve(episode) val serverUrl = fromServer?.audioUrl?.takeIf { fromServer.ready } @@ -108,10 +120,10 @@ class EpisodeDownloader( val result = downloadOverHttp(episode, serverUrl) if (result.isSuccess) { serverAudio?.record(episode, fromServer) - return result + return result.map { AudioResult(it, fromServer = true) } } } - return downloadOverHttp(episode, episode.mediaUrl) + return downloadOverHttp(episode, episode.mediaUrl).map { AudioResult(it, fromServer = false) } } private suspend fun downloadViaExtractor(episode: EpisodeEntity): Result { diff --git a/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt b/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt index 71907bd..0c0a4e3 100644 --- a/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt +++ b/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt @@ -1,5 +1,6 @@ package org.terst.listen.fingerprint +import org.terst.listen.data.DownloadState import org.terst.listen.data.EpisodeEntity import org.terst.listen.data.EpisodeMidrollEntity import org.terst.listen.data.insertedAdMs @@ -182,6 +183,16 @@ object SkipSummary { outro = offset(episode.detectedOutroMs, show?.skipOutroMs ?: 0), breaks = when { breaks.isNotEmpty() -> breaks + // A download that is not the server's own copy can never be refreshed in + // place -- a show with dynamic ad insertion serves a different file on every + // request, so the server's timestamps only mean anything against its own + // audio. "Not checked yet" would read as "the server has not looked", which + // promises this will resolve on its own; it never will without a re-download, + // so it gets its own, honest status instead. A download that predates this + // column defaults to the same "not the server's" and is treated identically -- + // the conservative answer for "we don't know" is "assume it cannot be checked". + episode.downloadState == DownloadState.DOWNLOADED && !episode.audioFromServer -> + listOf(BreakLine("Breaks", CANNOT_CHECK_THIS_COPY)) // Distinct from "none found": scanning runs nightly while charging, so a // freshly downloaded episode has genuinely not been looked at yet. Neither // status line names a kind, so both use a generic label rather than one of @@ -192,6 +203,12 @@ object SkipSummary { ) } + /** + * Shown instead of "Not checked yet" for a download that cannot be refreshed against the + * server -- a status, not a transient wait, so the wording says what would actually fix it. + */ + const val CANNOT_CHECK_THIS_COPY = "Can't check this copy; redownload to get the server's" + private fun describe(ms: Long): String = if (ms <= 0) "None" else "${ms / MS_PER_SECOND}s" diff --git a/app/src/main/java/org/terst/listen/server/SkipRefresher.kt b/app/src/main/java/org/terst/listen/server/SkipRefresher.kt new file mode 100644 index 0000000..d2cf4fe --- /dev/null +++ b/app/src/main/java/org/terst/listen/server/SkipRefresher.kt @@ -0,0 +1,39 @@ +package org.terst.listen.server + +import org.terst.listen.data.EpisodeDao +import org.terst.listen.download.ServerAudio + +/** + * Picks up improved skip detection on a feed refresh, for the episodes it is safe to re-query. + * + * `EpisodeDownloader` only asks the server once, at download time, so an episode's offsets are + * frozen to whatever the server knew that day -- and the server's answers have changed + * substantially as detection improved, with no way for an already-downloaded episode to catch + * up short of deleting and redownloading it. + * + * Re-querying is only safe for an episode that is downloaded *and* whose local file is already + * known to be the server's own copy ([org.terst.listen.data.EpisodeEntity.audioFromServer]). + * A show using dynamic ad insertion serves a different file on every request -- Radiolab's + * Grass Apes measured 22:05 from the publisher and 28:18 from the server -- so an offset + * computed against the server's current copy would be wrong for a file that came from the + * publisher instead. [EpisodeDao.downloadedFromServer] is the filter that keeps this from ever + * happening. + */ +class SkipRefresher( + private val episodes: EpisodeDao, + private val serverAudio: ServerAudio, +) { + + /** + * One episode's failure -- the server unreachable, a malformed response -- must not stop + * the rest of the library from being refreshed, the same reasoning as every other per-item + * loop in this app. + */ + suspend fun refreshAll() { + episodes.downloadedFromServer().forEach { episode -> + runCatching { + serverAudio.resolve(episode)?.let { info -> serverAudio.record(episode, info) } + } + } + } +} diff --git a/app/src/test/java/org/terst/listen/data/FakeDaos.kt b/app/src/test/java/org/terst/listen/data/FakeDaos.kt index a1dab05..c9937fb 100644 --- a/app/src/test/java/org/terst/listen/data/FakeDaos.kt +++ b/app/src/test/java/org/terst/listen/data/FakeDaos.kt @@ -222,10 +222,21 @@ class FakeEpisodeDao : EpisodeDao { state: DownloadState, localPath: String?, downloadedAtMs: Long?, + audioFromServer: Boolean, ) { - replace(id) { it.copy(downloadState = state, localPath = localPath, downloadedAtMs = downloadedAtMs) } + replace(id) { + it.copy( + downloadState = state, + localPath = localPath, + downloadedAtMs = downloadedAtMs, + audioFromServer = audioFromServer, + ) + } } + override suspend fun downloadedFromServer(downloaded: DownloadState) = + rows.filter { it.downloadState == downloaded && it.audioFromServer } + override suspend fun setChapters(id: String, json: String?) { replace(id) { it.copy(chaptersJson = json) } } diff --git a/app/src/test/java/org/terst/listen/data/SubscriptionRepositoryTest.kt b/app/src/test/java/org/terst/listen/data/SubscriptionRepositoryTest.kt index 2474f8a..63ac219 100644 --- a/app/src/test/java/org/terst/listen/data/SubscriptionRepositoryTest.kt +++ b/app/src/test/java/org/terst/listen/data/SubscriptionRepositoryTest.kt @@ -546,4 +546,28 @@ class SubscriptionRepositoryTest { assertEquals(1, outcomes.size) assertTrue(outcomes.values.single() is RefreshOutcome.Failed) } + + /** + * `refreshAll` is the one choke point every refresh route passes through, including the + * library's own refresh button -- a worker-only hook already missed that once in this app + * (see `onRefreshed`). Re-querying the skip server belongs here for the same reason. + */ + @Test + fun `refreshAll re-queries the skip server exactly once`() = runTest { + var calls = 0 + val repoWithSkips = SubscriptionRepository( + subs, + eps, + FeedFetcher(OkHttpClient()), + SubscriptionCollaborators(refreshSkips = { calls++ }), + ) { clock } + + server.enqueue(MockResponse().setBody(feed())) + repoWithSkips.subscribe(url()) + + server.enqueue(MockResponse().setBody(feed())) + repoWithSkips.refreshAll() + + assertEquals(1, calls) + } } diff --git a/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt b/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt index b35983a..d524200 100644 --- a/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt +++ b/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt @@ -284,4 +284,91 @@ class EpisodeDownloaderTest { assertNull("recorded offsets for audio it did not download", audio.recorded) } + + // ---- provenance: which file actually landed ----------------------------------------- + // + // A refresh can only safely re-query the skip server for an episode whose downloaded file + // is the server's own copy. A show using dynamic ad insertion serves a different file on + // every request, so this flag is the only way to know, after the fact, which copy is on + // disk -- getting it wrong would let a later refresh stamp a server-measured offset onto a + // publisher's file and seek the listener into the middle of the programme. + + @Test + fun `audio fetched from the skip server is marked as such`() = runTest { + server.enqueue(MockResponse().setBody("server-bytes")) + val audio = FakeServerAudio( + ServerEpisode(ready = true, audioUrl = server.url("/audio/abc").toString(), + durationMs = null, introMs = 1, outroMs = 2, detected = true) + ) + val ep = episode(url = "https://publisher.example/original.mp3").also { dao.rows += it } + + withServer(audio).download(ep, SubscriptionKind.PODCAST) + + assertTrue("the server's own audio must be marked as server-sourced", dao.rows.single().audioFromServer) + } + + @Test + fun `audio fetched straight from the publisher is not marked as the server's`() = runTest { + server.enqueue(MockResponse().setBody("publisher-bytes")) + val ep = episode(url = server.url("/original.mp3").toString()).also { dao.rows += it } + + withServer(FakeServerAudio(null)).download(ep, SubscriptionKind.PODCAST) + + assertFalse("the publisher's own audio must not be marked as the server's", + dao.rows.single().audioFromServer) + } + + @Test + fun `falling back to the publisher after the server fails is not marked as the server's`() = runTest { + server.enqueue(MockResponse().setResponseCode(502)) + server.enqueue(MockResponse().setBody("publisher-bytes")) + val audio = FakeServerAudio( + ServerEpisode(ready = true, audioUrl = server.url("/audio/broken").toString(), + durationMs = null, introMs = 1, outroMs = 2, detected = true) + ) + val ep = episode(url = server.url("/original.mp3").toString()).also { dao.rows += it } + + withServer(audio).download(ep, SubscriptionKind.PODCAST) + + assertFalse("the fallback file is the publisher's, not the server's", + dao.rows.single().audioFromServer) + } + + /** A YouTube/Facebook download never goes near the skip server at all. */ + @Test + fun `an extracted video download is not marked as server-sourced`() = runTest { + downloader.download( + episode(url = "https://www.youtube.com/watch?v=abc").also { dao.rows += it }, + SubscriptionKind.YOUTUBE_CHANNEL, + ) + // No extractor is configured in this test's downloader, so the download fails -- but + // even a failed attempt must not leave a stale "from the server" claim behind. + assertFalse(dao.rows.single().audioFromServer) + } + + /** + * A stale provenance claim from an earlier download must not survive a later one -- the + * same rule [EpisodeDao.updateDownload]'s own default follows for [EpisodeEntity.localPath]: + * a fresh download tells the whole truth about itself, not just the part that changed. + */ + @Test + fun `re-downloading from the publisher clears a provenance flag set by an earlier server download`() = runTest { + server.enqueue(MockResponse().setBody("server-bytes")) + val serverAudio = FakeServerAudio( + ServerEpisode(ready = true, audioUrl = server.url("/audio/abc").toString(), + durationMs = null, introMs = 1, outroMs = 2, detected = true) + ) + val ep = episode(url = server.url("/original.mp3").toString()).also { dao.rows += it } + withServer(serverAudio).download(ep, SubscriptionKind.PODCAST) + assertTrue(dao.rows.single().audioFromServer) + + File(dao.rows.single().localPath!!).delete() + server.enqueue(MockResponse().setBody("publisher-bytes")) + withServer(FakeServerAudio(null)).download(dao.rows.single(), SubscriptionKind.PODCAST) + + assertFalse( + "a previous download's provenance must not survive a re-download from elsewhere", + dao.rows.single().audioFromServer, + ) + } } diff --git a/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt b/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt index 367bf04..38541a7 100644 --- a/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt +++ b/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt @@ -2,6 +2,7 @@ package org.terst.listen.fingerprint import org.junit.Assert.assertEquals import org.junit.Test +import org.terst.listen.data.DownloadState import org.terst.listen.data.EpisodeEntity import org.terst.listen.data.EpisodeMidrollEntity import org.terst.listen.data.FingerprintKind @@ -54,6 +55,17 @@ class SkipSummaryTest { actualDurationMs = actualDurationMs, ) + /** + * A downloaded episode, with [audioFromServer] saying where its file came from -- null for + * "not downloaded at all". A separate, narrow helper rather than another parameter on + * [episodeRow]: none of the provenance tests care about offsets or break timing, so they + * do not need [episodeRow]'s other five parameters at all. + */ + private fun downloadedEpisode(audioFromServer: Boolean?) = episodeRow().copy( + downloadState = if (audioFromServer != null) DownloadState.DOWNLOADED else DownloadState.NONE, + audioFromServer = audioFromServer ?: false, + ) + private fun showRow(skipIntroMs: Long = 0, skipOutroMs: Long = 0) = SubscriptionEntity( id = "s1", feedUrl = "https://example.com/feed.xml", @@ -187,6 +199,42 @@ class SkipSummaryTest { assertEquals(listOf(SkipSummary.BreakLine("Breaks", "None found")), skips.breaks) } + /** + * A downloaded episode whose local file did not come from the skip server can never have + * its skips refreshed in place -- a show using dynamic ad insertion serves a different file + * on every request, so the server's timestamps only mean anything against its own copy. + * "Not checked yet" would read as "the server has not looked", which is a different and + * much more hopeful claim than "this copy can never be checked". A download that predates + * this column arrives with the same default (false) as an explicit "not the server's", so + * the two are deliberately indistinguishable here, same as in production. + */ + @Test + fun aDownloadNotSourcedFromTheServerSaysItCannotBeChecked() { + val skips = SkipSummary.episode( + downloadedEpisode(audioFromServer = false), + showRow(), + ranges = emptyList(), + ) + + assertEquals( + listOf(SkipSummary.BreakLine("Breaks", SkipSummary.CANNOT_CHECK_THIS_COPY)), + skips.breaks, + ) + } + + /** A server-sourced download with nothing yet is still a legitimate "not checked yet", + * unlike a publisher-sourced one, which can never become checked by waiting. */ + @Test + fun aServerSourcedDownloadWithNothingYetStillSaysNotCheckedYet() { + val skips = SkipSummary.episode( + downloadedEpisode(audioFromServer = true), + showRow(), + ranges = emptyList(), + ) + + assertEquals(listOf(SkipSummary.BreakLine("Breaks", "Not checked yet")), skips.breaks) + } + @Test fun episodeReportsTheOffsetsActuallyInForce() { val skips = SkipSummary.episode( diff --git a/app/src/test/java/org/terst/listen/server/SkipRefresherTest.kt b/app/src/test/java/org/terst/listen/server/SkipRefresherTest.kt new file mode 100644 index 0000000..9dffdb8 --- /dev/null +++ b/app/src/test/java/org/terst/listen/server/SkipRefresherTest.kt @@ -0,0 +1,176 @@ +package org.terst.listen.server + +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.terst.listen.data.DownloadState +import org.terst.listen.data.EpisodeEntity +import org.terst.listen.data.FakeEpisodeDao +import org.terst.listen.data.FakeMidrollDao +import org.terst.listen.data.FakePlaybackStateDao +import org.terst.listen.data.FakeShowFingerprintDao +import org.terst.listen.data.FakeSubscriptionDao +import org.terst.listen.data.SubscriptionEntity +import org.terst.listen.download.ServerAudio + +/** + * Re-queries the skip server for episodes it is safe to re-query: downloaded, and already known + * to be the server's own copy of the audio (see [EpisodeEntity.audioFromServer]). + * + * This is the only way a feed refresh can pick up improved detection without redownloading + * every episode by hand, and it is deliberately narrow: an episode fetched from the publisher + * must never have the server's timestamps stamped onto it, because dynamic ad insertion means + * those timestamps describe a different file. + */ +class SkipRefresherTest { + + private fun episode( + id: String, + downloadState: DownloadState = DownloadState.DOWNLOADED, + audioFromServer: Boolean = true, + ) = EpisodeEntity( + id = id, + subscriptionId = "s1", + guid = id, + title = "Episode $id", + mediaUrl = "https://cdn/$id.mp3", + mediaType = "audio/mpeg", + discoveredAtMs = 0, + downloadState = downloadState, + localPath = "/audio/$id.mp3".takeIf { downloadState == DownloadState.DOWNLOADED }, + audioFromServer = audioFromServer, + ) + + private fun info(introMs: Long, breaks: List = emptyList()) = ServerEpisode( + ready = true, + audioUrl = "https://server/x.mp3", + durationMs = null, + introMs = introMs, + outroMs = null, + detected = true, + breaks = breaks, + ) + + /** A fake collaborator, for the tests that only care whether resolve/record were called. */ + private class FakeServerAudio( + private val infoFor: Map = emptyMap(), + private val failing: Set = emptySet(), + ) : ServerAudio { + val resolved = mutableListOf() + val recorded = mutableListOf>() + + override suspend fun resolve(episode: EpisodeEntity): ServerEpisode? { + resolved += episode.id + check(episode.id !in failing) { "server unreachable: ${episode.id}" } + return infoFor[episode.id] + } + + override suspend fun record(episode: EpisodeEntity, info: ServerEpisode) { + recorded += episode.id to info + } + } + + @Test + fun `a downloaded server-sourced episode is re-queried`() = runTest { + val dao = FakeEpisodeDao().apply { rows += episode("e1") } + val audio = FakeServerAudio(infoFor = mapOf("e1" to info(5_000))) + + SkipRefresher(dao, audio).refreshAll() + + assertEquals(listOf("e1"), audio.resolved) + assertEquals(listOf("e1" to info(5_000)), audio.recorded) + } + + @Test + fun `a downloaded episode sourced from the publisher is not re-queried`() = runTest { + val dao = FakeEpisodeDao().apply { rows += episode("e1", audioFromServer = false) } + val audio = FakeServerAudio(infoFor = mapOf("e1" to info(5_000))) + + SkipRefresher(dao, audio).refreshAll() + + assertTrue("a publisher-sourced download must never be asked about", audio.resolved.isEmpty()) + assertTrue(audio.recorded.isEmpty()) + } + + @Test + fun `an episode that is not downloaded at all is not re-queried`() = runTest { + val dao = FakeEpisodeDao().apply { + rows += episode("e1", downloadState = DownloadState.NONE, audioFromServer = false) + } + val audio = FakeServerAudio(infoFor = mapOf("e1" to info(5_000))) + + SkipRefresher(dao, audio).refreshAll() + + assertTrue(audio.resolved.isEmpty()) + } + + @Test + fun `one episode's failure does not stop the others being refreshed`() = runTest { + val dao = FakeEpisodeDao().apply { + rows += episode("bad") + rows += episode("good") + } + val audio = FakeServerAudio(infoFor = mapOf("good" to info(7_000)), failing = setOf("bad")) + + SkipRefresher(dao, audio).refreshAll() + + assertEquals(listOf("good" to info(7_000)), audio.recorded) + } + + @Test + fun `an episode the server no longer has anything to say about is left alone`() = runTest { + val dao = FakeEpisodeDao().apply { rows += episode("e1") } + val audio = FakeServerAudio(infoFor = mapOf("e1" to null)) + + SkipRefresher(dao, audio).refreshAll() + + assertEquals(listOf("e1"), audio.resolved) + assertTrue(audio.recorded.isEmpty()) + } + + // ---- through the real ServerSkips, proving the refresh actually replaces ranges -------- + + private val show = SubscriptionEntity( + id = "s1", + feedUrl = "https://feeds.example.com/show.xml", + title = "A Show", + addedAtMs = 0, + ) + + @Test + fun `new ranges from a refresh replace the old ones rather than accumulating`() = runTest { + val episodeRow = episode("e1") + val dao = FakeEpisodeDao().apply { rows += episodeRow } + val subscriptions = FakeSubscriptionDao().apply { rows += show } + val fingerprints = FakeShowFingerprintDao().also { it.episodeRows = dao.rows } + val midrolls = FakeMidrollDao() + val playbackState = FakePlaybackStateDao(dao.rows) + + val transport = object : SkipTransport { + var next = """{"status":"ready","breaks":[{"startMs":0,"endMs":60000}]}""" + override suspend fun get(url: String, token: String) = next + override suspend fun post(url: String, token: String, body: String) = true + } + val serverSkips = ServerSkips( + SkipServer({ SkipServerConfig("https://listen.terst.org", "tok") }, transport), + subscriptions, + fingerprints, + playbackState, + midrolls, + ) + val refresher = SkipRefresher(dao, serverSkips) + + refresher.refreshAll() + assertEquals(listOf(0L to 60_000L), midrolls.rangeRows.filter { it.episodeId == "e1" } + .map { it.startMs to it.endMs }) + + transport.next = """{"status":"ready","breaks":[{"startMs":10000,"endMs":70000}]}""" + refresher.refreshAll() + + val ranges = midrolls.rangeRows.filter { it.episodeId == "e1" } + assertEquals("the stale range must be gone, not kept alongside the new one", 1, ranges.size) + assertEquals(10_000L, ranges.single().startMs) + assertEquals(70_000L, ranges.single().endMs) + } +} -- cgit v1.2.3