Harden media button event sequencing - #5646
Conversation
joashrajin
added
[Type] Bug
|
Claude Code is working… I'll analyze this and get back to you. |
|
Version |
|
Claude finished @joashrajin's task in 5m 50s —— View job Code Review: Harden media button event sequencing
The core change is sound: guarding I found one correctness issue, one behavioural divergence that will matter in #5645, and a few robustness/clarity items. Nothing blocking, but items 1–3 are worth resolving before the follow-up builds on this contract. Findings1. Rollback cleanup is skipped on cancellation ( 2. Two layers own errors, and they disagree ( 3. 4. The stated ordering guarantee is best-effort ( 5. 6. Test hardening ( 7. Thread-safety of Things I liked
Not verifiedI could not run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| private fun handleImmediatePlay() { | ||
| try { | ||
| onImmediatePlay() | ||
| } catch (e: CancellationException) { | ||
| throw e | ||
| } catch (e: Exception) { | ||
| onError(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
Error ownership is duplicated here, and it makes the rollback in MediaEventQueue dead code.
Because handleImmediatePlay catches Exception itself, no exception ever reaches MediaEventQueue.handleSingleTapEvent, so the rollback block at MediaEventQueue.kt:47-55 is unreachable from the only production caller. (CancellationException is rethrown, but the queue's catch (e: Exception) would catch it anyway since java.util.concurrent.CancellationException extends IllegalStateException.)
The two test files now assert opposite behaviour for the same scenario:
MediaEventQueueTest.kt:125— callback throws ⇒ window is discarded, the next tap resolves as a freshSingleTap.MediaButtonEventHandlerTest.kt:58— callback throws ⇒ window survives, the second tap resolves asDoubleTap.
Both are defensible, but only one is reachable in the app, and there is no test pinning which layer owns the decision. If someone later removes this try/catch (it looks redundant — the caller already catches), the user-visible behaviour silently flips from "double tap still works" to "double tap is swallowed".
Suggest picking one owner: either drop this try/catch and keep the queue's rollback (accepting that a failed immediate play discards a concurrent double tap), or drop the rollback in the queue and document in consumeEvent's KDoc that onImmediateSingleTap must not throw. Either way, a comment explaining the choice would help.
| try { | ||
| onImmediateSingleTap?.invoke() | ||
| } catch (e: Exception) { | ||
| stateMutex.withLock { | ||
| if (singleTapJob === newSingleTapJob) { | ||
| singleTapJob = null | ||
| newSingleTapJob.cancel() | ||
| } | ||
| } | ||
| throw e | ||
| } |
There was a problem hiding this comment.
Two problems with this rollback block:
1. The cleanup is skipped exactly when it is most likely to be needed. stateMutex.withLock is a suspend call inside a catch (e: Exception), and CancellationException is an Exception (java.util.concurrent.CancellationException → IllegalStateException). If the coroutine has been cancelled, this withLock throws CancellationException immediately, so singleTapJob is never cleared, newSingleTapJob.cancel() never runs, and the original exception is replaced by the cancellation. Wrap the cleanup in withContext(NonCancellable), or catch a narrower type and let cancellation flow past.
2. A concurrent double tap is silently lost. If a second tap already ran incrementTaps() (counter = 2) while the first coroutine was inside onImmediateSingleTap, the second tap returned null and is relying on this coroutine to emit DoubleTap. Rolling back throws instead, so the user's double tap disappears. Rethrowing after cleanup means nobody emits the resolved event.
} catch (e: Exception) {
withContext(NonCancellable) {
stateMutex.withLock {
if (singleTapJob === newSingleTapJob) {
singleTapJob = null
newSingleTapJob.cancel()
}
}
}
throw e
}
| // Register the event before returning to the framework callback. This preserves | ||
| // delivery order while the queue's timeout still resumes on the provided scope. | ||
| scope.launch(start = CoroutineStart.UNDISPATCHED) { |
There was a problem hiding this comment.
The ordering guarantee this comment claims is best-effort, not absolute — worth softening the wording so the next reader doesn't rely on it.
UNDISPATCHED runs the body on the caller's thread only until the first real suspension. consumeEvent immediately hits stateMutex.withLock, which takes the non-suspending fast path only when the mutex is uncontended. If two media-button events genuinely overlap (the case this PR is hardening against), the second one suspends on lock() and resumes on the scope's dispatcher — so registration happens after handle() has already returned true, which is exactly the ordering the comment says is preserved.
Second consideration: UNDISPATCHED also means onImmediatePlay executes synchronously on whatever thread onMediaButtonEvent was delivered on. For Media3 that's the app's main looper, so playbackManager.playPause(...) would now run inline on the main thread instead of on the callback scope's dispatcher as it does today. The KDoc says "must stay fast", but it would be good to confirm in #5645 that the wired-up onImmediatePlay is genuinely main-safe.
| if (outputEvent != null) { | ||
| // Output actions historically ran asynchronously on the callback scope. | ||
| yield() | ||
| onMediaEvent(outputEvent) | ||
| } |
There was a problem hiding this comment.
yield() is a fragile way to express "run this asynchronously": it is a no-op on a dispatcher whose isDispatchNeeded returns false (e.g. Dispatchers.Unconfined). With such a scope, and since multi-tap events return from consumeEvent without suspending, onMediaEvent would run inline on the framework callback stack — silently breaking the invariant in the KDoc.
Both current call sites use real dispatchers so this is latent rather than live, but scope.launch { onMediaEvent(outputEvent) } (or withContext(dispatcher)) states the intent directly and can't be defeated by the scope's dispatcher choice.
| fun handle(keyEvent: KeyEvent): Boolean { | ||
| if (keyEvent.action != KeyEvent.ACTION_DOWN) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Returning false for non-ACTION_DOWN events diverges from both callbacks this handler is meant to replace:
Media3SessionCallback.kt:200-202returnstruefor a media key withaction != ACTION_DOWN.MediaSessionManager.kt:1262falls through tosuper.onMediaButtonEvent(...).
Returning false tells Media3 the event was not consumed, so it applies its own default handling to the ACTION_UP half of every media-button press. Since every ACTION_DOWN is followed by an ACTION_UP, that risks a second action per press once #5645 wires this in (e.g. player.play()/pause() on top of the resolved tap action).
Two other gaps to keep in mind for #5645, since the handler is presented as owning "key-code mapping":
KEYCODE_MEDIA_PAUSE,KEYCODE_MEDIA_SKIP_FORWARDandKEYCODE_MEDIA_SKIP_BACKWARD(Media3SessionCallback.kt:210-245, including the PiP skip buttons) are not mapped here, so the callback must still intercept them before delegating.- The
LogBuffer.i(..., "media button event: keyCode=...")breadcrumb present in both callbacks has no equivalent here; it's load-bearing for diagnosing headphone bug reports.
Worth adding a test that asserts the intended return value for ACTION_UP so the contract is pinned before the integration PR depends on it.
| @Test | ||
| fun `handle concurrent immediate single taps exactly once`() = runBlocking { | ||
| val handler = MediaEventQueue(scopeProvider = { this }) | ||
| val immediateTapCount = AtomicInteger() | ||
| val eventCount = 8 | ||
| val startBarrier = CyclicBarrier(eventCount) | ||
| val dispatcher = Executors.newFixedThreadPool(eventCount).asCoroutineDispatcher() | ||
|
|
||
| dispatcher.use { | ||
| List(eventCount) { | ||
| async(dispatcher) { | ||
| startBarrier.await() | ||
| handler.consumeEvent(MediaEvent.SingleTap) { | ||
| immediateTapCount.incrementAndGet() | ||
| } | ||
| } | ||
| }.awaitAll() | ||
| } | ||
|
|
||
| assertEquals(1, immediateTapCount.get()) | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Good test — this is the one that actually exercises the mutex. Three suggestions:
-
Assert the resolved event, not just the callback count. Right now the test would still pass if the queue lost every event. Eight taps should collapse to exactly one non-null result, and it should be
TripleTap:val results = /* ... */.awaitAll() assertEquals(1, immediateTapCount.get()) assertEquals(listOf(MediaEvent.TripleTap), results.filterNotNull())
(
TripleTapbecausecounterreaches 8 andevent()maps anything > 2 toTripleTap.) -
This is
runBlocking, so the 600 ms window is real wall-clock time, unlike the siblingrunTestcases. That's unavoidable for genuine multi-thread contention, but it makes the test the slowest in the class — worth a comment so a future reader doesn't "fix" it by switching torunTestand quietly removing the concurrency. -
CyclicBarrier(8)on a pool sized exactly 8 deadlocks if the pool ever loses a thread, andCyclicBarrier.await()blocks the carrier threads rather than suspending.Executors.newCachedThreadPool()(or aCountDownLatch+Dispatchers.IO.limitedParallelism) removes that coupling between the participant count and the pool size.
| @@ -60,6 +81,8 @@ internal class MediaEventQueue( | |||
|
|
|||
| suspend fun await() = job.join() | |||
|
|
|||
| fun cancel() = job.cancel() | |||
|
|
|||
| fun incrementTaps() { | |||
| counter++ | |||
| } | |||
There was a problem hiding this comment.
Minor: counter is a plain Int mutated from arbitrary threads, so its cross-thread visibility depends entirely on callers happening to hold stateMutex. That's true today, but it's an invariant enforced nowhere in this class — and it's the reason the final read has to be wrapped in stateMutex.withLock { newSingleTapJob.event() } at line 57 even though nothing there mutates shared state.
An AtomicInteger makes the thread-safety self-evident, lets event() be called without the lock, and removes one lock acquisition from the hot path:
| private class SingleTapJob( | |
| scope: CoroutineScope, | |
| ) { | |
| private val counter = AtomicInteger(1) | |
| private val job = scope.launch { delay(600) } | |
| val isActive get() = job.isActive | |
| suspend fun await() = job.join() | |
| fun cancel() = job.cancel() | |
| fun incrementTaps() { | |
| counter.incrementAndGet() | |
| } | |
| fun event() = when (counter.get()) { | |
| 1 -> MediaEvent.SingleTap | |
| 2 -> MediaEvent.DoubleTap | |
| else -> MediaEvent.TripleTap | |
| } | |
| } |
(Requires import java.util.concurrent.atomic.AtomicInteger.) Alternatively, a // guarded by stateMutex comment on counter, singleTapJob and multiTapJob would document the invariant cheaply.
|
Version |
2 participants
Description
Media-button callbacks currently launch queue work onto a shared multi-threaded
scope.
MediaEventQueuestores its tap jobs and counter in unsynchronized mutablestate, so concurrent callbacks can create multiple tap windows or observe events
out of order.
This PR is the first, independently tested part of the #5645 stack:
mutex across the 600 ms disambiguation window.
MediaButtonEventHandlerthat owns key-code mapping, errorhandling, and ordered event registration.
resolved media actions asynchronous on the supplied scope.
NEXT/PREVIOUSfollowed by spuriousPLAYsuppression.The follow-up PR #5645 wires this processor into the Media3 and legacy session
callbacks and contains the user-facing explicit-play behavior. Keeping the
processor and its unit tests here leaves both PRs below the 500-line review
threshold.
Related to #5631
Testing Instructions
Run:
Confirm all 23 focused tests pass.
Run
./gradlew :modules:services:repositories:testDebugUnitTestand confirmall 860 tests pass.
Run
./gradlew spotlessCheck.No manual playback verification is needed for this preparatory PR because the
handler is integrated into the application callbacks in #5645.
On-device verification of the stacked integration (2026-08-07)
Installed GitHub's merge result
c20537ff7c021b7431c0f33c332cb52084505338,which combines this PR at
9d4471ea4cc870a22a2cba06457045fe4fa679b5with #5645 at
361b8f64e5f0052d36751cf5d96ddfda00afb43f, on a physicalSamsung SM-G990E running Android 16. Both the legacy and Media3 session paths
passed:
KEYCODE_MEDIA_PLAYresumed paused playback while the displayremained dozing; the explicit-play handler ran in approximately 5–10 ms rather
than after the 600 ms tap window.
configured double-tap action after the window.
NEXT/PREVIOUSfollowed immediately by the spuriousPLAYevent keptthe configured skip action and suppressed the trailing play action.
Playback was left paused, the Media3 beta flag was restored to its original
enabled state, and the display was returned to dozing. No Bluetooth audio
accessory was connected, so media keys were injected through Android's media-key
dispatcher with
adb. This verifies the app/session and screen-off behavior,but not a physical headset's Bluetooth transport behavior.
Screenshots or Screencast
Not applicable; there are no UI changes.
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xml— no stringsI have tested any UI changes...
Not applicable; there are no UI changes.