From 07becfeecbd38f59207673e6914642d491736491 Mon Sep 17 00:00:00 2001 From: kari-ts Date: Thu, 4 Jun 2026 13:18:28 -0700 Subject: [PATCH] android: fix stale health warnings Health warnings were debounced then combined with IPN state, which allowed later state changes to reuse the last debounced health value. This resulted in warnings being shown after they'd been dropped while the client was stopped or logged out. This uses flatMapLatest from IPN state instead so that switching away from 'Running' cancels any pending debounce and siwtches to an empty health stream. Fixes tailscale/corp#42871 Signed-off-by: kari-ts --- android/build.gradle | 1 + .../ipn/ui/notifier/HealthNotifier.kt | 44 ++- .../ipn/ui/notifier/HealthNotifierTest.kt | 250 ++++++++++++++++++ 3 files changed, 270 insertions(+), 25 deletions(-) create mode 100644 android/src/test/kotlin/com/tailcale/ipn/ui/notifier/HealthNotifierTest.kt diff --git a/android/build.gradle b/android/build.gradle index 8f64b3de8e..644324e451 100644 --- a/android/build.gradle +++ b/android/build.gradle @@ -170,6 +170,7 @@ dependencies { testImplementation 'org.mockito:mockito-core:5.12.0' testImplementation 'org.mockito:mockito-inline:5.2.0' testImplementation 'org.mockito.kotlin:mockito-kotlin:5.4.0' + testImplementation 'org.jetbrains.kotlinx:kotlinx-coroutines-test:1.8.1' debugImplementation("androidx.compose.ui:ui-tooling") implementation("androidx.compose.ui:ui-tooling-preview") diff --git a/android/src/main/java/com/tailscale/ipn/ui/notifier/HealthNotifier.kt b/android/src/main/java/com/tailscale/ipn/ui/notifier/HealthNotifier.kt index 1030d217b4..412bff0fa3 100644 --- a/android/src/main/java/com/tailscale/ipn/ui/notifier/HealthNotifier.kt +++ b/android/src/main/java/com/tailscale/ipn/ui/notifier/HealthNotifier.kt @@ -16,15 +16,14 @@ import com.tailscale.ipn.ui.model.Ipn import com.tailscale.ipn.ui.util.set import com.tailscale.ipn.util.TSLog import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.FlowPreview import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow -import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.emptyFlow +import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.launch -@OptIn(FlowPreview::class) class HealthNotifier( healthStateFlow: StateFlow, ipnStateFlow: StateFlow, @@ -45,32 +44,27 @@ class HealthNotifier( "wantrunning-false") init { - // This roughly matches the iOS/macOS implementation in terms of debouncing, and ingoring + // This roughly matches the iOS/macOS implementation in terms of debouncing, and ignoring // health warnings in various states. scope.launch { - healthStateFlow - .distinctUntilChanged { old, new -> - old?.Warnings?.keys.orEmpty() == new?.Warnings?.keys.orEmpty() - } - .debounce(3000) - .combine(ipnStateFlow, ::Pair) - .collect { pair -> - val health = pair.first - // Only deliver health notifications when the client is Running - when (val ipnState = pair.second) { - Ipn.State.Running -> { - TSLog.d(TAG, "Health updated: ${health?.Warnings?.keys?.sorted()}") - health?.Warnings?.let { - notifyHealthUpdated(it.values.mapNotNull { it }.toTypedArray()) - } - } - else -> { - TSLog.d(TAG, "Ignoring and dropping all health messages in state ${ipnState}") - dropAllWarnings() - return@collect - } + ipnStateFlow + .flatMapLatest { ipnState -> + if (ipnState == Ipn.State.Running) { + healthStateFlow + .distinctUntilChanged { old, new -> + old?.Warnings.orEmpty() == new?.Warnings.orEmpty() + } + .debounce(3000) + } else { + TSLog.d(TAG, "Ignoring and dropping all health messages in state $ipnState") + dropAllWarnings() + emptyFlow() } } + .collect { health -> + TSLog.d(TAG, "Health updated: ${health?.Warnings?.keys?.sorted()}") + health?.Warnings?.values?.filterNotNull()?.toTypedArray()?.let(::notifyHealthUpdated) + } } } diff --git a/android/src/test/kotlin/com/tailcale/ipn/ui/notifier/HealthNotifierTest.kt b/android/src/test/kotlin/com/tailcale/ipn/ui/notifier/HealthNotifierTest.kt new file mode 100644 index 0000000000..7211861c73 --- /dev/null +++ b/android/src/test/kotlin/com/tailcale/ipn/ui/notifier/HealthNotifierTest.kt @@ -0,0 +1,250 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package com.tailscale.ipn.ui.notifier + +import androidx.core.app.NotificationManagerCompat +import com.tailscale.ipn.UninitializedApp +import com.tailscale.ipn.ui.model.Health +import com.tailscale.ipn.ui.model.Health.UnhealthyState +import com.tailscale.ipn.ui.model.Ipn +import com.tailscale.ipn.util.TSLog +import com.tailscale.ipn.util.TSLog.LibtailscaleWrapper +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.test.advanceTimeBy +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.mockito.ArgumentMatchers.anyString +import org.mockito.Mockito.doNothing +import org.mockito.Mockito.mock + +@OptIn(ExperimentalCoroutinesApi::class) +class HealthNotifierTest { + + private lateinit var originalWrapper: LibtailscaleWrapper + + private fun derpWarning() = + UnhealthyState( + WarnableCode = "no-derp-connection", + Severity = Health.Severity.medium, + Title = "Relay server unavailable", + Text = "Could not connect to relay server.", + ImpactsConnectivity = true, + DependsOn = listOf("network-status", "no-derp-home", "warming-up"), + ) + + private fun warmingUpWarning() = + UnhealthyState( + WarnableCode = "warming-up", + Severity = Health.Severity.low, + Title = "Starting", + Text = "Tailscale is starting.", + ) + + private fun healthState(vararg warnings: UnhealthyState): Health.State { + return Health.State(Warnings = warnings.associateBy { it.WarnableCode }) + } + + private fun emptyHealth(): Health.State = Health.State(Warnings = emptyMap()) + + @Before + fun setUp() { + val logMock = mock(LibtailscaleWrapper::class.java) + doNothing().`when`(logMock).sendLog(anyString(), anyString()) + originalWrapper = TSLog.libtailscaleWrapper + TSLog.libtailscaleWrapper = logMock + + UninitializedApp.notificationManager = mock(NotificationManagerCompat::class.java) + } + + @After + fun tearDown() { + TSLog.libtailscaleWrapper = originalWrapper + } + + private fun kotlinx.coroutines.test.TestScope.settle() { + advanceTimeBy(4000) + runCurrent() + } + + private fun kotlinx.coroutines.test.TestScope.createRunningNotifier(): + Triple, MutableStateFlow, HealthNotifier> { + val healthFlow = MutableStateFlow(null) + val ipnFlow = MutableStateFlow(Ipn.State.Running) + val notifier = HealthNotifier(healthFlow, ipnFlow, backgroundScope) + runCurrent() + healthFlow.value = emptyHealth() + settle() + return Triple(healthFlow, ipnFlow, notifier) + } + + @Test + fun warningShownWhileRunningClearsAfterDebounce() = runTest { + val (healthFlow, _, notifier) = createRunningNotifier() + + healthFlow.value = healthState(derpWarning()) + settle() + assertEquals(1, notifier.currentWarnings.value.size) + + healthFlow.value = emptyHealth() + settle() + assertTrue(notifier.currentWarnings.value.isEmpty()) + } + + @Test + fun warningsDroppedWhenStateLeavesRunning() = runTest { + val (healthFlow, ipnFlow, notifier) = createRunningNotifier() + + healthFlow.value = healthState(derpWarning()) + settle() + assertEquals(1, notifier.currentWarnings.value.size) + + ipnFlow.value = Ipn.State.Stopped + runCurrent() + assertTrue(notifier.currentWarnings.value.isEmpty()) + } + + @Test + fun noWarningsWhileStoppedEvenWithUnhealthyState() = runTest { + val healthFlow = MutableStateFlow(null) + val ipnFlow = MutableStateFlow(Ipn.State.Stopped) + val notifier = HealthNotifier(healthFlow, ipnFlow, backgroundScope) + runCurrent() + + healthFlow.value = healthState(derpWarning()) + settle() + assertTrue(notifier.currentWarnings.value.isEmpty()) + } + + @Test + fun staleWarningFromStoppedDoesNotPersistAfterToggleOn() = runTest { + val (healthFlow, ipnFlow, notifier) = createRunningNotifier() + + ipnFlow.value = Ipn.State.Stopped + runCurrent() + healthFlow.value = healthState(derpWarning()) + settle() + assertTrue("Should have no warnings while Stopped", notifier.currentWarnings.value.isEmpty()) + + healthFlow.value = emptyHealth() + settle() + + ipnFlow.value = Ipn.State.Running + settle() + assertTrue( + "Stale DERP warning should NOT appear after toggle on", + notifier.currentWarnings.value.isEmpty(), + ) + } + + @Test + fun derpFlappingWhileStoppedThenToggleOnShowsNoStaleWarning() = runTest { + val (healthFlow, ipnFlow, notifier) = createRunningNotifier() + + ipnFlow.value = Ipn.State.Stopped + runCurrent() + + for (i in 1..5) { + healthFlow.value = healthState(derpWarning()) + advanceTimeBy(1000) + healthFlow.value = emptyHealth() + advanceTimeBy(1000) + } + healthFlow.value = healthState(derpWarning()) + settle() + assertTrue("No warnings while Stopped", notifier.currentWarnings.value.isEmpty()) + + ipnFlow.value = Ipn.State.Running + healthFlow.value = emptyHealth() + settle() + assertTrue( + "Should not show stale DERP warning", + notifier.currentWarnings.value.isEmpty(), + ) + } + + @Test + fun realDerpFailureAfterToggleOnShowsWarning() = runTest { + val (healthFlow, _, notifier) = createRunningNotifier() + + healthFlow.value = healthState(derpWarning()) + settle() + assertEquals( + "Should show DERP warning for real failure", + setOf("no-derp-connection"), + notifier.currentWarnings.value.map { it.WarnableCode }.toSet(), + ) + } + + @Test + fun warmingUpWarningsAreIgnored() = runTest { + val (healthFlow, _, notifier) = createRunningNotifier() + + healthFlow.value = healthState(warmingUpWarning(), derpWarning()) + settle() + assertTrue( + "DERP warning should be hidden by warming-up dependency", + notifier.currentWarnings.value.none { it.WarnableCode == "no-derp-connection" }, + ) + } + + @Test + fun ignoredWarnableCodesAreFiltered() = runTest { + val (healthFlow, _, notifier) = createRunningNotifier() + + val unstableWarning = + UnhealthyState( + WarnableCode = "is-using-unstable-version", + Severity = Health.Severity.low, + Title = "Unstable", + Text = "Using unstable version", + ) + healthFlow.value = healthState(unstableWarning) + settle() + assertTrue(notifier.currentWarnings.value.isEmpty()) + } + + @Test + fun iconSetForConnectivityImpactingWarning() = runTest { + val (healthFlow, _, notifier) = createRunningNotifier() + + healthFlow.value = healthState(derpWarning()) + settle() + assertTrue("Icon should not be null when warning present", notifier.currentIcon.value != null) + + healthFlow.value = emptyHealth() + settle() + assertNull("Icon should be null when no warnings", notifier.currentIcon.value) + } + + @Test + fun rapidStateTransitionsDoNotLeakStaleWarnings() = runTest { + val healthFlow = MutableStateFlow(null) + val ipnFlow = MutableStateFlow(Ipn.State.Stopped) + val notifier = HealthNotifier(healthFlow, ipnFlow, backgroundScope) + runCurrent() + + healthFlow.value = healthState(derpWarning()) + settle() + + ipnFlow.value = Ipn.State.Running + advanceTimeBy(500) + ipnFlow.value = Ipn.State.Stopped + advanceTimeBy(500) + ipnFlow.value = Ipn.State.Running + healthFlow.value = emptyHealth() + settle() + + assertTrue( + "No stale warnings after rapid toggling", + notifier.currentWarnings.value.isEmpty(), + ) + } +}