Guard ArrivalsViewModel.refresh() against out-of-order overlapping refreshes (#1933) (#1934)

* Guard ArrivalsViewModel.refresh() against out-of-order overlapping refreshes (#1933)

refresh() applied every completed result unconditionally. The 60s poll loop
awaits its suspend refresh() sequentially so it never overlaps itself, but
manualRefresh() and loadMore() each viewModelScope.launch { refresh() } — so a
user refresh (toolbar button, or the alert-dialog dismiss in
ArrivalActionHandlers) can run concurrently with an in-flight poll and finish
out of order. The older-started completion's `loaded.value = data` would then
clobber the fresher result, surfacing a spurious isStale flag or older arrivals
until the next poll corrected it (≤60s). The map snapshot lastLoaded() was
already protected by the repository's compareAndSet (#1932); this closes the
same gap one layer up, in the ViewModel's `loaded` StateFlow.

Fix: a monotonic refreshGeneration bumped at the start of each refresh; a
completion applies its result only while it's still the latest, else it drops
out (no state write, no map-snapshot emit). All reads/writes are confined to the
ViewModel dispatcher — the only suspension in refresh() is the fetch — so a
plain Int needs no synchronization.

Test drives two overlapping refreshes (per-call gates on the fake repository)
completing out of order and asserts the older-started stale completion cannot
overwrite the fresher one; verified red without the guard.

Fixes #1933

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Update lastResponseTime only for the winning refresh completion

Move the lastResponseTime stamp after the latest-wins guard so a superseded
(later-completing) refresh no longer pushes the poll timer past the fresher
completion's timestamp — the poll cadence now measures its 60s interval from the
shown data's arrival. Addresses review feedback on #1934.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

Author: bmander

onebusaway-android/src/main/java/org/onebusaway/android/ui/arrivals/ArrivalsViewModel.kt

@@ -100,18 +100,34 @@ class ArrivalsViewModel @AssistedInject constructor(
     var lastResponseTime: WallTime = WallTime(0L)
         private set
 
+    /**
+     * Bumped at the start of every [refresh]; a refresh applies its result only while it's still the
+     * latest. The poll loop awaits its own `refresh()` sequentially, but [manualRefresh]/[loadMore]
+     * each launch one, so a user refresh can run concurrently with an in-flight poll and finish out of
+     * order — without this guard the older-started one's `loaded.value = data` would clobber the fresher
+     * result (a spurious stale flag / older arrivals). Confined to the ViewModel dispatcher (the only
+     * suspension in [refresh] is the fetch), so a plain `Int` needs no synchronization. See #1933.
+     */
+    private var refreshGeneration = 0
+
     /**
      * Loads the arrivals once. Suspends until done so the screen's polling loop can measure the
      * 60s interval from completion. A failed refresh keeps any existing content (the repository
      * already returns the last good data as stale); it only surfaces [ArrivalsUiState.Error]
      * when there is nothing to show.
      *
-     * Returns true when a *fresh* network response landed — a stale fallback (the repository
-     * returns the last good data flagged `isStale` on failure-with-content) or a hard failure
-     * returns false. Consumed by [loadMore]; the poll loop ignores it.
+     * Returns true when a *fresh* network response landed and was applied — a stale fallback (the
+     * repository returns the last good data flagged `isStale` on failure-with-content), a hard failure,
+     * or a completion superseded by a newer refresh (#1933) returns false. Consumed by [loadMore]; the
+     * poll loop ignores it.
      */
     suspend fun refresh(): Boolean {
+        val generation = ++refreshGeneration
         val result = repository.getArrivals(stopId, minutesAfter)
+        // A newer refresh started while this one was in flight — drop this (now stale) completion so it
+        // can't overwrite the fresher state, emit an out-of-date map snapshot, or push the poll timer
+        // (measured against the *shown* data) past the fresher completion. See #1933.
+        if (generation != refreshGeneration) return false
         lastResponseTime = WallTime.now()
         return result.fold(
             onSuccess = { data ->

onebusaway-android/src/test/java/org/onebusaway/android/ui/arrivals/ArrivalsViewModelTest.kt

@@ -51,6 +51,12 @@ private class FakeArrivalsRepository(
      *  (e.g. to fire a superseding load-more before the first finishes). */
     var gate: CompletableDeferred<Unit>? = null
 
+    /** Per-call gate/result overrides indexed by call order, for driving *overlapping* loads that
+     *  complete out of order (issue #1933). When a slot is present the Nth [getArrivals] awaits that
+     *  gate and returns that result instead of [gate]/[result]. */
+    val callGates = mutableListOf<CompletableDeferred<Unit>>()
+    val callResults = mutableListOf<Result<ArrivalsData>>()
+
     var lastFavoriteSet: Pair<String, Boolean>? = null
 
     var lastFavoriteRoute: FavoriteRouteCall? = null
@@ -72,9 +78,10 @@ private class FakeArrivalsRepository(
         stopId: String,
         minutesAfter: Int
     ): Result<ArrivalsData> {
+        val call = requestedMinutesAfter.size
         requestedMinutesAfter.add(minutesAfter)
-        gate?.await()
-        return result
+        (callGates.getOrNull(call) ?: gate)?.await()
+        return callResults.getOrNull(call) ?: result
     }
 
     override suspend fun setStopFavorite(stopId: String, favorite: Boolean) {
@@ -254,6 +261,38 @@ class ArrivalsViewModelTest {
         assertEquals(listOf(65, 125), repository.requestedMinutesAfter)
     }
 
+    // --- Overlapping out-of-order refreshes (latest-wins guard, #1933) -------------------------
+
+    @Test
+    fun `an out-of-order overlapping refresh cannot clobber the fresher result with a stale one`() = runTest {
+        // The poll refresh (call #0) starts first but lands LAST as a stale fallback; a user refresh
+        // (call #1) starts later and lands FIRST with fresh data. The stale, older-started completion
+        // must not overwrite the fresh one it superseded. Regression for #1933.
+        val repository = FakeArrivalsRepository(Result.success(data()))
+        repository.callGates += CompletableDeferred() // call #0 (poll)
+        repository.callGates += CompletableDeferred() // call #1 (user refresh)
+        repository.callResults += Result.success(data(minutesAfter = 65, isStale = true)) // #0 stale fallback
+        repository.callResults += Result.success(data(minutesAfter = 125, isStale = false)) // #1 fresh
+        val viewModel = ArrivalsViewModel("1_100", repository)
+
+        viewModel.manualRefresh() // poll-like refresh, parks on gate #0
+        viewModel.manualRefresh() // superseding refresh, parks on gate #1
+
+        // The later-started refresh lands first with fresh data.
+        repository.callGates[1].complete(Unit)
+        advanceUntilIdle()
+        val fresh = viewModel.state.value as ArrivalsUiState.Content
+        assertFalse(fresh.isStale)
+        assertEquals(ServerTime(125 * 60_000L), fresh.windowEnd)
+
+        // The earlier-started refresh lands last as a stale fallback — it must be dropped, not applied.
+        repository.callGates[0].complete(Unit)
+        advanceUntilIdle()
+        val afterStale = viewModel.state.value as ArrivalsUiState.Content
+        assertFalse(afterStale.isStale)
+        assertEquals(ServerTime(125 * 60_000L), afterStale.windowEnd)
+    }
+
     @Test
     fun `toggle favorite optimistically updates the header and persists`() = runTest {
         val repository = FakeArrivalsRepository(Result.success(data(favorite = false)))