Skip to content

Commit b83495b

Browse files
kari-tskari-ts
authored
android: fix stale health warnings (#804)
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 <kari@tailscale.com>
1 parent 85fb41e commit b83495b

3 files changed

Lines changed: 270 additions & 25 deletions

File tree

‎android/build.gradle‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,7 @@ dependencies {
170170
testImplementation 'org.mockito:mockito-core:5.12.0'
171171
testImplementation 'org.mockito:mockito-inline:5.2.0'
172172
testImplementation 'org.mockito.kotlin:mockito-kotlin:5.4.0'
173+
testImplementation 'org.jetbrains.kotlinx:kotlinx-coroutines-test:1.8.1'
173174

174175
debugImplementation("androidx.compose.ui:ui-tooling")
175176
implementation("androidx.compose.ui:ui-tooling-preview")

‎android/src/main/java/com/tailscale/ipn/ui/notifier/HealthNotifier.kt‎

Lines changed: 19 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,14 @@ import com.tailscale.ipn.ui.model.Ipn
1616
import com.tailscale.ipn.ui.util.set
1717
import com.tailscale.ipn.util.TSLog
1818
import kotlinx.coroutines.CoroutineScope
19-
import kotlinx.coroutines.FlowPreview
2019
import kotlinx.coroutines.flow.MutableStateFlow
2120
import kotlinx.coroutines.flow.StateFlow
22-
import kotlinx.coroutines.flow.combine
2321
import kotlinx.coroutines.flow.debounce
2422
import kotlinx.coroutines.flow.distinctUntilChanged
23+
import kotlinx.coroutines.flow.emptyFlow
24+
import kotlinx.coroutines.flow.flatMapLatest
2525
import kotlinx.coroutines.launch
2626

27-
@OptIn(FlowPreview::class)
2827
class HealthNotifier(
2928
healthStateFlow: StateFlow<Health.State?>,
3029
ipnStateFlow: StateFlow<Ipn.State>,
@@ -45,32 +44,27 @@ class HealthNotifier(
4544
"wantrunning-false")
4645

4746
init {
48-
// This roughly matches the iOS/macOS implementation in terms of debouncing, and ingoring
47+
// This roughly matches the iOS/macOS implementation in terms of debouncing, and ignoring
4948
// health warnings in various states.
5049
scope.launch {
51-
healthStateFlow
52-
.distinctUntilChanged { old, new ->
53-
old?.Warnings?.keys.orEmpty() == new?.Warnings?.keys.orEmpty()
54-
}
55-
.debounce(3000)
56-
.combine(ipnStateFlow, ::Pair)
57-
.collect { pair ->
58-
val health = pair.first
59-
// Only deliver health notifications when the client is Running
60-
when (val ipnState = pair.second) {
61-
Ipn.State.Running -> {
62-
TSLog.d(TAG, "Health updated: ${health?.Warnings?.keys?.sorted()}")
63-
health?.Warnings?.let {
64-
notifyHealthUpdated(it.values.mapNotNull { it }.toTypedArray())
65-
}
66-
}
67-
else -> {
68-
TSLog.d(TAG, "Ignoring and dropping all health messages in state ${ipnState}")
69-
dropAllWarnings()
70-
return@collect
71-
}
50+
ipnStateFlow
51+
.flatMapLatest { ipnState ->
52+
if (ipnState == Ipn.State.Running) {
53+
healthStateFlow
54+
.distinctUntilChanged { old, new ->
55+
old?.Warnings.orEmpty() == new?.Warnings.orEmpty()
56+
}
57+
.debounce(3000)
58+
} else {
59+
TSLog.d(TAG, "Ignoring and dropping all health messages in state $ipnState")
60+
dropAllWarnings()
61+
emptyFlow()
7262
}
7363
}
64+
.collect { health ->
65+
TSLog.d(TAG, "Health updated: ${health?.Warnings?.keys?.sorted()}")
66+
health?.Warnings?.values?.filterNotNull()?.toTypedArray()?.let(::notifyHealthUpdated)
67+
}
7468
}
7569
}
7670

‎android/src/test/kotlin/com/tailcale/ipn/ui/notifier/HealthNotifierTest.kt‎

Lines changed: 250 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,250 @@
1+
// Copyright (c) Tailscale Inc & AUTHORS
2+
// SPDX-License-Identifier: BSD-3-Clause
3+
4+
package com.tailscale.ipn.ui.notifier
5+
6+
import androidx.core.app.NotificationManagerCompat
7+
import com.tailscale.ipn.UninitializedApp
8+
import com.tailscale.ipn.ui.model.Health
9+
import com.tailscale.ipn.ui.model.Health.UnhealthyState
10+
import com.tailscale.ipn.ui.model.Ipn
11+
import com.tailscale.ipn.util.TSLog
12+
import com.tailscale.ipn.util.TSLog.LibtailscaleWrapper
13+
import kotlinx.coroutines.ExperimentalCoroutinesApi
14+
import kotlinx.coroutines.flow.MutableStateFlow
15+
import kotlinx.coroutines.test.advanceTimeBy
16+
import kotlinx.coroutines.test.runCurrent
17+
import kotlinx.coroutines.test.runTest
18+
import org.junit.After
19+
import org.junit.Assert.assertEquals
20+
import org.junit.Assert.assertNull
21+
import org.junit.Assert.assertTrue
22+
import org.junit.Before
23+
import org.junit.Test
24+
import org.mockito.ArgumentMatchers.anyString
25+
import org.mockito.Mockito.doNothing
26+
import org.mockito.Mockito.mock
27+
28+
@OptIn(ExperimentalCoroutinesApi::class)
29+
class HealthNotifierTest {
30+
31+
private lateinit var originalWrapper: LibtailscaleWrapper
32+
33+
private fun derpWarning() =
34+
UnhealthyState(
35+
WarnableCode = "no-derp-connection",
36+
Severity = Health.Severity.medium,
37+
Title = "Relay server unavailable",
38+
Text = "Could not connect to relay server.",
39+
ImpactsConnectivity = true,
40+
DependsOn = listOf("network-status", "no-derp-home", "warming-up"),
41+
)
42+
43+
private fun warmingUpWarning() =
44+
UnhealthyState(
45+
WarnableCode = "warming-up",
46+
Severity = Health.Severity.low,
47+
Title = "Starting",
48+
Text = "Tailscale is starting.",
49+
)
50+
51+
private fun healthState(vararg warnings: UnhealthyState): Health.State {
52+
return Health.State(Warnings = warnings.associateBy { it.WarnableCode })
53+
}
54+
55+
private fun emptyHealth(): Health.State = Health.State(Warnings = emptyMap())
56+
57+
@Before
58+
fun setUp() {
59+
val logMock = mock(LibtailscaleWrapper::class.java)
60+
doNothing().`when`(logMock).sendLog(anyString(), anyString())
61+
originalWrapper = TSLog.libtailscaleWrapper
62+
TSLog.libtailscaleWrapper = logMock
63+
64+
UninitializedApp.notificationManager = mock(NotificationManagerCompat::class.java)
65+
}
66+
67+
@After
68+
fun tearDown() {
69+
TSLog.libtailscaleWrapper = originalWrapper
70+
}
71+
72+
private fun kotlinx.coroutines.test.TestScope.settle() {
73+
advanceTimeBy(4000)
74+
runCurrent()
75+
}
76+
77+
private fun kotlinx.coroutines.test.TestScope.createRunningNotifier():
78+
Triple<MutableStateFlow<Health.State?>, MutableStateFlow<Ipn.State>, HealthNotifier> {
79+
val healthFlow = MutableStateFlow<Health.State?>(null)
80+
val ipnFlow = MutableStateFlow(Ipn.State.Running)
81+
val notifier = HealthNotifier(healthFlow, ipnFlow, backgroundScope)
82+
runCurrent()
83+
healthFlow.value = emptyHealth()
84+
settle()
85+
return Triple(healthFlow, ipnFlow, notifier)
86+
}
87+
88+
@Test
89+
fun warningShownWhileRunningClearsAfterDebounce() = runTest {
90+
val (healthFlow, _, notifier) = createRunningNotifier()
91+
92+
healthFlow.value = healthState(derpWarning())
93+
settle()
94+
assertEquals(1, notifier.currentWarnings.value.size)
95+
96+
healthFlow.value = emptyHealth()
97+
settle()
98+
assertTrue(notifier.currentWarnings.value.isEmpty())
99+
}
100+
101+
@Test
102+
fun warningsDroppedWhenStateLeavesRunning() = runTest {
103+
val (healthFlow, ipnFlow, notifier) = createRunningNotifier()
104+
105+
healthFlow.value = healthState(derpWarning())
106+
settle()
107+
assertEquals(1, notifier.currentWarnings.value.size)
108+
109+
ipnFlow.value = Ipn.State.Stopped
110+
runCurrent()
111+
assertTrue(notifier.currentWarnings.value.isEmpty())
112+
}
113+
114+
@Test
115+
fun noWarningsWhileStoppedEvenWithUnhealthyState() = runTest {
116+
val healthFlow = MutableStateFlow<Health.State?>(null)
117+
val ipnFlow = MutableStateFlow(Ipn.State.Stopped)
118+
val notifier = HealthNotifier(healthFlow, ipnFlow, backgroundScope)
119+
runCurrent()
120+
121+
healthFlow.value = healthState(derpWarning())
122+
settle()
123+
assertTrue(notifier.currentWarnings.value.isEmpty())
124+
}
125+
126+
@Test
127+
fun staleWarningFromStoppedDoesNotPersistAfterToggleOn() = runTest {
128+
val (healthFlow, ipnFlow, notifier) = createRunningNotifier()
129+
130+
ipnFlow.value = Ipn.State.Stopped
131+
runCurrent()
132+
healthFlow.value = healthState(derpWarning())
133+
settle()
134+
assertTrue("Should have no warnings while Stopped", notifier.currentWarnings.value.isEmpty())
135+
136+
healthFlow.value = emptyHealth()
137+
settle()
138+
139+
ipnFlow.value = Ipn.State.Running
140+
settle()
141+
assertTrue(
142+
"Stale DERP warning should NOT appear after toggle on",
143+
notifier.currentWarnings.value.isEmpty(),
144+
)
145+
}
146+
147+
@Test
148+
fun derpFlappingWhileStoppedThenToggleOnShowsNoStaleWarning() = runTest {
149+
val (healthFlow, ipnFlow, notifier) = createRunningNotifier()
150+
151+
ipnFlow.value = Ipn.State.Stopped
152+
runCurrent()
153+
154+
for (i in 1..5) {
155+
healthFlow.value = healthState(derpWarning())
156+
advanceTimeBy(1000)
157+
healthFlow.value = emptyHealth()
158+
advanceTimeBy(1000)
159+
}
160+
healthFlow.value = healthState(derpWarning())
161+
settle()
162+
assertTrue("No warnings while Stopped", notifier.currentWarnings.value.isEmpty())
163+
164+
ipnFlow.value = Ipn.State.Running
165+
healthFlow.value = emptyHealth()
166+
settle()
167+
assertTrue(
168+
"Should not show stale DERP warning",
169+
notifier.currentWarnings.value.isEmpty(),
170+
)
171+
}
172+
173+
@Test
174+
fun realDerpFailureAfterToggleOnShowsWarning() = runTest {
175+
val (healthFlow, _, notifier) = createRunningNotifier()
176+
177+
healthFlow.value = healthState(derpWarning())
178+
settle()
179+
assertEquals(
180+
"Should show DERP warning for real failure",
181+
setOf("no-derp-connection"),
182+
notifier.currentWarnings.value.map { it.WarnableCode }.toSet(),
183+
)
184+
}
185+
186+
@Test
187+
fun warmingUpWarningsAreIgnored() = runTest {
188+
val (healthFlow, _, notifier) = createRunningNotifier()
189+
190+
healthFlow.value = healthState(warmingUpWarning(), derpWarning())
191+
settle()
192+
assertTrue(
193+
"DERP warning should be hidden by warming-up dependency",
194+
notifier.currentWarnings.value.none { it.WarnableCode == "no-derp-connection" },
195+
)
196+
}
197+
198+
@Test
199+
fun ignoredWarnableCodesAreFiltered() = runTest {
200+
val (healthFlow, _, notifier) = createRunningNotifier()
201+
202+
val unstableWarning =
203+
UnhealthyState(
204+
WarnableCode = "is-using-unstable-version",
205+
Severity = Health.Severity.low,
206+
Title = "Unstable",
207+
Text = "Using unstable version",
208+
)
209+
healthFlow.value = healthState(unstableWarning)
210+
settle()
211+
assertTrue(notifier.currentWarnings.value.isEmpty())
212+
}
213+
214+
@Test
215+
fun iconSetForConnectivityImpactingWarning() = runTest {
216+
val (healthFlow, _, notifier) = createRunningNotifier()
217+
218+
healthFlow.value = healthState(derpWarning())
219+
settle()
220+
assertTrue("Icon should not be null when warning present", notifier.currentIcon.value != null)
221+
222+
healthFlow.value = emptyHealth()
223+
settle()
224+
assertNull("Icon should be null when no warnings", notifier.currentIcon.value)
225+
}
226+
227+
@Test
228+
fun rapidStateTransitionsDoNotLeakStaleWarnings() = runTest {
229+
val healthFlow = MutableStateFlow<Health.State?>(null)
230+
val ipnFlow = MutableStateFlow(Ipn.State.Stopped)
231+
val notifier = HealthNotifier(healthFlow, ipnFlow, backgroundScope)
232+
runCurrent()
233+
234+
healthFlow.value = healthState(derpWarning())
235+
settle()
236+
237+
ipnFlow.value = Ipn.State.Running
238+
advanceTimeBy(500)
239+
ipnFlow.value = Ipn.State.Stopped
240+
advanceTimeBy(500)
241+
ipnFlow.value = Ipn.State.Running
242+
healthFlow.value = emptyHealth()
243+
settle()
244+
245+
assertTrue(
246+
"No stale warnings after rapid toggling",
247+
notifier.currentWarnings.value.isEmpty(),
248+
)
249+
}
250+
}

0 commit comments

Comments
 (0)
⚡