Merge pull request 'v2026.06.07 — Sonos: one SOAP failure no longer drops to local' (#88) from dev into main

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 <T> 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 <T> 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)
+        }
     }
 }

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() {

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.

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<IOException> {
+            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<SoapFaultException> {
+            retryTransientIo(attempts = 3, backoffMs = 0L) {
+                calls += 1
+                throw SoapFaultException("718", "Invalid InstanceID")
+            }
+        }
+        assertEquals(1, calls)
+    }
+}