From b616696a410a0bc158a51cb9577ba7f8b8254a26 Mon Sep 17 00:00:00 2001 From: darken Date: Wed, 29 Jul 2026 20:09:12 +0200 Subject: [PATCH] fix(monitor): Retract the stale ongoing notification on teardown --- .../main/ui/overview/OverviewViewModel.kt | 7 +-- .../eu/darken/capod/monitor/core/PodDevice.kt | 17 +++++- .../capod/monitor/core/PodDeviceTier.kt | 19 +++++-- .../monitor/core/worker/MonitorService.kt | 18 ++++++ .../capod/monitor/core/PodDeviceTest.kt | 52 +++++++++++++++++ .../capod/monitor/core/PodDeviceTierTest.kt | 15 ++++- .../monitor/core/worker/MonitorServiceTest.kt | 56 +++++++++++++++++++ 7 files changed, 171 insertions(+), 13 deletions(-) diff --git a/app/src/main/java/eu/darken/capod/main/ui/overview/OverviewViewModel.kt b/app/src/main/java/eu/darken/capod/main/ui/overview/OverviewViewModel.kt index d8fd7067..b431b66e 100644 --- a/app/src/main/java/eu/darken/capod/main/ui/overview/OverviewViewModel.kt +++ b/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 by lazy { - devices.filter { it.profileId != null }.sortedWith( - compareBy { it.tierRank() } - .thenBy { profileOrder[it.profileId] ?: Int.MAX_VALUE } - ) + devices.filter { it.profileId != null }.sortedWith(podDeviceTierComparator(profileOrder)) } /** diff --git a/app/src/main/java/eu/darken/capod/monitor/core/PodDevice.kt b/app/src/main/java/eu/darken/capod/monitor/core/PodDevice.kt index 0b763803..df49a04b 100644 --- a/app/src/main/java/eu/darken/capod/monitor/core/PodDevice.kt +++ b/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 /** diff --git a/app/src/main/java/eu/darken/capod/monitor/core/PodDeviceTier.kt b/app/src/main/java/eu/darken/capod/monitor/core/PodDeviceTier.kt index 361414b7..43575ab7 100644 --- a/app/src/main/java/eu/darken/capod/monitor/core/PodDeviceTier.kt +++ b/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): Comparator = + compareBy { it.tierRank() } + .thenBy { profileOrder[it.profileId] ?: Int.MAX_VALUE } + +/** + * Picks the user-perceived primary profiled device, per [podDeviceTierComparator]. */ fun List.primaryByTier(profileOrder: Map): PodDevice? = filter { it.profileId != null } - .minWithOrNull( - compareBy { it.tierRank() } - .thenBy { profileOrder[it.profileId] ?: Int.MAX_VALUE } - ) + .minWithOrNull(podDeviceTierComparator(profileOrder)) diff --git a/app/src/main/java/eu/darken/capod/monitor/core/worker/MonitorService.kt b/app/src/main/java/eu/darken/capod/monitor/core/worker/MonitorService.kt index ea2a8592..b9d10539 100644 --- a/app/src/main/java/eu/darken/capod/monitor/core/worker/MonitorService.kt +++ b/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) } @@ -412,6 +417,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 +437,15 @@ class MonitorService : Service() { log(TAG, WARN) { "Failed to cancel connected notification: ${e.message}" } } } + // The FGS notification can outlive stopSelf(), leaving whatever content was last posted + // stuck in the shade. notificationManager.cancel() alone is not enough while the + // notification is still bound to the foreground service, so detach it first. + 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." } } diff --git a/app/src/test/java/eu/darken/capod/monitor/core/PodDeviceTest.kt b/app/src/test/java/eu/darken/capod/monitor/core/PodDeviceTest.kt index 27b2fb18..0a3322b4 100644 --- a/app/src/test/java/eu/darken/capod/monitor/core/PodDeviceTest.kt +++ b/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() diff --git a/app/src/test/java/eu/darken/capod/monitor/core/PodDeviceTierTest.kt b/app/src/test/java/eu/darken/capod/monitor/core/PodDeviceTierTest.kt index 2d4d1a7b..f160b585 100644 --- a/app/src/test/java/eu/darken/capod/monitor/core/PodDeviceTierTest.kt +++ b/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 + } } diff --git a/app/src/test/java/eu/darken/capod/monitor/core/worker/MonitorServiceTest.kt b/app/src/test/java/eu/darken/capod/monitor/core/worker/MonitorServiceTest.kt index 63cc34a0..1307bdf4 100644 --- a/app/src/test/java/eu/darken/capod/monitor/core/worker/MonitorServiceTest.kt +++ b/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,54 @@ class MonitorServiceTest { shadowOf(service).lastForegroundNotification shouldBeSameInstanceAs dynamic shadowOf(service).lastForegroundNotificationId shouldBe MonitorNotifications.NOTIFICATION_ID } + + /** + * 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(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(relaxed = true) + service.notificationManager = manager + + service.postPrimaryNotification(notification("late")) + + verify(exactly = 0) { manager.notify(any(), 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(relaxed = true) + service.notificationManager = manager + + service.onDestroy() + + verify(exactly = 0) { manager.cancel(any()) } + } }