From fda38205825eff685a2504ce2b080552e5ebc53f Mon Sep 17 00:00:00 2001 From: Andrea Gottardo Date: Tue, 26 Nov 2024 12:18:22 -0800 Subject: [PATCH] HealthNotifier: prevent and drop all warnings in the Stopped state (#575) Updates tailscale/tailscale#12960 When the client is Stopped after running, a false positive DERP warnings was getting presented. This was not happening on Apple platforms because we never leave the client in a Stopped state, the extension instantly terminates. Since that's not the case on Android, this PR ensures that: - we do not present any warnings when the client is Stopped (nothing should be broken when nothing is running) - if we enter the Stopped state, any pre-existing warnings generated while the client was running are dropped Signed-off-by: Andrea Gottardo --- .../src/main/java/com/tailscale/ipn/App.kt | 2 +- .../ipn/ui/notifier/HealthNotifier.kt | 47 ++++++++++++++++--- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/android/src/main/java/com/tailscale/ipn/App.kt b/android/src/main/java/com/tailscale/ipn/App.kt index fde6541fc1..8d8a0762a9 100644 --- a/android/src/main/java/com/tailscale/ipn/App.kt +++ b/android/src/main/java/com/tailscale/ipn/App.kt @@ -158,7 +158,7 @@ class App : UninitializedApp(), libtailscale.AppContext, ViewModelStoreOwner { Request.setApp(app) Notifier.setApp(app) Notifier.start(applicationScope) - healthNotifier = HealthNotifier(Notifier.health, applicationScope) + healthNotifier = HealthNotifier(Notifier.health, Notifier.state, applicationScope) connectivityManager = this.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager NetworkChangeCallback.monitorDnsChanges(connectivityManager, dns) initViewModels() 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 4ebac07e20..8386bcb289 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 @@ -12,12 +12,14 @@ import com.tailscale.ipn.R import com.tailscale.ipn.UninitializedApp.Companion.notificationManager 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.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.launch @@ -25,6 +27,7 @@ import kotlinx.coroutines.launch @OptIn(FlowPreview::class) class HealthNotifier( healthStateFlow: StateFlow, + ipnStateFlow: StateFlow, scope: CoroutineScope, ) { companion object { @@ -45,11 +48,22 @@ class HealthNotifier( scope.launch { healthStateFlow .distinctUntilChanged { old, new -> old?.Warnings?.count() == new?.Warnings?.count() } + .combine(ipnStateFlow, ::Pair) .debounce(5000) - .collect { health -> - TSLog.d(TAG, "Health updated: ${health?.Warnings?.keys?.sorted()}") - health?.Warnings?.let { - notifyHealthUpdated(it.values.mapNotNull { it }.toTypedArray()) + .collect { pair -> + val health = pair.first + val ipnState = pair.second + // When the client is Stopped, no warnings should get added, and any warnings added + // previously should be removed. + if (ipnState == Ipn.State.Stopped) { + TSLog.d(TAG, "Ignoring and dropping all pre-existing health messages in the Stopped state") + dropAllWarnings() + return@collect + } else { + TSLog.d(TAG, "Health updated: ${health?.Warnings?.keys?.sorted()}") + health?.Warnings?.let { + notifyHealthUpdated(it.values.mapNotNull { it }.toTypedArray()) + } } } } @@ -65,8 +79,11 @@ class HealthNotifier( val removedByNewDependency: MutableSet = mutableSetOf() val isWarmingUp = warnings.any { it.WarnableCode == "warming-up" } - /// Checks if there is any warning in `warningsBeforeAdd` that needs to be removed because the new warning `w` - /// is listed as a dependency of a warning already in `warningsBeforeAdd`, and removes it. + /** + * dropDependenciesForAddedWarning checks if there is any warning in `warningsBeforeAdd` that + * needs to be removed because the new warning `w` is listed as a dependency of a warning + * already in `warningsBeforeAdd`, and removes it. + */ fun dropDependenciesForAddedWarning(w: UnhealthyState) { for (warning in warningsBeforeAdd) { warning.DependsOn?.let { @@ -112,6 +129,14 @@ class HealthNotifier( this.updateIcon() } + /** + * Sets the icon displayed to represent the overall health state. + * + * - If there are any high severity warnings, or warnings that affect internet connectivity, + * a warning icon is displayed. + * - If there are any other kind of warnings, an info icon is displayed. + * - If there are no warnings at all, no icon is set. + */ private fun updateIcon() { if (currentWarnings.value.isEmpty()) { this.currentIcon.set(null) @@ -145,6 +170,16 @@ class HealthNotifier( notificationManager.notify(code.hashCode(), notification) } + /** + * Removes all warnings currently displayed, including any system notifications, and + * updates the icon (causing it to be set to null since the set of warnings is empty). + */ + private fun dropAllWarnings() { + removeNotifications(this.currentWarnings.value) + this.currentWarnings.set(emptySet()) + this.updateIcon() + } + private fun removeNotifications(warnings: Set) { TSLog.d(TAG, "Removing notifications for $warnings") for (warning in warnings) {