diff --git a/android/app/src/main/java/com/fabledsword/minstrel/connectivity/NetworkStatusController.kt b/android/app/src/main/java/com/fabledsword/minstrel/connectivity/NetworkStatusController.kt index e59d8b1a..89fc5f39 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/connectivity/NetworkStatusController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/connectivity/NetworkStatusController.kt @@ -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(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() diff --git a/android/app/src/main/java/com/fabledsword/minstrel/connectivity/ReachabilityMachine.kt b/android/app/src/main/java/com/fabledsword/minstrel/connectivity/ReachabilityMachine.kt index 11d54f15..bc998515 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/connectivity/ReachabilityMachine.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/connectivity/ReachabilityMachine.kt @@ -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) } diff --git a/android/app/src/test/java/com/fabledsword/minstrel/connectivity/ReachabilityMachineTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/connectivity/ReachabilityMachineTest.kt index 430fd08c..d1f7fb68 100644 --- a/android/app/src/test/java/com/fabledsword/minstrel/connectivity/ReachabilityMachineTest.kt +++ b/android/app/src/test/java/com/fabledsword/minstrel/connectivity/ReachabilityMachineTest.kt @@ -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 }