Merge pull request #656 from d4rken-org/worktree-fix-unknown-notification

Fix: Ongoing notification stuck showing "Unknown"

app/src/main/java/eu/darken/capod/main/ui/overview/OverviewViewModel.kt

@@ -26,7 +26,7 @@ import eu.darken.capod.monitor.core.PodDevice
 import eu.darken.capod.monitor.core.battery.BatteryEstimate
 import eu.darken.capod.monitor.core.battery.BatteryEstimator
 import eu.darken.capod.monitor.core.battery.estimateFor
-import eu.darken.capod.monitor.core.tierRank
+import eu.darken.capod.monitor.core.podDeviceTierComparator
 import eu.darken.capod.monitor.core.worker.MonitorControl
 import eu.darken.capod.pods.core.apple.aap.AapConnectionManager
 import eu.darken.capod.pods.core.apple.aap.protocol.AapCommand
@@ -256,10 +256,7 @@ class OverviewViewModel @Inject constructor(
         }
 
         val profiledDevices: List<PodDevice> by lazy {
-            devices.filter { it.profileId != null }.sortedWith(
-                compareBy<PodDevice> { it.tierRank() }
-                    .thenBy { profileOrder[it.profileId] ?: Int.MAX_VALUE }
-            )
+            devices.filter { it.profileId != null }.sortedWith(podDeviceTierComparator(profileOrder))
         }
 
         /**

app/src/main/java/eu/darken/capod/monitor/core/PodDevice.kt

@@ -65,7 +65,22 @@ data class PodDevice(
     /** True when the profile's BR/EDR address is in the system's connected Bluetooth devices. */
     val isSystemConnected: Boolean = false,
 ) {
-    val model: PodModel get() = ble?.model ?: profileModel ?: cached?.model ?: PodModel.UNKNOWN
+    /**
+     * A profile's model is never null — it defaults to [PodModel.UNKNOWN] — so a profile that never
+     * learned a model would otherwise mask a perfectly good model sitting in the cache, and
+     * everything downstream ([hasCase], [hasDualPods], [hasEarDetection], the notification layout,
+     * the widget) would degrade to the unknown-device shape for the device's whole lifetime.
+     *
+     * Only the profile step skips UNKNOWN. A live snapshot reporting UNKNOWN is a real decoder
+     * result for an unrecognised model, not a missing answer, and must stay authoritative — its
+     * label says "Unknown device", so overriding it with a stale profile/cache model would pair a
+     * known-model layout with an unknown-model label.
+     */
+    val model: PodModel
+        get() = ble?.model
+            ?: profileModel?.takeUnless { it == PodModel.UNKNOWN }
+            ?: cached?.model
+            ?: PodModel.UNKNOWN
     /** Bonded BR/EDR address (from profile). Used for AAP commands. */
     val address: BluetoothAddress? get() = ble?.meta?.profile?.address ?: profileAddress ?: cached?.address
     /**

app/src/main/java/eu/darken/capod/monitor/core/PodDeviceTier.kt

@@ -20,12 +20,19 @@ fun PodDevice.tierRank(): Int = when {
 }
 
 /**
- * Picks the user-perceived primary profiled device: lowest [tierRank], with
- * the user's profile-list order as the tiebreaker.
+ * Sort order for the user-perceived primary profiled device: lowest [tierRank] first, then the
+ * user's profile-list order, which `profiles_priority_hint` promises is authoritative.
+ *
+ * Shared by [primaryByTier] and the dashboard's device list so the notification, the popup, the
+ * quick-settings tile and the dashboard card can never disagree about which device is primary.
+ */
+fun podDeviceTierComparator(profileOrder: Map<String, Int>): Comparator<PodDevice> =
+    compareBy<PodDevice> { it.tierRank() }
+        .thenBy { profileOrder[it.profileId] ?: Int.MAX_VALUE }
+
+/**
+ * Picks the user-perceived primary profiled device, per [podDeviceTierComparator].
  */
 fun List<PodDevice>.primaryByTier(profileOrder: Map<String, Int>): PodDevice? =
     filter { it.profileId != null }
-        .minWithOrNull(
-            compareBy<PodDevice> { it.tierRank() }
-                .thenBy { profileOrder[it.profileId] ?: Int.MAX_VALUE }
-        )
+        .minWithOrNull(podDeviceTierComparator(profileOrder))

app/src/main/java/eu/darken/capod/monitor/core/worker/MonitorService.kt

@@ -98,6 +98,7 @@ class MonitorService : Service() {
     @Volatile private var monitorGeneration = 0
     private var foregroundStartFailed = false
     private var injectionComplete = false
+    @Volatile private var destroyed = false
 
     @Volatile private var lastNotification: Notification? = null
 
@@ -137,6 +138,10 @@ class MonitorService : Service() {
      * re-satisfy the foreground obligation with the notification the user is currently seeing.
      */
     internal fun postPrimaryNotification(notification: Notification) {
+        if (destroyed) {
+            log(TAG, VERBOSE) { "Skipping notification post, service is being destroyed." }
+            return
+        }
         lastNotification = notification
         notificationManager.notify(MonitorNotifications.NOTIFICATION_ID, notification)
     }
@@ -179,7 +184,18 @@ class MonitorService : Service() {
     override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int {
         // Every startForegroundService() re-arms the startForeground() obligation,
         // so every onStartCommand has to satisfy it again, no matter how it exits.
-        if (!promoteToForeground(lastNotification ?: MonitorNotifications.createEarlyNotification(this))) {
+        //
+        // Only reuse the cached notification while a monitor session is actually live. It is a fully
+        // built object whose `when` is frozen at build time, so re-promoting it to open a NEW session
+        // would re-post the previous session's last content under its original timestamp — which is
+        // how a stale "unknown device" frame survives a teardown/restart cycle. Rebuilding is not an
+        // option here: this has to stay cheap enough to satisfy the obligation before DI is ready.
+        //
+        // Deliberately does NOT clear the cache here: a collector on Dispatchers.Default can post
+        // between the read below and any write, so clearing would discard a frame that is actually
+        // current. Invalidation happens where a new session is launched instead.
+        val reusable = lastNotification?.takeIf { monitoringJob?.isActive == true }
+        if (!promoteToForeground(reusable ?: MonitorNotifications.createEarlyNotification(this))) {
             stopSelf(startId)
             return START_NOT_STICKY
         }
@@ -201,6 +217,10 @@ class MonitorService : Service() {
 
         val generation = ++monitorGeneration
         monitorScope.coroutineContext.cancelChildren()
+        // A new session starts here, so the previous session's last frame must not be re-promoted
+        // into it. Safe at this point: the old collectors are cancelled and the new ones haven't run,
+        // so there is no current notification to discard.
+        lastNotification = null
 
         monitoringJob = monitorScope.launch {
             try {
@@ -412,6 +432,10 @@ class MonitorService : Service() {
 
     override fun onDestroy() {
         log(TAG, VERBOSE) { "onDestroy()" }
+        // Set before cancelling: cancellation doesn't await the collectors, so one already past its
+        // suspension point can still reach postPrimaryNotification() and re-post what we just took
+        // down. The flag makes that post a no-op instead.
+        destroyed = true
         monitorScope.cancel("Service destroyed")
 
         if (injectionComplete) {
@@ -428,6 +452,17 @@ class MonitorService : Service() {
                     log(TAG, WARN) { "Failed to cancel connected notification: ${e.message}" }
                 }
             }
+            // Defence in depth: AOSP reaps the FGS notification on destroy by itself (verified on
+            // API 36), but reports of a stuck ongoing notification on Samsung suggest that is not
+            // universal. Retract it explicitly. cancel() alone is not the foreground-service
+            // lifecycle operation and can be ignored while the notification is still bound, so
+            // detach first. Both calls are idempotent.
+            try {
+                stopForeground(Service.STOP_FOREGROUND_REMOVE)
+                notificationManager.cancel(MonitorNotifications.NOTIFICATION_ID)
+            } catch (e: Exception) {
+                log(TAG, WARN) { "Failed to cancel monitor notification: ${e.message}" }
+            }
         } else {
             log(TAG, WARN) { "onDestroy: Skipping notification cleanup, injection was incomplete." }
         }

app/src/test/java/eu/darken/capod/monitor/core/PodDeviceTest.kt

@@ -1,5 +1,6 @@
 package eu.darken.capod.monitor.core
 
+import eu.darken.capod.monitor.core.cache.CachedDeviceState
 import eu.darken.capod.pods.core.apple.ble.BlePodSnapshot
 import eu.darken.capod.pods.core.apple.ble.devices.HasCase
 import eu.darken.capod.pods.core.apple.ble.devices.HasChargeDetectionDual
@@ -98,6 +99,57 @@ class PodDeviceTest : BaseTest() {
         device.model shouldBe PodModel.UNKNOWN
     }
 
+    /**
+     * A profile's model defaults to UNKNOWN and is never null, so treating it as an answer would
+     * let a profile that never learned a model shadow the cache for the whole device's lifetime.
+     */
+    @Test
+    fun `UNKNOWN profile model falls through to the cache`() {
+        val device = PodDevice(
+            profileId = "a",
+            ble = null,
+            aap = null,
+            profileModel = PodModel.UNKNOWN,
+            cached = cachedState(model = PodModel.AIRPODS_PRO2_USBC),
+        )
+        device.model shouldBe PodModel.AIRPODS_PRO2_USBC
+    }
+
+    @Test
+    fun `known profile model wins over the cache`() {
+        val device = PodDevice(
+            profileId = "a",
+            ble = null,
+            aap = null,
+            profileModel = PodModel.AIRPODS_PRO3,
+            cached = cachedState(model = PodModel.AIRPODS_PRO2_USBC),
+        )
+        device.model shouldBe PodModel.AIRPODS_PRO3
+    }
+
+    /**
+     * An UNKNOWN from a live snapshot is a real decoder verdict on an unrecognised model, not a
+     * missing answer — overriding it would pair a known-model layout with "Unknown device" as the
+     * label, since getLabel() keeps preferring the BLE snapshot.
+     */
+    @Test
+    fun `live UNKNOWN model is not overridden by profile or cache`() {
+        val device = PodDevice(
+            profileId = "a",
+            ble = mockk(relaxed = true) { every { model } returns PodModel.UNKNOWN },
+            aap = null,
+            profileModel = PodModel.AIRPODS_PRO3,
+            cached = cachedState(model = PodModel.AIRPODS_PRO2_USBC),
+        )
+        device.model shouldBe PodModel.UNKNOWN
+    }
+
+    private fun cachedState(model: PodModel) = CachedDeviceState(
+        profileId = "a",
+        model = model,
+        lastSeenAt = Instant.EPOCH,
+    )
+
     @Test
     fun `identifier delegates to BLE`() {
         val id = BlePodSnapshot.Id()

app/src/test/java/eu/darken/capod/monitor/core/PodDeviceTierTest.kt

@@ -12,6 +12,7 @@ class PodDeviceTierTest : BaseTest() {
         profileId: String?,
         isSystemConnected: Boolean = false,
         isLive: Boolean = false,
+        profileModel: PodModel = PodModel.AIRPODS_PRO,
     ): PodDevice {
         // isLive is derived from ble != null || aap != null. Use a minimal AapPodState so we
         // don't need to fabricate a BLE snapshot for the live case.
@@ -20,7 +21,7 @@ class PodDeviceTierTest : BaseTest() {
             profileId = profileId,
             ble = null,
             aap = aap,
-            profileModel = PodModel.AIRPODS_PRO,
+            profileModel = profileModel,
             isSystemConnected = isSystemConnected,
         )
     }
@@ -81,4 +82,16 @@ class PodDeviceTierTest : BaseTest() {
         val devices = listOf(withoutOrder, withOrder)
         devices.primaryByTier(profileOrder = mapOf("a" to 0)) shouldBe withOrder
     }
+
+    /**
+     * `profiles_priority_hint` promises profile order is authoritative once tier is equal — no
+     * model- or data-based reordering may creep in ahead of it.
+     */
+    @Test
+    fun `primaryByTier keeps profile order for a device with no known model`() {
+        val blankButFirst = device(profileId = "a", profileModel = PodModel.UNKNOWN)
+        val known = device(profileId = "b")
+        val devices = listOf(known, blankButFirst)
+        devices.primaryByTier(profileOrder = mapOf("a" to 0, "b" to 1)) shouldBe blankButFirst
+    }
 }

app/src/test/java/eu/darken/capod/monitor/core/worker/MonitorServiceTest.kt

@@ -6,9 +6,12 @@ import android.app.NotificationManager
 import android.app.Service
 import androidx.core.app.NotificationCompat
 import eu.darken.capod.monitor.ui.MonitorNotifications
+import io.kotest.matchers.nulls.shouldBeNull
 import io.kotest.matchers.shouldBe
 import io.kotest.matchers.types.shouldBeSameInstanceAs
 import io.kotest.matchers.types.shouldNotBeSameInstanceAs
+import io.mockk.mockk
+import io.mockk.verify
 import kotlinx.coroutines.Job
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -36,6 +39,9 @@ class MonitorServiceTest {
         MonitorService::class.java.getDeclaredField(name).apply { isAccessible = true }.set(this, value)
     }
 
+    private fun MonitorService.getField(name: String): Any? =
+        MonitorService::class.java.getDeclaredField(name).apply { isAccessible = true }.get(this)
+
     private fun notification(title: String): Notification =
         NotificationCompat.Builder(context, MonitorNotifications.NOTIFICATION_CHANNEL_ID)
             .setContentTitle(title)
@@ -118,4 +124,95 @@ class MonitorServiceTest {
         shadowOf(service).lastForegroundNotification shouldBeSameInstanceAs dynamic
         shadowOf(service).lastForegroundNotificationId shouldBe MonitorNotifications.NOTIFICATION_ID
     }
+
+    /**
+     * A built Notification's `when` is frozen, so re-promoting the previous session's last frame to
+     * open a new one re-posts stale content under its original timestamp — the way an "unknown
+     * device" placeholder survives a teardown/restart cycle.
+     */
+    @Test
+    fun `a start with no live monitor does not reuse the previous notification`() {
+        val service = createService()
+        service.readyForMonitoring()
+
+        val stale = notification("stale")
+        service.postPrimaryNotification(stale)
+
+        // Monitor session has ended. Production never nulls monitoringJob, it leaves the completed
+        // job in place, so a finished Job is the faithful state here.
+        service.setField("monitoringJob", Job().apply { complete() })
+
+        service.onStartCommand(MonitorService.intent(context), 0, 1) shouldBe Service.START_STICKY
+
+        shadowOf(service).lastForegroundNotification shouldNotBeSameInstanceAs stale
+        service.getField("lastNotification") shouldNotBeSameInstanceAs stale
+    }
+
+    /**
+     * Invalidation is deliberately at session launch, not next to the promote: a collector on
+     * Dispatchers.Default can post between the read and a write there, so clearing at promote time
+     * could discard a frame that is genuinely current.
+     */
+    @Test
+    fun `launching a new session drops the previous session's notification`() {
+        val service = createService()
+        service.readyForMonitoring()
+        service.postPrimaryNotification(notification("previous"))
+
+        // forceStart bypasses the "already monitoring" early return, so a new session is launched.
+        service.onStartCommand(MonitorService.intent(context, forceStart = true), 0, 1) shouldBe
+            Service.START_STICKY
+
+        service.getField("lastNotification").shouldBeNull()
+    }
+
+    /**
+     * The FGS notification can outlive `stopSelf()`. Leaving it up strands whatever content was last
+     * posted — including the unknown-device placeholder built before the first BLE scan batch landed.
+     *
+     * Asserted as an interaction, not as shadow end-state: Robolectric's `ShadowService.onDestroy()`
+     * tears the foreground notification down by itself, so an end-state check passes either way.
+     */
+    @Test
+    fun `onDestroy retracts the monitor notification`() {
+        val service = createService()
+        service.readyForMonitoring()
+        val manager = mockk<NotificationManager>(relaxed = true)
+        service.notificationManager = manager
+
+        service.onDestroy()
+
+        verify { manager.cancel(MonitorNotifications.NOTIFICATION_ID) }
+    }
+
+    /**
+     * Scope cancellation doesn't await the collectors, so one already past its suspension point can
+     * still post — re-creating the very notification onDestroy just took down.
+     */
+    @Test
+    fun `a post that lands after onDestroy is dropped`() {
+        val service = createService()
+        service.readyForMonitoring()
+        service.onDestroy()
+
+        val manager = mockk<NotificationManager>(relaxed = true)
+        service.notificationManager = manager
+
+        service.postPrimaryNotification(notification("late"))
+
+        verify(exactly = 0) { manager.notify(any<Int>(), any()) }
+        service.getField("lastNotification").shouldBeNull()
+    }
+
+    @Test
+    fun `onDestroy skips notification cleanup when injection never completed`() {
+        val service = createService()
+        service.setField("injectionComplete", false)
+        val manager = mockk<NotificationManager>(relaxed = true)
+        service.notificationManager = manager
+
+        service.onDestroy()
+
+        verify(exactly = 0) { manager.cancel(any<Int>()) }
+    }
 }