fix(reaction): Serialize media key dispatch and read playback snapshots

Two pre-existing concurrency defects in MediaControl (#647). Lost update: sendPlay() wrote capPaused after the suspending sendKey(), whose delay(100) is a window in which a concurrent sendPause(rememberForResume = true) could arm the flag only to have it overwritten. Stem presses run on the app scope while ear, sleep and conversation reactions run on the monitor scope, so the senders really do race. The compound check/dispatch/flag sequence now runs under a Mutex, the flag is cleared before the first suspension, and the key pair completes under NonCancellable so cancellation cannot strand an unpaired DOWN event. Coalescing blind spot: the playback callback ignored its configs argument and read live isMusicActive, so queued deliveries all observed the newest state and an inactive to active edge in between was never seen, leaving capPaused stale. The edge is now derived from the delivered snapshot. A pause that passes the live active check records that observation so an already-queued music-start snapshot cannot drain later and read as a fresh edge.
2026-09-15 02:36:12 -04:00 · 2026-07-28 23:52:04 +02:00
parent edd3cf0c2b
commit 5e28eeed38
2 changed files with 246 additions and 27 deletions
@@ -10,7 +10,11 @@ import eu.darken.capod.common.debug.logging.Logging.Priority.INFO
 import eu.darken.capod.common.debug.logging.Logging.Priority.WARN
 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.sync.Mutex
+import kotlinx.coroutines.sync.withLock
+import kotlinx.coroutines.withContext
 import javax.inject.Inject
 import javax.inject.Singleton

@@ -32,9 +36,28 @@ class MediaControl @Inject constructor(
    @Volatile private var capPaused: Boolean = false
    @Volatile private var lastKnownMusicActive: Boolean = false

+    /**
+     * The compound isMusicActive-check → key dispatch → [capPaused] transition must not interleave
+     * across concurrent callers: the `delay(100)` suspension inside [sendKey] is the window in which
+     * a second sender could observe stale state and lose the other's flag update (issue #647).
+     * Callers genuinely race — stem presses run on the app scope, ear/sleep/conversation reactions
+     * on the monitor scope.
+     */
+    private val dispatchLock = Mutex()
+
    private val playbackCallback = object : AudioManager.AudioPlaybackCallback() {
        override fun onPlaybackConfigChanged(configs: List<AudioPlaybackConfiguration>) {
-            val nowActive = audioManager.isMusicActive
+            // The edge is derived from this delivery's own snapshot, not a live isMusicActive read:
+            // per-invocation snapshots preserve the sequence when deliveries queue up behind the
+            // handler (a live read makes every queued delivery see the newest state, so an
+            // inactive→active edge in the middle of the queue is never observed). It also keeps a
+            // binder call out of the callback body.
+            //
+            // Residual: API 36+ may replace a pending config message with the newest one, delivering
+            // a single callback for two transitions. A state that is never delivered is unrecoverable
+            // at the receiver; when that happens capPaused stays stale and costs one redundant
+            // (idempotent) MEDIA_PLAY on a later pod-in — the same cost as before, just rarer.
+            val nowActive = configs.any { it.isMusicStream() }
            if (!lastKnownMusicActive && nowActive) {
                // Music started by some source (could be us via sendPlay or someone else).
                // Either way, our pause memory is stale — drop it so a future pod-in doesn't
@@ -45,6 +68,18 @@ class MediaControl @Inject constructor(
        }
    }

+    /**
+     * Parity with what [AudioManager.isMusicActive] counts (active STREAM_MUSIC players), using the
+     * platform's own attribute→stream mapping (flags and OEM strategies included) instead of a
+     * hand-rolled usage set. Ambiguous or exceptional configs count as not-music: that errs toward a
+     * missed clear (one redundant, idempotent MEDIA_PLAY) rather than a false clear (lost auto-resume).
+     */
+    private fun AudioPlaybackConfiguration.isMusicStream(): Boolean = try {
+        audioAttributes.volumeControlStream == AudioManager.STREAM_MUSIC
+    } catch (e: IllegalArgumentException) {
+        false
+    }
+
    init {
        // Both calls below are binder transactions into AudioService. On the main thread they sat on
        // the cold-start critical path (MediaControl is constructed during App.onCreate via Hilt) and
@@ -73,14 +108,20 @@ class MediaControl @Inject constructor(
    val wasRecentlyPausedByCap: Boolean
        get() = capPaused

-    suspend fun sendPlay() {
+    suspend fun sendPlay() = dispatchLock.withLock { sendPlayLocked() }
+
+    private suspend fun sendPlayLocked() {
        log(TAG, INFO) { "sendPlay()" }
        if (audioManager.isMusicActive && !capPaused) {
            log(TAG, INFO) { "Music is already playing, not sending play" }
            return
        }
-        sendKey(KeyEvent.KEYCODE_MEDIA_PLAY)
+        // Cleared before the first suspension: sendKey's delay is a cancellation point (the key pair
+        // itself completes under NonCancellable, but the caller can still be cancelled at the lock
+        // boundaries), so clearing first means a cancelled resume can never strand a stale armed
+        // flag. Under the mutex the ordering no longer matters for racing senders.
        capPaused = false
+        sendKey(KeyEvent.KEYCODE_MEDIA_PLAY)
    }

    /**
@@ -102,7 +143,11 @@ class MediaControl @Inject constructor(
     * on the return value rather than checking [isPlaying] themselves to avoid a
     * check-then-act race with the audio system.
     */
-    suspend fun sendPause(rememberForResume: Boolean = false): Boolean {
+    suspend fun sendPause(rememberForResume: Boolean = false): Boolean = dispatchLock.withLock {
+        sendPauseLocked(rememberForResume)
+    }
+
+    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" }
@@ -115,6 +160,11 @@ class MediaControl @Inject constructor(
        // 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
    }
@@ -123,26 +173,31 @@ class MediaControl @Inject constructor(
     * 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.
     */
-    suspend fun sendStop() {
+    suspend fun sendStop() = dispatchLock.withLock {
        log(TAG, INFO) { "sendStop()" }
        capPaused = false
        sendKey(KeyEvent.KEYCODE_MEDIA_STOP)
    }

    suspend fun sendPlayPause() {
-        log(TAG) { "sendPlayPause()" }
-        if (capPaused) {
-            sendPlay()
-            return
-        }
-        if (audioManager.isMusicActive) {
-            sendPause()
-        } else {
-            sendPlay()
+        dispatchLock.withLock {
+            log(TAG) { "sendPlayPause()" }
+            if (capPaused) {
+                sendPlayLocked()
+                return@withLock
+            }
+            if (audioManager.isMusicActive) {
+                sendPauseLocked(rememberForResume = false)
+            } else {
+                sendPlayLocked()
+            }
        }
    }

-    internal suspend fun sendKey(keyCode: Int) {
+    internal suspend fun sendKey(keyCode: Int) = withContext(NonCancellable) {
+        // DOWN and UP must always be dispatched as a pair: once DOWN is out, cancellation may not
+        // strand it unpaired or the media session keeps seeing a held key. Bounded — this defers
+        // cancellation by at most the 100ms delay below.
        log(TAG) { "Sending up+down KeyEvent: $keyCode" }
        val eventTime = timeSource.uptimeMillis()
        audioManager.dispatchMediaKeyEvent(KeyEvent(eventTime, eventTime, KeyEvent.ACTION_DOWN, keyCode, 0))
@@ -1,7 +1,10 @@
 package eu.darken.capod.common

+import android.media.AudioAttributes
 import android.media.AudioManager
+import android.media.AudioPlaybackConfiguration
 import android.os.Handler
+import android.view.KeyEvent
 import io.mockk.CapturingSlot
 import io.mockk.Runs
 import io.mockk.clearMocks
@@ -11,7 +14,11 @@ import io.mockk.mockk
 import io.mockk.slot
 import io.mockk.verify
 import io.mockk.verifyOrder
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.test.advanceUntilIdle
+import kotlinx.coroutines.test.runCurrent
 import kotlinx.coroutines.test.runTest
+import org.junit.jupiter.api.Assertions.assertEquals
 import org.junit.jupiter.api.Assertions.assertFalse
 import org.junit.jupiter.api.Assertions.assertTrue
 import org.junit.jupiter.api.BeforeEach
@@ -50,8 +57,34 @@ class MediaControlTest : BaseTest() {
        initRunnableSlot.captured.run()
    }

-    private fun fireCallback() {
-        playbackCallbackSlot.captured.onPlaybackConfigChanged(emptyList())
+    private fun playbackConfig(stream: Int): AudioPlaybackConfiguration {
+        val attributes = mockk<AudioAttributes>()
+        every { attributes.volumeControlStream } returns stream
+        return mockk<AudioPlaybackConfiguration>().also {
+            every { it.audioAttributes } returns attributes
+        }
+    }
+
+    /**
+     * The callback derives the edge from the delivered snapshot, so the snapshot has to carry the
+     * state — stubbing `isMusicActive` no longer influences it.
+     */
+    private fun fireCallback(musicActive: Boolean) {
+        val configs: List<AudioPlaybackConfiguration> =
+            if (musicActive) listOf(playbackConfig(AudioManager.STREAM_MUSIC)) else emptyList()
+        playbackCallbackSlot.captured.onPlaybackConfigChanged(configs)
+    }
+
+    private fun fireCallbackWithStream(stream: Int) {
+        playbackCallbackSlot.captured.onPlaybackConfigChanged(listOf(playbackConfig(stream)))
+    }
+
+    private fun fireCallbackWithUnmappableConfig() {
+        val attributes = mockk<AudioAttributes>()
+        every { attributes.volumeControlStream } throws IllegalArgumentException("Unknown usage")
+        val config = mockk<AudioPlaybackConfiguration>()
+        every { config.audioAttributes } returns attributes
+        playbackCallbackSlot.captured.onPlaybackConfigChanged(listOf(config))
    }

    @Test
@@ -194,20 +227,20 @@ class MediaControlTest : BaseTest() {
    @Test
    fun `wasRecentlyPausedByCap clears when music transitions inactive to active from any source`() = runTest {
        every { audioManager.isMusicActive } returns true
-        fireCallback() // seed lastKnownMusicActive=true
+        fireCallback(true) // seed lastKnownMusicActive=true

        mediaControl.sendPause(rememberForResume = true)
        assertTrue(mediaControl.wasRecentlyPausedByCap)

        // Music goes inactive (CAP's pause took effect).
        every { audioManager.isMusicActive } returns false
-        fireCallback()
+        fireCallback(false)
        assertTrue(mediaControl.wasRecentlyPausedByCap)

        // Music starts again (e.g. user manually resumed via phone). Sticky flag must clear so
        // a later pod-in doesn't fire a stray play key on top of already-playing music.
        every { audioManager.isMusicActive } returns true
-        fireCallback()
+        fireCallback(true)
        assertFalse(mediaControl.wasRecentlyPausedByCap)
    }

@@ -218,7 +251,7 @@ class MediaControlTest : BaseTest() {
        // callback would clear it, then sendPause's post-dispatch line would put it back to
        // true while music is genuinely playing again — wrongly arming a future pod-in resume.
        every { audioManager.isMusicActive } returns true
-        fireCallback() // seed lastKnownMusicActive=true
+        fireCallback(true) // seed lastKnownMusicActive=true

        // sendKey is implemented with an internal delay(100). Drive a callback during that
        // window by sending a single coalesced inactive→active sequence right after kicking
@@ -227,9 +260,9 @@ class MediaControlTest : BaseTest() {
        every { audioManager.dispatchMediaKeyEvent(any()) } answers {
            // First DOWN dispatch: pretend music briefly went inactive then active mid-pause.
            every { audioManager.isMusicActive } returns false
-            fireCallback()
+            fireCallback(false)
            every { audioManager.isMusicActive } returns true
-            fireCallback()
+            fireCallback(true)
        }

        mediaControl.sendPause(rememberForResume = true)
@@ -241,15 +274,146 @@ class MediaControlTest : BaseTest() {
        assertFalse(mediaControl.wasRecentlyPausedByCap)
    }

+    @Test
+    fun `callback edge detection uses the event snapshot so coalesced queued deliveries still clear the flag`() =
+        runTest {
+            // Deliveries queue up on the callback handler. A live isMusicActive read makes every
+            // queued delivery observe the newest state, so an inactive→active edge that happened
+            // while they were queued is never seen and capPaused stays stale.
+            every { audioManager.isMusicActive } returns true
+            fireCallback(true)
+
+            mediaControl.sendPause(rememberForResume = true)
+            assertTrue(mediaControl.wasRecentlyPausedByCap)
+
+            // Live state stays "active" the whole time — only the snapshots carry the sequence.
+            fireCallback(false)
+            fireCallback(true)
+
+            assertFalse(mediaControl.wasRecentlyPausedByCap)
+        }
+
+    @Test
+    fun `replacement delivery that drops the intermediate inactive snapshot leaves the flag armed`() = runTest {
+        // Pins the documented residual: API 36+ may replace a pending config message with the
+        // newest one, so two transitions arrive as a single callback. A state that is never
+        // delivered is unrecoverable at the receiver — the accepted cost is a stale capPaused
+        // producing one redundant (idempotent) MEDIA_PLAY on a later pod-in.
+        every { audioManager.isMusicActive } returns true
+        fireCallback(true)
+
+        mediaControl.sendPause(rememberForResume = true)
+        assertTrue(mediaControl.wasRecentlyPausedByCap)
+
+        fireCallback(true)
+
+        assertTrue(mediaControl.wasRecentlyPausedByCap)
+    }
+
+    @Test
+    fun `non-music playback configs do not count as a music-start edge`() = runTest {
+        // Init drained with isMusicActive=false, so lastKnownMusicActive starts false.
+        every { audioManager.isMusicActive } returns true
+
+        mediaControl.sendPause(rememberForResume = true)
+        assertTrue(mediaControl.wasRecentlyPausedByCap)
+
+        // The pause records music as active; bring the observation back to inactive so the
+        // following deliveries are candidates for an inactive→active edge.
+        fireCallback(false)
+        assertTrue(mediaControl.wasRecentlyPausedByCap)
+
+        fireCallbackWithStream(AudioManager.STREAM_NOTIFICATION)
+        assertTrue(mediaControl.wasRecentlyPausedByCap)
+
+        // A config whose attributes have no legacy stream mapping counts as not-music too.
+        fireCallbackWithUnmappableConfig()
+        assertTrue(mediaControl.wasRecentlyPausedByCap)
+    }
+
+    @Test
+    fun `queued music-start snapshot draining after an owned pause does not clear the fresh arm`() = runTest {
+        // Init drained with isMusicActive=false, so lastKnownMusicActive starts false.
+        every { audioManager.isMusicActive } returns true
+
+        mediaControl.sendPause(rememberForResume = true)
+        assertTrue(mediaControl.wasRecentlyPausedByCap)
+
+        // A music-start snapshot that was already queued when the pause armed the flag drains now.
+        // The pause's own live active-check already observed that music, so this must not read as a
+        // fresh inactive→active edge.
+        fireCallback(true)
+        assertTrue(mediaControl.wasRecentlyPausedByCap)
+
+        // A genuine resume afterwards still clears it.
+        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),
+        // so a sendPause arming capPaused inside that window was overwritten back to false.
+        every { audioManager.isMusicActive } returns true
+        mediaControl.sendPause(rememberForResume = true)
+        assertTrue(mediaControl.wasRecentlyPausedByCap)
+        clearMocks(audioManager, answers = false, recordedCalls = true)
+
+        val dispatched = mutableListOf<KeyEvent>()
+        // KeyEvent getters throw in plain JVM unit tests (mockable android.jar), so the dispatched
+        // events are counted, and the pairs are told apart by the flag state each one was sent
+        // under: the play pair dispatches with capPaused=false, the pause pair with capPaused=true.
+        // Interleaved dispatch would therefore not produce false,false,true,true.
+        val armedAtDispatch = mutableListOf<Boolean>()
+        every { audioManager.dispatchMediaKeyEvent(capture(dispatched)) } answers {
+            armedAtDispatch += mediaControl.wasRecentlyPausedByCap
+        }
+
+        val playJob = launch { mediaControl.sendPlay() }
+        runCurrent()
+        val pauseJob = launch { mediaControl.sendPause(rememberForResume = true) }
+        runCurrent()
+        advanceUntilIdle()
+        playJob.join()
+        pauseJob.join()
+
+        assertTrue(mediaControl.wasRecentlyPausedByCap)
+        assertEquals(4, dispatched.size)
+        assertEquals(listOf(false, false, true, true), armedAtDispatch)
+    }
+
+    @Test
+    fun `cancellation during sendPlay dispatch neither strands the flag nor an unpaired key event`() = runTest {
+        every { audioManager.isMusicActive } returns true
+        mediaControl.sendPause(rememberForResume = true)
+        assertTrue(mediaControl.wasRecentlyPausedByCap)
+        clearMocks(audioManager, answers = false, recordedCalls = true)
+
+        val dispatched = mutableListOf<KeyEvent>()
+        every { audioManager.dispatchMediaKeyEvent(capture(dispatched)) } just Runs
+
+        val job = launch { mediaControl.sendPlay() }
+        runCurrent() // DOWN is out, we are inside sendKey's delay
+        job.cancel()
+        advanceUntilIdle()
+
+        // Cleared before the first suspension, so a cancelled resume cannot strand a stale arm.
+        assertFalse(mediaControl.wasRecentlyPausedByCap)
+        // NonCancellable completes the pair rather than leaving a held key.
+        assertEquals(2, dispatched.size)
+    }
+
    @Test
    fun `playback callback is registered with the injected handler rather than null`() {
-        // Regression for the ANR cluster in onPlaybackConfigChanged: passing `null` here binds
-        // callback delivery to the main looper, and the callback body does a binder call.
+        // Regression for the ANR cluster around the playback callback: registration itself is
+        // binder work into AudioService and must not run on the main looper, which is what passing
+        // `null` here would select. The handler choice also decides the Looper that serializes
+        // callback delivery, and the snapshot-based edge detection depends on that ordering.
        // Scope of this assertion: it only proves the injected handler is forwarded, not which
        // Looper that handler is bound to — a JVM unit test cannot inspect a Looper here (this
        // module does not use Robolectric). The Looper identity is covered instead by
-        // `AndroidModule.audioCallbackHandler()`, which is the single place that constructs it,
-        // and by the runtime QA check asserting the registration logs thread `CAPod-MediaControl`.
+        // `AndroidModule.audioCallbackHandler()`, which is the single place that constructs it.
        verify { audioManager.registerAudioPlaybackCallback(any(), handler) }
    }