From 448c9f2e742a4e7c27a99d3cf30ead421bc599e3 Mon Sep 17 00:00:00 2001
From: Bryan Van Deusen <bryan.vandeusen@fabledsword.com>
Date: Wed, 3 Jun 2026 12:56:45 -0400
Subject: [PATCH] chore(android): UPnP picker - log selectUpnp failures + drop
 dead fetchJob

Code-quality review flagged two non-blockers on commit 03cdff54:

1. selectUpnp's runCatching swallowed SOAP / token-mint failures
   silently - OkHttp's logger doesn't see them since they happen in
   our own deserialize / parse code. Adds Timber.w on the failure
   path so operator's on-device Sonos verification can find the
   cause in logcat instead of staring at "nothing happened".

2. UpnpDiscoveryController's fetchJob field was assigned but never
   read or cancelled. appScope is process-lifetime so the launched
   coroutine dies with the process - no explicit cancellation is
   needed. Drop the field + the now-unused Job import.
---
 .../minstrel/player/output/OutputPickerController.kt  | 11 +++++++----
 .../player/output/upnp/UpnpDiscoveryController.kt     |  8 ++++----
 2 files changed, 11 insertions(+), 8 deletions(-)

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 206b0c97..76ee9b9b 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
@@ -19,6 +19,7 @@ import kotlinx.coroutines.flow.stateIn
 import kotlinx.coroutines.launch
 import retrofit2.Retrofit
 import retrofit2.create
+import timber.log.Timber
 import javax.inject.Inject
 import javax.inject.Singleton
 
@@ -171,10 +172,10 @@ class OutputPickerController @Inject constructor(
      * track, set the renderer's URI, play, then pause local playback so
      * audio yields to the speaker. Wrapped in `runCatching` at each
      * step — token failure, transport-lookup failure, and SOAP failure
-     * each abandon the selection cleanly rather than crashing. Errors
-     * surface to the operator via OkHttp logging for now; spec'd UI
-     * error surfacing lands in a follow-up if hardware reveals it's
-     * needed.
+     * each abandon the selection cleanly rather than crashing. Failures
+     * log at warn level via Timber so on-device verification can find
+     * the cause in logcat (OkHttp's logger doesn't cover our own
+     * deserialize / SOAP-parse code paths).
      */
     private suspend fun selectUpnp(route: OutputRoute) {
         val trackId = playerController.uiState.value.currentTrack?.id ?: return
@@ -184,6 +185,8 @@ class OutputPickerController @Inject constructor(
             transport.setAVTransportURI(token.url)
             transport.play()
             playerController.pause()
+        }.onFailure { e ->
+            Timber.w(e, "UPnP select failed for route ${route.id}")
         }
     }
 
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 485a9301..ad0a8ebb 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
@@ -5,7 +5,6 @@ import com.fabledsword.minstrel.di.ApplicationScope
 import dagger.hilt.android.qualifiers.ApplicationContext
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
-import kotlinx.coroutines.Job
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.flow.asStateFlow
@@ -45,11 +44,12 @@ class UpnpDiscoveryController @Inject constructor(
     private val routesInternal = MutableStateFlow<List<UpnpRoute>>(emptyList())
     val routes: StateFlow<List<UpnpRoute>> = routesInternal.asStateFlow()
 
-    private var fetchJob: Job? = null
-
     init {
         ssdp.start(appScope)
-        fetchJob = appScope.launch(Dispatchers.IO) {
+        // appScope is process-lifetime (SupervisorJob + Dispatchers.Default),
+        // so the launched collector dies with the process — no explicit
+        // cancellation needed.
+        appScope.launch(Dispatchers.IO) {
             ssdp.discoveries.collect { locationUrl -> handleDiscovery(locationUrl) }
         }
     }