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 <[email protected]>

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()

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

@@ -12,19 +12,22 @@ 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
 
 @OptIn(FlowPreview::class)
 class HealthNotifier(
     healthStateFlow: StateFlow<Health.State?>,
+    ipnStateFlow: StateFlow<Ipn.State>,
     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<UnhealthyState> = 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<UnhealthyState>) {
     TSLog.d(TAG, "Removing notifications for $warnings")
     for (warning in warnings) {