fix: drift audit batch 3b — cold-boot resume correctness
android / Build + lint + test (push) Successful in 3m49s
android / Build + lint + test (push) Successful in 3m49s
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.
This commit is contained in:
@@ -102,10 +102,30 @@ class PlayerController @Inject constructor(
|
|||||||
*/
|
*/
|
||||||
private var queueRefs: List<TrackRef> = emptyList()
|
private var queueRefs: List<TrackRef> = 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<Unit>()
|
||||||
|
|
||||||
init {
|
init {
|
||||||
scope.launch { connectAndObserve() }
|
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) ──────────────
|
// ── Transport (no-op until the controller is connected) ──────────────
|
||||||
|
|
||||||
fun play() { mediaController?.play() }
|
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
|
* [source] tags the queue with its origin (e.g. "for_you") so the
|
||||||
* server-side rotation reporter can advance it; carried in
|
* server-side rotation reporter can advance it; carried in
|
||||||
* MediaItem extras.
|
* 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<TrackRef>, initialIndex: Int = 0, source: String? = null) {
|
fun setQueue(
|
||||||
|
tracks: List<TrackRef>,
|
||||||
|
initialIndex: Int = 0,
|
||||||
|
source: String? = null,
|
||||||
|
autoplay: Boolean = true,
|
||||||
|
) {
|
||||||
val controller = mediaController ?: return
|
val controller = mediaController ?: return
|
||||||
queueRefs = tracks
|
queueRefs = tracks
|
||||||
val items = tracks.map { it.toMediaItem(source) }
|
val items = tracks.map { it.toMediaItem(source) }
|
||||||
controller.setMediaItems(items, initialIndex, /* startPositionMs = */ 0L)
|
controller.setMediaItems(items, initialIndex, /* startPositionMs = */ 0L)
|
||||||
controller.prepare()
|
controller.prepare()
|
||||||
controller.play()
|
if (autoplay) controller.play()
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -282,6 +316,11 @@ class PlayerController @Inject constructor(
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
mediaController = controller
|
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)
|
startPositionPolling(controller)
|
||||||
controller.addListener(
|
controller.addListener(
|
||||||
object : Player.Listener {
|
object : Player.Listener {
|
||||||
|
|||||||
@@ -54,12 +54,22 @@ class ResumeController @Inject constructor(
|
|||||||
.getOrNull()
|
.getOrNull()
|
||||||
?.takeIf { it.tracks.isNotEmpty() }
|
?.takeIf { it.tracks.isNotEmpty() }
|
||||||
?: return
|
?: 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(
|
playerController.setQueue(
|
||||||
tracks = payload.tracks,
|
tracks = payload.tracks,
|
||||||
initialIndex = payload.queueIndex
|
initialIndex = payload.queueIndex
|
||||||
.coerceAtLeast(0)
|
.coerceAtLeast(0)
|
||||||
.coerceAtMost(payload.tracks.lastIndex),
|
.coerceAtMost(payload.tracks.lastIndex),
|
||||||
source = payload.source,
|
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
|
// PositionMs not seeked yet — Player connection may still be
|
||||||
// establishing; the seek call would no-op. Acceptable to start
|
// establishing; the seek call would no-op. Acceptable to start
|
||||||
|
|||||||
Reference in New Issue
Block a user