From 5014f7548ec396f25ecb9288944a671d0b3a1761 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 18:21:39 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20drift=20audit=20batch=203b=20=E2=80=94?= =?UTF-8?q?=20cold-boot=20resume=20correctness?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related findings from the 2026-06-02 drift audit (#552): - **#560 (Android)** PlayerController.setQueue() unconditionally called controller.play() at the end, with no way for ResumeController to opt out. Cold-boot resume therefore restored the persisted queue AND auto-started playback, which surprised users who had paused mid-track before backgrounding the app. Add `autoplay: Boolean = true` parameter; ResumeController passes false. Every existing setQueue call site continues to autoplay (the default is unchanged). - **#562 (Android)** ResumeController.restore() ran from MinstrelApplication.onCreate alongside PlayerController's own init {} block that asynchronously binds the MediaController to MinstrelPlayerService. On fast devices with slow IPC the restore could land before mediaController was non-null; PlayerController.setQueue early-returns on null mediaController, so the restored queue was silently dropped — the user would open the app to an empty player after explicitly using "resume previous queue". Add `awaitReady()` suspend that completes when the MediaController binding lands; ResumeController awaits it before calling setQueue. The two fixes ship together because the autoplay opt-out only matters once the await fix guarantees the queue actually reaches the player. --- .../minstrel/player/PlayerController.kt | 45 +++++++++++++++++-- .../minstrel/player/ResumeController.kt | 10 +++++ 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt index 9fc44bb7..102bcba1 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt @@ -102,10 +102,30 @@ class PlayerController @Inject constructor( */ private var queueRefs: List = emptyList() + /** + * Completes when [mediaController] is non-null and the listener has + * been attached. Used by [awaitReady] so cold-boot callers like + * [ResumeController] can wait for the IPC bind before calling + * transport methods that would otherwise no-op silently. Drift + * #562 caught the race where a fast restore() landed before the + * MediaSessionService connection was up, silently dropping the + * restored queue. + */ + private val readyDeferred = kotlinx.coroutines.CompletableDeferred() + init { scope.launch { connectAndObserve() } } + /** + * Suspends until the MediaController binding to + * [MinstrelPlayerService] is up and a Player.Listener is attached, + * so a subsequent transport call (setQueue, startRadio, playNext, + * …) will actually reach the player rather than being silently + * swallowed by the `mediaController ?: return` guards. + */ + suspend fun awaitReady(): Unit = readyDeferred.await() + // ── Transport (no-op until the controller is connected) ────────────── fun play() { mediaController?.play() } @@ -144,18 +164,32 @@ class PlayerController @Inject constructor( } /** - * Replace the queue with [tracks] and start playing from [initialIndex]. + * Replace the queue with [tracks] starting at [initialIndex]. * [source] tags the queue with its origin (e.g. "for_you") so the * server-side rotation reporter can advance it; carried in * MediaItem extras. + * + * [autoplay] controls whether playback starts immediately. The + * default is true to preserve the "user pressed play on a tile" + * UX every existing caller relies on. Drift #560: cold-boot + * resume passes autoplay = false so the persisted queue is + * restored without auto-starting playback after the user has + * been away from the app — starting audio on cold launch was + * surprising for users who had paused mid-track before + * backgrounding. */ - fun setQueue(tracks: List, initialIndex: Int = 0, source: String? = null) { + fun setQueue( + tracks: List, + initialIndex: Int = 0, + source: String? = null, + autoplay: Boolean = true, + ) { val controller = mediaController ?: return queueRefs = tracks val items = tracks.map { it.toMediaItem(source) } controller.setMediaItems(items, initialIndex, /* startPositionMs = */ 0L) controller.prepare() - controller.play() + if (autoplay) controller.play() } /** @@ -282,6 +316,11 @@ class PlayerController @Inject constructor( return } mediaController = controller + // Drift #562: signal awaitReady() callers (ResumeController et al.) + // that the controller is bound. Listener is attached below in + // the same coroutine so by the time downstream code runs after + // awaitReady, the Player.Listener is wired too. + if (!readyDeferred.isCompleted) readyDeferred.complete(Unit) startPositionPolling(controller) controller.addListener( object : Player.Listener { diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/ResumeController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/ResumeController.kt index 1f666738..fddf8e46 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/ResumeController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/ResumeController.kt @@ -54,12 +54,22 @@ class ResumeController @Inject constructor( .getOrNull() ?.takeIf { it.tracks.isNotEmpty() } ?: return + // Drift #562: wait for the MediaController binding before + // calling setQueue, otherwise restore() racing the IPC + // handshake silently drops the persisted queue (PlayerController + // setQueue early-returns when mediaController is null). + playerController.awaitReady() playerController.setQueue( tracks = payload.tracks, initialIndex = payload.queueIndex .coerceAtLeast(0) .coerceAtMost(payload.tracks.lastIndex), source = payload.source, + // Drift #560: cold-boot resume must NOT auto-start + // playback. The user has been away from the app; starting + // audio on cold launch is surprising. They tap play to + // resume. Queue + position are restored; transport is idle. + autoplay = false, ) // PositionMs not seeked yet — Player connection may still be // establishing; the seek call would no-op. Acceptable to start