From 2d604c1e6a562d4b2dc55939be6fe9602d53b0d8 Mon Sep 17 00:00:00 2001 From: darken Date: Tue, 28 Jul 2026 21:50:53 +0200 Subject: [PATCH] fix(reaction): Serialize pause arming behind queued snapshots --- .../eu/darken/capod/common/MediaControl.kt | 61 +++++++++++++------ .../darken/capod/common/MediaControlTest.kt | 40 +++++++++++- 2 files changed, 83 insertions(+), 18 deletions(-) diff --git a/app/src/main/java/eu/darken/capod/common/MediaControl.kt b/app/src/main/java/eu/darken/capod/common/MediaControl.kt index f7de26b0..95d0ba93 100644 --- a/app/src/main/java/eu/darken/capod/common/MediaControl.kt +++ b/app/src/main/java/eu/darken/capod/common/MediaControl.kt @@ -12,11 +12,13 @@ import eu.darken.capod.common.debug.logging.log import eu.darken.capod.common.debug.logging.logTag import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.delay +import kotlinx.coroutines.suspendCancellableCoroutine import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext import javax.inject.Inject import javax.inject.Singleton +import kotlin.coroutines.resume @Singleton class MediaControl @Inject constructor( @@ -149,26 +151,51 @@ class MediaControl @Inject constructor( private suspend fun sendPauseLocked(rememberForResume: Boolean): Boolean { log(TAG, INFO) { "sendPause(rememberForResume=$rememberForResume)" } - if (!audioManager.isMusicActive) { - log(TAG, INFO) { "Music is not playing, not sending pause" } - return false + // The arm still strictly precedes the key dispatch, now serialized on the callback handler. + // An inactive→active playback callback that fires during the dispatch (e.g. a fast user + // resume on the phone, or another app grabbing audio focus and immediately starting) must be + // able to clear capPaused without this call overwriting it back to true on a stale pause. + // Both run under NonCancellable so a cancellation can never arm without dispatching. + return withContext(NonCancellable) { + if (!armPauseOnCallbackThread(rememberForResume)) { + log(TAG, INFO) { "Music is not playing, not sending pause" } + return@withContext false + } + sendKey(KeyEvent.KEYCODE_MEDIA_PAUSE) + true } - // Set BEFORE the suspending sendKey() call. If we set after, an inactive→active - // playback callback that fires during the dispatch (e.g. a fast user resume on the - // phone, or another app grabbing audio focus and immediately starting) could clear - // capPaused mid-dispatch and we'd then overwrite it back to true on a stale pause. - // This single explicit assignment also covers the contract that an explicit user - // pause cancels a pending auto-resume. - capPaused = rememberForResume - // The live active-check we just passed is itself an observation of music activity. Recording - // it stops a music-start snapshot that was still queued on the handler when this pause armed - // capPaused from draining afterwards and reading as a fresh inactive→active edge that would - // wrongly clear the new arm. - lastKnownMusicActive = true - sendKey(KeyEvent.KEYCODE_MEDIA_PAUSE) - return true } + /** + * Checks whether music is playing and, if so, arms [capPaused] to [rememberForResume] — the + * single explicit assignment that also covers the contract that an explicit user pause cancels a + * pending auto-resume. Returns whether music was active, i.e. whether a pause is worth dispatching. + * + * Posting serializes the arm behind every already-queued playback snapshot, so a stale pre-pause + * inactive→active pair cannot drain after the arm and read as a fresh music-start edge that + * clears it. Snapshots that arrive after this runnable are genuinely later and should clear it. + * The live isMusicActive check is itself the observation of music activity that gets recorded + * into [lastKnownMusicActive]. + * + * Posting is unconditional: no sender runs on the callback looper and a suspending post cannot + * deadlock, so a same-looper fast-path would be dead code. + * + * This moves sendPause's isMusicActive binder read onto the (near-idle, non-main) callback + * thread — intentional. + */ + private suspend fun armPauseOnCallbackThread(rememberForResume: Boolean): Boolean = + suspendCancellableCoroutine { cont -> + val arm = Runnable { + val active = audioManager.isMusicActive + if (active) { + capPaused = rememberForResume + lastKnownMusicActive = true + } + cont.resume(active) + } + if (!audioCallbackHandler.post(arm)) arm.run() // looper quitting: degrade to inline arming + } + /** * Dispatches MEDIA_STOP and clears any pending auto-resume — Stop is an explicit user * "stay stopped" action, so a later pod-in must not auto-resume from a prior auto-pause. diff --git a/app/src/test/java/eu/darken/capod/common/MediaControlTest.kt b/app/src/test/java/eu/darken/capod/common/MediaControlTest.kt index 96fb57ee..7a93c7f0 100644 --- a/app/src/test/java/eu/darken/capod/common/MediaControlTest.kt +++ b/app/src/test/java/eu/darken/capod/common/MediaControlTest.kt @@ -55,6 +55,9 @@ class MediaControlTest : BaseTest() { mediaControl = MediaControl(audioManager, timeSource, handler) // Drain the init runnable so `playbackCallbackSlot` is populated for `fireCallback()`. initRunnableSlot.captured.run() + // Everything posted after init (the pause arm) runs inline and synchronously, which keeps + // the arm-before-dispatch ordering that the tests below observe. + every { handler.post(any()) } answers { firstArg().run(); true } } private fun playbackConfig(stream: Int): AudioPlaybackConfiguration { @@ -351,6 +354,34 @@ class MediaControlTest : BaseTest() { assertFalse(mediaControl.wasRecentlyPausedByCap) } + @Test + fun `stale queued snapshot pair draining before the arm does not clear it`() = runTest { + // Regression pin for the queued-pair race: a pre-pause inactive→active pair (e.g. a track + // change gap) that was already delivered but not yet drained when the pause armed. On the + // pre-fix code the pair drained after the caller-side arm and the active snapshot read as a + // fresh music-start edge that cleared it. Arming on the callback handler queues the arm + // behind the pair instead. + every { audioManager.isMusicActive } returns true + fireCallback(true) // seed lastKnownMusicActive=true + + // The pair is queued ahead of the arm, so it drains before the posted runnable runs. + every { handler.post(any()) } answers { + fireCallback(false) + fireCallback(true) + firstArg().run() + true + } + + mediaControl.sendPause(rememberForResume = true) + assertTrue(mediaControl.wasRecentlyPausedByCap) + + // A genuinely later resume still clears the arm. + every { handler.post(any()) } answers { firstArg().run(); true } + fireCallback(false) + fireCallback(true) + assertFalse(mediaControl.wasRecentlyPausedByCap) + } + @Test fun `concurrent sendPause during sendPlay dispatch keeps the later pause armed`() = runTest { // Repro for the lost update: sendPlay's flag write used to land after sendKey's delay(100), @@ -461,7 +492,14 @@ class MediaControlTest : BaseTest() { every { freshAudioManager.dispatchMediaKeyEvent(any()) } just Runs every { freshAudioManager.registerAudioPlaybackCallback(any(), any()) } just Runs val freshHandler = mockk() - every { freshHandler.post(any()) } returns true + // The first post is the init runnable and is deliberately left undrained — that is the + // premise of this test. Everything posted afterwards (the pause arm) runs inline. + var initRunnable: Runnable? = null + every { freshHandler.post(any()) } answers { + val posted = firstArg() + if (initRunnable == null) initRunnable = posted else posted.run() + true + } val undrained = MediaControl(freshAudioManager, timeSource, freshHandler)