From dabca3ad24d561754c6d0dd725eb319e8cc437de Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 7 Jun 2026 10:54:14 -0400 Subject: [PATCH] fix(android): stop single SOAP failure from dropping Sonos to local When the phone was locked, a transport command (lock-screen/Bluetooth/ watch media button, or queue-resync play) issued during a WiFi power-save stall would hit the 2s connect timeout and throw. handleSoapFailure then cancelled the poll loop and fired onDrop on that single failure -- showing "Disconnected from " and reverting to local playback, even though the renderer was perfectly reachable. This bypassed the poll loop's deliberate 30-consecutive-failure tolerance (DROP_THRESHOLD, bumped from 3 precisely for screen-off WiFi sleep / Doze). Make the 1 Hz poll loop the sole drop arbiter: - Transport SOAP commands retry transient IO failures (retryTransientIo: 3 attempts, 400ms backoff) so a brief WiFi stall lands the command once WiFi wakes instead of abandoning it. A SoapFaultException (renderer answered, rejected the action) is not retried -- the device is alive. - handleTransportFailure no longer cancels the poll loop or fires onDrop; it logs and nudges 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 via DROP_THRESHOLD. Extract retryTransientIo as an internal top-level fn + unit test covering first-success, retry-then-succeed, exhaust-and-rethrow, and no-retry-on- SoapFault. Refresh now-stale drop-heuristic comments. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../player/MinstrelForwardingPlayer.kt | 112 ++++++++++++++---- .../minstrel/player/RemotePlayerState.kt | 8 +- .../player/output/OutputPickerController.kt | 9 +- .../minstrel/player/RetryTransientIoTest.kt | 65 ++++++++++ 4 files changed, 165 insertions(+), 29 deletions(-) create mode 100644 android/app/src/test/java/com/fabledsword/minstrel/player/RetryTransientIoTest.kt 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) + } +}