From 228d7d1f0c58bf7802779ebcf46647441b4ec02b Mon Sep 17 00:00:00 2001 From: darken Date: Wed, 29 Apr 2026 17:11:46 +0200 Subject: [PATCH] fix(reaction): Tighten auto-pause debounce after review findings - Commit pending pause when a trusted source (AAP / BLE_IRK_MATCH) corroborates the not-worn condition mid-debounce, instead of dropping pending silently. - Scope debounceFreshness to not-worn samples only; identical both-in samples no longer pass distinctUntilChangedBy and can't accidentally trigger BLE-only auto-play confirmation. - Add resetTolerance to PendingPauseDebounce so a single corrupt count-up advert no longer kills a legitimate pending pause; reorder reset checks so rawDecision.shouldPlay resets immediately. - Drop bleKeyState from the INFO autoPause log; source already encodes trust without leaking key-configuration state to logcat. - Add flow-level MonitorFlowTests verifying the distinctUntilChangedBy interaction with seenLastAt freshness, plus the #557-direction test (AAP-worn vs corrupt-BLE-not-worn) and rebound-tolerance test. - Clarify in BLE_ANONYMOUS KDoc that the path is unreachable in production via DeviceMonitor.primaryDevice. --- .../reaction/core/playpause/PlayPause.kt | 60 ++++++- .../core/playpause/PlayPauseLogicTest.kt | 154 +++++++++++++++++- 2 files changed, 204 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/eu/darken/capod/reaction/core/playpause/PlayPause.kt b/app/src/main/java/eu/darken/capod/reaction/core/playpause/PlayPause.kt index 4db379c3..09c50524 100644 --- a/app/src/main/java/eu/darken/capod/reaction/core/playpause/PlayPause.kt +++ b/app/src/main/java/eu/darken/capod/reaction/core/playpause/PlayPause.kt @@ -228,7 +228,6 @@ class PlayPause @Inject constructor( "autoPause triggered: source=$source, " + "wasWorn=${prevState.bothInEar}, isWorn=${currState.bothInEar}, " + "podCount=${prevState.podCount}->${currState.podCount}, " + - "bleKeyState=${current.bleKeyState}, " + "aapEar=${current.aap?.aapEarDetection != null}, " + "aapConn=${current.aap?.connectionState}, " + "pauseSent=$pauseSent, reason=${decision.reason}" @@ -427,8 +426,25 @@ class PlayPause @Inject constructor( } // Non-debounce-eligible trusted sources (AAP / BLE_IRK_MATCH) and disabled debounce: - // pass raw decision through and clear any active pending. + // pass raw decision through and clear any active pending. Special case: if a + // pending was active and the trusted source still shows the not-worn condition + // (currentState.podCount <= initialPodCount and no play decision), commit the + // pause now — the trusted source corroborates what BLE-debounce was waiting on. if (!needsDebounce || !autoPauseEnabled || profileId == null) { + if (activePending != null && autoPauseEnabled && + currentState.podCount <= activePending.initialPodCount && + !rawDecision.shouldPlay + ) { + return PauseDebounceResult( + decision = PlayPauseDecision( + shouldPlay = false, + shouldPause = true, + reason = "Pause confirmed by trusted source ($source) after BLE-debounce", + ), + pending = null, + event = PauseDebounceEvent.COMMITTED, + ) + } val event = if (activePending != null) PauseDebounceEvent.RESET else PauseDebounceEvent.NONE return PauseDebounceResult(decision = rawDecision, pending = null, event = event) } @@ -456,8 +472,24 @@ class PlayPause @Inject constructor( } // Confirmation phase: pending exists. - // Reset cases: pod returned (count went up) or raw decision wants to play. - if (currentState.podCount > activePending.initialPodCount || rawDecision.shouldPlay) { + // Reset cases — checked in order of authority: + // 1. Raw decision wants to play → genuine play signal, reset immediately. + // 2. Pod count went up → tolerate one rebound sample (corrupt count-up + // protection), reset only on the second consecutive count-up. + if (rawDecision.shouldPlay) { + return PauseDebounceResult(decision = rawDecision, pending = null, event = PauseDebounceEvent.RESET) + } + if (currentState.podCount > activePending.initialPodCount) { + if (activePending.resetTolerance > 0) { + return PauseDebounceResult( + decision = rawDecision.copy( + shouldPause = false, + reason = "Debouncing pause (rebound tolerated)", + ), + pending = activePending.copy(resetTolerance = activePending.resetTolerance - 1), + event = PauseDebounceEvent.ADVANCED, + ) + } return PauseDebounceResult(decision = rawDecision, pending = null, event = PauseDebounceEvent.RESET) } @@ -563,7 +595,11 @@ class PlayPause @Inject constructor( * identity key. Identity-authenticated; debounce skipped. * - [BLE_PROFILE_FALLBACK]: BLE advertisement assigned to a profile via signal-quality * fallback (no IRK match). Could be a stray advert from a nearby pair. Debounced. - * - [BLE_ANONYMOUS]: BLE advertisement with no profile match. Debounced. + * - [BLE_ANONYMOUS]: BLE advertisement with no profile match. Filtered out by + * [primaryDevice] in production (devices without profileId are dropped before + * reaching the reaction layer); kept here defensively in case the upstream filter + * changes. Note: [PendingPauseDebounce] is profile-keyed, so a null profileId can + * never sustain pending state even if this branch is hit. * - [NO_LIVE_BLE]: No live BLE snapshot at all (cache-only or empty). Not debounced — * the cached state is not "fresh evidence" so it must not advance the debounce counter. * Any active pending is cleared. @@ -574,6 +610,11 @@ class PlayPause @Inject constructor( val profileId: String, val initialPodCount: Int, val confirmationsRemaining: Int, + // Tolerates one count-up rebound sample before the pending is reset. Mirrors the + // count-down debounce on the pause side: a single corrupt advert that briefly + // shows a pod returning shouldn't kill the pending, since the next sample may + // confirm the pods are still out. + val resetTolerance: Int = 1, ) /** Discrete event produced by [applyPauseDebounce] for diagnostic logging. */ @@ -623,8 +664,13 @@ class PlayPause @Inject constructor( internal fun PodDevice.toPlayPauseMonitorKey(): PlayPauseMonitorKey { val source = earDetectionSource() - val needsFreshness = source == EarDetectionSource.BLE_PROFILE_FALLBACK || - source == EarDetectionSource.BLE_ANONYMOUS + // Freshness is needed only on debounce-eligible NOT-WORN samples. Including + // worn samples would also break distinctUntilChangedBy for identical both-in + // samples, accidentally enabling BLE-only auto-play confirmation to fire on + // repeated unauthenticated adverts. + val needsFreshness = (source == EarDetectionSource.BLE_PROFILE_FALLBACK || + source == EarDetectionSource.BLE_ANONYMOUS) && + isBeingWorn != true return PlayPauseMonitorKey( profileId = profileId, autoPlay = reactions.autoPlay, diff --git a/app/src/test/java/eu/darken/capod/reaction/core/playpause/PlayPauseLogicTest.kt b/app/src/test/java/eu/darken/capod/reaction/core/playpause/PlayPauseLogicTest.kt index 0e3beedc..9979eafa 100644 --- a/app/src/test/java/eu/darken/capod/reaction/core/playpause/PlayPauseLogicTest.kt +++ b/app/src/test/java/eu/darken/capod/reaction/core/playpause/PlayPauseLogicTest.kt @@ -1,19 +1,33 @@ package eu.darken.capod.reaction.core.playpause +import eu.darken.capod.common.MediaControl +import eu.darken.capod.common.bluetooth.BluetoothManager2 +import eu.darken.capod.monitor.core.DeviceMonitor import eu.darken.capod.monitor.core.PodDevice +import eu.darken.capod.pods.core.apple.PodModel import eu.darken.capod.pods.core.apple.aap.AapPodState import eu.darken.capod.pods.core.apple.aap.protocol.AapSetting import eu.darken.capod.pods.core.apple.ble.DualBlePodSnapshot +import eu.darken.capod.pods.core.apple.ble.devices.ApplePods import eu.darken.capod.pods.core.apple.ble.devices.DualApplePods +import eu.darken.capod.profiles.core.ReactionConfig import eu.darken.capod.reaction.core.playpause.PlayPause.EarDetectionState import io.kotest.matchers.shouldBe import io.kotest.matchers.shouldNotBe +import io.mockk.coEvery +import io.mockk.coVerify import io.mockk.every import io.mockk.mockk +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Nested import org.junit.jupiter.api.Test import testhelpers.BaseTest +import java.time.Instant class PlayPauseLogicTest : BaseTest() { @@ -1132,14 +1146,43 @@ class PlayPauseLogicTest : BaseTest() { } @Test - fun `BLE_PROFILE_FALLBACK pod returns mid-debounce - pending cleared`() { - // First detection: both pods removed + fun `BLE_PROFILE_FALLBACK first count-up is tolerated as a rebound`() { + // First detection: both pods removed (initialPodCount=0). A single corrupt + // count-up sample (pod=1) should be tolerated — the helper holds pending + // and decrements resetTolerance. val pending = PlayPause.PendingPauseDebounce( profileId = "profile", initialPodCount = 0, confirmationsRemaining = 1, + resetTolerance = 1, + ) + val current = EarDetectionState.fromDualPod(true, false) + + val result = playPause.applyPauseDebounce( + pending = pending, + profileId = "profile", + source = PlayPause.EarDetectionSource.BLE_PROFILE_FALLBACK, + rawDecision = noopDecision, + currentState = current, + autoPauseEnabled = true, + ) + + result.decision.shouldPause shouldBe false + result.pending shouldNotBe null + result.pending!!.resetTolerance shouldBe 0 + result.event shouldBe PlayPause.PauseDebounceEvent.ADVANCED + } + + @Test + fun `BLE_PROFILE_FALLBACK second count-up resets pending (tolerance exhausted)`() { + // After the first rebound was tolerated, resetTolerance=0. A second count-up + // sample resets pending — this is a genuine pod-return signal. + val pending = PlayPause.PendingPauseDebounce( + profileId = "profile", + initialPodCount = 0, + confirmationsRemaining = 1, + resetTolerance = 0, ) - // Pod returned: count went from 0 -> 1 (worn back) val current = EarDetectionState.fromDualPod(true, false) val result = playPause.applyPauseDebounce( @@ -1153,6 +1196,7 @@ class PlayPauseLogicTest : BaseTest() { result.decision.shouldPause shouldBe false result.pending shouldBe null + result.event shouldBe PlayPause.PauseDebounceEvent.RESET } @Test @@ -1326,6 +1370,43 @@ class PlayPauseLogicTest : BaseTest() { state.podCount shouldBe 0 } + @Test + fun `AAP says worn while BLE per-side falsely says not-worn - returns worn (#557 scenario)`() { + // The motivating bug for the per-side gap fix: AAP authoritative state says + // pods worn, but BLE fallback bits say not-worn. AAP must win — this is the + // scenario users hit in #557 when high-RF interference flipped BLE bits. + val ble = mockk(relaxed = true) { + every { isLeftPodInEar } returns false // BLE corrupt: says not-worn + every { isRightPodInEar } returns false + every { isBeingWorn } returns false + every { isEitherPodInEar } returns false + every { primaryPod } returns DualBlePodSnapshot.Pod.LEFT + } + val aapEarDetection = AapSetting.EarDetection( + primaryPod = AapSetting.EarDetection.PodPlacement.IN_EAR, + secondaryPod = AapSetting.EarDetection.PodPlacement.IN_EAR, + ) + val aapPrimary = AapSetting.PrimaryPod(pod = AapSetting.PrimaryPod.Pod.LEFT) + val aap = AapPodState( + connectionState = AapPodState.ConnectionState.READY, + settings = mapOf( + AapSetting.EarDetection::class to aapEarDetection, + AapSetting.PrimaryPod::class to aapPrimary, + ), + ) + val device = PodDevice( + profileId = "test", + ble = ble, + aap = aap, + ) + + val state = with(playPause) { device.toEarDetectionState() } + + // AAP says worn → aggregate must reflect worn even though BLE bits say not-worn. + state.bothInEar shouldBe true + state.podCount shouldBe 2 + } + @Test fun `AAP absent - falls back to BLE per-side`() { val ble = mockk(relaxed = true) { @@ -1450,4 +1531,71 @@ class PlayPauseLogicTest : BaseTest() { state.podCount shouldBe 2 } } + + @Nested + inner class MonitorFlowTests { + + private fun buildBle(seenAt: Instant, leftWorn: Boolean, rightWorn: Boolean) = + mockk(relaxed = true) { + every { meta } returns ApplePods.AppleMeta( + isIRKMatch = false, + profile = mockk(relaxed = true), + ) + every { seenLastAt } returns seenAt + every { isLeftPodInEar } returns leftWorn + every { isRightPodInEar } returns rightWorn + every { isBeingWorn } returns (leftWorn && rightWorn) + every { isEitherPodInEar } returns (leftWorn || rightWorn) + every { primaryPod } returns DualBlePodSnapshot.Pod.LEFT + every { model } returns PodModel.AIRPODS_PRO3 + } + + private fun buildDevice(seenAt: Instant, leftWorn: Boolean, rightWorn: Boolean) = + PodDevice( + profileId = "test-profile", + ble = buildBle(seenAt, leftWorn, rightWorn), + aap = null, + profileModel = PodModel.AIRPODS_PRO3, + reactions = ReactionConfig(autoPlay = true, autoPause = true), + ) + + @Test + fun `flow - 3 consecutive not-worn unauthenticated samples fire pause exactly once`() = runTest { + val deviceFlow = MutableStateFlow>(emptyList()) + val deviceMonitor: DeviceMonitor = mockk(relaxed = true) { + every { devices } returns deviceFlow + } + val bluetoothManager: BluetoothManager2 = mockk(relaxed = true) { + every { connectedDevices } returns flowOf(listOf(mockk(relaxed = true))) + } + val mediaControl: MediaControl = mockk(relaxed = true) { + every { isPlaying } returns true + every { wasRecentlyPausedByCap } returns false + coEvery { sendPause() } returns true + } + val flowPlayPause = PlayPause(deviceMonitor, bluetoothManager, mediaControl) + + val now = Instant.parse("2026-01-01T00:00:00Z") + val job = launch { flowPlayPause.monitor().collect {} } + + // T0: worn baseline + deviceFlow.value = listOf(buildDevice(now, leftWorn = true, rightWorn = true)) + advanceUntilIdle() + + // T1, T2, T3: identical not-worn samples with incrementing seenLastAt. + // Without the freshness fix, distinctUntilChangedBy would collapse T2 and T3 + // and the debounce counter could never advance. With it, the helper sees + // 3 samples and commits the pause on the third. + deviceFlow.value = listOf(buildDevice(now.plusMillis(1000), leftWorn = false, rightWorn = false)) + advanceUntilIdle() + deviceFlow.value = listOf(buildDevice(now.plusMillis(2000), leftWorn = false, rightWorn = false)) + advanceUntilIdle() + deviceFlow.value = listOf(buildDevice(now.plusMillis(3000), leftWorn = false, rightWorn = false)) + advanceUntilIdle() + + coVerify(exactly = 1) { mediaControl.sendPause() } + + job.cancel() + } + } }