diff options
| -rw-r--r-- | .coverage-baseline | 2 | ||||
| -rw-r--r-- | CLAUDE.md | 24 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/data/Daos.kt | 17 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/work/RetentionSweeper.kt | 15 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/work/Workers.kt | 10 | ||||
| -rw-r--r-- | app/src/test/java/org/terst/listen/data/FakeDaos.kt | 9 | ||||
| -rw-r--r-- | app/src/test/java/org/terst/listen/work/RetentionSweeperTest.kt | 32 | ||||
| -rw-r--r-- | docs/detection-findings.md | 59 |
8 files changed, 128 insertions, 40 deletions
diff --git a/.coverage-baseline b/.coverage-baseline index d294bec..1f799fd 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -93.02 +93.03 @@ -194,15 +194,21 @@ by default and correct; it is simply waiting for a publisher who marks breaks. - **Auto-downloads are not retried.** A rule's download that fails is left FAILED rather than requeued: retrying forever against a 404 would burn the battery, and the failed row is visible in the episode list. Retry is the manual Download button. -- **Tier 3 has never confirmed a real intro, outro or midroll.** The pipeline runs on hardware - and the worker succeeds, but three consecutive Up First downloads produced no candidate at - either end. Two live hypotheses -- an 8-second reference window against a ~6-second sting, - and pre-roll ads moving the intro to a different offset in every file. **See - `docs/detection-findings.md`**, which is the brief for the desktop session that settles it: - what is already known, the experiments in order, and what not to touch. Do not tune - `DEFAULT_REFERENCE_SECONDS`, `MIN_SCORE` or `CONFIRM_THRESHOLD` from the app -- the app - cannot show you what a change costs in false positives, and the threshold is the whole - safety mechanism. +- **Tier 3 had never confirmed a real intro, outro or midroll, and root cause is now found and + fixed.** Not the correlation algorithm -- a real production-algorithm simulation against real + Up First audio confirmed cleanly at the app's actual defaults before any of this. The real + cause: `RetentionPolicy`'s one-day grace period routinely reclaimed a show's pending episode's + file before the next download arrived to compare it against, for a fast, promptly-listened-to + show like Up First -- so the detector kept discarding its only candidate and restarting instead + of ever comparing two episodes. Confirmed live on the phone (a fresh download's + `FingerprintWorker` run replaced the stale pending episode instead of advancing toward a + comparison). Fixed by protecting a show's pending episode from reclaim, for both intro/outro + and midroll -- `RetentionSweeper` now takes a `pendingEpisodeIds` seam and + `RetentionWorker` supplies the union of both mechanisms' pending ids. **Not yet re-verified on + a device.** Full history in `docs/detection-findings.md`. Do not tune + `DEFAULT_REFERENCE_SECONDS`, `MIN_SCORE` or `CONFIRM_THRESHOLD` from the app -- they were never + the problem, the app cannot show you what a change costs in false positives, and the threshold + is the whole safety mechanism. - **Android Auto is verified in a real car**, as of 2026-09-28. It appears, and connects. The notification Peter saw on first connect was Android Auto's own new-media-app announcement, 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 dec1ba0..2ca227c 100644 --- a/app/src/main/java/org/terst/listen/data/Daos.kt +++ b/app/src/main/java/org/terst/listen/data/Daos.kt @@ -354,6 +354,14 @@ interface ShowFingerprintDao { @Query("UPDATE episodes SET detectedOutroMs = :ms WHERE id = :id") suspend fun setDetectedOutroMs(id: String, ms: Long) + + /** + * Every episode id any show currently relies on as its pending intro/outro comparison basis, + * across both kinds. [org.terst.listen.work.RetentionSweeper] reads this to avoid reclaiming + * one out from under detection -- see its own doc comment for why that race matters. + */ + @Query("SELECT DISTINCT pendingEpisodeId FROM show_fingerprints WHERE pendingEpisodeId IS NOT NULL") + suspend fun getAllPendingEpisodeIds(): List<String> } /** @@ -394,6 +402,15 @@ interface MidrollDao { @Query("UPDATE episodes SET midrollCheckedAtMs = :ms WHERE id = :id") suspend fun setMidrollCheckedAtMs(id: String, ms: Long) + + /** + * Every episode id any show currently relies on as its midroll-discovery pending basis. + * [org.terst.listen.work.RetentionSweeper] reads this for the same reason it reads + * [ShowFingerprintDao.getAllPendingEpisodeIds] -- reclaiming this episode's file defeats + * discovery the same way, silently, before the next download ever gets compared against it. + */ + @Query("SELECT DISTINCT midrollPendingEpisodeId FROM subscriptions WHERE midrollPendingEpisodeId IS NOT NULL") + suspend fun getAllMidrollPendingEpisodeIds(): List<String> } /** diff --git a/app/src/main/java/org/terst/listen/work/RetentionSweeper.kt b/app/src/main/java/org/terst/listen/work/RetentionSweeper.kt index cd329b3..fd80379 100644 --- a/app/src/main/java/org/terst/listen/work/RetentionSweeper.kt +++ b/app/src/main/java/org/terst/listen/work/RetentionSweeper.kt @@ -16,13 +16,26 @@ class RetentionSweeper( private val policy: RetentionPolicy, /** Injected so the undeletable-file path can be exercised without a read-only filesystem. */ private val deleteFile: (File) -> Boolean = { it.delete() }, + /** + * Episode ids Tier 3 is currently relying on as a show's pending comparison basis, across + * both fingerprinting mechanisms -- `ShowFingerprintEntity.pendingEpisodeId` and + * `SubscriptionEntity.midrollPendingEpisodeId`. + * + * A lambda rather than the DAOs themselves: this class only needs the resulting id set, not + * two more collaborators to know about, and a real caller composes it from wherever those + * ids actually live. Reclaiming one of these out from under detection is the exact race that + * left Up First's detector discarding its only candidate and restarting forever instead of + * ever comparing two episodes -- see docs/detection-findings.md. + */ + private val pendingEpisodeIds: suspend () -> Set<String> = { emptySet() }, ) { /** Returns how many episodes were reclaimed. */ suspend fun sweep(nowMs: Long): Int { // The query is a coarse filter; the policy makes the decision, including cases SQL // cannot express such as an episode resumed after being marked played. - val candidates = episodes.playedDownloadsBefore(nowMs) + val protected = pendingEpisodeIds() + val candidates = episodes.playedDownloadsBefore(nowMs).filterNot { it.id in protected } var reclaimed = 0 for (episode in policy.selectDeletable(candidates, nowMs)) { diff --git a/app/src/main/java/org/terst/listen/work/Workers.kt b/app/src/main/java/org/terst/listen/work/Workers.kt index 87f6814..9c48db0 100644 --- a/app/src/main/java/org/terst/listen/work/Workers.kt +++ b/app/src/main/java/org/terst/listen/work/Workers.kt @@ -65,8 +65,14 @@ class RetentionWorker(context: Context, params: WorkerParameters) : CoroutineWor override suspend fun doWork(): Result { val graph = Graph.get(applicationContext) - val reclaimed = RetentionSweeper(graph.episodes, graph.retention) - .sweep(System.currentTimeMillis()) + val reclaimed = RetentionSweeper( + graph.episodes, + graph.retention, + pendingEpisodeIds = { + (graph.fingerprints.getAllPendingEpisodeIds() + graph.midrolls.getAllMidrollPendingEpisodeIds()) + .toSet() + }, + ).sweep(System.currentTimeMillis()) return Result.success(androidx.work.Data.Builder().putInt("reclaimed", reclaimed).build()) } } 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 8b00c3e..14d1394 100644 --- a/app/src/test/java/org/terst/listen/data/FakeDaos.kt +++ b/app/src/test/java/org/terst/listen/data/FakeDaos.kt @@ -310,6 +310,9 @@ class FakeShowFingerprintDao : ShowFingerprintDao { val index = episodeRows.indexOfFirst { it.id == id } if (index >= 0) episodeRows[index] = transform(episodeRows[index]) } + + override suspend fun getAllPendingEpisodeIds() = + rows.mapNotNull { it.pendingEpisodeId }.distinct() } class FakeMidrollDao : MidrollDao { @@ -320,6 +323,9 @@ class FakeMidrollDao : MidrollDao { * runs against the real `episodes` table. */ var episodeRows: MutableList<EpisodeEntity> = mutableListOf() + /** Backs [getAllMidrollPendingEpisodeIds], which reads the real `subscriptions` table. */ + var subscriptionRows: MutableList<SubscriptionEntity> = mutableListOf() + override suspend fun getCandidates(subscriptionId: String) = candidateRows.filter { it.subscriptionId == subscriptionId } @@ -348,6 +354,9 @@ class FakeMidrollDao : MidrollDao { val index = episodeRows.indexOfFirst { it.id == id } if (index >= 0) episodeRows[index] = episodeRows[index].copy(midrollCheckedAtMs = ms) } + + override suspend fun getAllMidrollPendingEpisodeIds() = + subscriptionRows.mapNotNull { it.midrollPendingEpisodeId }.distinct() } class FakeQueueDao : QueueDao { diff --git a/app/src/test/java/org/terst/listen/work/RetentionSweeperTest.kt b/app/src/test/java/org/terst/listen/work/RetentionSweeperTest.kt index 65b739f..f587170 100644 --- a/app/src/test/java/org/terst/listen/work/RetentionSweeperTest.kt +++ b/app/src/test/java/org/terst/listen/work/RetentionSweeperTest.kt @@ -141,4 +141,36 @@ class RetentionSweeperTest { assertEquals(0, off.sweep(nowMs = 100 * day)) assertTrue(File(episode.localPath!!).exists()) } + + /** + * The race that left Up First's detector restarting forever instead of ever comparing two + * episodes: a show's Tier 3 pending episode reclaimed before the next download arrives to + * compare it against. See docs/detection-findings.md. + */ + @Test + fun `an episode still relied on as a Tier 3 pending basis is not reclaimed`() = runTest { + val episode = downloaded("a", playedAtMs = 0) + val protective = RetentionSweeper( + dao, + RetentionPolicy(gracePeriodMs = day), + pendingEpisodeIds = { setOf("a") }, + ) + + assertEquals(0, protective.sweep(nowMs = 2 * day)) + assertTrue(File(episode.localPath!!).exists()) + assertEquals(DownloadState.DOWNLOADED, dao.rows.single().downloadState) + } + + @Test + fun `an episode past its grace period but not pending anywhere is still reclaimed`() = runTest { + val episode = downloaded("a", playedAtMs = 0) + val protective = RetentionSweeper( + dao, + RetentionPolicy(gracePeriodMs = day), + pendingEpisodeIds = { setOf("some-other-episode") }, + ) + + assertEquals(1, protective.sweep(nowMs = 2 * day)) + assertTrue(!File(episode.localPath!!).exists()) + } } diff --git a/docs/detection-findings.md b/docs/detection-findings.md index 4e8e721..6c7385d 100644 --- a/docs/detection-findings.md +++ b/docs/detection-findings.md @@ -1,14 +1,14 @@ # Tier 3 detection: what is known, and the root cause of why nothing confirmed -Written 2026-09-29, in three passes across the same day as the picture sharpened. **Resolved as -of the last pass**: the correlation algorithm and its tuned constants were never the problem. -Up First specifically confirms nothing because `RetentionPolicy`'s one-day grace period usually -reclaims a played episode's file before the *next* one is downloaded and processed, for a show -this fast and this promptly listened to -- so the detector keeps discarding its only pending -candidate and restarting, rather than ever getting two episodes' files to compare at once. See -"The live test, and the actual root cause" below for the mechanism, and "What this means for a -fix" for what's actually worth changing (a retention/fingerprint coupling decision, not a -constant to tune). +Written 2026-09-29, in four passes across the same day as the picture sharpened and then got +fixed. The correlation algorithm and its tuned constants were never the problem. Up First +confirmed nothing because `RetentionPolicy`'s one-day grace period usually reclaimed a played +episode's file before the *next* one was downloaded and processed, for a show this fast and this +promptly listened to -- so the detector kept discarding its only pending candidate and +restarting, rather than ever getting two episodes' files to compare at once. See "The live test, +and the actual root cause" below for the mechanism, and "The fix (made)" for what actually +shipped -- protecting a show's pending episode from reclaim, for both fingerprinting mechanisms. +**Not yet re-verified on a device.** The rest of this document is kept in the order it was actually found, including the two hypotheses that turned out to be wrong -- both cost real time to check and are worth not @@ -31,6 +31,9 @@ not a score that failed. See "What the phone actually showed" for the detail. real time. That's what found the actual mechanism -- see "The live test, and the actual root cause." +**Fourth pass, the fix:** Peter picked exempting a pending episode from reclaim over the other +two options. Shipped for both intro/outro and midroll -- see "The fix (made)." + --- ## What is settled @@ -210,24 +213,26 @@ executed (Doze/battery-restriction deferral of a one-shot job, still unresolved something force-stopped the app in a way that drops pending `WorkSpec` rows before they run. Secondary to the retention-race finding above, and not reproduced in the live test. -## What this means for a fix (not made -- needs a decision, not a desktop tweak) - -The retention-race finding points at the actual place to fix, and it isn't -`DEFAULT_REFERENCE_SECONDS`/`MIN_SCORE`/`CONFIRM_THRESHOLD`: those were never the problem. -Options, roughly in order of how invasive they are: - -1. **Exempt an episode retention would otherwise reclaim while it is a show's `pendingEpisodeId`.** - Keeps the file exactly as long as Tier 3 actually needs it and no longer; the natural fix, but - couples `RetentionSweeper` to fingerprint state it currently knows nothing about. -2. **Extend the grace period specifically for a show still building its first candidate.** Cruder, - and still racy for a show played even faster than its grace period, just less often. -3. **Accept the miss and let it self-heal.** The current behaviour already does this -- it just - restarts forever for a fast show instead of ever confirming. Only really an "option" if fast - shows are an acceptable gap in Tier 3's coverage. - -This is Peter's call, not a desktop one -- it trades file-retention precision against Tier 3 -coverage for exactly the shows (short, frequent, promptly played) most likely to also carry the -heaviest dynamic ad insertion, which is the case that matters most. +## The fix (made) + +Peter chose option 1: exempt an episode retention would otherwise reclaim while it is a show's +pending comparison basis, for both mechanisms. `RetentionSweeper` gained a `pendingEpisodeIds` +seam (`suspend () -> Set<String>`, defaulting to empty so every existing caller and test was +unaffected) and filters its reclaim candidates against it before the grace-period policy ever +sees them. `RetentionWorker` wires the real thing: the union of +`ShowFingerprintDao.getAllPendingEpisodeIds()` (intro/outro) and +`MidrollDao.getAllMidrollPendingEpisodeIds()` (midroll, reading +`SubscriptionEntity.midrollPendingEpisodeId`) -- both new, narrow queries added for exactly this. + +Applied to both mechanisms, not just the one that got diagnosed: `MidrollUpdater`'s discovery has +the identical shape (a rolling pending episode compared against whatever downloads next) and +would race retention the same way for a fast show, just silently -- discovery finding nothing +that round rather than a visible frozen row, since it has no fallback state to get stuck in. + +Not yet re-verified on a device: the next live test is downloading two Up First episodes close +enough together that the first is still within its (now-irrelevant, since it's protected) grace +period, and confirming `pendingEpisodeId` actually advances to a real second-episode comparison +instead of restarting again. ## Experiments, in order (updated) |