fix(connectivity): probe on foreground; a burst can't corroborate ServerDown — #1209

Two changes so a network handoff stops making the app refuse to play music. ## Correction first: half of what I proposed already existed I recommended "require corroboration before ServerDown, since Unstable is non-gating." ReachabilityMachine has done exactly that since it was written — onProbeFailure takes Reachable → Unstable, and escalates only on corroboration or the 120s backstop. There is even a test named `single probe failure is unstable not down`. I proposed building a thing that shipped months ago. Reading the machine properly turned up the real gap, which is narrower and more specific. ## 1. Probe when the app returns to the foreground The genuine missing piece, and #1209's own note had it backwards: it listed this as "already happens via link probe." It doesn't. `recheck()` had exactly two callers — a button in VersionTooOldBanner and pull-to-refresh — and nothing observed ProcessLifecycleOwner. The link probe fires on a connectivity *change*, so an app backgrounded on stable Wi-Fi gets none. That made a stale ServerDown outlive its cause: the poll loop's delay() is throttled while screen-off/doze, so recovery waited for whenever the OS next let the loop run. June's capture recovering at "EXACTLY 22:31:10 app_foreground" was the throttled delay resuming, not a deliberate probe — same timestamp, different mechanism, and that difference is the whole bug. NetworkStatusController now implements DefaultLifecycleObserver and calls the existing recheck() on ON_START. force = true, so it also bypasses ARBITRATE_MIN_GAP_MS: a user opening the app is exactly when a stale banner and a refused track are most visible, and it's once per foreground. ## 2. A burst of op failures no longer corroborates itself The actual defect in the escalation path. Corroboration required 2 op failures within 30s — but a link handoff fails every in-flight request at once, so a burst is ONE event producing N failures, not N independent observations that the server is gone. Two simultaneous failures walked straight to Unreachable. onOpFailure now drops a failure landing within CORROBORATION_MIN_SPACING_MS (3s) of the last recorded one. Above the sub-second window a handoff occupies, low enough that a real outage still corroborates within seconds once anything retries. ## Why this matters more than the task implied #1209 called the follow-ups "cosmetic in the diagnostics". They aren't. OfflineGatedDataSource.gateOnHealth() throws OfflineException on ServerDown BEFORE touching the network, and TrackRow disables rows. So a spurious ServerDown means the app declines to play uncached tracks that would play fine — for a blip that already resolved. The note's "captured skips advanced fine" was timing luck, not evidence the gate is harmless. ## Tests `two op failures plus a failed probe escalate immediately` used timestamps 500ms apart, which the new rule treats as a burst — so I re-spaced it and renamed it `two SPACED op failures...`. That's a deliberate reversal of an encoded expectation, not a broken test being patched. Also re-spaced `stale op failures do not corroborate` (used 0 and 1_000): left alone it would still have passed, but for the wrong reason — burst-dropping rather than staleness — and a test that can't fail for its stated reason is worse than no test. Added: a burst of four failures plus a failed probe stays Unstable, and a burst that never recovers still escalates via the sustained backstop, so dropping duplicates can't make a real outage undetectable. The foreground hook itself is unverifiable in a JVM test (ProcessLifecycleOwner needs the framework, and there's no instrumentation lane). Checked instead that nothing constructs NetworkStatusController outside Hilt, so init's ProcessLifecycleOwner.get() only runs on the main thread during Application.onCreate — the same pattern LiveEventsDispatcher already uses.
2026-08-05 14:41:48 -04:00
parent a9ca49dc4e
commit 78aa9befb6
3 changed files with 94 additions and 8 deletions
@@ -1,6 +1,9 @@
 package com.fabledsword.minstrel.connectivity

 import androidx.compose.runtime.staticCompositionLocalOf
+import androidx.lifecycle.DefaultLifecycleObserver
+import androidx.lifecycle.LifecycleOwner
+import androidx.lifecycle.ProcessLifecycleOwner
 import com.fabledsword.minstrel.BuildConfig
 import com.fabledsword.minstrel.auth.AuthStore
 import com.fabledsword.minstrel.di.ApplicationScope
@@ -41,6 +44,12 @@ private const val ARBITRATE_MIN_GAP_MS = 2_000L
 *  - reportSuccess / reportFailure from the API interceptor, the audio data
 *    source, and the playback-error reporter.
 *  - recheck() from pull-to-refresh and the banner.
+ *  - a forced probe when the app returns to the foreground (#1209). Without
+ *    it a stale ServerDown outlived the condition that caused it: the poll
+ *    loop's delay() is throttled while screen-off/doze, so recovery waited on
+ *    whenever the OS next let the loop run. Meanwhile ServerDown makes
+ *    OfflineGatedDataSource refuse every uncached track, so the app declined
+ *    to play music that would have played fine.
 *
 * Version compatibility is a byproduct of the same /healthz response.
 *
@@ -53,7 +62,7 @@ class NetworkStatusController @Inject constructor(
    connectivity: ConnectivityObserver,
    private val authStore: AuthStore,
    retrofit: Retrofit,
-) {
+) : DefaultLifecycleObserver {
    private val api: HealthzApi = retrofit.create(HealthzApi::class.java)
    private val machine = ReachabilityMachine()
    private val lastProbeAtMs = AtomicLong(0)
@@ -74,6 +83,7 @@ class NetworkStatusController @Inject constructor(
    private val intents = Channel<Intent>(Channel.UNLIMITED)

    init {
+        ProcessLifecycleOwner.get().lifecycle.addObserver(this)
        scope.launch { reduceLoop() }
        scope.launch {
            connectivity.online.collect { up ->
@@ -100,6 +110,20 @@ class NetworkStatusController @Inject constructor(
        scope.launch { probeOnce(force = true) }
    }

+    /**
+     * App returned to the foreground — probe now rather than waiting for the
+     * poll loop (#1209).
+     *
+     * The link-return probe in `init` does NOT cover this: it fires on a
+     * connectivity *change*, and an app backgrounded on stable Wi-Fi sees none.
+     * force = true so this also bypasses the ARBITRATE_MIN_GAP_MS throttle —
+     * a user bringing the app up is exactly when a stale banner and a refused
+     * track are most visible, and it's a once-per-foreground cost.
+     */
+    override fun onStart(owner: LifecycleOwner) {
+        recheck()
+    }
+
    private suspend fun reduceLoop() {
        for (intent in intents) {
            val now = System.currentTimeMillis()
@@ -4,6 +4,24 @@ internal const val ESCALATE_AFTER_MS = 120_000L
 internal const val CORROBORATION_WINDOW_MS = 30_000L
 internal const val CORROBORATION_OP_THRESHOLD = 2

+/**
+ * Minimum gap between op failures for them to count as SEPARATE evidence
+ * (#1209).
+ *
+ * A link handoff fails every in-flight request at once, so a burst is one
+ * event producing N failures — not N independent observations that the server
+ * is gone. Without this, two simultaneous failures corroborated each other
+ * straight to Unreachable, and ServerDown makes OfflineGatedDataSource refuse
+ * every uncached track. The app declined to play music that would have played
+ * fine, for a blip that had already resolved.
+ *
+ * 3s is comfortably above the sub-second window an OS handoff occupies while
+ * still letting a genuine outage corroborate within seconds once a client
+ * retries. The sustained-time backstop covers the case where nothing retries
+ * at all — and if nothing is asking, a late ServerDown costs nothing.
+ */
+internal const val CORROBORATION_MIN_SPACING_MS = 3_000L
+
 /**
 * Pure reachability state machine. No Android, no coroutines, no real clock —
 * every entry point takes `nowMs`, so it is fully deterministic and unit-
@@ -46,9 +64,17 @@ class ReachabilityMachine {
        recentOpFailures.clear()
    }

-    /** A real network op failed. Ambiguous on its own — records corroboration. */
+    /**
+     * A real network op failed. Ambiguous on its own — records corroboration.
+     *
+     * Failures arriving within [CORROBORATION_MIN_SPACING_MS] of the last
+     * recorded one are dropped rather than stacked: see that constant for why
+     * a burst must not corroborate itself.
+     */
    fun onOpFailure(nowMs: Long) {
        pruneOpFailures(nowMs)
+        val last = recentOpFailures.lastOrNull()
+        if (last != null && nowMs - last < CORROBORATION_MIN_SPACING_MS) return
        recentOpFailures.addLast(nowMs)
    }

@@ -50,12 +50,46 @@ class ReachabilityMachineTest {
    }

    @Test
-    fun `two op failures plus a failed probe escalate immediately`() {
+    fun `two SPACED op failures plus a failed probe escalate immediately`() {
        val m = machine()
        m.onLinkChange(up = true)
        m.onOpFailure(nowMs = 1_000)
-        m.onOpFailure(nowMs = 1_500) // corroboration reached
-        m.onProbeFailure(nowMs = 2_000) // probe agrees → fast ServerDown
+        // Spacing matters as of #1209: these must be far enough apart to be
+        // separate evidence rather than one event's worth of fallout. This
+        // test previously used 1_500 — 500ms — which is now deliberately
+        // treated as a burst and does NOT corroborate.
+        m.onOpFailure(nowMs = 1_000 + CORROBORATION_MIN_SPACING_MS)
+        m.onProbeFailure(nowMs = 1_000 + CORROBORATION_MIN_SPACING_MS + 500)
+        assertEquals(ServerHealth.ServerDown, m.health())
+    }
+
+    // The #1209 mechanism: an OS network handoff fails every in-flight request
+    // at once. That must NOT reach ServerDown, because ServerDown makes
+    // OfflineGatedDataSource refuse uncached tracks outright — the app would
+    // decline to play music that plays fine, for a blip already over.
+    @Test
+    fun `a burst of op failures does not corroborate itself into ServerDown`() {
+        val m = machine()
+        m.onLinkChange(up = true)
+        m.onOpFailure(nowMs = 1_000)
+        m.onOpFailure(nowMs = 1_050)
+        m.onOpFailure(nowMs = 1_100)
+        m.onOpFailure(nowMs = 1_200)
+        m.onProbeFailure(nowMs = 1_500)
+        // Unstable is non-gating, so playback keeps working.
+        assertEquals(ServerHealth.Unstable, m.health())
+    }
+
+    @Test
+    fun `a burst still escalates via the sustained backstop if it never recovers`() {
+        val m = machine()
+        m.onLinkChange(up = true)
+        m.onOpFailure(nowMs = 1_000)
+        m.onOpFailure(nowMs = 1_050)
+        m.onProbeFailure(nowMs = 1_500) // unstable, streak starts here
+        // Dropping burst duplicates must not make a REAL outage undetectable —
+        // the time backstop is what guarantees escalation either way.
+        m.onProbeFailure(nowMs = 1_500 + ESCALATE_AFTER_MS)
        assertEquals(ServerHealth.ServerDown, m.health())
    }

@@ -64,7 +98,7 @@ class ReachabilityMachineTest {
        val m = machine()
        m.onLinkChange(up = true)
        m.onOpFailure(nowMs = 1_000)
-        m.onOpFailure(nowMs = 1_500)
+        m.onOpFailure(nowMs = 1_000 + CORROBORATION_MIN_SPACING_MS)
        m.onSuccess() // arbiter says server is fine
        assertEquals(ServerHealth.Healthy, m.health())
    }
@@ -74,9 +108,11 @@ class ReachabilityMachineTest {
        val m = machine()
        m.onLinkChange(up = true)
        m.onOpFailure(nowMs = 0)
-        m.onOpFailure(nowMs = 1_000)
+        // Spaced so this test exercises STALENESS, not the burst rule — with
+        // 1_000 it would have passed for the wrong reason after #1209.
+        m.onOpFailure(nowMs = CORROBORATION_MIN_SPACING_MS)
        // both op failures are now older than the corroboration window:
-        m.onProbeFailure(nowMs = 1_000 + CORROBORATION_WINDOW_MS + 1)
+        m.onProbeFailure(nowMs = CORROBORATION_MIN_SPACING_MS + CORROBORATION_WINDOW_MS + 1)
        assertEquals(ServerHealth.Unstable, m.health()) // not enough fresh corroboration
    }