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 14737b7e..053fb8df 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 @@ -12,12 +12,15 @@ import androidx.media3.common.MediaItem import androidx.media3.common.Player import com.fabledsword.minstrel.player.output.ActiveUpnp 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 kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.cancel +import kotlinx.coroutines.delay import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.isActive @@ -39,9 +42,15 @@ import timber.log.Timber * wrapped ExoPlayer stays paused at the position it had when the * route was selected. * - * Drop heuristic: 3 consecutive poll failures fire [onDrop]. The - * factory wraps that callback into a SharedFlow consumed by the - * NowPlaying surface as a snackbar. + * Drop heuristic: the 1 Hz poll loop is the *sole* arbiter of route + * liveness -- [RemotePlayerState.recordPollFailure]'s rolling threshold + * (DROP_THRESHOLD consecutive failures) fires [onDrop]. A failed transport + * command (play/pause/seek/next) does NOT drop on its own: a locked phone's + * WiFi power-save can stall a single command's socket I/O for a second or + * two while the renderer is perfectly reachable, so commands retry on + * transient IO failure and otherwise defer to the poll loop. The factory + * wraps the [onDrop] callback into a SharedFlow consumed by the NowPlaying + * surface as a snackbar. * * Queue mode: OutputPickerController loads the full queue into Sonos's * native queue via ClearQueue + AddURIToQueue, then points the @@ -174,12 +183,12 @@ class MinstrelForwardingPlayer( } else { remoteState.setPlayIntent(true) scope.launch { - runCatching { active.avTransport.play() } + runCatching { retryTransport { active.avTransport.play() } } .onSuccess { remoteState.applyTransportPlaying() notifyRemoteStateChanged() } - .onFailure { handleSoapFailure(active, it) } + .onFailure { handleTransportFailure(active, it) } } } } @@ -196,12 +205,12 @@ class MinstrelForwardingPlayer( } else { remoteState.setPlayIntent(false) scope.launch { - runCatching { active.avTransport.pause() } + runCatching { retryTransport { active.avTransport.pause() } } .onSuccess { remoteState.applyTransportPaused() notifyRemoteStateChanged() } - .onFailure { handleSoapFailure(active, it) } + .onFailure { handleTransportFailure(active, it) } } } } @@ -224,8 +233,8 @@ class MinstrelForwardingPlayer( trackNumber = remoteState.trackNumber, ) scope.launch { - runCatching { active.avTransport.seek(positionMs) } - .onFailure { handleSoapFailure(active, it) } + runCatching { retryTransport { active.avTransport.seek(positionMs) } } + .onFailure { handleTransportFailure(active, it) } } } } @@ -255,11 +264,13 @@ class MinstrelForwardingPlayer( ) scope.launch { runCatching { - active.avTransport.seekToTrack(mediaItemIndex + 1) - if (positionMs > 0L) { - active.avTransport.seek(positionMs) + retryTransport { + active.avTransport.seekToTrack(mediaItemIndex + 1) + if (positionMs > 0L) { + active.avTransport.seek(positionMs) + } } - }.onFailure { handleSoapFailure(active, it) } + }.onFailure { handleTransportFailure(active, it) } } } @@ -282,8 +293,8 @@ class MinstrelForwardingPlayer( SystemClock.elapsedRealtime() + PENDING_TRANSPORT_SAFETY_TIMEOUT_MS, ) scope.launch { - runCatching { active.avTransport.next() } - .onFailure { handleSoapFailure(active, it) } + runCatching { retryTransport { active.avTransport.next() } } + .onFailure { handleTransportFailure(active, it) } } } @@ -305,8 +316,8 @@ class MinstrelForwardingPlayer( SystemClock.elapsedRealtime() + PENDING_TRANSPORT_SAFETY_TIMEOUT_MS, ) scope.launch { - runCatching { active.avTransport.previous() } - .onFailure { handleSoapFailure(active, it) } + runCatching { retryTransport { active.avTransport.previous() } } + .onFailure { handleTransportFailure(active, it) } } } @@ -387,11 +398,34 @@ class MinstrelForwardingPlayer( super.release() } - private fun handleSoapFailure(active: ActiveUpnp, t: Throwable) { - pollJob?.cancel() - Timber.w(t, "UPnP transport call failed on %s", active.routeName) - remoteState.applyError(t) - handler.post { onDrop(active.routeName) } + /** + * Run a transport SOAP block, retrying transport-level (IO) failures a + * few times with backoff. A locked phone's WiFi power-save can stall the + * first socket I/O for a second or two; a retry lets the command land once + * WiFi wakes instead of being abandoned. A [SoapFaultException] (the + * renderer answered and rejected the action) is NOT retried -- the device + * is alive and retrying won't change its verdict. + */ + private suspend fun retryTransport(block: suspend () -> T): T = + retryTransientIo(TRANSPORT_RETRY_ATTEMPTS, TRANSPORT_RETRY_BACKOFF_MS, block) + + /** + * A transport SOAP command failed even after retries. Deliberately does + * NOT declare the route dropped: the 1 Hz poll loop is the single arbiter + * of liveness (DROP_THRESHOLD consecutive poll failures). A locked phone's + * WiFi power-save can fail one command while the renderer is perfectly + * reachable; dropping on a single command falsely kicked playback back to + * the phone. We log, leave the poll loop running, and nudge an immediate + * poll so the UI reconciles to Sonos's actual state -- if the renderer is + * truly gone, the poll loop trips the drop on its own. + */ + private fun handleTransportFailure(active: ActiveUpnp, t: Throwable) { + if (t is SoapFaultException) { + Timber.w(t, "UPnP transport rejected by %s -- device alive, no drop", active.routeName) + } else { + Timber.w(t, "UPnP transport failed on %s -- poll loop arbitrates", active.routeName) + } + pollTrigger.trySend(Unit) } private fun onActiveChanged(active: ActiveUpnp?) { @@ -515,5 +549,39 @@ class MinstrelForwardingPlayer( // when Sonos's reported Track matches the wrapped player; this only // kicks in if SOAP fails or Sonos drops the ack entirely. const val PENDING_TRANSPORT_SAFETY_TIMEOUT_MS = 5_000L + // Transport commands retry transient IO failures so a single WiFi + // power-save stall (locked phone) doesn't abandon the command. 3 + // attempts x the SoapClient's 2s connect timeout + backoff bounds the + // worst case at ~7s; a still-failing command then defers to the poll. + const val TRANSPORT_RETRY_ATTEMPTS = 3 + const val TRANSPORT_RETRY_BACKOFF_MS = 400L + } +} + +/** + * Retry [block] on transient transport-level ([IOException]) failures, with + * [backoffMs] between attempts, up to [attempts] total. Anything that is not + * an [IOException] -- notably [SoapFaultException], where the renderer + * answered and rejected the action -- propagates immediately: the device is + * alive, so retrying won't change its verdict. Extracted from + * [MinstrelForwardingPlayer] so a locked phone's WiFi power-save stall doesn't + * abandon a single transport command (the bug that falsely reverted Sonos + * playback to local audio). + */ +internal suspend fun retryTransientIo( + attempts: Int, + backoffMs: Long, + block: suspend () -> T, +): T { + var attempt = 0 + while (true) { + try { + return block() + } catch (io: IOException) { + attempt += 1 + if (attempt >= attempts) throw io + Timber.w(io, "transient transport failure (attempt %d) -- retrying", attempt) + delay(backoffMs) + } } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/RemotePlayerState.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/RemotePlayerState.kt index bbaab6db..9e5b2e93 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/RemotePlayerState.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/RemotePlayerState.kt @@ -11,11 +11,13 @@ import javax.inject.Singleton * Updates flow in from: * - 1Hz GetPositionInfo poll -> applyPositionInfo() * - Transport SOAP calls landing 200 OK -> applyTransport{Playing,Paused,Stopped}() - * - Error paths -> applyError() (drop fallback) or recordPollFailure() + * - Poll failures -> recordPollFailure() * - * The poll-failure counter implements the rolling-3 drop heuristic: 3 + * The poll-failure counter is the sole drop arbiter: [DROP_THRESHOLD] * consecutive poll failures = remote considered dropped (returns true - * from recordPollFailure for the caller to surface). Success resets it. + * from recordPollFailure for the caller to surface). Any success resets it. + * Failed transport commands no longer drop the route -- they retry and + * otherwise defer to this counter (see MinstrelForwardingPlayer). */ @Singleton class RemotePlayerState @Inject constructor() { 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 bef9838b..8d690954 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 @@ -162,7 +162,8 @@ class OutputPickerController @Inject constructor( // CALLBACK_FLAG_PASSIVE_DISCOVERY constant; absent flag = passive.) mediaRouter.addCallback(selector, callback) // When MinstrelForwardingPlayer reports the active UPnP route has - // dropped (3+ consecutive poll failures or a transport SOAP exception), + // dropped (the poll loop's consecutive-failure threshold tripping -- + // the sole drop arbiter; a failed transport command no longer drops), // clear the UPnP selection state and fall back to local ExoPlayer at // the last-known remote position. This mirrors selectSystem's disconnect // path but skips the Stop SOAP since the device is already unreachable. @@ -420,9 +421,9 @@ class OutputPickerController @Inject constructor( } /** - * Called when the active UPnP route drops unexpectedly (poll-failure - * threshold or SOAP exception). Captures the last remote position + - * play state, clears UPnP selection, and resumes local ExoPlayer at + * Called when the active UPnP route drops unexpectedly (the poll loop's + * consecutive-failure threshold tripping). Captures the last remote + * position + play state, clears UPnP selection, and resumes local ExoPlayer at * the same point. Skips the Stop SOAP (device already unreachable). * The snackbar is handled independently by the NowPlaying surface * collecting the same [PlayerFactory.dropEvents] via PlayerController. diff --git a/android/app/src/test/java/com/fabledsword/minstrel/player/RetryTransientIoTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/player/RetryTransientIoTest.kt new file mode 100644 index 00000000..77e2194a --- /dev/null +++ b/android/app/src/test/java/com/fabledsword/minstrel/player/RetryTransientIoTest.kt @@ -0,0 +1,65 @@ +package com.fabledsword.minstrel.player + +import com.fabledsword.minstrel.player.output.upnp.SoapFaultException +import kotlinx.coroutines.test.runTest +import org.junit.jupiter.api.Test +import java.io.IOException +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith + +/** + * Locks in the transport-command retry semantics behind the locked-phone + * Sonos drop fix: a transient IO stall (WiFi power-save) must be retried, a + * SOAP fault from a responding renderer must NOT be retried, and an + * exhausted retry must rethrow rather than loop forever. Without this, a + * single failed command falsely reverted Sonos playback to the phone. + */ +class RetryTransientIoTest { + + @Test + fun `returns immediately on first success`() = runTest { + var calls = 0 + val result = retryTransientIo(attempts = 3, backoffMs = 0L) { + calls += 1 + "ok" + } + assertEquals("ok", result) + assertEquals(1, calls) + } + + @Test + fun `retries transient IO failure then succeeds`() = runTest { + var calls = 0 + val result = retryTransientIo(attempts = 3, backoffMs = 0L) { + calls += 1 + if (calls < 2) throw IOException("connect timeout") + "ok" + } + assertEquals("ok", result) + assertEquals(2, calls) + } + + @Test + fun `rethrows IO failure after exhausting attempts`() = runTest { + var calls = 0 + assertFailsWith { + retryTransientIo(attempts = 3, backoffMs = 0L) { + calls += 1 + throw IOException("still asleep") + } + } + assertEquals(3, calls) + } + + @Test + fun `does not retry a SOAP fault from a responding renderer`() = runTest { + var calls = 0 + assertFailsWith { + retryTransientIo(attempts = 3, backoffMs = 0L) { + calls += 1 + throw SoapFaultException("718", "Invalid InstanceID") + } + } + assertEquals(1, calls) + } +}