From 78aa9befb6ef6fb34ab4e031589aef5dc4d93877 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 5 Aug 2026 14:41:48 -0400 Subject: [PATCH] =?UTF-8?q?fix(connectivity):=20probe=20on=20foreground;?= =?UTF-8?q?=20a=20burst=20can't=20corroborate=20ServerDown=20=E2=80=94=20#?= =?UTF-8?q?1209?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../connectivity/NetworkStatusController.kt | 26 +++++++++- .../connectivity/ReachabilityMachine.kt | 28 ++++++++++- .../connectivity/ReachabilityMachineTest.kt | 48 ++++++++++++++++--- 3 files changed, 94 insertions(+), 8 deletions(-) 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 }