From e7ba6bcc3d5e7e4451850c0e16add0fc70735eeb Mon Sep 17 00:00:00 2001 From: Cobb Date: Thu, 22 Oct 2026 16:42:38 -0700 Subject: [PATCH 1/9] player: keep the service in the foreground while autoplay finds the next video Autoplay starts when the last item ends and then looks up the next video over the network. Media3 1.4.1 takes the playback service out of the foreground as soon as the player ends (its rule is play-when-ready and READY or BUFFERING), so the next video was handed to a service that had already left the foreground. Android 12 and later do not let an app in the background start a foreground service (the documented platform restriction; not reproduced on a device here), and Media3 only logs that refusal, so with the screen off or another app in front the next video played from a service that was not in the foreground. The report is that it then stops a few seconds in; how the system stops it was not measured on a device. The service now answers Media3's foreground question through AutoplayHandoff: while an attempt is in flight it keeps the foreground, and it lets go when the next video is handed over, or when the attempt finds nothing, fails, is refused by the player or runs past 30 seconds, in which case it asks Media3 to look again straight away. One attempt runs at a time; a newer one replaces an older one without losing the hold, and the service drops any attempt when it is destroyed. The lookup and hand-over themselves are unchanged: same candidate pick, same allowlist gate, same staleness fence, same setPlayingFrom. --- .../straw/feature/player/AutoplayHandoff.kt | 100 ++++++++++ .../straw/feature/player/PlaybackService.kt | 108 +++++++---- .../feature/player/AutoplayHandoffTest.kt | 178 ++++++++++++++++++ .../feature/player/AutoplayWiringTest.kt | 50 +++++ 4 files changed, 403 insertions(+), 33 deletions(-) create mode 100644 strawApp/src/main/kotlin/com/sulkta/straw/feature/player/AutoplayHandoff.kt create mode 100644 strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayHandoffTest.kt create mode 100644 strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/AutoplayHandoff.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/AutoplayHandoff.kt new file mode 100644 index 000000000..a90f56fa3 --- /dev/null +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/AutoplayHandoff.kt @@ -0,0 +1,100 @@ +/* + * SPDX-FileCopyrightText: 2026 Sulkta + * SPDX-License-Identifier: GPL-3.0-or-later + * + * One autoplay attempt at a time, from the end of a video to the moment the next one is handed to + * the player, and whether the playback service has to stay in the foreground meanwhile. + * + * Why the hold exists. Autoplay starts when the last item ends, and finding the next video is a + * network round trip (channel list, then stream info). Media3 1.4.1 decides on its own that a + * service whose player has ended no longer needs the foreground: its rule is "play when ready, and + * READY or BUFFERING" (MediaNotificationManager.shouldRunInForeground), and ENDED is neither, so it + * calls stopForeground. By the time the next video is handed over, the service is out of the + * foreground and Media3 has to put it back. Android 12 and later do not let an app that is in the + * background start a foreground service (the platform's documented restriction; not reproduced on a + * device for this change). Media3 catches that refusal and only logs "Failed to start foreground", + * so the next video goes on playing from a service that is not in the foreground. That a device + * then stops that video a few seconds in is what was reported; the exact way the system stops it + * was not measured. + * + * Holding the foreground across the round trip means the service never leaves it, so the next + * video starts in a service that is already in the foreground. Media3 still repeats its start + * request on each update; whether Android accepts that for a service already in the foreground was + * not measured, and if it refuses, Media3 catches it and the service stays in the foreground, since + * nothing asked it to stop. The hold ends when the next video has been handed over (the player is + * then BUFFERING with play-when-ready set, which Media3's own rule answers yes to once its + * notification controller has seen it), or when the attempt ends without one, in which case + * [onReleasedWithoutHandOver] asks Media3 to look again so the service leaves the foreground + * straight away instead of at its next unrelated update. + * + * In production all four members are used from the main thread: [handOverContext] is the main + * dispatcher, and the main thread is where Media3 asks the service about its notification. + */ + +package com.sulkta.straw.feature.player + +import kotlin.coroutines.CoroutineContext +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job +import kotlinx.coroutines.launch +import kotlinx.coroutines.withTimeoutOrNull + +class AutoplayHandoff( + private val scope: CoroutineScope, + private val handOverContext: CoroutineContext, + /** Upper bound on one attempt. A chosen bound, not a measured resolve time. */ + private val timeoutMs: Long, + private val onReleasedWithoutHandOver: () -> Unit, +) { + /** True from [start] until that attempt hands a video over, gives up, or is cancelled. */ + var inFlight: Boolean = false + private set + + // Bumped by every start and cancel, so an older attempt that finishes late cannot release the + // hold a newer one has taken. + private var attempt = 0 + private var job: Job? = null + + /** + * What the service tells Media3 about the foreground. [media3Says] is Media3's own answer; + * an attempt in flight keeps the foreground even when Media3 would drop it. + */ + fun foregroundRequired(media3Says: Boolean): Boolean = media3Says || inFlight + + /** + * Start an attempt, replacing any attempt still in flight. [resolve] finds the next video + * (null when there is none); [handOver] gives it to the player and returns whether it did. + * The hold is taken before this returns. + */ + fun start(resolve: suspend () -> T?, handOver: (T) -> Boolean) { + val mine = ++attempt + job?.cancel() + inFlight = true + job = scope.launch(handOverContext) { + var handedOver = false + try { + val next = withTimeoutOrNull(timeoutMs) { resolve() } + if (next != null) handedOver = handOver(next) + } catch (c: CancellationException) { + throw c + } catch (_: Throwable) { + // A failed lookup or hand-over ends this attempt like an empty one. + } finally { + if (mine == attempt) { + inFlight = false + job = null + if (!handedOver) onReleasedWithoutHandOver() + } + } + } + } + + /** Drop any attempt in flight without asking Media3 to look again (the service is going away). */ + fun cancel() { + attempt++ + job?.cancel() + job = null + inFlight = false + } +} diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlaybackService.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlaybackService.kt index 3df7e4ff1..f35d51f77 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlaybackService.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlaybackService.kt @@ -50,6 +50,7 @@ import com.sulkta.straw.data.AutoplayMode import com.sulkta.straw.data.History import com.sulkta.straw.data.Resume import com.sulkta.straw.data.Settings +import com.sulkta.straw.feature.detail.ResolvedPlayback import com.sulkta.straw.feature.detail.resolveStreamPlayback import com.sulkta.straw.net.IosSafeHttpDataSource import com.sulkta.straw.net.STRAW_USER_AGENT @@ -71,6 +72,19 @@ class PlaybackService : MediaSessionService() { private var settingsWatcherJob: Job? = null private var resumePollJob: Job? = null + // One autoplay attempt at a time. It keeps the service in the foreground from the end of a + // video until the next one is handed over; see AutoplayHandoff for why. + private val autoplay = AutoplayHandoff( + scope = StrawApp.globalScope, + handOverContext = Dispatchers.Main, + timeoutMs = AUTOPLAY_TIMEOUT_MS, + onReleasedWithoutHandOver = { + // Nothing new is playing, so let Media3 take the service out of the foreground now. + // With false here no foreground start is requested, so this call cannot be refused. + mediaSession?.let { onUpdateNotification(it, false) } + }, + ) + override fun onCreate() { super.onCreate() @@ -269,9 +283,17 @@ class PlaybackService : MediaSessionService() { } } + /** + * Media3 calls this on every notification update with its own answer to "must the service be + * in the foreground". An autoplay attempt in flight overrides a no, so the service does not + * leave the foreground in the gap between one video ending and the next being handed over. + */ + override fun onUpdateNotification(session: MediaSession, startInForegroundRequired: Boolean) { + super.onUpdateNotification(session, autoplay.foregroundRequired(startInForegroundRequired)) + } + private fun tryAutoplay(mode: AutoplayMode) { val current = NowPlaying.current.value ?: return - val uploaderUrl = current.uploaderUrl // The video this autoplay is advancing FROM. Candidate resolution // below is a ~500ms+ network round-trip; if the user manually // starts a different video (or another autoplay fires) in that @@ -283,30 +305,17 @@ class PlaybackService : MediaSessionService() { // We need the channel URL for the SameChannel path; YtRelated // re-resolves the current video's info. If we don't have what // we need, silently bail — better than a half-baked surprise. - val controller = (mediaSession?.player as? Player) ?: return - StrawApp.globalScope.launch { - runCatchingCancellable { - val candidateUrl = withContext(Dispatchers.IO) { - pickAutoplayCandidate(mode, current.streamUrl, uploaderUrl) - } ?: return@runCatchingCancellable - // Final allowlist gate before we hit strawcore with a - // URL whose origin was the extractor. Same defense as - // VideoDetailViewModel.load. / - // HIGH-3 family — every uniffi.strawcore.* site that - // takes a user-influenced URL needs this gate. - if (!isAllowedYtUrl(candidateUrl)) return@runCatchingCancellable - val info = withContext(Dispatchers.IO) { - uniffi.strawcore.streamInfo(candidateUrl) - } - val resolved = resolveStreamPlayback(info) - withContext(Dispatchers.Main) { - // Staleness fence (see triggeredFrom above): only - // advance if the video that ended is still the current - // one. If the user started something else mid-resolve, - // bail rather than hijack it. - if (NowPlaying.current.value?.streamUrl != triggeredFrom) { - return@withContext - } + val controller = mediaSession?.player ?: return + autoplay.start( + resolve = { resolveAutoplayNext(mode, current) }, + handOver = { next -> + // Staleness fence (see triggeredFrom above): only + // advance if the video that ended is still the current + // one. If the user started something else mid-resolve, + // bail rather than hijack it. + if (NowPlaying.current.value?.streamUrl != triggeredFrom) { + false + } else { // setPlayingFrom, NOT enqueueLast — at STATE_ENDED the // just-finished video is still loaded (mediaItemCount==1), // so enqueueLast would only append at index 1 and the @@ -315,16 +324,42 @@ class PlaybackService : MediaSessionService() { // assumed the queue was empty). setPlayingFrom replaces + // prepare()s + plays, which is exactly "play the next one." controller.setPlayingFrom( - streamUrl = candidateUrl, - title = info.title, - uploader = info.uploader, - thumbnail = info.thumbnail, - resolved = resolved, - uploaderUrl = info.uploaderUrl, + streamUrl = next.streamUrl, + title = next.info.title, + uploader = next.info.uploader, + thumbnail = next.info.thumbnail, + resolved = next.resolved, + uploaderUrl = next.info.uploaderUrl, ) + // setPlayingFrom returns early when the resolved + // playback has no stream it can build an item from. + NowPlaying.current.value?.streamUrl == next.streamUrl } - } - } + }, + ) + } + + /** The next video for autoplay, resolved and ready to hand to the player. */ + private class AutoplayNext( + val streamUrl: String, + val info: uniffi.strawcore.StreamInfo, + val resolved: ResolvedPlayback, + ) + + private suspend fun resolveAutoplayNext( + mode: AutoplayMode, + current: NowPlayingItem, + ): AutoplayNext? = withContext(Dispatchers.IO) { + val candidateUrl = pickAutoplayCandidate(mode, current.streamUrl, current.uploaderUrl) + ?: return@withContext null + // Final allowlist gate before we hit strawcore with a + // URL whose origin was the extractor. Same defense as + // VideoDetailViewModel.load. / + // HIGH-3 family — every uniffi.strawcore.* site that + // takes a user-influenced URL needs this gate. + if (!isAllowedYtUrl(candidateUrl)) return@withContext null + val info = uniffi.strawcore.streamInfo(candidateUrl) + AutoplayNext(candidateUrl, info, resolveStreamPlayback(info)) } private suspend fun pickAutoplayCandidate( @@ -405,6 +440,7 @@ class PlaybackService : MediaSessionService() { // away-without-pause case. Read before cancelling the poll // job (the job's last tick may not have landed yet). mediaSession?.player?.let { captureResumePosition(it) } + autoplay.cancel() resumePollJob?.cancel() resumePollJob = null settingsWatcherJob?.cancel() @@ -430,6 +466,12 @@ class PlaybackService : MediaSessionService() { /** Scrub-point write cadence while the player is alive. */ private const val RESUME_POLL_INTERVAL_MS = 5_000L + + /** + * How long one autoplay attempt may hold the foreground before it gives up. A chosen + * bound on a channel lookup plus a stream lookup, not a measured time. + */ + private const val AUTOPLAY_TIMEOUT_MS = 30_000L } } diff --git a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayHandoffTest.kt b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayHandoffTest.kt new file mode 100644 index 000000000..5dfe4ab26 --- /dev/null +++ b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayHandoffTest.kt @@ -0,0 +1,178 @@ +/* + * SPDX-FileCopyrightText: 2026 Sulkta + * SPDX-License-Identifier: GPL-3.0-or-later + * + * The autoplay hand-over keeps the playback service in the foreground from the end of a video until + * the next one is handed to the player, and lets go on every way an attempt can end. + * + * "Media3 says no" below stands for the player having ended: Media3 1.4.1 answers false for + * STATE_ENDED, which is what the service is told while autoplay looks up the next video. + */ + +package com.sulkta.straw.feature.player + +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicInteger +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.awaitCancellation +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class AutoplayHandoffTest { + + private class Harness(timeoutMs: Long = 60_000L) { + val released = AtomicInteger(0) + val releasedLatch = CountDownLatch(1) + val handedOver = mutableListOf() + val handoff = AutoplayHandoff( + scope = CoroutineScope(SupervisorJob() + Dispatchers.Unconfined), + handOverContext = Dispatchers.Unconfined, + timeoutMs = timeoutMs, + onReleasedWithoutHandOver = { + released.incrementAndGet() + releasedLatch.countDown() + }, + ) + + fun accept(next: String): Boolean { + handedOver += next + return true + } + } + + @Test + fun theServiceStaysInTheForegroundUntilTheNextVideoIsHandedOver() { + val h = Harness() + val lookup = CompletableDeferred() + assertFalse("no attempt yet: Media3's no stands", h.handoff.foregroundRequired(false)) + + h.handoff.start(resolve = { lookup.await() }, handOver = h::accept) + + assertTrue("the hold is taken before start returns", h.handoff.inFlight) + assertTrue( + "while the next video is looked up the service must stay in the foreground", + h.handoff.foregroundRequired(false), + ) + assertTrue(h.handedOver.isEmpty()) + + lookup.complete("next") + + assertEquals(listOf("next"), h.handedOver) + assertFalse("handed over: the hold is released", h.handoff.inFlight) + assertFalse(h.handoff.foregroundRequired(false)) + assertEquals("a hand-over leaves the foreground to Media3, it does not ask again", 0, h.released.get()) + } + + @Test + fun media3SayingYesIsNeverOverridden() { + val h = Harness() + assertTrue(h.handoff.foregroundRequired(true)) + val lookup = CompletableDeferred() + h.handoff.start(resolve = { lookup.await() }, handOver = h::accept) + assertTrue(h.handoff.foregroundRequired(true)) + lookup.complete("next") + assertTrue(h.handoff.foregroundRequired(true)) + } + + @Test + fun anAttemptThatFindsNothingLetsTheServiceLeaveTheForeground() { + val h = Harness() + val lookup = CompletableDeferred() + h.handoff.start(resolve = { lookup.await() }, handOver = h::accept) + lookup.complete(null) + + assertTrue(h.handedOver.isEmpty()) + assertFalse(h.handoff.inFlight) + assertFalse(h.handoff.foregroundRequired(false)) + assertEquals("Media3 is asked to look again once", 1, h.released.get()) + } + + @Test + fun aFailedLookupLetsTheServiceLeaveTheForeground() { + val h = Harness() + val lookup = CompletableDeferred() + h.handoff.start(resolve = { lookup.await() }, handOver = h::accept) + lookup.completeExceptionally(IllegalStateException("extractor failed")) + + assertTrue(h.handedOver.isEmpty()) + assertFalse(h.handoff.inFlight) + assertEquals(1, h.released.get()) + } + + @Test + fun aHandOverThatDidNotHappenLetsTheServiceLeaveTheForeground() { + // The player refuses when the user started another video meanwhile, or when the + // resolved playback had no stream to build an item from. + val h = Harness() + val lookup = CompletableDeferred() + h.handoff.start(resolve = { lookup.await() }, handOver = { false }) + lookup.complete("next") + + assertFalse(h.handoff.inFlight) + assertEquals(1, h.released.get()) + } + + @Test + fun aHandOverThatThrowsLetsTheServiceLeaveTheForeground() { + val h = Harness() + val lookup = CompletableDeferred() + h.handoff.start(resolve = { lookup.await() }, handOver = { throw IllegalStateException("player gone") }) + lookup.complete("next") + + assertFalse(h.handoff.inFlight) + assertEquals(1, h.released.get()) + } + + @Test + fun anAttemptThatTakesTooLongGivesUp() { + val h = Harness(timeoutMs = 50L) + h.handoff.start(resolve = { awaitCancellation() }, handOver = h::accept) + assertTrue(h.handoff.inFlight) + + assertTrue("the attempt must give up on its own", h.releasedLatch.await(10, TimeUnit.SECONDS)) + assertFalse(h.handoff.inFlight) + assertTrue(h.handedOver.isEmpty()) + assertEquals(1, h.released.get()) + } + + @Test + fun aNewerAttemptKeepsTheHoldWhenTheOlderOneIsReplaced() { + val h = Harness() + val older = CompletableDeferred() + val newer = CompletableDeferred() + h.handoff.start(resolve = { older.await() }, handOver = h::accept) + h.handoff.start(resolve = { newer.await() }, handOver = h::accept) + + assertTrue("replacing the older attempt must not release the newer one's hold", h.handoff.inFlight) + assertEquals(0, h.released.get()) + + older.complete("stale") + assertTrue("the replaced attempt hands nothing over", h.handedOver.isEmpty()) + assertTrue(h.handoff.inFlight) + + newer.complete("next") + assertEquals(listOf("next"), h.handedOver) + assertFalse(h.handoff.inFlight) + assertEquals(0, h.released.get()) + } + + @Test + fun cancelDropsTheHoldWithoutAskingMedia3Again() { + val h = Harness() + val lookup = CompletableDeferred() + h.handoff.start(resolve = { lookup.await() }, handOver = h::accept) + h.handoff.cancel() + + assertFalse(h.handoff.inFlight) + assertFalse(h.handoff.foregroundRequired(false)) + lookup.complete("next") + assertTrue(h.handedOver.isEmpty()) + assertEquals("the service is going away; nothing to ask Media3", 0, h.released.get()) + } +} diff --git a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt new file mode 100644 index 000000000..d7d1476a7 --- /dev/null +++ b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt @@ -0,0 +1,50 @@ +/* + * SPDX-FileCopyrightText: 2026 Sulkta + * SPDX-License-Identifier: GPL-3.0-or-later + * + * The playback service routes Media3's foreground decision and every autoplay attempt through + * AutoplayHandoff. AutoplayHandoffTest covers what the hand-over does; these checks cover that the + * service uses it, which a refactor could otherwise drop while every other test still passes. + */ + +package com.sulkta.straw.feature.player + +import java.io.File +import org.junit.Assert.assertTrue +import org.junit.Test + +class AutoplayWiringTest { + + private val service: String by lazy { + val f = File("src/main/kotlin/com/sulkta/straw/feature/player/PlaybackService.kt") + assertTrue("source not found: ${f.absolutePath}", f.isFile) + f.readText() + } + + @Test + fun media3sForegroundDecisionGoesThroughTheHandoff() { + assertTrue( + "PlaybackService must pass Media3's answer through autoplay.foregroundRequired", + Regex( + """override fun onUpdateNotification\(\s*session: MediaSession,\s*""" + + """startInForegroundRequired: Boolean,?\s*\)\s*\{\s*super\.onUpdateNotification\(\s*""" + + """session,\s*autoplay\.foregroundRequired\(startInForegroundRequired\),?\s*\)\s*\}""", + ).containsMatchIn(service), + ) + } + + @Test + fun everyAutoplayAttemptGoesThroughTheHandoff() { + assertTrue("tryAutoplay must start its attempt with autoplay.start", service.contains("autoplay.start(")) + assertTrue( + "the release must ask Media3 to look again with false", + Regex("""onReleasedWithoutHandOver = \{[^}]*onUpdateNotification\(it, false\)""").containsMatchIn(service), + ) + } + + @Test + fun theServiceDropsAnAttemptWhenItIsDestroyed() { + val onDestroy = service.substringAfter("override fun onDestroy()", missingDelimiterValue = "") + assertTrue("onDestroy must cancel the attempt", onDestroy.contains("autoplay.cancel()")) + } +} -- 2.49.1 From b0a5e7213115b8d9efc36a0919df4f23dd9ef42b Mon Sep 17 00:00:00 2001 From: Cobb Date: Thu, 22 Oct 2026 17:11:17 -0700 Subject: [PATCH 2/9] test: the autoplay hand-over reports whether the player took the video A mutant that made the hand-over always answer yes survived the suite: setPlayingFrom returns early when it cannot build an item, and a yes then releases the hold without asking Media3 to look again. The service code cannot run in a JVM test, so this checks the source, as the other wiring checks do. --- .../sulkta/straw/feature/player/AutoplayWiringTest.kt | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt index d7d1476a7..6823990e5 100644 --- a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt +++ b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt @@ -42,6 +42,17 @@ class AutoplayWiringTest { ) } + @Test + fun theHandOverReportsWhetherThePlayerTookTheVideo() { + // setPlayingFrom returns early when it cannot build an item. Answering true regardless + // would release the hold without asking Media3 to look again, leaving the service in the + // foreground over an ended player. + assertTrue( + "the hand-over must return whether NowPlaying moved to the new video", + service.contains("NowPlaying.current.value?.streamUrl == next.streamUrl"), + ) + } + @Test fun theServiceDropsAnAttemptWhenItIsDestroyed() { val onDestroy = service.substringAfter("override fun onDestroy()", missingDelimiterValue = "") -- 2.49.1 From 737eb50cf4d6c3aa3c1ff390f2d01a8228df810a Mon Sep 17 00:00:00 2001 From: Cobb Date: Thu, 22 Oct 2026 17:29:30 -0700 Subject: [PATCH 3/9] diag: remove sending logs to a server The owner retired the log service the app sent to, so the "Send logs" button in Settings and the send from the crash handler go, with everything that existed only for them: LogShipper, the crash-handler install in StrawApp, the build-time token (its read in strawApp/build.gradle.kts, the BuildConfig field and the CI env line), LogDump.captureScrubbedLinesBlocking and the second, lighter scrub profile that only the shipper used. Kept, because other code uses it: LogDump.capture and scrubLine (the "Export logs" share sheet and the on-screen error strings), strawLogI, and BuildConfig itself. LogDumpScrubTest now covers the one profile left (still 10 tests). Measured by the builder with this same patch applied to e7ba6bcc3 (the parent of the test-only commit b0a5e7213), not yet on this commit's parent: 46 of 46 unit tests pass; an old-versus-new comparison of the scrubber over 48,212 log lines found 0 differences; assembleDebug passes with the token unset and with a placeholder set; a search of the built APK finds no trace of the log host, the ingest URL, the token or the button text, where a control build of the parent commit has them. --- .forgejo/workflows/build.yml | 4 - strawApp/build.gradle.kts | 25 -- .../main/kotlin/com/sulkta/straw/StrawApp.kt | 7 - .../feature/detail/VideoDetailViewModel.kt | 5 +- .../sulkta/straw/feature/diag/LogShipper.kt | 236 ------------------ .../straw/feature/player/ExpandablePlayer.kt | 4 +- .../straw/feature/player/PlayerScreen.kt | 4 +- .../straw/feature/settings/SettingsScreen.kt | 34 --- .../kotlin/com/sulkta/straw/util/LogDump.kt | 116 ++------- .../com/sulkta/straw/util/LogDumpScrubTest.kt | 54 ++-- 10 files changed, 44 insertions(+), 445 deletions(-) delete mode 100644 strawApp/src/main/kotlin/com/sulkta/straw/feature/diag/LogShipper.kt diff --git a/.forgejo/workflows/build.yml b/.forgejo/workflows/build.yml index 466a663c2..fc330dc97 100644 --- a/.forgejo/workflows/build.yml +++ b/.forgejo/workflows/build.yml @@ -104,10 +104,6 @@ jobs: STRAW_KEYSTORE_PASS: android STRAW_KEY_ALIAS: androiddebugkey STRAW_KEY_PASS: android - # Dogfood log-shipping: the app is compile-DISABLED unless this is set - # (empty ⇒ no crash handler, no network, Settings row hidden). Repo - # secret; absent on contributor forks so their builds don't phone home. - STRAW_LOGS_INGEST_TOKEN: ${{ secrets.STRAW_LOGS_INGEST_TOKEN }} # Keep the 4-ABI cross-compile off the container rootfs. CARGO_TARGET_DIR: ${{ github.workspace }}/cargo-target # ionice idle class + low nice so the build yields disk/CPU to the diff --git a/strawApp/build.gradle.kts b/strawApp/build.gradle.kts index 1ad275dc5..888c544e3 100644 --- a/strawApp/build.gradle.kts +++ b/strawApp/build.gradle.kts @@ -10,7 +10,6 @@ */ import com.android.build.api.dsl.ApplicationExtension -import java.util.Properties plugins { alias(libs.plugins.android.application) @@ -37,30 +36,6 @@ configure { versionCode = STRAW_VERSION_CODE versionName = STRAW_VERSION_NAME resValue("string", "app_name", "Straw") - - // Dogfood log-shipping token (feature/diag/LogShipper.kt). This repo - // is PUBLIC — the token must NEVER be committed, so it's injected at - // build time and defaults to "": - // 1. env var STRAW_LOGS_INGEST_TOKEN — CI: add a Forgejo Actions - // repo secret STRAW_LOGS_INGEST_TOKEN and pass it in the - // "Assemble debug APK" step's env block in - // .forgejo/workflows/build.yml. - // 2. `straw.logs.ingest.token=…` in local.properties (gitignored) - // for local dogfood builds. - // Empty token ⇒ LogShipper is fully disabled (no-op, zero network, - // no Settings row) — contributor builds neither break nor phone home. - val strawLogsToken: String = System.getenv("STRAW_LOGS_INGEST_TOKEN") - ?: run { - val props = Properties() - val f = rootProject.file("local.properties") - if (f.exists()) f.inputStream().use { props.load(it) } - props.getProperty("straw.logs.ingest.token") - } - ?: "" - // Escape so a pathological token can't break out of the generated - // String literal in BuildConfig.java. - val escapedToken = strawLogsToken.replace("\\", "\\\\").replace("\"", "\\\"") - buildConfigField("String", "STRAW_LOGS_TOKEN", "\"$escapedToken\"") } // Explicit signing so CI / release builds reuse ONE keystore instead of diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/StrawApp.kt b/strawApp/src/main/kotlin/com/sulkta/straw/StrawApp.kt index e98f4f64a..e1922b988 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/StrawApp.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/StrawApp.kt @@ -21,7 +21,6 @@ import com.sulkta.straw.data.SearchCache import com.sulkta.straw.data.Settings import com.sulkta.straw.data.Subscriptions import com.sulkta.straw.feature.dataimport.SettingsImport -import com.sulkta.straw.feature.diag.LogShipper import com.sulkta.straw.feature.feed.FeedRefreshScheduler import com.sulkta.straw.feature.update.UpdateScheduler import com.sulkta.straw.feature.update.runUpdateCheck @@ -91,12 +90,6 @@ class StrawApp : Application(), SingletonImageLoader.Factory { override fun onCreate() { super.onCreate() - // Dogfood crash shipping — wrap the default uncaught-exception - // handler FIRST so even an init crash below gets shipped. The - // wrapper always chains to the previous handler (normal crash - // behavior is untouched) and is a no-op in builds without an - // injected STRAW_LOGS_TOKEN. - LogShipper.installCrashHandler(this) // Path C-7: route Rust `log::*` calls into Android logcat under tag // "strawcore". Without this, every log line emitted from rustypipe / // strawcore is silently dropped, making playback regressions invisible diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt index 336c6e6f9..952f29a0f 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt @@ -332,9 +332,8 @@ class VideoDetailViewModel : ViewModel() { if (t is CancellationException) throw t if (_ui.value.loadedUrl != streamUrl) return@launch // FULL scrub — this reason is rendered to the UI below (and so - // is screenshot-reachable). The always-log line seeds the - // dogfood ring with the user-visible extraction failure; the - // ring's own (lighter) scrub applies at send time. + // is screenshot-reachable). The always-log line puts the + // user-visible extraction failure in a log export. val reason = com.sulkta.straw.util.LogDump.scrubLine( t.message ?: t.javaClass.simpleName, ) diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/diag/LogShipper.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/diag/LogShipper.kt deleted file mode 100644 index 26ba8fd75..000000000 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/diag/LogShipper.kt +++ /dev/null @@ -1,236 +0,0 @@ -/* - * SPDX-FileCopyrightText: 2026 Sulkta - * SPDX-License-Identifier: GPL-3.0-or-later - * - * Dogfood log shipping — POST a scrubbed logcat window to Kayos's - * ingest service (logs.sulkta.com). Two triggers: - * - * * crash — a default-UncaughtExceptionHandler wrapper captures the - * log ring + the throwable's stacktrace, attempts one - * hard-capped best-effort send, then chains to the - * previously-installed handler so the process still dies - * (and Android still shows its crash dialog) normally. - * * manual — the Settings → Diagnostics "Send logs to Kayos" button. - * - * PRIVACY: every line — including the crash stacktrace — goes through - * LogDump.scrubLineDogfood before leaving the device (the lighter - * profile for our own logs.sulkta.com: it still redacts every - * credential/URL/token, but keeps bare video/channel/playlist ids as - * outage-triage signal); the server re-scrubs, but the phone is the - * primary gate. device_id is a per-install random - * UUID (no ANDROID_ID / IMEI / hardware fingerprint). And the whole - * feature only exists in builds where BuildConfig.STRAW_LOGS_TOKEN was - * injected (see strawApp/build.gradle.kts) — for everyone else - * `enabled` is a compile-time false, so R8 strips the network path and - * a contributor build cannot phone home. - * - * DELIVERY: strictly best-effort. Any failure (offline, 401/413/429, - * timeout) is swallowed; nothing here may ever crash or block the app. - */ - -package com.sulkta.straw.feature.diag - -import android.content.Context -import com.sulkta.straw.BuildConfig -import com.sulkta.straw.util.LogDump -import com.sulkta.straw.util.runCatchingCancellable -import java.io.PrintWriter -import java.io.StringWriter -import java.text.SimpleDateFormat -import java.util.Date -import java.util.Locale -import java.util.TimeZone -import java.util.UUID -import java.util.concurrent.TimeUnit -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.withContext -import kotlinx.serialization.SerialName -import kotlinx.serialization.Serializable -import kotlinx.serialization.encodeToString -import kotlinx.serialization.json.Json -import okhttp3.MediaType.Companion.toMediaType -import okhttp3.OkHttpClient -import okhttp3.Request -import okhttp3.RequestBody.Companion.toRequestBody - -object LogShipper { - - /** Not a secret — only the bearer token is. */ - private const val INGEST_URL = "https://logs.sulkta.com/ingest" - - /** Server caps: 10k lines / 1 MB body. Stay well under both. */ - private const val MAX_LINES = 2_000 - private const val MAX_BODY_BYTES = 700_000 - - /** Hard cap on how long the crash path may delay process death. */ - private const val CRASH_JOIN_MS = 4_000L - - private const val PREFS = "straw_diag" - private const val KEY_DEVICE_ID = "device_id" - - /** - * Compile-time gate: builds without an injected token have no - * shipping at all — no network, no crash handler, no Settings row. - */ - val enabled: Boolean - get() = BuildConfig.STRAW_LOGS_TOKEN.isNotEmpty() - - // Short timeouts — this is telemetry, not payload. callTimeout is - // the absolute ceiling on the whole request so nothing here can - // hold a WorkManager-style wakelock pattern open. - private val http: OkHttpClient by lazy { - OkHttpClient.Builder() - .connectTimeout(5, TimeUnit.SECONDS) - .writeTimeout(5, TimeUnit.SECONDS) - .readTimeout(5, TimeUnit.SECONDS) - .callTimeout(10, TimeUnit.SECONDS) - .build() - } - - @Serializable - private data class Envelope( - @SerialName("device_id") val deviceId: String, - @SerialName("app_version") val appVersion: String, - @SerialName("sent_at") val sentAt: String, - val reason: String, - val lines: List, - ) - - /** - * Stable per-install id: `straw-`, minted once and - * persisted. Deliberately NOT ANDROID_ID/IMEI — it identifies an - * install for log correlation, never the hardware or the person. - * commit() (not apply()) because both call paths are already on a - * background thread and the crash path dies right after — an - * async write would lose the id and mint a new one per crash. - */ - private fun deviceId(context: Context): String { - val sp = context.getSharedPreferences(PREFS, Context.MODE_PRIVATE) - sp.getString(KEY_DEVICE_ID, null)?.let { return it } - val id = "straw-${UUID.randomUUID()}" - sp.edit().putString(KEY_DEVICE_ID, id).commit() - return id - } - - private fun isoNowUtc(): String = - SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'", Locale.US) - .apply { timeZone = TimeZone.getTimeZone("UTC") } - .format(Date()) - - /** - * Manual trigger (Settings). Captures + scrubs the log ring and - * POSTs it. Returns true iff the server 2xx'd — the UI shows a - * toast either way; failures never throw. - */ - suspend fun send(context: Context, reason: String): Boolean = - withContext(Dispatchers.IO) { - if (!enabled) return@withContext false - runCatchingCancellable { - postLines( - context = context, - reason = reason, - lines = LogDump.captureScrubbedLinesBlocking(MAX_LINES), - ) - }.getOrDefault(false) - } - - /** - * Wrap the process-default uncaught-exception handler. Ship-then- - * chain: whatever happens in our path (including a throw), the - * previous handler ALWAYS runs, so the app still crashes/reports - * exactly as before. No-op without a token. - */ - fun installCrashHandler(context: Context) { - if (!enabled) return - val appContext = context.applicationContext - val previous = Thread.getDefaultUncaughtExceptionHandler() - Thread.setDefaultUncaughtExceptionHandler { thread, throwable -> - try { - shipCrash(appContext, throwable) - } catch (_: Throwable) { - // Never let diagnostics mask the real crash. - } - previous?.uncaughtException(thread, throwable) - } - } - - @Volatile - private var crashShipStarted = false - - /** - * Crash-path send. The crashing thread may be main (network there - * throws NetworkOnMainThreadException) and is about to die either - * way, so the capture+POST runs on a dedicated worker thread and - * we join with a hard cap — the death is delayed at most - * [CRASH_JOIN_MS], never hung. - */ - private fun shipCrash(context: Context, throwable: Throwable) { - // One attempt per process — also breaks recursion if the ship - // path itself crashes on another thread mid-flight. - if (crashShipStarted) return - crashShipStarted = true - val worker = Thread { - try { - val trace = StringWriter().also { - throwable.printStackTrace(PrintWriter(it)) - }.toString() - .split('\n') - // Dogfood profile: this whole shipper targets our own - // logs.sulkta.com, so keep diagnostic ids while still - // scrubbing every credential/URL/token the trace may carry. - .take(400) - .map(LogDump::scrubLineDogfood) - val ring = LogDump.captureScrubbedLinesBlocking(MAX_LINES) - postLines( - context = context, - reason = "crash", - lines = ring + "--- uncaught exception ---" + trace, - ) - } catch (_: Throwable) { - // Best-effort only. - } - } - worker.name = "straw-crash-ship" - worker.isDaemon = true - worker.start() - try { - worker.join(CRASH_JOIN_MS) - } catch (_: InterruptedException) { - Thread.currentThread().interrupt() - } - } - - /** - * Build the envelope and POST. `lines` MUST already be scrubbed - * (both call paths run everything through LogDump.scrubLine). - * Trims oldest-first to stay under the server's 1 MB cap — one - * bounded request, no chunking needed at our line cap. - */ - private fun postLines(context: Context, reason: String, lines: List): Boolean { - var budget = MAX_BODY_BYTES - val kept = ArrayList(lines.size) - for (i in lines.indices.reversed()) { - // +8 ≈ JSON quotes/comma + escaping slack per line. - val cost = lines[i].toByteArray(Charsets.UTF_8).size + 8 - if (cost > budget) break - budget -= cost - kept.add(lines[i]) - } - kept.reverse() - val envelope = Envelope( - deviceId = deviceId(context), - appVersion = "${BuildConfig.VERSION_NAME} (${BuildConfig.VERSION_CODE})", - sentAt = isoNowUtc(), - reason = reason, - lines = kept, - ) - val body = Json.encodeToString(envelope) - .toRequestBody("application/json; charset=utf-8".toMediaType()) - val request = Request.Builder() - .url(INGEST_URL) - .header("Authorization", "Bearer ${BuildConfig.STRAW_LOGS_TOKEN}") - .post(body) - .build() - return http.newCall(request).execute().use { it.isSuccessful } - } -} diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ExpandablePlayer.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ExpandablePlayer.kt index a9b518f46..43b8a50e9 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ExpandablePlayer.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ExpandablePlayer.kt @@ -486,8 +486,8 @@ private fun InlinePlayerSurface( // FULL scrub — this is rendered in the on-screen error banner. val scrubbed = "${error.errorCodeName}: ${LogDump.scrubLine(raw)}" playbackError = scrubbed - // Always-log so the dogfood ring keeps the user-visible - // playback failure (the ring's own scrub applies at send time). + // Always-log so a log export carries the user-visible + // playback failure. strawLogI("StrawPlayer", "playback error: $scrubbed") NowPlaying.clear() } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt index be4724b19..d8d710a2e 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt @@ -135,8 +135,8 @@ fun PlayerScreen( // FULL scrub — this is rendered in the on-screen error banner. val scrubbed = "${error.errorCodeName}: ${LogDump.scrubLine(raw)}" playbackError = scrubbed - // Always-log so the dogfood ring keeps the user-visible - // playback failure (the ring's own scrub applies at send time). + // Always-log so a log export carries the user-visible + // playback failure. strawLogI("StrawPlayer", "playback error: $scrubbed") // Also clear NowPlaying so the minibar doesn't keep // claiming a dead session is loaded. diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/settings/SettingsScreen.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/settings/SettingsScreen.kt index f08def7da..72ddd2f8a 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/settings/SettingsScreen.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/settings/SettingsScreen.kt @@ -66,7 +66,6 @@ import com.sulkta.straw.data.Settings import com.sulkta.straw.data.ThemeMode import com.sulkta.straw.feature.dataimport.ImportResult import com.sulkta.straw.feature.dataimport.SettingsImport -import com.sulkta.straw.feature.diag.LogShipper import com.sulkta.straw.feature.feed.SubscriptionFeedViewModel import com.sulkta.straw.feature.search.SearchViewModel import com.sulkta.straw.util.LogDump @@ -773,39 +772,6 @@ fun SettingsScreen() { Text(if (logDumping) "Exporting…" else "Export logs…") } - // Dogfood log shipping — only rendered in builds with an - // injected ingest token (LogShipper.enabled is compile-time - // false otherwise, so contributor builds show no phantom row). - if (LogShipper.enabled) { - Spacer(modifier = Modifier.height(16.dp)) - Text( - "Or send the same scrubbed log dump straight to Kayos " + - "(logs.sulkta.com) — no share sheet. URLs, video ids, " + - "and anything token-shaped are redacted on-device first.", - style = MaterialTheme.typography.bodySmall, - color = MaterialTheme.colorScheme.onSurfaceVariant, - ) - Spacer(modifier = Modifier.height(12.dp)) - var logSending by remember { mutableStateOf(false) } - OutlinedButton( - enabled = !logSending, - onClick = { - logSending = true - scope.launch { - val ok = LogShipper.send(context, reason = "manual") - logSending = false - Toast.makeText( - context, - if (ok) "Logs sent — thanks!" else "Couldn't reach the log server", - Toast.LENGTH_LONG, - ).show() - } - }, - ) { - Text(if (logSending) "Sending…" else "Send logs to Kayos") - } - } - Spacer(modifier = Modifier.height(32.dp)) Text( "Import from NewPipe / Tubular", diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/util/LogDump.kt b/strawApp/src/main/kotlin/com/sulkta/straw/util/LogDump.kt index cacb6e3f9..2cc01902c 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/util/LogDump.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/util/LogDump.kt @@ -95,38 +95,6 @@ object LogDump { } } - /** - * Pull recent logcat, scrub every line, keep only the newest - * [maxLines]. BLOCKING — callers must already be off the main - * thread (LogShipper runs on Dispatchers.IO; the crash handler - * runs this on its own short-lived worker thread, where a suspend - * fun can't be awaited without an event loop). - * - * Best-effort by design: a logcat exec failure returns whatever - * was read (possibly empty) instead of throwing, so the crash - * shipper can still deliver the stacktrace. - */ - fun captureScrubbedLinesBlocking(maxLines: Int): List { - val ring = ArrayDeque() - runCatching { - val pid = Process.myPid() - val cmd = arrayOf("logcat", "-d", "-v", "threadtime", "--pid=$pid") - val proc = ProcessBuilder(*cmd).redirectErrorStream(true).start() - proc.inputStream.bufferedReader().useLines { lines -> - lines.forEach { line -> - if (ring.size == maxLines) ring.removeFirst() - ring.addLast(line) - } - } - proc.waitFor() - } - // Scrub only the retained window — cheaper than scrubbing lines - // that get dropped, and nothing raw ever leaves this function. - // This is the dogfood ingest path (LogShipper → logs.sulkta.com, our - // own infra), so use the lighter profile that keeps diagnostic ids. - return ring.map(::scrubLineDogfood) - } - /** * Pre-redact known credential- and identity-shaped substrings * before they hit disk or the wire. Cheap line-level pass — @@ -134,13 +102,11 @@ object LogDump { * approach catches every documented leak vector at zero * allocation cost. * - * BIAS: over-redaction. This FULL profile backs the share-sheet - * export and every on-screen error string, which can be handed to a - * chooser app or screenshotted, so a false positive costs a little - * debug signal while a false negative leaks what someone watched. - * Anything URL-, token-, id-, or address-shaped goes. (The dogfood - * ingest path to our own logs.sulkta.com uses the lighter - * [scrubLineDogfood] instead — it keeps bare identifiers.) + * BIAS: over-redaction. This backs the share-sheet export and every + * on-screen error string, which can be handed to a chooser app or + * screenshotted, so a false positive costs a little debug signal + * while a false negative leaks what someone watched. Anything URL-, + * token-, id-, or address-shaped goes. * * Public so error-handler call sites (PlayerScreen / VideoDetail * `playbackError`) can scrub Media3's `PlaybackException.message` @@ -148,30 +114,7 @@ object LogDump { * request URI for HttpDataSource exceptions, which would otherwise * be a leak via screenshot. */ - fun scrubLine(line: String): String = scrub(line, keepIdentifiers = false) - - /** - * DOGFOOD profile — for the logs.sulkta.com ingest path ONLY (our own - * infra, driven by [LogShipper]). Scrubs every real credential / PII - * vector exactly as [scrubLine] does (googlevideo URLs, bearer tokens, - * cred headers/params, signed params, URL tails, emails, high-entropy - * tokens, IPs), but KEEPS the bare YouTube identifiers — 11-char video - * ids, channel/playlist ids, schemeless youtu.be links — because those - * are the diagnostic signal we triage an outage on, and they only ever - * reach a server we run. NEVER use this for the share-sheet export or a - * string rendered to the UI; those keep the FULL [scrubLine] profile. - */ - fun scrubLineDogfood(line: String): String = scrub(line, keepIdentifiers = true) - - /** - * Shared scrub core. [keepIdentifiers] = true selects the lighter - * DOGFOOD profile: it SKIPS the four identifier replacements (schemeless - * youtu.be, channel id, playlist id, bare video id) so those survive, - * while every credential / high-entropy / IP vector is still redacted in - * BOTH profiles. Step ORDER is unchanged from the original full pass, so - * `keepIdentifiers = false` is byte-for-byte the old behavior. - */ - private fun scrub(line: String, keepIdentifiers: Boolean): String { + fun scrubLine(line: String): String { var s = line // Pre-signed googlevideo URLs: keep the host label visible, drop // host prefix + path + query (session-bound streaming creds). @@ -190,45 +133,24 @@ object LogDump { // /vi// thumbnails, search terms, etc. live. s = URL_TAIL_RE.replace(s, "$1/") // Schemeless YT links pasted into log messages ("youtu.be/xyz"). - // KEPT on the dogfood profile — the id is diagnostic signal. - if (!keepIdentifiers) { - s = SCHEMELESS_YT_RE.replace(s, "$1/") - } + s = SCHEMELESS_YT_RE.replace(s, "$1/") // Emails. s = EMAIL_RE.replace(s, "") // YouTube channel / playlist / bare video ids (watch behavior). - // KEPT on the dogfood profile — the signal we triage outages on. - if (!keepIdentifiers) { - s = CHANNEL_ID_RE.replace(s, "") - s = PLAYLIST_ID_RE.replace(s, "") - // Bare 11-char video-id-shaped tokens ("dQw4w9WgXcQ") logged - // outside any URL (rustypipe/strawcore do this). Requiring a - // digit/_/- inside the run spares ordinary 11-letter words; - // real ids virtually always contain one. Pure-alpha ids are - // still caught earlier when keyed (v=…) or inside a URL. - s = VIDEO_ID_CANDIDATE_RE.replace(s) { m -> - if (m.value.any { it.isDigit() || it == '_' || it == '-' }) "" else m.value - } + s = CHANNEL_ID_RE.replace(s, "") + s = PLAYLIST_ID_RE.replace(s, "") + // Bare 11-char video-id-shaped tokens ("dQw4w9WgXcQ") logged + // outside any URL (rustypipe/strawcore do this). Requiring a + // digit/_/- inside the run spares ordinary 11-letter words; + // real ids virtually always contain one. Pure-alpha ids are + // still caught earlier when keyed (v=…) or inside a URL. + s = VIDEO_ID_CANDIDATE_RE.replace(s) { m -> + if (m.value.any { it.isDigit() || it == '_' || it == '-' }) "" else m.value } // Long high-entropy runs (hashes, visitor data, unlabeled - // tokens): 20+ [A-Za-z0-9_-] chars containing a digit. Scrubbed in - // BOTH profiles — visitorData & friends are creds, not signal. On the - // dogfood profile a token that is exactly a channel/playlist id - // (UC…, PL…/UU…/LL…/RD…/OLAK5uy_…) is KEPT — those ride past the - // skipped id passes above but LONG_TOKEN would otherwise redact them, - // and they're the same triage signal as the video id we keep. Every - // other high-entropy run (visitorData, hashes) still redacts. - s = if (keepIdentifiers) { - LONG_TOKEN_RE.replace(s) { m -> - if (CHANNEL_ID_RE.matches(m.value) || PLAYLIST_ID_RE.matches(m.value)) { - m.value - } else { - "" - } - } - } else { - LONG_TOKEN_RE.replace(s, "") - } + // tokens): 20+ [A-Za-z0-9_-] chars containing a digit. visitorData + // and friends are creds, not signal. + s = LONG_TOKEN_RE.replace(s, "") // IP addresses (v4 + v6 — v6 patterns are shaped so threadtime // HH:MM:SS timestamps can never match). s = IPV4_RE.replace(s, "") diff --git a/strawApp/src/test/kotlin/com/sulkta/straw/util/LogDumpScrubTest.kt b/strawApp/src/test/kotlin/com/sulkta/straw/util/LogDumpScrubTest.kt index a59a667d8..e482b99db 100644 --- a/strawApp/src/test/kotlin/com/sulkta/straw/util/LogDumpScrubTest.kt +++ b/strawApp/src/test/kotlin/com/sulkta/straw/util/LogDumpScrubTest.kt @@ -2,9 +2,9 @@ * SPDX-FileCopyrightText: 2026 Sulkta * SPDX-License-Identifier: GPL-3.0-or-later * - * Unit tests for LogDump's two scrub profiles. + * Unit tests for LogDump's scrub profile. * - * These are pure-JVM tests over LogDump.scrubLine / scrubLineDogfood — the + * These are pure-JVM tests over LogDump.scrubLine. The * regexes have no Android dependency, so they run under the standard * `:strawApp:testDebugUnitTest` task with junit on the test classpath. * @@ -29,9 +29,8 @@ class LogDumpScrubTest { fun rustPathWithDoubleColonSurvives() { // The bug: `strawcore::stream` was scrubbed to `strawcorstream` // because `e::` read as a compressed IPv6. It must now pass through - // untouched in BOTH profiles. + // untouched. assertEquals("strawcore::stream", LogDump.scrubLine("strawcore::stream")) - assertEquals("strawcore::stream", LogDump.scrubLineDogfood("strawcore::stream")) // A three-segment path — `::foo` must not clip `::f` (foo starts with // the hex letter `f`). assertEquals("strawcore::stream::foo", LogDump.scrubLine("strawcore::stream::foo")) @@ -53,69 +52,54 @@ class LogDumpScrubTest { assertFalse(LogDump.scrubLine("bound peer fe80::1 up").contains("fe80")) } - // ---- B2: dogfood profile KEEPS diagnostic identifiers ---------------- + // ---- B2: identifiers and credentials are redacted ------------------- @Test - fun dogfoodKeepsBareVideoIdFullScrubs() { - val id = "dQw4w9WgXcQ" - assertEquals(id, LogDump.scrubLineDogfood(id)) - assertEquals("", LogDump.scrubLine(id)) + fun bareVideoIdScrubs() { + assertEquals("", LogDump.scrubLine("dQw4w9WgXcQ")) } @Test - fun dogfoodKeepsYoutubeShortLinkFullScrubs() { + fun schemelessYoutubeLinkScrubs() { val line = "open youtu.be/dQw4w9WgXcQ now" - // Dogfood: the youtu.be/ stays — the id is the diagnostic signal. - assertEquals(line, LogDump.scrubLineDogfood(line)) - // Full: the schemeless YT link is redacted. assertTrue(LogDump.scrubLine(line).contains("")) assertFalse(LogDump.scrubLine(line).contains("dQw4w9WgXcQ")) } @Test - fun dogfoodKeepsChannelAndPlaylistIdFullScrubs() { - // These are ≥20 chars so LONG_TOKEN would otherwise redact them even - // on the dogfood profile; the channel/playlist exemption keeps them. + fun channelAndPlaylistIdsScrubToTheirPlaceholders() { val channelId = "UCuAXFkgsw1L7xaCfnd5JJOw" // UC + 22, has digits val playlistId = "PLbpi6ZahtOH6Blw3RGYpWkSByi_T7Rygb" - assertEquals(channelId, LogDump.scrubLineDogfood(channelId)) - assertEquals(playlistId, LogDump.scrubLineDogfood(playlistId)) - // Full profile redacts them to their labelled placeholders. assertEquals("", LogDump.scrubLine(channelId)) assertEquals("", LogDump.scrubLine(playlistId)) } @Test - fun dogfoodStillScrubsGenericHighEntropyToken() { - // visitorData-shaped: ≥20 chars with a digit, NOT a channel/playlist - // prefix → still `` on the dogfood profile (it's a cred). - val visitorData = "CgtVQzEyMzQ1Njc4OTBhYg" - assertEquals("", LogDump.scrubLineDogfood(visitorData)) - assertEquals("", LogDump.scrubLine(visitorData)) + fun highEntropyTokenScrubs() { + // visitorData-shaped: 20+ chars with a digit, not a channel/playlist prefix. + assertEquals("", LogDump.scrubLine("CgtVQzEyMzQ1Njc4OTBhYg")) } - // ---- B2: dogfood profile STILL scrubs real credentials / PII --------- - @Test - fun dogfoodStillScrubsSignedGooglevideoUrl() { + fun signedGooglevideoUrlScrubs() { val line = "url=https://r5---sn-abc.googlevideo.com/videoplayback?expire=99&sig=DEADBEEFSIG&pot=SECRETPOT" - val out = LogDump.scrubLineDogfood(line) + val out = LogDump.scrubLine(line) assertTrue(out.contains("")) assertFalse(out.contains("DEADBEEFSIG")) assertFalse(out.contains("SECRETPOT")) } @Test - fun dogfoodStillScrubsBearerToken() { - val out = LogDump.scrubLineDogfood("hdr Authorization: Bearer ya29.A0ARLongTokenValue123") + fun bearerTokenScrubs() { + val out = LogDump.scrubLine("hdr Authorization: Bearer ya29.A0ARLongTokenValue123") assertTrue(out.contains("")) assertFalse(out.contains("ya29")) } @Test - fun dogfoodStillScrubsSignedParams() { - val out = LogDump.scrubLineDogfood("req q&sig=DEADBEEF123&pot=POTVAL456 done") + fun signedParamsScrub() { + val out = LogDump.scrubLine("req q&sig=DEADBEEF123&pot=POTVAL456 done") assertTrue(out.contains("sig=")) assertTrue(out.contains("pot=")) assertFalse(out.contains("DEADBEEF123")) @@ -123,8 +107,8 @@ class LogDumpScrubTest { } @Test - fun dogfoodStillScrubsIpAddresses() { - val out = LogDump.scrubLineDogfood("peer 203.0.113.7 and fe80::1 up") + fun ipAddressesScrub() { + val out = LogDump.scrubLine("peer 203.0.113.7 and fe80::1 up") assertTrue(out.contains("")) assertFalse(out.contains("203.0.113.7")) assertFalse(out.contains("fe80")) -- 2.49.1 From c62f195489e54571710cbc3b86143d12b774dbb7 Mon Sep 17 00:00:00 2001 From: Cobb Date: Thu, 22 Oct 2026 18:45:47 -0700 Subject: [PATCH 4/9] test: run the service's autoplay decisions in JVM tests, not text matches The audit of b0a5e7213 found the playback service's autoplay wiring guarded by matches on its source text. A comment, a dead branch or "|| true" passed them and a harmless rewrite failed them, and nothing tested how the service builds and drives its handoff: the 30 second bound, the main-thread hand-over, the end of a video starting an attempt, the staleness fence, the allowlist gate. What the service decides about autoplay moves, unchanged in behaviour, to ServiceAutoplay.kt: how it builds its AutoplayHandoff (hand-overs on Dispatchers.Main, the 30 second bound, a release that asks Media3 again with false), which player state starts an attempt, the allowlist gate on the next video, and the fence on the hand-over with the check that the player took the video. ServiceAutoplayTest runs them with Dispatchers.Main replaced by a test dispatcher on virtual time (kotlinx-coroutines-test, a new test-only dependency). What stays in PlaybackService is its connection to Media3: the onUpdateNotification override, building the handoff, attaching the end-of-video listener, tryAutoplay, and the cancel in onDestroy. AutoplayWiringTest now reads those from the compiled class instead of the source text (CompiledReach: what a method calls, following the class's own helpers, lambdas and nested classes). It sees calls, not values. AutoplayHandoffTest now dispatches the way the main thread does (a launched attempt runs later, never inside start) on virtual time, and adds: the release asks Media3 after the hold is dropped; a failed lookup or hand-over does not reach the scope's exception handler; an attempt gives up at its bound and not a millisecond before. One test keeps Dispatchers.Unconfined on purpose, for a dispatcher that resumes the replaced attempt inside job.cancel(). Correction to b0a5e7213: its message says "The service code cannot run in a JVM test, so this checks the source". The service class cannot run there, but the decisions in it can once they are taken out of it, which is what this commit does; and the source check that commit added was passed by a comment or a dead branch, as the audit measured. Measured in throwaway build containers (mutants one at a time, the test JVM under ulimit -v 8388608, MALLOC_ARENA_MAX=2, timeout 900). At 737eb50 the shipped suite (46 tests) let the audit's mutants H06, H11, H15, H28, S03, S04, S05, S08, S09, S10, S11, S12, S13 and S14 survive and failed on its harmless rewrites F01, F02 and F03. On this tree: 62 of 62 JVM tests pass; the service mutants passing Media3's answer straight through, building the release with no session or a no-op update, commenting out or removing the onDestroy cancel, and wrapping it in "if (false)" all fail AutoplayWiringTest; commenting out the update argument does not compile. The full mutant run is at the head of this round. --- strawApp/build.gradle.kts | 2 + .../straw/feature/player/PlaybackService.kt | 81 ++--- .../straw/feature/player/ServiceAutoplay.kt | 114 +++++++ .../feature/player/AutoplayHandoffTest.kt | 106 +++++- .../feature/player/AutoplayWiringTest.kt | 79 +++-- .../straw/feature/player/CompiledReach.kt | 218 +++++++++++++ .../feature/player/ServiceAutoplayTest.kt | 302 ++++++++++++++++++ 7 files changed, 807 insertions(+), 95 deletions(-) create mode 100644 strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ServiceAutoplay.kt create mode 100644 strawApp/src/test/kotlin/com/sulkta/straw/feature/player/CompiledReach.kt create mode 100644 strawApp/src/test/kotlin/com/sulkta/straw/feature/player/ServiceAutoplayTest.kt diff --git a/strawApp/build.gradle.kts b/strawApp/build.gradle.kts index 888c544e3..939274db5 100644 --- a/strawApp/build.gradle.kts +++ b/strawApp/build.gradle.kts @@ -184,6 +184,8 @@ dependencies { // Before this line that source set had no test framework, so it was // never compiled or run (the note at the top of LogDumpScrubTest). testImplementation(libs.junit) + // Virtual time and a stand-in for Dispatchers.Main in the autoplay tests. + testImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-test:1.11.0") } // ============================================================================= diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlaybackService.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlaybackService.kt index f35d51f77..4cfc675d2 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlaybackService.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlaybackService.kt @@ -73,16 +73,12 @@ class PlaybackService : MediaSessionService() { private var resumePollJob: Job? = null // One autoplay attempt at a time. It keeps the service in the foreground from the end of a - // video until the next one is handed over; see AutoplayHandoff for why. - private val autoplay = AutoplayHandoff( + // video until the next one is handed over; see AutoplayHandoff for why. When an attempt ends + // without a hand-over it asks Media3 again through onUpdateNotification below. + private val autoplay = serviceAutoplayHandoff( scope = StrawApp.globalScope, - handOverContext = Dispatchers.Main, - timeoutMs = AUTOPLAY_TIMEOUT_MS, - onReleasedWithoutHandOver = { - // Nothing new is playing, so let Media3 take the service out of the foreground now. - // With false here no foreground start is requested, so this call cannot be refused. - mediaSession?.let { onUpdateNotification(it, false) } - }, + session = { mediaSession }, + updateNotification = ::onUpdateNotification, ) override fun onCreate() { @@ -167,14 +163,6 @@ class PlaybackService : MediaSessionService() { // press), kick off a background fetch and re-claim with the // freshened segments. NowPlaying.claim handles the // "same-streamUrl with fresher metadata" case via its CAS. - // - // Autoplay at end-of-queue: when STATE_ENDED fires and there's - // no next item in the queue, consult Settings.autoplayMode and - // pick a candidate. SameChannel → call channelInfo on the - // current uploader, take the first un-watched (gated on - // autoplaySkipWatched). YtRelated → would re-call streamInfo - // and pick info.related[0] but strawcore returns empty for - // related today, so it's a no-op until that lands. player.addListener(object : Player.Listener { override fun onMediaItemTransition(item: MediaItem?, reason: Int) { if (item == null) return @@ -212,22 +200,20 @@ class PlaybackService : MediaSessionService() { // covers the play-through case. if (!isPlaying) captureResumePosition(player) } - - override fun onPlaybackStateChanged(state: Int) { - if (state != Player.STATE_ENDED) return - val mode = Settings.get().autoplayMode.value - if (mode == AutoplayMode.Off) return - // Media3 auto-advances inside the queue; we only kick - // in when the queue has truly run out. mediaItemCount - // hits 0 after the engine reports STATE_ENDED in some - // edge cases — handle both. - val atEnd = player.mediaItemCount <= 1 || - player.currentMediaItemIndex >= player.mediaItemCount - 1 - if (!atEnd) return - tryAutoplay(mode) - } }) + // Autoplay at end-of-queue: when STATE_ENDED fires and there's + // no next item in the queue, consult Settings.autoplayMode and + // pick a candidate. SameChannel: call channelInfo on the + // current uploader, take the first un-watched (gated on + // autoplaySkipWatched). YtRelated: would re-call streamInfo + // and pick info.related[0] but strawcore returns empty for + // related today, so it's a no-op until that lands. + player.startAutoplayWhenItEnds( + mode = { Settings.get().autoplayMode.value }, + start = ::tryAutoplay, + ) + // Periodic scrub-point write. Stays on Main so player reads are // thread-safe; the SP write inside record() is async (apply()). // 5s cadence is the sweet spot — finer is wasted disk churn, @@ -313,9 +299,7 @@ class PlaybackService : MediaSessionService() { // advance if the video that ended is still the current // one. If the user started something else mid-resolve, // bail rather than hijack it. - if (NowPlaying.current.value?.streamUrl != triggeredFrom) { - false - } else { + handOverIfUnchanged(from = triggeredFrom, nextUrl = next.streamUrl) { // setPlayingFrom, NOT enqueueLast — at STATE_ENDED the // just-finished video is still loaded (mediaItemCount==1), // so enqueueLast would only append at index 1 and the @@ -331,9 +315,6 @@ class PlaybackService : MediaSessionService() { resolved = next.resolved, uploaderUrl = next.info.uploaderUrl, ) - // setPlayingFrom returns early when the resolved - // playback has no stream it can build an item from. - NowPlaying.current.value?.streamUrl == next.streamUrl } }, ) @@ -350,16 +331,16 @@ class PlaybackService : MediaSessionService() { mode: AutoplayMode, current: NowPlayingItem, ): AutoplayNext? = withContext(Dispatchers.IO) { - val candidateUrl = pickAutoplayCandidate(mode, current.streamUrl, current.uploaderUrl) - ?: return@withContext null - // Final allowlist gate before we hit strawcore with a - // URL whose origin was the extractor. Same defense as - // VideoDetailViewModel.load. / - // HIGH-3 family — every uniffi.strawcore.* site that - // takes a user-influenced URL needs this gate. - if (!isAllowedYtUrl(candidateUrl)) return@withContext null - val info = uniffi.strawcore.streamInfo(candidateUrl) - AutoplayNext(candidateUrl, info, resolveStreamPlayback(info)) + // resolveAllowedCandidate is the final allowlist gate before strawcore sees a URL + // whose origin was the extractor (HIGH-3 family: a uniffi.strawcore.* site that takes + // a user-influenced URL needs this gate). + resolveAllowedCandidate( + pick = { pickAutoplayCandidate(mode, current.streamUrl, current.uploaderUrl) }, + resolve = { candidateUrl -> + val info = uniffi.strawcore.streamInfo(candidateUrl) + AutoplayNext(candidateUrl, info, resolveStreamPlayback(info)) + }, + ) } private suspend fun pickAutoplayCandidate( @@ -466,12 +447,6 @@ class PlaybackService : MediaSessionService() { /** Scrub-point write cadence while the player is alive. */ private const val RESUME_POLL_INTERVAL_MS = 5_000L - - /** - * How long one autoplay attempt may hold the foreground before it gives up. A chosen - * bound on a channel lookup plus a stream lookup, not a measured time. - */ - private const val AUTOPLAY_TIMEOUT_MS = 30_000L } } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ServiceAutoplay.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ServiceAutoplay.kt new file mode 100644 index 000000000..bbae5c55e --- /dev/null +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ServiceAutoplay.kt @@ -0,0 +1,114 @@ +/* + * SPDX-FileCopyrightText: 2026 Sulkta + * SPDX-License-Identifier: GPL-3.0-or-later + * + * The playback service's side of autoplay, kept out of PlaybackService so that a JVM test can run + * it: how the service builds its AutoplayHandoff, when a player state starts an attempt, the + * allowlist gate on the next video, and when the hand-over may go ahead. PlaybackService only + * connects these to Media3; AutoplayWiringTest checks those connections in the compiled service. + */ + +package com.sulkta.straw.feature.player + +import androidx.media3.common.Player +import com.sulkta.straw.data.AutoplayMode +import com.sulkta.straw.util.isAllowedYtUrl +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers + +/** + * How long one autoplay attempt may hold the foreground before it gives up. A chosen bound on a + * channel lookup plus a stream lookup, not a measured time. + */ +internal const val AUTOPLAY_TIMEOUT_MS = 30_000L + +/** + * The handoff PlaybackService uses. Hand-overs run on the main thread, the thread the player was + * built on (ExoPlayer throws when it is used from another one), and an attempt is bounded by + * [AUTOPLAY_TIMEOUT_MS]. + * + * When an attempt ends without a hand-over, [updateNotification] is called with the service's + * current [session] and false, so that Media3 decides again whether the service stays in the + * foreground, now rather than at its next unrelated update. With false, Media3 1.4.1 updates the + * notification and leaves the foreground only if no session should run in it, instead of starting + * the foreground. + */ +internal fun serviceAutoplayHandoff( + scope: CoroutineScope, + session: () -> S?, + updateNotification: (S, Boolean) -> Unit, +): AutoplayHandoff = AutoplayHandoff( + scope = scope, + handOverContext = Dispatchers.Main, + timeoutMs = AUTOPLAY_TIMEOUT_MS, + onReleasedWithoutHandOver = { session()?.let { updateNotification(it, false) } }, +) + +/** + * The mode to start an autoplay attempt with after the player reports [playbackState], or null + * for none. Only an ended player at the last item of its queue starts one (inside the queue Media3 + * advances on its own), and only with autoplay on. [mode] is read only when the player has ended. + */ +internal fun autoplayModeAfter( + playbackState: Int, + mediaItemCount: Int, + currentMediaItemIndex: Int, + mode: () -> AutoplayMode, +): AutoplayMode? { + if (playbackState != Player.STATE_ENDED) return null + val m = mode() + if (m == AutoplayMode.Off) return null + // mediaItemCount can be 0 after the player reports STATE_ENDED in some edge cases; handle both. + val atEnd = mediaItemCount <= 1 || currentMediaItemIndex >= mediaItemCount - 1 + return if (atEnd) m else null +} + +/** + * Calls [start] with the autoplay mode each time this player ends at the last item of its queue + * with autoplay on (see [autoplayModeAfter]). + */ +internal fun Player.startAutoplayWhenItEnds(mode: () -> AutoplayMode, start: (AutoplayMode) -> Unit) { + val player = this + addListener( + object : Player.Listener { + override fun onPlaybackStateChanged(playbackState: Int) { + autoplayModeAfter( + playbackState, + player.mediaItemCount, + player.currentMediaItemIndex, + mode, + )?.let(start) + } + }, + ) +} + +/** + * The next video for autoplay: [pick] chooses its URL (null when there is none) and [resolve] + * looks it up. The URL came from the extractor, so one outside the YouTube allowlist is dropped + * before it reaches [resolve], the same gate VideoDetailViewModel.load applies before its own + * lookup. + */ +internal suspend fun resolveAllowedCandidate( + pick: suspend () -> String?, + resolve: suspend (String) -> R, +): R? { + val url = pick() ?: return null + if (!isAllowedYtUrl(url)) return null + return resolve(url) +} + +/** + * Hands the next video to the player through [play] if NowPlaying still names [from], the video + * that ended, and returns whether the player took it. If NowPlaying moved on while the next one was + * looked up (the user started another video, or closed the player), that stands. + * + * The player took the video when NowPlaying names [nextUrl] afterwards: [play] (setPlayingFrom) + * returns early, leaving NowPlaying as it was, when the resolved playback has no stream it can + * build an item from. + */ +internal fun handOverIfUnchanged(from: String, nextUrl: String, play: () -> Unit): Boolean { + if (NowPlaying.current.value?.streamUrl != from) return false + play() + return NowPlaying.current.value?.streamUrl == nextUrl +} diff --git a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayHandoffTest.kt b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayHandoffTest.kt index 5dfe4ab26..662187f40 100644 --- a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayHandoffTest.kt +++ b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayHandoffTest.kt @@ -7,43 +7,52 @@ * * "Media3 says no" below stands for the player having ended: Media3 1.4.1 answers false for * STATE_ENDED, which is what the service is told while autoplay looks up the next video. + * + * The harness dispatches the way the main thread does in production: a launched attempt runs when + * the test lets the scheduler run, never inside start(), and its timeout runs on virtual time. */ package com.sulkta.straw.feature.player -import java.util.concurrent.CountDownLatch -import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicInteger +import java.util.concurrent.atomic.AtomicReference import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.CoroutineExceptionHandler import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.awaitCancellation +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.TestCoroutineScheduler import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Test class AutoplayHandoffTest { private class Harness(timeoutMs: Long = 60_000L) { + val scheduler = TestCoroutineScheduler() + val escaped = AtomicReference(null) val released = AtomicInteger(0) - val releasedLatch = CountDownLatch(1) val handedOver = mutableListOf() + private val main = StandardTestDispatcher(scheduler) val handoff = AutoplayHandoff( - scope = CoroutineScope(SupervisorJob() + Dispatchers.Unconfined), - handOverContext = Dispatchers.Unconfined, + scope = CoroutineScope( + SupervisorJob() + main + CoroutineExceptionHandler { _, t -> escaped.set(t) }, + ), + handOverContext = main, timeoutMs = timeoutMs, - onReleasedWithoutHandOver = { - released.incrementAndGet() - releasedLatch.countDown() - }, + onReleasedWithoutHandOver = { released.incrementAndGet() }, ) fun accept(next: String): Boolean { handedOver += next return true } + + fun run() = scheduler.runCurrent() } @Test @@ -54,7 +63,8 @@ class AutoplayHandoffTest { h.handoff.start(resolve = { lookup.await() }, handOver = h::accept) - assertTrue("the hold is taken before start returns", h.handoff.inFlight) + assertTrue("the hold is taken before start returns, before the attempt runs", h.handoff.inFlight) + h.run() assertTrue( "while the next video is looked up the service must stay in the foreground", h.handoff.foregroundRequired(false), @@ -62,6 +72,7 @@ class AutoplayHandoffTest { assertTrue(h.handedOver.isEmpty()) lookup.complete("next") + h.run() assertEquals(listOf("next"), h.handedOver) assertFalse("handed over: the hold is released", h.handoff.inFlight) @@ -75,8 +86,10 @@ class AutoplayHandoffTest { assertTrue(h.handoff.foregroundRequired(true)) val lookup = CompletableDeferred() h.handoff.start(resolve = { lookup.await() }, handOver = h::accept) + h.run() assertTrue(h.handoff.foregroundRequired(true)) lookup.complete("next") + h.run() assertTrue(h.handoff.foregroundRequired(true)) } @@ -85,7 +98,9 @@ class AutoplayHandoffTest { val h = Harness() val lookup = CompletableDeferred() h.handoff.start(resolve = { lookup.await() }, handOver = h::accept) + h.run() lookup.complete(null) + h.run() assertTrue(h.handedOver.isEmpty()) assertFalse(h.handoff.inFlight) @@ -93,16 +108,40 @@ class AutoplayHandoffTest { assertEquals("Media3 is asked to look again once", 1, h.released.get()) } + @Test + fun theReleaseAsksMedia3AfterTheHoldIsDropped() { + // In the service the release calls onUpdateNotification, which asks foregroundRequired + // what to tell Media3. Asked while the hold was still up, it would keep the service in the + // foreground over an ended player. + val answer = AtomicReference(null) + val scheduler = TestCoroutineScheduler() + val main = StandardTestDispatcher(scheduler) + lateinit var handoff: AutoplayHandoff + handoff = AutoplayHandoff( + scope = CoroutineScope(SupervisorJob() + main), + handOverContext = main, + timeoutMs = 60_000L, + onReleasedWithoutHandOver = { answer.set(handoff.foregroundRequired(false)) }, + ) + handoff.start(resolve = { null as String? }, handOver = { true }) + scheduler.runCurrent() + + assertEquals("the release must tell Media3 no", false, answer.get()) + } + @Test fun aFailedLookupLetsTheServiceLeaveTheForeground() { val h = Harness() val lookup = CompletableDeferred() h.handoff.start(resolve = { lookup.await() }, handOver = h::accept) + h.run() lookup.completeExceptionally(IllegalStateException("extractor failed")) + h.run() assertTrue(h.handedOver.isEmpty()) assertFalse(h.handoff.inFlight) assertEquals(1, h.released.get()) + assertNull("a failed lookup ends the attempt inside the handoff", h.escaped.get()) } @Test @@ -112,7 +151,9 @@ class AutoplayHandoffTest { val h = Harness() val lookup = CompletableDeferred() h.handoff.start(resolve = { lookup.await() }, handOver = { false }) + h.run() lookup.complete("next") + h.run() assertFalse(h.handoff.inFlight) assertEquals(1, h.released.get()) @@ -123,20 +164,29 @@ class AutoplayHandoffTest { val h = Harness() val lookup = CompletableDeferred() h.handoff.start(resolve = { lookup.await() }, handOver = { throw IllegalStateException("player gone") }) + h.run() lookup.complete("next") + h.run() assertFalse(h.handoff.inFlight) assertEquals(1, h.released.get()) + assertNull("a failed hand-over ends the attempt inside the handoff", h.escaped.get()) } @Test - fun anAttemptThatTakesTooLongGivesUp() { - val h = Harness(timeoutMs = 50L) + fun anAttemptGivesUpAtItsBoundAndNotBefore() { + val h = Harness(timeoutMs = 30_000L) h.handoff.start(resolve = { awaitCancellation() }, handOver = h::accept) - assertTrue(h.handoff.inFlight) + h.run() - assertTrue("the attempt must give up on its own", h.releasedLatch.await(10, TimeUnit.SECONDS)) - assertFalse(h.handoff.inFlight) + h.scheduler.advanceTimeBy(29_999L) + h.run() + assertTrue("one millisecond before the bound the attempt still holds", h.handoff.inFlight) + assertEquals(0, h.released.get()) + + h.scheduler.advanceTimeBy(1L) + h.run() + assertFalse("at the bound the attempt gives up", h.handoff.inFlight) assertTrue(h.handedOver.isEmpty()) assertEquals(1, h.released.get()) } @@ -147,31 +197,57 @@ class AutoplayHandoffTest { val older = CompletableDeferred() val newer = CompletableDeferred() h.handoff.start(resolve = { older.await() }, handOver = h::accept) + h.run() h.handoff.start(resolve = { newer.await() }, handOver = h::accept) + h.run() assertTrue("replacing the older attempt must not release the newer one's hold", h.handoff.inFlight) assertEquals(0, h.released.get()) older.complete("stale") + h.run() assertTrue("the replaced attempt hands nothing over", h.handedOver.isEmpty()) assertTrue(h.handoff.inFlight) newer.complete("next") + h.run() assertEquals(listOf("next"), h.handedOver) assertFalse(h.handoff.inFlight) assertEquals(0, h.released.get()) } + @Test + fun aReplacedAttemptCannotReleaseTheNewerHoldOnADispatcherThatResumesAtOnce() { + // Production passes Dispatchers.Main, not Dispatchers.Main.immediate. A dispatcher that + // resumes at once (Unconfined here) runs the cancelled attempt's cleanup inside start's + // job.cancel(), so start must claim the newer attempt before it cancels the older one. + val released = AtomicInteger(0) + val handoff = AutoplayHandoff( + scope = CoroutineScope(SupervisorJob() + Dispatchers.Unconfined), + handOverContext = Dispatchers.Unconfined, + timeoutMs = 60_000L, + onReleasedWithoutHandOver = { released.incrementAndGet() }, + ) + val older = CompletableDeferred() + handoff.start(resolve = { older.await() }, handOver = { true }) + handoff.start(resolve = { CompletableDeferred().await() }, handOver = { true }) + + assertTrue(handoff.inFlight) + assertEquals(0, released.get()) + } + @Test fun cancelDropsTheHoldWithoutAskingMedia3Again() { val h = Harness() val lookup = CompletableDeferred() h.handoff.start(resolve = { lookup.await() }, handOver = h::accept) + h.run() h.handoff.cancel() assertFalse(h.handoff.inFlight) assertFalse(h.handoff.foregroundRequired(false)) lookup.complete("next") + h.run() assertTrue(h.handedOver.isEmpty()) assertEquals("the service is going away; nothing to ask Media3", 0, h.released.get()) } diff --git a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt index 6823990e5..55153a731 100644 --- a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt +++ b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt @@ -2,60 +2,85 @@ * SPDX-FileCopyrightText: 2026 Sulkta * SPDX-License-Identifier: GPL-3.0-or-later * - * The playback service routes Media3's foreground decision and every autoplay attempt through - * AutoplayHandoff. AutoplayHandoffTest covers what the hand-over does; these checks cover that the - * service uses it, which a refactor could otherwise drop while every other test still passes. + * PlaybackService is a framework service and cannot run in a JVM test, so everything it decides + * about autoplay lives in ServiceAutoplay.kt and AutoplayHandoff.kt, where ServiceAutoplayTest and + * AutoplayHandoffTest run it. What stays in the service is the connection to Media3: these checks + * read the compiled service (CompiledReach) for those connections. They see calls, not values: a + * call whose result is computed and then not used would still pass. */ package com.sulkta.straw.feature.player -import java.io.File +import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Test class AutoplayWiringTest { - private val service: String by lazy { - val f = File("src/main/kotlin/com/sulkta/straw/feature/player/PlaybackService.kt") - assertTrue("source not found: ${f.absolutePath}", f.isFile) - f.readText() - } + private val reach = CompiledReach(AutoplayWiringTest::class.java.classLoader!!) + + private val service = "com/sulkta/straw/feature/player/PlaybackService" + private val handoff = "com/sulkta/straw/feature/player/AutoplayHandoff" + private val serviceAutoplay = "com/sulkta/straw/feature/player/ServiceAutoplayKt" + private val updateNotification = "onUpdateNotification" + private val updateNotificationDesc = "(Landroidx/media3/session/MediaSession;Z)V" + + private fun Set.calls(owner: String, name: String, desc: String? = null) = + any { it.owner == owner && it.name == name && (desc == null || it.desc == desc) } @Test fun media3sForegroundDecisionGoesThroughTheHandoff() { + val r = reach.from(service, updateNotification, updateNotificationDesc) assertTrue( - "PlaybackService must pass Media3's answer through autoplay.foregroundRequired", - Regex( - """override fun onUpdateNotification\(\s*session: MediaSession,\s*""" + - """startInForegroundRequired: Boolean,?\s*\)\s*\{\s*super\.onUpdateNotification\(\s*""" + - """session,\s*autoplay\.foregroundRequired\(startInForegroundRequired\),?\s*\)\s*\}""", - ).containsMatchIn(service), + "onUpdateNotification must ask autoplay.foregroundRequired", + r.calls(handoff, "foregroundRequired"), + ) + assertTrue( + "and hand the answer to Media3's own onUpdateNotification", + r.calls("androidx/media3/session/MediaSessionService", updateNotification, updateNotificationDesc), ) } @Test - fun everyAutoplayAttemptGoesThroughTheHandoff() { - assertTrue("tryAutoplay must start its attempt with autoplay.start", service.contains("autoplay.start(")) + fun theServiceBuildsItsHandoffWithServiceAutoplayHandoff() { + val built = reach.from(service, "", "()V") + assertTrue("the service's handoff comes from serviceAutoplayHandoff", built.calls(serviceAutoplay, "serviceAutoplayHandoff")) assertTrue( - "the release must ask Media3 to look again with false", - Regex("""onReleasedWithoutHandOver = \{[^}]*onUpdateNotification\(it, false\)""").containsMatchIn(service), + "its release asks Media3 again through the service's own onUpdateNotification", + built.calls(service, updateNotification, updateNotificationDesc), + ) + assertTrue("with the service's current session", built.calls(service, "mediaSession")) + assertFalse( + "no method of the service builds an AutoplayHandoff another way", + reach.fromAnyMethodOf(service).calls(handoff, ""), ) } @Test - fun theHandOverReportsWhetherThePlayerTookTheVideo() { - // setPlayingFrom returns early when it cannot build an item. Answering true regardless - // would release the hold without asking Media3 to look again, leaving the service in the - // foreground over an ended player. + fun theEndOfAVideoStartsAnAttemptThroughTheHandoff() { + val created = reach.from(service, "onCreate", "()V") + assertTrue("onCreate listens for the end of a video", created.calls(serviceAutoplay, "startAutoplayWhenItEnds")) + assertTrue("and starts tryAutoplay with the autoplay setting", created.calls(service, "tryAutoplay")) + assertTrue(created.calls("com/sulkta/straw/data/SettingsStore", "getAutoplayMode")) + + val tried = reach.from(service, "tryAutoplay", "(Lcom/sulkta/straw/data/AutoplayMode;)V") + assertTrue("tryAutoplay runs its attempt in the handoff", tried.calls(handoff, "start")) + assertTrue("the next video passes the allowlist gate", tried.calls(serviceAutoplay, "resolveAllowedCandidate")) + assertTrue("the hand-over goes through the fence", tried.calls(serviceAutoplay, "handOverIfUnchanged")) assertTrue( - "the hand-over must return whether NowPlaying moved to the new video", - service.contains("NowPlaying.current.value?.streamUrl == next.streamUrl"), + "and hands the video to the player with setPlayingFrom", + // A call with default arguments compiles to setPlayingFrom$default. + tried.any { it.owner == "com/sulkta/straw/feature/player/StrawMediaControllerKt" && it.name.startsWith("setPlayingFrom") }, ) } @Test fun theServiceDropsAnAttemptWhenItIsDestroyed() { - val onDestroy = service.substringAfter("override fun onDestroy()", missingDelimiterValue = "") - assertTrue("onDestroy must cancel the attempt", onDestroy.contains("autoplay.cancel()")) + val destroyed = reach.from(service, "onDestroy", "()V") + assertTrue("onDestroy must cancel the attempt", destroyed.calls(handoff, "cancel")) + // Controls: the walk reads onDestroy (it reaches Media3's onDestroy) and is not + // everything the class touches (it does not reach start). + assertTrue(destroyed.calls("androidx/media3/session/MediaSessionService", "onDestroy", "()V")) + assertFalse(destroyed.calls(handoff, "start")) } } diff --git a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/CompiledReach.kt b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/CompiledReach.kt new file mode 100644 index 000000000..3b57502ee --- /dev/null +++ b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/CompiledReach.kt @@ -0,0 +1,218 @@ +/* + * SPDX-FileCopyrightText: 2026 Sulkta + * SPDX-License-Identifier: GPL-3.0-or-later + * + * Reads compiled classes to say what a method reaches: the methods it calls and the fields it + * reads, plus everything reached through the methods of its own class (private helpers and + * synthetic accessors), the bodies of the lambdas it creates (invokedynamic), and the classes + * nested in its class that it instantiates (anonymous objects, function references, suspend + * lambdas). It parses only as much of the class-file format (the JVM specification, chapter 4) as + * that needs. + * + * Used where code cannot run in a JVM test because it is a framework callback. Unlike a check on + * the source text, a comment or a renamed local does not change what the compiled method calls. + */ + +package com.sulkta.straw.feature.player + +import java.io.ByteArrayInputStream +import java.io.DataInputStream +import java.nio.ByteBuffer + +internal class CompiledReach(private val loader: ClassLoader) { + + /** A method or field, by its JVM owner (slash-separated), name and descriptor. */ + data class Member(val owner: String, val name: String, val desc: String) + + private class ClassRef(val nameIndex: Int) + private class MemberRef(val classIndex: Int, val nameAndTypeIndex: Int) + private class NameAndType(val nameIndex: Int, val descIndex: Int) + private class MethodHandle(val referenceIndex: Int) + private class InvokeDynamic(val bootstrapIndex: Int) + + private class Method(val name: String, val desc: String, val code: ByteArray?) + + private class ClassFile( + val pool: Array, + val methods: List, + val bootstrapArguments: List>, + ) + + private val classes = mutableMapOf() + + /** Every member reached from [owner].[name] with descriptor [desc]. */ + fun from(owner: String, name: String, desc: String): Set { + require(load(owner).methods.any { it.name == name && it.desc == desc }) { + "$owner has no method $name$desc" + } + return walk(owner, listOf(Member(owner, name, desc))) + } + + /** Every member reached from any method declared in [owner]. */ + fun fromAnyMethodOf(owner: String): Set = + walk(owner, load(owner).methods.map { Member(owner, it.name, it.desc) }) + + private fun walk(root: String, start: List): Set { + val reached = mutableSetOf() + val seen = mutableSetOf() + val work = ArrayDeque(start) + fun follow(m: Member) { + reached += m + if (m.owner == root || m.owner.startsWith("$root$")) work += m + } + while (work.isNotEmpty()) { + val m = work.removeFirst() + if (!seen.add(m)) continue + val cf = load(m.owner) + // An inherited method has no body in this class; what it calls is not ours. + val code = cf.methods.firstOrNull { it.name == m.name && it.desc == m.desc }?.code ?: continue + forEachInstruction(code) { op, at -> + when (op) { + // getstatic, getfield, invokevirtual, invokespecial, invokestatic, invokeinterface + 0xb2, 0xb4, 0xb6, 0xb7, 0xb8, 0xb9 -> follow(member(cf, u2(code, at + 1))) + // invokedynamic: the method handles among its bootstrap arguments are the + // lambda bodies and method references it creates. + 0xba -> { + val indy = cf.pool[u2(code, at + 1)] as InvokeDynamic + for (arg in cf.bootstrapArguments[indy.bootstrapIndex]) { + val handle = cf.pool[arg] as? MethodHandle ?: continue + follow(member(cf, handle.referenceIndex)) + } + } + // new: a class nested in the root (an anonymous object, a function reference, + // a suspend lambda) is walked whole. + 0xbb -> { + val cls = className(cf, u2(code, at + 1)) + if (cls.startsWith("$root$")) { + for (method in load(cls).methods) work += Member(cls, method.name, method.desc) + } + } + } + } + } + return reached + } + + private fun load(name: String): ClassFile = classes.getOrPut(name) { + val bytes = loader.getResourceAsStream("$name.class")?.use { it.readBytes() } + ?: error("compiled class not found on the test classpath: $name") + parse(bytes) + } + + private fun className(cf: ClassFile, index: Int): String = + cf.pool[(cf.pool[index] as ClassRef).nameIndex] as String + + private fun member(cf: ClassFile, index: Int): Member { + val ref = cf.pool[index] as MemberRef + val nat = cf.pool[ref.nameAndTypeIndex] as NameAndType + return Member(className(cf, ref.classIndex), cf.pool[nat.nameIndex] as String, cf.pool[nat.descIndex] as String) + } + + private fun parse(bytes: ByteArray): ClassFile { + val d = DataInputStream(ByteArrayInputStream(bytes)) + check(d.readInt() == 0xCAFEBABE.toInt()) { "not a class file" } + d.readUnsignedShort() // minor version + d.readUnsignedShort() // major version + val count = d.readUnsignedShort() + val pool = arrayOfNulls(count) + var i = 1 + while (i < count) { + when (val tag = d.readUnsignedByte()) { + 1 -> pool[i] = d.readUTF() + 3, 4 -> d.readInt() + 5, 6 -> { + d.readLong() + i++ // a long or double takes two entries + } + 7 -> pool[i] = ClassRef(d.readUnsignedShort()) + 8, 16, 19, 20 -> d.readUnsignedShort() + 9, 10, 11 -> pool[i] = MemberRef(d.readUnsignedShort(), d.readUnsignedShort()) + 12 -> pool[i] = NameAndType(d.readUnsignedShort(), d.readUnsignedShort()) + 15 -> { + d.readUnsignedByte() // reference kind + pool[i] = MethodHandle(d.readUnsignedShort()) + } + 17, 18 -> { + pool[i] = InvokeDynamic(d.readUnsignedShort()) + d.readUnsignedShort() // name and type + } + else -> error("unknown constant pool tag $tag") + } + i++ + } + d.readUnsignedShort() // access flags + d.readUnsignedShort() // this class + d.readUnsignedShort() // super class + repeat(d.readUnsignedShort()) { d.readUnsignedShort() } // interfaces + repeat(d.readUnsignedShort()) { // fields + repeat(3) { d.readUnsignedShort() } + repeat(d.readUnsignedShort()) { skipAttribute(d) } + } + val methods = List(d.readUnsignedShort()) { + d.readUnsignedShort() // access flags + val name = pool[d.readUnsignedShort()] as String + val desc = pool[d.readUnsignedShort()] as String + var code: ByteArray? = null + repeat(d.readUnsignedShort()) { + val attr = pool[d.readUnsignedShort()] as String + val body = ByteArray(d.readInt()).also { d.readFully(it) } + if (attr == "Code") { + // max_stack u2, max_locals u2, code_length u4, code + val length = ByteBuffer.wrap(body, 4, 4).int + code = body.copyOfRange(8, 8 + length) + } + } + Method(name, desc, code) + } + var bootstrap = emptyList>() + repeat(d.readUnsignedShort()) { + val attr = pool[d.readUnsignedShort()] as String + val body = ByteArray(d.readInt()).also { d.readFully(it) } + if (attr == "BootstrapMethods") { + val b = DataInputStream(ByteArrayInputStream(body)) + bootstrap = List(b.readUnsignedShort()) { + b.readUnsignedShort() // the bootstrap method itself + List(b.readUnsignedShort()) { b.readUnsignedShort() } + } + } + } + return ClassFile(pool, methods, bootstrap) + } + + private fun skipAttribute(d: DataInputStream) { + d.readUnsignedShort() + d.skipBytes(d.readInt()) + } + + private fun u2(code: ByteArray, at: Int): Int = + ((code[at].toInt() and 0xff) shl 8) or (code[at + 1].toInt() and 0xff) + + private fun s4(code: ByteArray, at: Int): Int = ByteBuffer.wrap(code, at, 4).int + + /** Calls [visit] with each instruction's opcode and offset, stepping over operands. */ + private fun forEachInstruction(code: ByteArray, visit: (Int, Int) -> Unit) { + var pc = 0 + while (pc < code.size) { + val op = code[pc].toInt() and 0xff + visit(op, pc) + pc += when (op) { + 0x10, 0x12, in 0x15..0x19, in 0x36..0x3a, 0xa9, 0xbc -> 2 + 0x11, 0x13, 0x14, 0x84, in 0x99..0xa8, in 0xb2..0xb8, 0xbb, 0xbd, 0xc0, 0xc1, 0xc6, 0xc7 -> 3 + 0xc5 -> 4 + 0xb9, 0xba, 0xc8, 0xc9 -> 5 + 0xc4 -> if ((code[pc + 1].toInt() and 0xff) == 0x84) 6 else 4 // wide + 0xaa -> { // tableswitch: padding to a multiple of 4, default, low, high, offsets + val p = (pc + 4) and 3.inv() + val low = s4(code, p + 4) + val high = s4(code, p + 8) + p + 12 + (high - low + 1) * 4 - pc + } + 0xab -> { // lookupswitch: padding, default, npairs, pairs + val p = (pc + 4) and 3.inv() + p + 8 + s4(code, p + 4) * 8 - pc + } + else -> 1 + } + } + } +} diff --git a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/ServiceAutoplayTest.kt b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/ServiceAutoplayTest.kt new file mode 100644 index 000000000..76cd29fd0 --- /dev/null +++ b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/ServiceAutoplayTest.kt @@ -0,0 +1,302 @@ +/* + * SPDX-FileCopyrightText: 2026 Sulkta + * SPDX-License-Identifier: GPL-3.0-or-later + * + * The playback service's side of autoplay (ServiceAutoplay.kt): the handoff it builds, the player + * state that starts an attempt, the allowlist gate on the next video, and the fence on the + * hand-over. Dispatchers.Main is replaced by a test dispatcher, so "runs on the main thread" here + * means "runs when the test lets that dispatcher run, on the test's own thread". + */ + +package com.sulkta.straw.feature.player + +import androidx.media3.common.Player +import com.sulkta.straw.data.AutoplayMode +import java.lang.reflect.Proxy +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicInteger +import java.util.concurrent.atomic.AtomicReference +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.awaitCancellation +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.TestCoroutineScheduler +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.setMain +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test + +@OptIn(ExperimentalCoroutinesApi::class) +class ServiceAutoplayTest { + + private val scheduler = TestCoroutineScheduler() + + @Before + fun setUp() { + Dispatchers.setMain(StandardTestDispatcher(scheduler)) + NowPlaying.clear() + } + + @After + fun tearDown() { + Dispatchers.resetMain() + NowPlaying.clear() + } + + /** Stands for the service's MediaSession. */ + private class Session + + private class Built(session: Session?) { + val updates = mutableListOf>() + val handoff = serviceAutoplayHandoff( + scope = CoroutineScope(SupervisorJob()), + session = { session }, + updateNotification = { s, startInForegroundRequired -> updates += s to startInForegroundRequired }, + ) + } + + // --- the handoff the service builds ------------------------------------------------------- + + @Test + fun theServicesHandOverRunsOnTheMainThread() { + val testThread = Thread.currentThread() + val ranOn = AtomicReference(null) + val ran = CountDownLatch(1) + val h = Built(Session()).handoff + + h.start( + resolve = { "next" }, + handOver = { _: String -> + ranOn.set(Thread.currentThread()) + ran.countDown() + true + }, + ) + assertNull("nothing runs inside start: the main thread runs it later", ranOn.get()) + + scheduler.runCurrent() + assertTrue(ran.await(5, TimeUnit.SECONDS)) + assertSame("the hand-over must run on the main thread, where the player lives", testThread, ranOn.get()) + } + + @Test + fun theServicesAttemptGivesUpAtThirtySecondsAndNotBefore() { + val session = Session() + val b = Built(session) + b.handoff.start(resolve = { awaitCancellation() }, handOver = { _: String -> true }) + scheduler.runCurrent() + + scheduler.advanceTimeBy(29_999L) + scheduler.runCurrent() + assertTrue(b.handoff.inFlight) + assertTrue(b.updates.isEmpty()) + + scheduler.advanceTimeBy(1L) + scheduler.runCurrent() + assertFalse(b.handoff.inFlight) + assertEquals(listOf(session to false), b.updates) + } + + @Test + fun anAttemptThatEndsWithoutAHandOverAsksMedia3AgainWithFalse() { + val session = Session() + val b = Built(session) + b.handoff.start(resolve = { null as String? }, handOver = { true }) + scheduler.runCurrent() + + assertFalse(b.handoff.inFlight) + assertEquals(listOf(session to false), b.updates) + } + + @Test + fun theServicesReleaseAsksMedia3AfterTheHoldIsDropped() { + // The service passes its own onUpdateNotification, which asks foregroundRequired. + val answer = AtomicReference(null) + lateinit var h: AutoplayHandoff + h = serviceAutoplayHandoff( + scope = CoroutineScope(SupervisorJob()), + session = { Session() }, + updateNotification = { _, startInForegroundRequired -> + answer.set(h.foregroundRequired(startInForegroundRequired)) + }, + ) + h.start(resolve = { null as String? }, handOver = { true }) + scheduler.runCurrent() + + assertEquals(false, answer.get()) + } + + @Test + fun aHandOverLeavesMedia3Alone() { + val b = Built(Session()) + b.handoff.start(resolve = { "next" }, handOver = { true }) + scheduler.runCurrent() + + assertFalse(b.handoff.inFlight) + assertTrue(b.updates.isEmpty()) + } + + @Test + fun aReleaseAfterTheSessionIsGoneAsksNothing() { + val b = Built(null) + b.handoff.start(resolve = { null as String? }, handOver = { true }) + scheduler.runCurrent() + + assertFalse(b.handoff.inFlight) + assertTrue(b.updates.isEmpty()) + } + + // --- which player state starts an attempt ------------------------------------------------- + + private class FakePlayer( + var playbackState: Int = Player.STATE_ENDED, + var playWhenReady: Boolean = true, + var mediaItemCount: Int = 1, + var currentMediaItemIndex: Int = 0, + ) { + val listeners = mutableListOf() + val player: Player = Proxy.newProxyInstance( + Player::class.java.classLoader, + arrayOf(Player::class.java), + ) { proxy, method, args -> + when (method.name) { + "getPlaybackState" -> playbackState + "getPlayWhenReady" -> playWhenReady + "getMediaItemCount" -> mediaItemCount + "getCurrentMediaItemIndex" -> currentMediaItemIndex + "addListener" -> { + listeners += args!![0] as Player.Listener + null + } + "hashCode" -> System.identityHashCode(proxy) + "equals" -> proxy === args!![0] + "toString" -> "FakePlayer" + else -> throw UnsupportedOperationException(method.name) + } + } as Player + } + + @Test + fun anEndedPlayerAtTheLastItemStartsAnAttemptWithTheMode() { + assertEquals(AutoplayMode.SameChannel, autoplayModeAfter(Player.STATE_ENDED, 1, 0) { AutoplayMode.SameChannel }) + assertEquals(AutoplayMode.YtRelated, autoplayModeAfter(Player.STATE_ENDED, 3, 2) { AutoplayMode.YtRelated }) + assertEquals( + "the player can report no items once it has ended", + AutoplayMode.SameChannel, + autoplayModeAfter(Player.STATE_ENDED, 0, 0) { AutoplayMode.SameChannel }, + ) + } + + @Test + fun nothingElseStartsAnAttempt() { + val reads = AtomicInteger(0) + val mode = { + reads.incrementAndGet() + AutoplayMode.SameChannel + } + for (state in listOf(Player.STATE_IDLE, Player.STATE_BUFFERING, Player.STATE_READY)) { + assertNull("state $state", autoplayModeAfter(state, 1, 0, mode)) + } + assertEquals("the setting is read only when the player has ended", 0, reads.get()) + assertNull("inside the queue Media3 advances itself", autoplayModeAfter(Player.STATE_ENDED, 3, 1, mode)) + assertNull("autoplay off", autoplayModeAfter(Player.STATE_ENDED, 1, 0) { AutoplayMode.Off }) + } + + @Test + fun aPlayerThatEndsAtTheLastItemStartsAutoplay() { + val fake = FakePlayer() + val started = mutableListOf() + fake.player.startAutoplayWhenItEnds(mode = { AutoplayMode.SameChannel }, start = { started += it }) + assertEquals("one listener on the player", 1, fake.listeners.size) + + fake.listeners.single().onPlaybackStateChanged(Player.STATE_READY) + assertTrue(started.isEmpty()) + + fake.listeners.single().onPlaybackStateChanged(Player.STATE_ENDED) + assertEquals(listOf(AutoplayMode.SameChannel), started) + + fake.mediaItemCount = 2 + fake.listeners.single().onPlaybackStateChanged(Player.STATE_ENDED) + assertEquals("the player's own queue position is used", listOf(AutoplayMode.SameChannel), started) + } + + // --- the allowlist gate --------------------------------------------------------------------- + + @Test + fun anAllowedCandidateIsLookedUp() = runBlocking { + val looked = mutableListOf() + val url = "https://www.youtube.com/watch?v=abcdefghijk" + val r = resolveAllowedCandidate(pick = { url }, resolve = { u -> looked += u; "info:$u" }) + assertEquals("info:$url", r) + assertEquals(listOf(url), looked) + } + + @Test + fun aCandidateOutsideTheAllowlistNeverReachesTheLookup() = runBlocking { + val looked = mutableListOf() + for (url in listOf("https://example.com/watch?v=abcdefghijk", "file:///sdcard/x", "not a url")) { + assertNull(url, resolveAllowedCandidate(pick = { url }, resolve = { u -> looked += u; "info" })) + } + assertTrue("nothing outside the allowlist reached the extractor: $looked", looked.isEmpty()) + assertNull(resolveAllowedCandidate(pick = { null }, resolve = { u -> looked += u; "info" })) + assertTrue(looked.isEmpty()) + } + + // --- the fence on the hand-over ------------------------------------------------------------- + + private val a = "https://www.youtube.com/watch?v=aaaaaaaaaaa" + private val b = "https://www.youtube.com/watch?v=bbbbbbbbbbb" + private val c = "https://www.youtube.com/watch?v=ccccccccccc" + + private fun nowPlaying(url: String) { + NowPlaying.claim(NowPlayingItem(streamUrl = url, title = "", uploader = "", thumbnail = null)) + } + + /** What setPlayingFrom does to NowPlaying when it can build an item. */ + private fun playing(url: String): () -> Unit = { nowPlaying(url) } + + @Test + fun theNextVideoIsHandedOverWhenNothingChanged() { + nowPlaying(a) + val played = AtomicInteger(0) + val took = handOverIfUnchanged(from = a, nextUrl = b) { + played.incrementAndGet() + nowPlaying(b) + } + assertTrue(took) + assertEquals(1, played.get()) + assertEquals(b, NowPlaying.current.value?.streamUrl) + } + + @Test + fun anotherVideoStartedMeanwhileIsNotReplaced() { + nowPlaying(a) + nowPlaying(c) // the user started C while the next video was looked up + val played = AtomicInteger(0) + val took = handOverIfUnchanged(from = a, nextUrl = b) { + played.incrementAndGet() + nowPlaying(b) + } + assertFalse("a stale attempt reports that nothing was handed over", took) + assertEquals("the user's video is not replaced", 0, played.get()) + assertEquals(c, NowPlaying.current.value?.streamUrl) + } + + @Test + fun aPlayerThatCouldNotBuildTheItemDidNotTakeIt() { + nowPlaying(a) + // setPlayingFrom returns early, before claiming NowPlaying, when it cannot build an item. + assertFalse(handOverIfUnchanged(from = a, nextUrl = b) {}) + assertTrue(handOverIfUnchanged(from = a, nextUrl = b, play = playing(b))) + } +} -- 2.49.1 From a5f757cae9c6f5560808fe0d0585d4b15f356e76 Mon Sep 17 00:00:00 2001 From: Cobb Date: Thu, 22 Oct 2026 18:49:42 -0700 Subject: [PATCH 5/9] player: autoplay does not replace a video the user replayed, paused or stopped The audit (F8) read that the staleness fence on the autoplay hand-over compares URLs only. A play request on the ended video (from the notification or the minibar) replays it without moving NowPlaying, so a lookup that finished afterwards replaced the replay with the next video, and setPlayingFrom set play-when-ready even if the user had paused since (read in the Media3 and straw sources; not reproduced on a device). The hand-over now also needs the player to be still ended with play-when-ready set, the state it is in when a video ends with nobody touching it. A play request from a controller replays an ended video (Media3 1.4.1 handles it with Util.handlePlayButtonAction, which seeks to the start and plays), and a pause or a stop clears one of the two; in each case the user's action stands, nothing is handed over, and Media3 is asked again whether the service stays in the foreground, as for any attempt that ends without a hand-over. ExoPlayer leaves play-when-ready set at the end of a video unless it was told to pause at the end of each item, which straw does not do (read in the ExoPlayer 1.4.1 source). The attempt is not cancelled when the user acts: its lookup runs on until it ends, holding the foreground until then. Cancelling it on a user action needs a way to tell the user's commands from the player events the hand-over itself causes (a session callback on controller commands, for example); that is a new mechanism, left out of this fix round. Measured: the JVM tests pass on this commit; the new test covers a replay (READY and BUFFERING), a replay then pause, a pause while ended and a stop. Device behaviour NOT MEASURED. --- .../straw/feature/player/PlaybackService.kt | 10 +++--- .../straw/feature/player/ServiceAutoplay.kt | 21 +++++++++--- .../feature/player/ServiceAutoplayTest.kt | 33 ++++++++++++++++--- 3 files changed, 51 insertions(+), 13 deletions(-) diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlaybackService.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlaybackService.kt index 4cfc675d2..59c4f0340 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlaybackService.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlaybackService.kt @@ -295,11 +295,11 @@ class PlaybackService : MediaSessionService() { autoplay.start( resolve = { resolveAutoplayNext(mode, current) }, handOver = { next -> - // Staleness fence (see triggeredFrom above): only - // advance if the video that ended is still the current - // one. If the user started something else mid-resolve, - // bail rather than hijack it. - handOverIfUnchanged(from = triggeredFrom, nextUrl = next.streamUrl) { + // Staleness fence (see triggeredFrom above): only advance if the video that + // ended is still the current one and the player is still ended and set to play. + // If the user started something else, replayed it or paused mid-resolve, bail + // rather than hijack it. + handOverIfUnchanged(controller, from = triggeredFrom, nextUrl = next.streamUrl) { // setPlayingFrom, NOT enqueueLast — at STATE_ENDED the // just-finished video is still loaded (mediaItemCount==1), // so enqueueLast would only append at index 1 and the diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ServiceAutoplay.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ServiceAutoplay.kt index bbae5c55e..ba543efa0 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ServiceAutoplay.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ServiceAutoplay.kt @@ -99,16 +99,29 @@ internal suspend fun resolveAllowedCandidate( } /** - * Hands the next video to the player through [play] if NowPlaying still names [from], the video - * that ended, and returns whether the player took it. If NowPlaying moved on while the next one was - * looked up (the user started another video, or closed the player), that stands. + * Hands the next video to [player] through [play] if nothing has changed since the video [from] + * ended, and returns whether the player took it. + * + * Nothing has changed means NowPlaying still names [from] (nobody started another video or closed + * the player while the next one was looked up), and [player] is still ended with play-when-ready + * set. A play request from a controller (the notification, the minibar) replays an ended video: + * Media3 1.4.1 handles it with Util.handlePlayButtonAction, which seeks to the start and plays. + * Pause or stop clears one of the two. ExoPlayer leaves play-when-ready set when a video ends unless + * it was told to pause at the end of each item, which straw does not do. When something changed, + * that stands and nothing is handed over. * * The player took the video when NowPlaying names [nextUrl] afterwards: [play] (setPlayingFrom) * returns early, leaving NowPlaying as it was, when the resolved playback has no stream it can * build an item from. */ -internal fun handOverIfUnchanged(from: String, nextUrl: String, play: () -> Unit): Boolean { +internal fun handOverIfUnchanged( + player: Player, + from: String, + nextUrl: String, + play: () -> Unit, +): Boolean { if (NowPlaying.current.value?.streamUrl != from) return false + if (player.playbackState != Player.STATE_ENDED || !player.playWhenReady) return false play() return NowPlaying.current.value?.streamUrl == nextUrl } diff --git a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/ServiceAutoplayTest.kt b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/ServiceAutoplayTest.kt index 76cd29fd0..02259f2ab 100644 --- a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/ServiceAutoplayTest.kt +++ b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/ServiceAutoplayTest.kt @@ -269,7 +269,7 @@ class ServiceAutoplayTest { fun theNextVideoIsHandedOverWhenNothingChanged() { nowPlaying(a) val played = AtomicInteger(0) - val took = handOverIfUnchanged(from = a, nextUrl = b) { + val took = handOverIfUnchanged(FakePlayer().player, from = a, nextUrl = b) { played.incrementAndGet() nowPlaying(b) } @@ -283,7 +283,7 @@ class ServiceAutoplayTest { nowPlaying(a) nowPlaying(c) // the user started C while the next video was looked up val played = AtomicInteger(0) - val took = handOverIfUnchanged(from = a, nextUrl = b) { + val took = handOverIfUnchanged(FakePlayer().player, from = a, nextUrl = b) { played.incrementAndGet() nowPlaying(b) } @@ -296,7 +296,32 @@ class ServiceAutoplayTest { fun aPlayerThatCouldNotBuildTheItemDidNotTakeIt() { nowPlaying(a) // setPlayingFrom returns early, before claiming NowPlaying, when it cannot build an item. - assertFalse(handOverIfUnchanged(from = a, nextUrl = b) {}) - assertTrue(handOverIfUnchanged(from = a, nextUrl = b, play = playing(b))) + assertFalse(handOverIfUnchanged(FakePlayer().player, from = a, nextUrl = b) {}) + assertTrue(handOverIfUnchanged(FakePlayer().player, from = a, nextUrl = b, play = playing(b))) + } + + @Test + fun aVideoTheUserReplayedPausedOrStoppedIsNotReplaced() { + val cases = listOf( + // a controller's play on an ended player: Media3 seeks to the start and plays + "replayed" to FakePlayer(playbackState = Player.STATE_READY), + "replaying, still buffering" to FakePlayer(playbackState = Player.STATE_BUFFERING), + "replayed, then paused" to FakePlayer(playbackState = Player.STATE_READY, playWhenReady = false), + // a pause request while the player is ended + "paused at the end" to FakePlayer(playWhenReady = false), + "stopped" to FakePlayer(playbackState = Player.STATE_IDLE), + ) + for ((what, fake) in cases) { + NowPlaying.clear() + nowPlaying(a) + val played = AtomicInteger(0) + val took = handOverIfUnchanged(fake.player, from = a, nextUrl = b) { + played.incrementAndGet() + nowPlaying(b) + } + assertFalse(what, took) + assertEquals("$what: the next video is not handed over", 0, played.get()) + assertEquals(a, NowPlaying.current.value?.streamUrl) + } } } -- 2.49.1 From bd4177d93d4af86e030550498ccdfa1c6ae5530c Mon Sep 17 00:00:00 2001 From: Cobb Date: Thu, 22 Oct 2026 18:52:09 -0700 Subject: [PATCH 6/9] player: the autoplay bound no longer cuts off a lookup strawcore would finish The audit (F6) read that the 30 second bound on an autoplay attempt is shorter than strawcore's own limits. An attempt makes at most two extraction calls, and strawcore abandons each one at 60 seconds (EXTRACT_TIMEOUT in rust/strawcore/src/runtime.rs), so on a slow link the bound could cancel a lookup that before e7ba6bcc3, which added the bound, ran to its end and played the next video, as it still can with the app on screen. It asked whether the bound should only release the hold and let the lookup finish. It does not here: a lookup that finishes after the hold is released hands the next video to a service that has left the foreground, which in the background is the refused foreground start that e7ba6bcc3 exists to avoid (read; not reproduced on a device). The bound moves to 125 seconds instead, past the 120 seconds two extractions can take, and stays a guard against an attempt stuck outside strawcore. The 5 seconds over 120 are a chosen margin, not a measured time. Cost: an attempt stuck outside strawcore holds the foreground, with the ended video's notification, for up to 125 seconds instead of 30. The audit measured the two lookups at 0.66 to 0.93 seconds on a wired link; on a mobile link they are NOT MEASURED. Measured: the JVM tests pass on this commit; the service's attempt is still in flight at 124,999 ms of virtual time and gives up at 125,000 ms. --- .../sulkta/straw/feature/player/ServiceAutoplay.kt | 12 +++++++++--- .../straw/feature/player/ServiceAutoplayTest.kt | 4 ++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ServiceAutoplay.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ServiceAutoplay.kt index ba543efa0..b12b0d47d 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ServiceAutoplay.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/ServiceAutoplay.kt @@ -17,10 +17,16 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers /** - * How long one autoplay attempt may hold the foreground before it gives up. A chosen bound on a - * channel lookup plus a stream lookup, not a measured time. + * How long one autoplay attempt may hold the foreground before it gives up. + * + * An attempt makes at most two strawcore extraction calls (the channel or the current video, then + * the next video), and strawcore abandons each extraction at 60 seconds (EXTRACT_TIMEOUT in + * rust/strawcore/src/runtime.rs). This bound sits past the 120 seconds two extractions can take, + * so it does not cut short a lookup strawcore is still willing to finish, except by the time the + * attempt spends outside the extractions, which is not measured. The extra 5 seconds is a chosen + * margin for that time, not a measured one. */ -internal const val AUTOPLAY_TIMEOUT_MS = 30_000L +internal const val AUTOPLAY_TIMEOUT_MS = 125_000L /** * The handoff PlaybackService uses. Hand-overs run on the main thread, the thread the player was diff --git a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/ServiceAutoplayTest.kt b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/ServiceAutoplayTest.kt index 02259f2ab..05547e15c 100644 --- a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/ServiceAutoplayTest.kt +++ b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/ServiceAutoplayTest.kt @@ -90,13 +90,13 @@ class ServiceAutoplayTest { } @Test - fun theServicesAttemptGivesUpAtThirtySecondsAndNotBefore() { + fun theServicesAttemptGivesUpAt125SecondsAndNotBefore() { val session = Session() val b = Built(session) b.handoff.start(resolve = { awaitCancellation() }, handOver = { _: String -> true }) scheduler.runCurrent() - scheduler.advanceTimeBy(29_999L) + scheduler.advanceTimeBy(124_999L) scheduler.runCurrent() assertTrue(b.handoff.inFlight) assertTrue(b.updates.isEmpty()) -- 2.49.1 From 5c403e535bfaedb4f00894e2003c72b2b9520501 Mon Sep 17 00:00:00 2001 From: Cobb Date: Thu, 22 Oct 2026 18:57:57 -0700 Subject: [PATCH 7/9] player: SponsorBlock skips apply only to the video they belong to The audit (F4) read that with the app in the background the SponsorBlock skip loop keeps the first video's segments: it gets its item through collectAsStateWithLifecycle, which stops following NowPlaying while the activity is stopped, and by the audit's check of the compiled dispatcher its delay loop goes on running. Each later video (autoplay, or the queue) would then be skipped to the ends of the first one's segments, and a segment ending within half a second of a later video's end would end it and start the next autoplay. Before e7ba6bcc3 the background chain reportedly stopped after one video; with it the chain can run on, which makes this reachable. The decision moves to segmentToSkip (SponsorBlockSkip.kt), which reads NowPlaying at the moment of each decision and applies segments only while it names the video they belong to. NowPlaying moves with the player: the service claims it for the next video before handing that video over, and for a queue item when the player moves to it. With the app in front the composition still restarts the loop for a new item; the check also covers the moment between NowPlaying moving and that restart. Measured: 66 of 66 JVM tests pass on this commit's tree. Removing the NowPlaying check, which gives the behaviour before this commit, fails SponsorBlockSkipTest.aVideosSegmentsAreNotSkippedOnTheNextOne. On a device NOT MEASURED, including whether the loop runs while the activity is stopped. --- .../straw/feature/player/PlayerScreen.kt | 17 +---- .../straw/feature/player/SponsorBlockSkip.kt | 39 ++++++++++++ .../feature/player/SponsorBlockSkipTest.kt | 63 +++++++++++++++++++ 3 files changed, 104 insertions(+), 15 deletions(-) create mode 100644 strawApp/src/main/kotlin/com/sulkta/straw/feature/player/SponsorBlockSkip.kt create mode 100644 strawApp/src/test/kotlin/com/sulkta/straw/feature/player/SponsorBlockSkipTest.kt diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt index d8d710a2e..92bf6896c 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt @@ -73,7 +73,6 @@ import androidx.media3.common.util.UnstableApi import androidx.media3.ui.PlayerView import com.sulkta.straw.OverlayChromeColor import com.sulkta.straw.feature.detail.VideoDetailViewModel -import com.sulkta.straw.net.SbSegment import com.sulkta.straw.util.LogDump import com.sulkta.straw.util.strawLogI import kotlinx.coroutines.delay @@ -398,7 +397,8 @@ fun SponsorBlockSkipLoop() { continue } val posSec = controller.currentPosition / 1000.0 - val s = pickActiveSegment(segments, posSec, skipped) ?: continue + // Skips only while NowPlaying still names cur's video: see segmentToSkip. + val s = segmentToSkip(cur.streamUrl, segments, posSec, skipped) ?: continue strawLogI( "StrawSb", "skip: ${s.category} ${s.startSec}s..${s.endSec}s (pos=$posSec)", @@ -419,16 +419,3 @@ fun SponsorBlockSkipLoop() { } } } - -private fun pickActiveSegment( - segments: List, - posSec: Double, - skipped: Set, -): SbSegment? = segments.firstOrNull { s -> - val uuidNotSkipped = s.UUID == null || s.UUID !in skipped - val interval = s.endSec - s.startSec > 0.1 - // Drop the prior -0.05 exclusion — combined with the loop's - // 150ms polling cadence, short SB segments could fall in the - // gap and silently skip the skip. - uuidNotSkipped && interval && posSec >= s.startSec && posSec < s.endSec -} diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/SponsorBlockSkip.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/SponsorBlockSkip.kt new file mode 100644 index 000000000..91fe63358 --- /dev/null +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/SponsorBlockSkip.kt @@ -0,0 +1,39 @@ +/* + * SPDX-FileCopyrightText: 2026 Sulkta + * SPDX-License-Identifier: GPL-3.0-or-later + * + * The decision inside the SponsorBlock skip loop (SponsorBlockSkipLoop in PlayerScreen.kt), kept + * out of the composable so that a JVM test can run it. + */ + +package com.sulkta.straw.feature.player + +import com.sulkta.straw.net.SbSegment + +/** + * The segment to skip at [posSec], or null. + * + * [segments] belong to the video [segmentsFor], and they are applied only while NowPlaying names + * that video, read here at the moment of the decision. The skip loop gets its item through + * collectAsStateWithLifecycle, which stops following NowPlaying while the activity is stopped, so + * with the app in the background the loop can still hold one video's segments after the player + * has moved to another one (autoplay, the queue). NowPlaying itself moves with the player: the + * service claims it for the next video before handing that video to the player, and for a queue + * item when the player moves to it. + */ +internal fun segmentToSkip( + segmentsFor: String, + segments: List, + posSec: Double, + skipped: Set, +): SbSegment? { + if (NowPlaying.current.value?.streamUrl != segmentsFor) return null + return segments.firstOrNull { s -> + val uuidNotSkipped = s.UUID == null || s.UUID !in skipped + val interval = s.endSec - s.startSec > 0.1 + // Drop the prior -0.05 exclusion: combined with the loop's + // 150ms polling cadence, short SB segments could fall in the + // gap and silently skip the skip. + uuidNotSkipped && interval && posSec >= s.startSec && posSec < s.endSec + } +} diff --git a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/SponsorBlockSkipTest.kt b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/SponsorBlockSkipTest.kt new file mode 100644 index 000000000..400dcc20f --- /dev/null +++ b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/SponsorBlockSkipTest.kt @@ -0,0 +1,63 @@ +/* + * SPDX-FileCopyrightText: 2026 Sulkta + * SPDX-License-Identifier: GPL-3.0-or-later + * + * The SponsorBlock skip loop applies a video's segments only to that video. With the app in the + * background the loop can still hold the first video's segments after autoplay or the queue has + * moved the player on; skipping by them would seek the new video to the ends of the old one's + * segments, and a segment ending within half a second of the new video's end would end it. + */ + +package com.sulkta.straw.feature.player + +import com.sulkta.straw.net.SbSegment +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Test + +class SponsorBlockSkipTest { + + private val a = "https://www.youtube.com/watch?v=aaaaaaaaaaa" + private val b = "https://www.youtube.com/watch?v=bbbbbbbbbbb" + private val sponsor = SbSegment(UUID = "u1", category = "sponsor", segment = listOf(10.0, 40.0)) + + private fun nowPlaying(url: String) { + NowPlaying.claim(NowPlayingItem(streamUrl = url, title = "", uploader = "", thumbnail = null)) + } + + @Before + fun setUp() = NowPlaying.clear() + + @After + fun tearDown() = NowPlaying.clear() + + @Test + fun aVideosSegmentsAreSkippedOnThatVideo() { + nowPlaying(a) + assertEquals(sponsor, segmentToSkip(a, listOf(sponsor), posSec = 12.0, skipped = emptySet())) + } + + @Test + fun aVideosSegmentsAreNotSkippedOnTheNextOne() { + nowPlaying(a) + nowPlaying(b) // autoplay or the queue moved on while the loop still holds A + assertNull( + "the player has moved to B; A's segments must not seek it", + segmentToSkip(a, listOf(sponsor), posSec = 12.0, skipped = emptySet()), + ) + NowPlaying.clear() + assertNull("nothing is playing", segmentToSkip(a, listOf(sponsor), posSec = 12.0, skipped = emptySet())) + } + + @Test + fun theSegmentRulesAreUnchanged() { + nowPlaying(a) + assertNull("before the segment", segmentToSkip(a, listOf(sponsor), posSec = 9.9, skipped = emptySet())) + assertNull("at its end", segmentToSkip(a, listOf(sponsor), posSec = 40.0, skipped = emptySet())) + assertNull("already skipped once", segmentToSkip(a, listOf(sponsor), posSec = 12.0, skipped = setOf("u1"))) + val tiny = SbSegment(UUID = "u2", category = "sponsor", segment = listOf(10.0, 10.05)) + assertNull("too short to act on", segmentToSkip(a, listOf(tiny), posSec = 10.01, skipped = emptySet())) + } +} -- 2.49.1 From 2cac335880907c2a2e8070f879a7607b9c17679b Mon Sep 17 00:00:00 2001 From: Cobb Date: Thu, 22 Oct 2026 18:58:37 -0700 Subject: [PATCH 8/9] player: say what Media3 and Android's sources show about a refused start Two claims in AutoplayHandoff's header, from the audit (F9). Correction to e7ba6bcc3: its message and this header said that Media3 only logs a refused foreground start. That holds on the path where Media3 asks the service about its notification: MediaSessionService .onUpdateNotificationInternal catches the refusal and logs "Failed to start foreground". The path that applies a notification once its artwork has loaded (MediaNotificationManager.onNotificationUpdated) starts the foreground without that catch. Both read in the Media3 1.4.1 source. The header also said it was not measured whether Android accepts a repeated start request from a service already in the foreground. Android's ActiveServices answers that by reading (android-16.0.0_r1; an Android 14 copy has the same recompute rule), and the header now says what it reads: the allowance is worked out again only while it is denied, and ActiveServices clears it when the service leaves the foreground or is torn down, on a first start long after a plain start, and on a repeated start of a short or time-limited service type, which mediaPlayback is not. Read, not measured on a device; GrapheneOS NOT MEASURED. Comments only; the JVM tests pass on this commit. --- .../straw/feature/player/AutoplayHandoff.kt | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/AutoplayHandoff.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/AutoplayHandoff.kt index a90f56fa3..95c0dfcba 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/AutoplayHandoff.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/AutoplayHandoff.kt @@ -12,16 +12,24 @@ * calls stopForeground. By the time the next video is handed over, the service is out of the * foreground and Media3 has to put it back. Android 12 and later do not let an app that is in the * background start a foreground service (the platform's documented restriction; not reproduced on a - * device for this change). Media3 catches that refusal and only logs "Failed to start foreground", - * so the next video goes on playing from a service that is not in the foreground. That a device - * then stops that video a few seconds in is what was reported; the exact way the system stops it - * was not measured. + * device for this change). On the path where Media3 asks the service about its notification + * (MediaSessionService.onUpdateNotificationInternal), Media3 catches that refusal and only logs + * "Failed to start foreground", so the next video goes on playing from a service that is not in the + * foreground. The path that applies a notification once its artwork has loaded + * (MediaNotificationManager.onNotificationUpdated) starts the foreground without that catch. Both + * read in the Media3 1.4.1 source. That a device then stops that video a few seconds in is what was + * reported; the exact way the system stops it was not measured. * * Holding the foreground across the round trip means the service never leaves it, so the next * video starts in a service that is already in the foreground. Media3 still repeats its start - * request on each update; whether Android accepts that for a service already in the foreground was - * not measured, and if it refuses, Media3 catches it and the service stays in the foreground, since - * nothing asked it to stop. The hold ends when the next video has been handed over (the player is + * request on each update. Read in Android's ActiveServices (android-16.0.0_r1; an Android 14 copy + * has the same recompute rule), not measured on a device: a start request is refused when the + * service's foreground-start allowance is denied; the allowance is worked out again only while it + * is denied; and ActiveServices clears it when the service leaves the foreground or is torn down, + * on a first start long after a plain start, and on a repeated start of a short or time-limited + * service type, which mediaPlayback is not. By that reading a service that has not left the + * foreground keeps the allowance it started with, and the repeated request is not refused. The + * hold ends when the next video has been handed over (the player is * then BUFFERING with play-when-ready set, which Media3's own rule answers yes to once its * notification controller has seen it), or when the attempt ends without one, in which case * [onReleasedWithoutHandOver] asks Media3 to look again so the service leaves the foreground -- 2.49.1 From dfed10b15a84fa1c12cc4c726833c1be725d01bc Mon Sep 17 00:00:00 2001 From: Cobb Date: Thu, 22 Oct 2026 20:18:51 -0700 Subject: [PATCH 9/9] test: follow Media3's foreground answer through the compiled override The wiring checks moved from source text to the compiled service in c62f19548, and they see calls but not values. That let three rewrites of the onUpdateNotification override pass that the earlier text checks caught: passing true instead of Media3's answer, and computing foregroundRequired but handing Media3 the original answer (which would bring back the background stop this branch fixes). The test now also reads the override's instructions: Media3's answer is loaded once, goes straight into AutoplayHandoff.foregroundRequired, and what MediaSessionService.onUpdateNotification receives is that result (directly or through a local), not a constant. Measured in review on 2cac33588 plus this change: testDebugUnitTest 66/66; the harmless rewrites (a local val, autoplay.let { }) still pass; the rewrites above now fail. Still not covered: the release callback's value and an extra condition on onDestroy's cancel. --- .../feature/player/AutoplayWiringTest.kt | 26 ++++++++++++++++++- .../straw/feature/player/CompiledReach.kt | 15 +++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt index 55153a731..ad514a24c 100644 --- a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt +++ b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/AutoplayWiringTest.kt @@ -6,11 +6,14 @@ * about autoplay lives in ServiceAutoplay.kt and AutoplayHandoff.kt, where ServiceAutoplayTest and * AutoplayHandoffTest run it. What stays in the service is the connection to Media3: these checks * read the compiled service (CompiledReach) for those connections. They see calls, not values: a - * call whose result is computed and then not used would still pass. + * call whose result is computed and then not used would still pass. The one exception is the + * onUpdateNotification override, where the test also follows Media3's answer through the compiled + * instructions into foregroundRequired and on to Media3. */ package com.sulkta.straw.feature.player +import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Test @@ -39,6 +42,27 @@ class AutoplayWiringTest { "and hand the answer to Media3's own onUpdateNotification", r.calls("androidx/media3/session/MediaSessionService", updateNotification, updateNotificationDesc), ) + // Values, not only calls: Media3's answer (the Boolean in local slot 2, read by iload_2) + // is read once and goes straight into foregroundRequired, and what Media3's own + // onUpdateNotification gets is that call's result (directly, or through a local), not a + // constant. + val foregroundRequired = CompiledReach.Member(handoff, "foregroundRequired", "(Z)Z") + val code = reach.instructions(service, updateNotification, updateNotificationDesc) + val reads = code.indices.filter { code[it].first == 0x1c } + assertEquals("Media3's answer is read once", 1, reads.size) + assertEquals( + "and handed straight to autoplay.foregroundRequired", + foregroundRequired, + code.getOrNull(reads.single() + 1)?.second, + ) + val superCall = code.indexOfFirst { + it.first == 0xb7 && it.second?.owner == "androidx/media3/session/MediaSessionService" + } + val handed = code.getOrNull(superCall - 1) + assertTrue( + "Media3's onUpdateNotification gets the handoff's answer: $handed", + handed?.second == foregroundRequired || handed?.first in setOf(0x15, 0x1a, 0x1b, 0x1c, 0x1d), + ) } @Test diff --git a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/CompiledReach.kt b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/CompiledReach.kt index 3b57502ee..e2421e9e7 100644 --- a/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/CompiledReach.kt +++ b/strawApp/src/test/kotlin/com/sulkta/straw/feature/player/CompiledReach.kt @@ -48,6 +48,21 @@ internal class CompiledReach(private val loader: ClassLoader) { return walk(owner, listOf(Member(owner, name, desc))) } + /** + * The instructions of [owner].[name] with descriptor [desc], in order: each opcode, and the + * field or method it names (getstatic to invokeinterface), else null. + */ + fun instructions(owner: String, name: String, desc: String): List> { + val cf = load(owner) + val code = cf.methods.firstOrNull { it.name == name && it.desc == desc }?.code + ?: error("$owner has no method body $name$desc") + val out = mutableListOf>() + forEachInstruction(code) { op, at -> + out += op to (if (op in 0xb2..0xb9) member(cf, u2(code, at + 1)) else null) + } + return out + } + /** Every member reached from any method declared in [owner]. */ fun fromAnyMethodOf(owner: String): Set = walk(owner, load(owner).methods.map { Member(owner, it.name, it.desc) }) -- 2.49.1