fix(player): identity-locked UPnP cursor + single index writer — #171

The local ExoPlayer cursor and the Sonos renderer were two competing
sources of truth for "what's playing" during a cast. The delegate cursor
lagged (forward-only, index-based, size-capped sync, skipped during every
load/re-cast window), and TWO writers of PlayerUiState.queueIndex fought:
PlayerController.onEvents (reading the lagging cursor) stomped the
Sonos-derived index the position tick published, so the in-app player
flickered to the pre-cast track and the notification metadata went stale.

Step 1 — MinstrelForwardingPlayer.syncLocalCursorToRemote (replaces
maybeSyncLocalCursor): align the paused delegate cursor to the track the
renderer is actually playing, matched by track-id parsed from the Sonos
TrackURI (/api/tracks/{id}/stream) against delegate MediaItem.mediaId
(== TrackRef.id). Both directions; survives queue-reload index wobble;
nearest-occurrence tiebreak for duplicate tracks; falls back to the Sonos
Track index; suppressed during load and while a user transport is pending
Sonos's ack. The cursor is now the single authoritative "current track"
that both the in-app UI (onEvents) and the notification (getCurrentMediaItem)
read.

Step 2 — PlayerController: the position tick now patches only
position/duration/play-pause/buffer; onEvents is the sole writer of
queueIndex/currentTrack. Removed desiredQueueIndex, the forward-only
trackChanged path, and publishTickIfChanged. One writer, no stomp.

Part of milestone #171 (unify local + UPnP behind one cursor). Fixes the
flicker + stale-notification symptoms; supersedes #1211/#608/#612.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt

@@ -17,6 +17,7 @@ import com.fabledsword.minstrel.player.output.ActiveUpnpHolder
 import com.fabledsword.minstrel.player.output.upnp.SoapFaultException
 import com.fabledsword.minstrel.player.output.upnp.TransportState
 import java.io.IOException
+import kotlin.math.abs
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.Job
@@ -58,8 +59,10 @@ import timber.log.Timber
  * native queue via ClearQueue + AddURIToQueue, then points the
  * transport at x-rincon-queue:<udn>#0. Skip/prev/seekTo delegate to
  * AVTransport Next/Previous/SeekToTrack so Sonos manages gap-free
- * advance natively. PollLoop syncs the local cursor by comparing the
- * 1-based Track index from GetPositionInfo.
+ * advance natively. PollLoop keeps the local cursor aligned to the
+ * track the renderer is actually playing, matched by track identity
+ * (the id in GetPositionInfo's TrackURI) so it survives queue-reload
+ * index drift -- see [syncLocalCursorToRemote].
  */
 class MinstrelForwardingPlayer(
     private val delegate: Player,
@@ -526,18 +529,8 @@ class MinstrelForwardingPlayer(
 
     /**
      * One poll tick: read position + transport state from Sonos, apply to
-     * [remoteState], and forward-sync the local cursor to Sonos's Track
-     * index when not in queue load.
-     *
-     * Cursor sync is gated on `holder.target == null` (= not loading)
-     * because during load Sonos reports Track=1 while we're still
-     * appending, and syncing would race the SetAV+Seek that lands
-     * after. Outside load, forward sync catches Sonos auto-advances
-     * (queue end-of-track), Sonos-app driven Next presses, and any
-     * drift after a brief poll-failure burst that didn't trip the
-     * drop threshold. Forward-only because a Next override we just
-     * issued can race with a poll still reporting the prior Track --
-     * the next poll catches up safely.
+     * [remoteState], and sync the local cursor to the track the renderer is
+     * actually playing (see [syncLocalCursorToRemote]).
      */
     private suspend fun pollOnce(active: ActiveUpnp) {
         val info = active.avTransport.getPositionInfo()
@@ -553,7 +546,7 @@ class MinstrelForwardingPlayer(
             trackUri = info.trackUri,
             trackNumber = info.track,
         )
-        maybeSyncLocalCursor(info.track)
+        syncLocalCursorToRemote(sonosTrack = info.track, trackUri = info.trackUri)
         val transport = active.avTransport.getTransportInfo()
         when (transport.state) {
             TransportState.PLAYING -> {
@@ -577,23 +570,95 @@ class MinstrelForwardingPlayer(
         notifyRemoteStateChanged()
     }
 
-    private fun maybeSyncLocalCursor(sonosTrack: Int) {
-        if (holder.target.value != null) return
-        if (sonosTrack <= 0) return
-        val sonosIdx = sonosTrack - 1
+    /**
+     * Align the paused local delegate cursor to the track the renderer is
+     * actually playing, so the un-overridden current-item getters
+     * (getCurrentMediaItem/Index -- what both the in-app player UI via
+     * PlayerController.onEvents AND the MediaSession notification read) always
+     * resolve to that track. This is THE single source of truth for "what's
+     * playing" during a cast.
+     *
+     * Identity-first: match the track-id embedded in the renderer's current URI
+     * against the delegate's MediaItem.mediaId (PlayerController sets mediaId =
+     * TrackRef.id), so the index-offset wobble across queue reloads / re-casts
+     * can never park the cursor on a stale (pre-cast) track. Falls back to the
+     * 1-based Sonos Track index only when the URI can't be resolved to a queue
+     * item. Unlike the old forward-only sync this moves the cursor in BOTH
+     * directions -- a renderer Previous / re-cast to a lower track pulls it back
+     * too. Runs on the application looper (delegate access contract).
+     *
+     * Suppressed while:
+     *  - loading ([isLoadingUpnp]) -- the native Sonos queue is still being
+     *    (re)uploaded, so its Track index is meaningless; and
+     *  - a user transport (next/prev/seekTo idx) is pending Sonos's ack --
+     *    the override already advanced the delegate optimistically, and Sonos
+     *    still reports the OLD track for a beat, so syncing now would undo the
+     *    user's press. The pending deadline is honoured directly so a missed
+     *    clear can't wedge the sync.
+     */
+    private fun syncLocalCursorToRemote(sonosTrack: Int, trackUri: String) {
+        if (isLoadingUpnp()) return
+        val pendingDeadline = remoteState.pendingTransportDeadlineMs
+        if (pendingDeadline > 0L && SystemClock.elapsedRealtime() < pendingDeadline) return
         handler.post {
-            val localIdx = delegate.currentMediaItemIndex
-            if (sonosIdx > localIdx && sonosIdx < delegate.mediaItemCount) {
+            val target = resolveRemoteCursorIndex(sonosTrack, trackUri) ?: return@post
+            if (target != delegate.currentMediaItemIndex) {
                 Timber.w(
-                    "UPnP cursor catch-up: local=%d -> sonos=%d",
-                    localIdx, sonosIdx,
+                    "UPnP cursor sync: local=%d -> %d",
+                    delegate.currentMediaItemIndex, target,
                 )
-                delegate.seekTo(sonosIdx, 0L)
+                delegate.seekTo(target, 0L)
             }
         }
     }
 
+    /**
+     * Most-authoritative -> most-degraded (project rule #48): the track-id
+     * parsed from the renderer's current URI wins; else the 1-based Sonos Track
+     * index; else null (leave the cursor put rather than surface a wrong track).
+     * Must run on the application looper -- reads the delegate timeline.
+     */
+    private fun resolveRemoteCursorIndex(sonosTrack: Int, trackUri: String): Int? {
+        val byId = trackIdFromStreamUri(trackUri)?.let { id ->
+            nearestIndexWithMediaId(id, preferNear = sonosTrack - 1)
+        }
+        return byId ?: (sonosTrack - 1).takeIf { it in 0 until delegate.mediaItemCount }
+    }
+
+    /**
+     * Index of the delegate MediaItem whose mediaId == [mediaId], preferring the
+     * occurrence nearest [preferNear] so a track that appears twice in the queue
+     * doesn't snap the cursor across the queue. null if the id isn't present.
+     */
+    private fun nearestIndexWithMediaId(mediaId: String, preferNear: Int): Int? {
+        var best: Int? = null
+        var bestDist = Int.MAX_VALUE
+        for (i in 0 until delegate.mediaItemCount) {
+            if (delegate.getMediaItemAt(i).mediaId == mediaId) {
+                val dist = abs(i - preferNear)
+                if (dist < bestDist) {
+                    best = i
+                    bestDist = dist
+                }
+            }
+        }
+        return best
+    }
+
+    /** Extract `{id}` from a `.../api/tracks/{id}/stream...` stream URI. */
+    private fun trackIdFromStreamUri(uri: String): String? {
+        val start = uri.indexOf(TRACKS_PATH_MARKER)
+        if (start < 0) return null
+        val idStart = start + TRACKS_PATH_MARKER.length
+        val idEnd = uri.indexOf('/', idStart)
+        return if (idEnd > idStart) uri.substring(idStart, idEnd) else null
+    }
+
     private companion object {
+        // Path segment that precedes the track-id in a stream URI
+        // (…/api/tracks/{id}/stream…) — used to map the renderer's current URI
+        // back to a delegate MediaItem by identity.
+        const val TRACKS_PATH_MARKER = "/api/tracks/"
         const val POLL_INTERVAL_MS = 1_000L
         const val NON_PLAYING_CONFIRM = 2
         const val SEEK_ACK_WINDOW_MS = 2_000L

android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt

@@ -598,18 +598,19 @@ class PlayerController @Inject constructor(
     }
 
     /**
-     * One position-polling tick. Owns track-change detection too: when UPnP
-     * is active and the wrapped ExoPlayer is paused, `delegate.seekTo` from
-     * `maybeSyncLocalCursor` may not fire `onMediaItemTransition`, leaving
-     * uiState.queueIndex stuck on the old track even after Sonos has
-     * advanced. So the tick reads Sonos's reported Track as the source of
-     * truth, rebuilds the index/title fields itself, and force-syncs the
-     * wrapped player as defense in depth.
+     * One position-polling tick. Patches ONLY the smoothly-changing transport
+     * fields (position / duration / play-pause / buffer) via `.copy()`. The
+     * queue index + current track are owned solely by [onEvents], which reads
+     * the authoritative delegate cursor: during a cast,
+     * [MinstrelForwardingPlayer.syncLocalCursorToRemote] keeps that cursor on
+     * the track the renderer is actually playing (matched by identity), so there
+     * is exactly one writer of "what's playing" and a stale tick can never
+     * revert it to the old track. (Previously the tick was a second index writer
+     * that fought onEvents — the flicker-to-stale-track bug.)
      */
     private fun tickPositionPoll(controller: MediaController) {
         val upnpActive = activeUpnpHolder.active.value != null
         resolvePendingTransport(controller, upnpActive)
-        val pendingTransport = upnpActive && remoteState.pendingTransportDeadlineMs > 0L
         val effectiveIsPlaying =
             if (upnpActive) remoteState.isPlaying else controller.isPlaying
         val effectivePosition = if (upnpActive) {
@@ -617,27 +618,29 @@ class PlayerController @Inject constructor(
         } else {
             controller.currentPosition
         }
-        val desiredIdx = desiredQueueIndex(controller, upnpActive)
+        val idx = controller.currentMediaItemIndex
         val current = uiStateInternal.value
         val newPos = effectivePosition.coerceAtLeast(0)
         val newDur = effectiveDuration(
             upnpActive,
             remoteState.durationMs,
             controller.duration,
-            desiredIdx = desiredIdx,
-            controllerIdx = controller.currentMediaItemIndex,
+            desiredIdx = idx,
+            controllerIdx = idx,
         )
         val newBuf = controller.bufferedPosition.coerceAtLeast(0)
-        // Track adjustments are forward-only AND suppressed while a user
-        // transport press is pending Sonos confirmation. Together those keep
-        // either direction of user input from being undone by a stale poll.
-        val trackChanged = !pendingTransport &&
-            desiredIdx > current.queueIndex &&
-            desiredIdx in queueRefs.indices
-        publishTickIfChanged(
-            current, trackChanged, desiredIdx,
-            effectiveIsPlaying, newPos, newDur, newBuf,
-        )
+        val somethingChanged = current.isPlaying != effectiveIsPlaying ||
+            current.positionMs != newPos ||
+            current.durationMs != newDur ||
+            current.bufferedPositionMs != newBuf
+        if (somethingChanged) {
+            uiStateInternal.value = current.copy(
+                isPlaying = effectiveIsPlaying,
+                positionMs = newPos,
+                durationMs = newDur,
+                bufferedPositionMs = newBuf,
+            )
+        }
     }
 
     /**
@@ -654,48 +657,6 @@ class PlayerController @Inject constructor(
         }
     }
 
-    @Suppress("LongParameterList") // assembled at one tick call site; refactor would cost clarity
-    private fun publishTickIfChanged(
-        current: PlayerUiState,
-        trackChanged: Boolean,
-        desiredIdx: Int,
-        effectiveIsPlaying: Boolean,
-        newPos: Long,
-        newDur: Long,
-        newBuf: Long,
-    ) {
-        val somethingChanged = trackChanged ||
-            current.isPlaying != effectiveIsPlaying ||
-            current.positionMs != newPos ||
-            current.durationMs != newDur
-        if (!somethingChanged) return
-        val newTrack = if (trackChanged) queueRefs[desiredIdx] else current.currentTrack
-        val newIdx = if (trackChanged) desiredIdx else current.queueIndex
-        uiStateInternal.value = current.copy(
-            currentTrack = newTrack,
-            queueIndex = newIdx,
-            isPlaying = effectiveIsPlaying,
-            positionMs = newPos,
-            durationMs = newDur,
-            bufferedPositionMs = newBuf,
-        )
-        // Intentionally do NOT call controller.seekTo here. That would route
-        // through MinstrelForwardingPlayer's seekTo override and re-issue
-        // AVTransport.SeekToTrack to Sonos -- which seeks Sonos back to the
-        // start of the same track it's already playing, restarting the song.
-        // The wrapped player's index is kept in sync by maybeSyncLocalCursor's
-        // delegate.seekTo (which bypasses the override). If it lags briefly,
-        // the next pollOnce catches up; the uiState above already reflects
-        // Sonos's truth for the user.
-    }
-
-    private fun desiredQueueIndex(controller: MediaController, upnpActive: Boolean): Int =
-        if (upnpActive) {
-            (remoteState.trackNumber - 1).coerceAtLeast(0)
-        } else {
-            controller.currentMediaItemIndex
-        }
-
     // ── Remote position interpolation state ──────────────────────────────
     // remoteState.positionMs is only refreshed by ForwardingPlayer's 1Hz
     // SOAP poll (and only when the round-trip completes -- screen-off WiFi