Fix concurrent playback transitions on Android Automotive

[joashrajin]

Description

Prevents concurrent and stale playback transitions from creating or retaining multiple audio players on Android Automotive.

Playback commands now:

• capture their ordering token before asynchronous work starts
• discard superseded preparation and delayed player events
• serialize queue selection, player replacement, episode binding, and play/load operations
• preserve callbacks when a newer command is a legitimate no-op

This keeps the fix focused in the shared playback manager while covering Play, Pause, Stop, completion, autoplay, error recovery, queue removal, and Cast handoffs.

Fixes PCDROID-569

Testing Instructions

1. Run ./gradlew spotlessCheck.
2. Run ./gradlew :modules:services:repositories:testDebugUnitTest.
3. Run ./gradlew :automotive:compileDebugKotlin.
4. On Android Automotive, start an episode from the browse tree and quickly issue another Play, Pause, or Stop command while it prepares.
5. Repeat with a different episode, the already-current episode, and after disconnecting from Cast.
6. Confirm only one audio stream is audible, Pause/Stop silences playback, and no audio remains after leaving the source.

Screenshots or Screencast

Not applicable — no UI changes.

Checklist

• If this is a user-facing change, I have added an entry in CHANGELOG.md
• Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
• I have considered whether it makes sense to add tests for my changes
• All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
• Any jetpack compose components I added or changed are covered by compose previews
• I have updated (or requested that someone edit) the Event Horizon schema to reflect any new or changed analytics.

I have tested any UI changes...

• with different themes
• with a landscape orientation
• with the device set to have a large display and font size
• for accessibility with TalkBack

[claude]

Claude finished @joashrajin's task in 4m 12s —— View job

---

Code Review

I reviewed the full diff (PlaybackManager.kt +603/-181, the new PlayerTransitionCoordinator, and its test). Overall this is a well-structured approach to a genuinely hard concurrency problem, and the coordinator abstraction is clean. My feedback is below — two inline comments and some notes.

• Read PR context & changed files
• Analyze concurrency logic in PlaybackManager
• Review PlayerTransitionCoordinator + tests
• Post review feedback

What looks good 👍

• PlayerTransitionCoordinator is a solid abstraction. Monotonic version token + Mutex for serialization + source→token binding is a reasonable way to both order transitions and prevent a player being swapped while it's being configured. The unit tests cover the key scenarios (newer-wins, serialization, re-check after acquiring the mutex, stale event handling, no-op adoption).
• resetPlayer() now uses try/finally to reset resettingPlayer — a real improvement over the previous code, where an exception in the block would have left the flag stuck at true forever. Marking it @Volatile is appropriate since it's read/written across dispatchers.
• Replacing the mutable forcePlayerSwitch field (which had a read-and-reset side effect) with an explicit threaded parameter is cleaner and less surprising.
• Splitting loadCurrentEpisode into resolve-then-commit (resolveStreamOverride off the critical section, applyStreamOverride only after re-confirming currency) is a nice way to keep expensive/async work out of the serialized region.

Issues & concerns

1. removeEpisode can silently drop a removal in a race (inline on PlaybackManager.kt:1531)
removesCurrentPlayback is decided outside removeMutex, but isCurrentEpisode is recomputed inside it. If they disagree (transition token null but episode is current under the lock), ?: return@withLock exits without removing the episode. Before this PR the removal always happened. Details and a fix link are in the inline comment.

2. Player events are now gated on transition currency + binding (inline on PlaybackManager.kt:2906)
This is a meaningful behavior change: ambient events (DurationAvailable, BufferingStateChanged, PlayerPlaying/Paused, SeekComplete, MetadataAvailable, etc.) are dropped whenever the bound token isn't current or the player is unbound. The design depends on every path that calls beginTransition() eventually committing / calling completeNoOpTransition / being superseded — otherwise a still-current player is stranded and stops updating the UI. Worth verifying each new path upholds that invariant. (I traced the modified paths and they appear to, but it's fragile.)

3. Test coverage is limited to the coordinator. The 400+ lines of PlaybackManager rewiring — the actual bug surface (Play/Pause/Stop/completion/autoplay/error-recovery/remove/Cast) — have no new tests. Given this is critical playback code and the fix is inherently timing-dependent, a couple of PlaybackManager-level tests reproducing the original "two players / double audio" race (rapid Play→Play, Play→Pause while preparing) would substantially de-risk this and guard against regressions. There's an existing test harness in this module (PlaybackManagerNetworkWatcherTest, PrefetchNextEpisodeTest) to build on.

Minor notes

• removeEpisode (public) now calls getCurrentEpisode() / player?.episodeUuid synchronously on the caller's thread (previously this ran inside launch). Likely harmless, but it moves a bit of work onto whatever thread invokes it (possibly main).
• In onCompletion, the episode-UUID mismatch check moved earlier, so on a stale completion the timer-cancel / sleep-timer handling that used to run is now skipped. This looks intentional and arguably more correct, just flagging the behavior change.

Note on verification

I reviewed statically only — I did not run spotlessCheck, the repositories unit tests, or :automotive:compileDebugKotlin (the testing steps in the PR description). I'd recommend confirming CI is green for those before merge.

Nice work tackling a tricky race; the two inline items (especially #1) are the ones I'd want addressed before this lands.

[dangermattic]

	1 Warning
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

	1 Message
📖	This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

[wpmobilebot]

Version 8.18 has now entered code-freeze, so the milestone of this PR has been updated to 8.19.