Merge pull request 'fix(android): UPnP cast resilience — drop-suppression, session adopt, recovery hardening' (#98) from dev into main

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

@@ -10,6 +10,8 @@ import androidx.lifecycle.ProcessLifecycleOwner
 import androidx.media3.common.ForwardingPlayer
 import androidx.media3.common.MediaItem
 import androidx.media3.common.Player
+import com.fabledsword.minstrel.connectivity.NetworkStatusController
+import com.fabledsword.minstrel.connectivity.ServerHealth
 import com.fabledsword.minstrel.player.output.ActiveUpnp
 import com.fabledsword.minstrel.player.output.ActiveUpnpHolder
 import com.fabledsword.minstrel.player.output.upnp.SoapFaultException
@@ -64,6 +66,7 @@ class MinstrelForwardingPlayer(
     private val holder: ActiveUpnpHolder,
     private val remoteState: RemotePlayerState,
     private val castNetworkLock: CastNetworkLock,
+    private val networkStatus: NetworkStatusController,
     private val onDrop: (routeName: String) -> Unit,
 ) : ForwardingPlayer(delegate) {
 
@@ -117,6 +120,20 @@ class MinstrelForwardingPlayer(
         scope.launch {
             holder.active.collect { active -> onActiveChanged(active) }
         }
+        scope.launch {
+            // When the phone's own network recovers, reconcile a held cast
+            // session immediately instead of waiting up to POLL_INTERVAL_MS:
+            // the renderer kept playing, so one round-trip re-confirms it and
+            // resyncs the cursor/position. Only a transition *into* Healthy
+            // while a route is active matters.
+            var prev = networkStatus.state.value
+            networkStatus.state.collect { next ->
+                if (next == ServerHealth.Healthy && prev != ServerHealth.Healthy && isRemote()) {
+                    pollTrigger.trySend(Unit)
+                }
+                prev = next
+            }
+        }
         // Process lifecycle is observed on the main thread; ProcessLifecycleOwner's
         // addObserver requires it. The observer just trySend's to the channel.
         handler.post {
@@ -460,14 +477,19 @@ class MinstrelForwardingPlayer(
 
     @OptIn(ExperimentalCoroutinesApi::class) // onTimeout / select.onReceive
     private suspend fun pollLoop(active: ActiveUpnp) {
+        var networkDropSuppressed = false
         while (scope.isActive && holder.active.value?.routeId == active.routeId) {
             val outcome = runCatching { pollOnce(active) }
             if (outcome.isSuccess) {
                 remoteState.recordPollSuccess()
+                networkDropSuppressed = false
             } else if (remoteState.recordPollFailure()) {
-                Timber.w("UPnP drop threshold tripped for %s", active.routeName)
+                if (networkStatus.state.value == ServerHealth.Healthy) {
-                handler.post { onDrop(active.routeName) }
+                    Timber.w("UPnP drop threshold tripped for %s", active.routeName)
-                return
+                    handler.post { onDrop(active.routeName) }
+                    return
+                }
+                networkDropSuppressed = suppressDropForNetwork(active, networkDropSuppressed)
             }
             // Race the normal cadence against any external wake (activity
             // resume). Whichever wins continues to the next pollOnce.
@@ -478,6 +500,30 @@ class MinstrelForwardingPlayer(
         }
     }
 
+    /**
+     * The poll-failure threshold tripped, but the phone's *own* network is down
+     * (state is not [ServerHealth.Healthy]) -- so this is "we can't see the
+     * renderer right now," not "the renderer died." A UPnP renderer streams
+     * autonomously and is almost certainly still playing; meanwhile a local
+     * fallback couldn't reach the server either, so dropping would only blast
+     * local audio out of a pocketed phone while the speaker keeps going. Hold
+     * the route and keep polling -- a successful poll once the network returns
+     * reconciles to the renderer's real state. Clear the failure streak so
+     * recovery re-evaluates the renderer from scratch instead of re-dropping on
+     * the first post-recovery hiccup. Returns the (latched) suppression flag so
+     * the rationale logs once per outage, not every tick.
+     */
+    private fun suppressDropForNetwork(active: ActiveUpnp, alreadySuppressed: Boolean): Boolean {
+        if (!alreadySuppressed) {
+            Timber.w(
+                "UPnP drop suppressed for %s -- phone network %s, holding route",
+                active.routeName, networkStatus.state.value,
+            )
+        }
+        remoteState.recordPollSuccess() // clear streak; not a renderer failure
+        return true
+    }
+
     /**
      * One poll tick: read position + transport state from Sonos, apply to
      * [remoteState], and forward-sync the local cursor to Sonos's Track

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

@@ -192,6 +192,20 @@ class PlayerController @Inject constructor(
         controller.play()
     }
 
+    /**
+     * Align the local cursor to [index] + [positionMs] WITHOUT auto-playing.
+     * Used by the UPnP adopt path: when attaching to a renderer that's already
+     * playing our queue, we sync the local cursor to the renderer's current
+     * track/position so skip/seek and the UI map onto its live session — the
+     * renderer keeps the audio, the local player stays paused. Unlike
+     * [seekToIndex] this issues no play().
+     */
+    fun moveCursorTo(index: Int, positionMs: Long) {
+        val controller = mediaController ?: return
+        if (index !in queueRefs.indices) return
+        runOnControllerThread(controller) { controller.seekTo(index, positionMs) }
+    }
+
     /**
      * Replace the queue with [tracks] starting at [initialIndex].
      * [source] tags the queue with its origin (e.g. "for_you") so the

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

@@ -82,6 +82,7 @@ class PlayerFactory @Inject constructor(
             holder = activeUpnpHolder,
             remoteState = remoteState,
             castNetworkLock = CastNetworkLock(context),
+            networkStatus = serverHealth,
             onDrop = { name -> emitDrop(name) },
         )
     }

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

@@ -15,6 +15,7 @@ import com.fabledsword.minstrel.player.output.upnp.AVTransportClient
 import com.fabledsword.minstrel.player.output.upnp.RenderingControlClient
 import com.fabledsword.minstrel.player.output.upnp.SoapClient
 import com.fabledsword.minstrel.player.output.upnp.SoapFaultException
+import com.fabledsword.minstrel.player.output.upnp.TransportState
 import com.fabledsword.minstrel.player.output.upnp.UpnpDiscoveryController
 import com.fabledsword.minstrel.player.output.upnp.bareUdn
 import dagger.hilt.android.qualifiers.ApplicationContext
@@ -29,6 +30,7 @@ import kotlinx.coroutines.flow.stateIn
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
+import okhttp3.HttpUrl.Companion.toHttpUrlOrNull
 import okhttp3.OkHttpClient
 import timber.log.Timber
 import javax.inject.Inject
@@ -172,6 +174,31 @@ class OutputPickerController @Inject constructor(
         }
         scope.launch { observeQueueChangesForSonosResync() }
         scope.launch { observeIdleRevertWhileUpnp() }
+        scope.launch { observeSelectedRouteDisappearance() }
+    }
+
+    /**
+     * Revert the UPnP selection to the phone when the selected renderer leaves
+     * discovery (powered off / off the LAN) while we are NOT actively casting
+     * to it. Without this the selection could pin to a ghost route, so a later
+     * "play" would target a speaker that's gone. An *active* cast is left alone
+     * -- its own poll loop arbitrates liveness and won't drop on a mere
+     * discovery gap. Routes leave discovery via [UpnpDiscoveryController]'s
+     * post-picker-scan prune; this reacts to the resulting routes change.
+     */
+    private suspend fun observeSelectedRouteDisappearance() {
+        upnpDiscovery.routes.collect { routes ->
+            val selected = selectedUpnpRouteIdInternal.value
+            val present = routes.any { it.id == selected }
+            val casting = activeUpnpHolder.active.value?.routeId == selected
+            if (selected != null && !present && !casting) {
+                Timber.w(
+                    "Output: selected UPnP route %s left discovery; reverting to phone",
+                    selected,
+                )
+                selectedUpnpRouteIdInternal.value = null
+            }
+        }
     }
 
     /**
@@ -541,6 +568,13 @@ class OutputPickerController @Inject constructor(
         // local audio while we queue up Sonos.
         playerController.pause()
         selectedUpnpRouteIdInternal.value = effectiveRoute.id
+        // If the renderer is already playing this exact queue (a "stray"
+        // session we got disconnected from), attach to it in place instead of
+        // wiping + reloading -- no jarring restart, and skip/seek land on its
+        // live queue. Falls through to the clear+reload path when it isn't ours.
+        if (tryAdoptRunningSession(transport, rendering, effectiveRoute, uiState.queue)) {
+            return@withLock
+        }
         // Mark UPnP loading. ForwardingPlayer overrides drop transport commands
         // silently while target is set but active is null -- the user's premature
         // taps don't hit Sonos's stale state from a prior session.
@@ -567,6 +601,102 @@ class OutputPickerController @Inject constructor(
         }
     }
 
+    /**
+     * The renderer's current position + transport state, validated as an
+     * adoptable continuation of our local queue: same track id at the same
+     * 1-based index. Null means "not ours / not playing" -> caller reloads.
+     */
+    private data class AdoptPlan(
+        val localIndex: Int,
+        val positionMs: Long,
+        val durationMs: Long,
+        val trackUri: String,
+        val trackNumber: Int,
+        val playing: Boolean,
+    )
+
+    /**
+     * Attach to a renderer that is *already* playing our queue (a session the
+     * phone got disconnected from but the speaker kept streaming) instead of
+     * clearing + reloading it. Probes the renderer's live state, and if it's
+     * mid-playback on the same track id at the same queue index, syncs the
+     * local cursor to match and wires [ActiveUpnpHolder] -- the speaker never
+     * skips a beat, and skip/seek now drive its live queue. Returns false (and
+     * the caller does the full clear+reload) when the renderer isn't playing
+     * our queue. The local player is already paused by the caller.
+     */
+    private suspend fun tryAdoptRunningSession(
+        transport: AVTransportClient,
+        rendering: RenderingControlClient?,
+        route: OutputRoute,
+        localQueue: List<TrackRef>,
+    ): Boolean {
+        val plan = probeAdoptable(transport, route, localQueue) ?: return false
+        Timber.w(
+            "UPnP adopt: attaching to live session on %s at track %d (%dms)",
+            route.name, plan.trackNumber, plan.positionMs,
+        )
+        // active is still null here, so this aligns the *local* cursor without
+        // emitting SOAP; the holder.set below then starts the poll loop, which
+        // reconciles to the renderer's advancing position from here.
+        playerController.moveCursorTo(plan.localIndex, plan.positionMs)
+        remoteState.applyPositionInfo(
+            positionMs = plan.positionMs,
+            durationMs = plan.durationMs,
+            trackUri = plan.trackUri,
+            trackNumber = plan.trackNumber,
+        )
+        remoteState.setPlayIntent(plan.playing)
+        activeUpnpHolder.set(
+            ActiveUpnp(
+                routeId = route.id,
+                routeName = route.name,
+                avTransport = transport,
+                rendering = rendering,
+            ),
+        )
+        return true
+    }
+
+    private suspend fun probeAdoptable(
+        transport: AVTransportClient,
+        route: OutputRoute,
+        localQueue: List<TrackRef>,
+    ): AdoptPlan? {
+        val probe = runCatching {
+            transport.getTransportInfo() to transport.getPositionInfo()
+        }.getOrElse {
+            Timber.w(it, "UPnP adopt: probe failed for %s; will reload", route.name)
+            return null
+        }
+        val (info, pos) = probe
+        val idx0 = pos.track - 1
+        val playingId = trackIdFromStreamUrl(pos.trackUri)
+        val midPlayback =
+            info.state == TransportState.PLAYING || info.state == TransportState.PAUSED
+        val aligned = midPlayback && playingId != null &&
+            idx0 in localQueue.indices && localQueue[idx0].id == playingId
+        return if (aligned) {
+            AdoptPlan(
+                localIndex = idx0,
+                positionMs = pos.relTimeMs,
+                durationMs = pos.trackDurationMs,
+                trackUri = pos.trackUri,
+                trackNumber = pos.track,
+                playing = info.state == TransportState.PLAYING,
+            )
+        } else {
+            null
+        }
+    }
+
+    /** Extract our track id from a stream URL `.../api/tracks/<id>/stream...`. */
+    private fun trackIdFromStreamUrl(url: String): String? {
+        val segments = url.toHttpUrlOrNull()?.pathSegments ?: return null
+        val i = segments.indexOf("tracks")
+        return if (i >= 0) segments.getOrNull(i + 1) else null
+    }
+
     private suspend fun loadQueueOnSonos(
         transport: AVTransportClient,
         route: OutputRoute,

android/app/src/main/java/com/fabledsword/minstrel/player/output/upnp/UpnpDiscoveryController.kt

@@ -8,6 +8,7 @@ import com.fabledsword.minstrel.player.output.upnp.sonos.ZoneGroupTopologyClient
 import dagger.hilt.android.qualifiers.ApplicationContext
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.delay
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.flow.asStateFlow
@@ -17,6 +18,7 @@ import okhttp3.HttpUrl.Companion.toHttpUrlOrNull
 import okhttp3.OkHttpClient
 import okhttp3.Request
 import timber.log.Timber
+import java.util.concurrent.ConcurrentHashMap
 import java.util.concurrent.TimeUnit
 import javax.inject.Inject
 import javax.inject.Singleton
@@ -58,6 +60,12 @@ class UpnpDiscoveryController @Inject constructor(
     private val routesInternal = MutableStateFlow<List<UpnpRoute>>(emptyList())
     val routes: StateFlow<List<UpnpRoute>> = routesInternal.asStateFlow()
 
+    // Wall-clock of the last NOTIFY / M-SEARCH response per route id. Drives
+    // post-active-scan pruning so a powered-off renderer leaves the picker
+    // (and can't pin a stale selection) instead of lingering for the process
+    // lifetime -- upsert only ever added before.
+    private val lastSeenMs = ConcurrentHashMap<String, Long>()
+
     private val sonosTopologyInternal = MutableStateFlow<List<SonosZoneGroup>>(emptyList())
     val sonosTopology: StateFlow<List<SonosZoneGroup>> = sonosTopologyInternal.asStateFlow()
 
@@ -78,7 +86,31 @@ class UpnpDiscoveryController @Inject constructor(
      * that just opened).
      */
     fun upgradeDiscovery() {
-        appScope.launch { ssdp.requestActiveScan() }
+        appScope.launch {
+            val scanStartMs = System.currentTimeMillis()
+            ssdp.requestActiveScan()
+            // Give present devices time to answer the M-SEARCH (+ our follow-up
+            // description fetch), then drop any route that didn't re-announce.
+            // Pruning is tied to picker-open scans only -- no background timer,
+            // so route rows never flicker while the user isn't looking.
+            delay(ACTIVE_SCAN_PRUNE_DELAY_MS)
+            pruneRoutesNotSeenSince(scanStartMs)
+        }
+    }
+
+    /**
+     * Drop routes whose last NOTIFY / M-SEARCH response predates [cutoffMs]
+     * -- i.e. they didn't answer the active scan that just ran, so they're
+     * gone from the LAN. Called only after [upgradeDiscovery]'s scan window.
+     */
+    private fun pruneRoutesNotSeenSince(cutoffMs: Long) {
+        val before = routesInternal.value
+        val survivors = before.filter { (lastSeenMs[it.id] ?: 0L) >= cutoffMs }
+        if (survivors.size == before.size) return
+        val removed = before.filterNot { it in survivors }.map { it.id }
+        removed.forEach { lastSeenMs.remove(it) }
+        Timber.w("UPnP discovery: pruned %d stale route(s): %s", removed.size, removed)
+        routesInternal.value = survivors
     }
 
     /**
@@ -115,6 +147,7 @@ class UpnpDiscoveryController @Inject constructor(
     }
 
     private fun upsertRoute(route: UpnpRoute) {
+        lastSeenMs[route.id] = System.currentTimeMillis()
         val current = routesInternal.value
         val idx = current.indexOfFirst { it.id == route.id }
         routesInternal.value = if (idx < 0) {
@@ -240,6 +273,11 @@ class UpnpDiscoveryController @Inject constructor(
         // trips the drop-recovery quickly. Conservative enough not to false-drop
         // a slow-but-alive LAN; read timeout still inherits the shared client.
         const val CONTROL_CONNECT_TIMEOUT_SECONDS = 2L
+
+        // Grace window after an M-SEARCH burst before pruning non-responders.
+        // Covers the SSDP round-trip plus our per-device description fetch;
+        // generous enough that a present-but-slow renderer isn't false-pruned.
+        const val ACTIVE_SCAN_PRUNE_DELAY_MS = 4_000L
     }
 }