From d6290a3ef00e4552e4f792abdbe7ec716ad1042b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 12 Jun 2026 20:48:40 -0400 Subject: [PATCH 1/2] fix(android): don't drop Sonos (or blast local audio) when the phone's own network is down Observed on device: casting to Sonos on battery + screen locked, a transient ~67s reachability gap (NetworkStatus -> ServerDown while WiFi itself stayed associated) starved the 1 Hz poll past DROP_THRESHOLD. The poll loop then dropped the route and fell back to the local player, which honored the play-intent -- so the phone suddenly started playing the song out loud locally while the Sonos was still happily streaming it. A poll failure during a phone-side network outage means "we can't see the renderer right now," not "the renderer died": a UPnP renderer streams autonomously and keeps playing, and the local player we'd fall back to can't reach the server either. Dropping is strictly worse than waiting. Gate the drop on NetworkStatusController: only drop when the phone's network is Healthy (renderer genuinely unreachable on an otherwise-fine link). While Unstable/ServerDown/Offline, hold the route, keep polling, and clear the failure streak so recovery re-evaluates from scratch rather than re-dropping on the first post-recovery hiccup. The poll reconciles to the renderer's real (advanced) position once the network returns. Complements the CastNetworkLock fix: the lock reduces how often these gaps happen; this stops a gap that does happen from punishing the user. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../player/MinstrelForwardingPlayer.kt | 38 +++++++++++++++++-- .../minstrel/player/PlayerFactory.kt | 1 + 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt index 52977949..9fe589b5 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt +++ b/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) { @@ -460,14 +463,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) - handler.post { onDrop(active.routeName) } - return + if (networkStatus.state.value == ServerHealth.Healthy) { + Timber.w("UPnP drop threshold tripped for %s", active.routeName) + 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 +486,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 diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerFactory.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerFactory.kt index 47ecfc39..7c3a2cad 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerFactory.kt +++ b/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) }, ) } -- 2.52.0 From fd7d6dac4c4349c2af24b8346f57e8e77a74dc62 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 12 Jun 2026 21:20:26 -0400 Subject: [PATCH 2/2] feat(android): adopt a running UPnP session + tighten cast recovery/anti-stickiness Three related improvements to UPnP/Sonos session handling, on top of the WiFi-lock + drop-suppression fixes. 1. Adopt a running session instead of clear+reload (the headline). Selecting a renderer always did removeAllTracksFromQueue + full reload -- a jarring restart if the speaker was already playing our queue (e.g. after the phone got disconnected but the autonomous Sonos kept going). selectUpnp now probes the renderer first; if it's mid-playback on the same track id at the same queue index, we ATTACH in place: sync the local cursor to its position, wire ActiveUpnpHolder, start polling -- no clear, no reload, and skip/seek immediately drive its live queue. Falls through to clear+reload when it isn't our queue. New PlayerController.moveCursorTo aligns the local cursor without auto-playing. 2. Discovery expiry + selection revert (anti-stickiness). upsertRoute only ever added, so a powered-off renderer lingered in the picker forever and could pin a stale selection. Stamp lastSeen per route; after the picker's active M-SEARCH scan, prune routes that didn't re-announce. A collector reverts the selection to the phone when the selected route leaves discovery while we're not actively casting -- so a later play never targets a ghost. Pruning is tied to picker-open scans only (no background timer -> no row flicker). 3. Reconcile immediately on network recovery. When NetworkStatus flips back to Healthy while a route is active, nudge an immediate poll instead of waiting up to POLL_INTERVAL_MS -- the held session re-confirms the renderer in one round-trip. Verified (read-only): the tap-play-onto-dead-route fallback still fires when the phone's network is Healthy (the poll-loop drop path is unchanged for that case); the drop-suppression gate only holds during phone-side outages, where a local fallback couldn't play either. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../player/MinstrelForwardingPlayer.kt | 14 ++ .../minstrel/player/PlayerController.kt | 14 ++ .../player/output/OutputPickerController.kt | 130 ++++++++++++++++++ .../output/upnp/UpnpDiscoveryController.kt | 40 +++++- 4 files changed, 197 insertions(+), 1 deletion(-) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt index 9fe589b5..49f0cdc5 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt @@ -120,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 { 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 e0f49216..f571f6a9 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 @@ -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 diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt index 8d690954..80d9a6b1 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt +++ b/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, + ): 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, + ): 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//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, diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/output/upnp/UpnpDiscoveryController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/output/upnp/UpnpDiscoveryController.kt index cbf22ddc..8426c875 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/output/upnp/UpnpDiscoveryController.kt +++ b/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>(emptyList()) val routes: StateFlow> = 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() + private val sonosTopologyInternal = MutableStateFlow>(emptyList()) val sonosTopology: StateFlow> = 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 } } -- 2.52.0