Forward CancellationException across the api/repository layer (#1928)

bmander · claude · web-flow · commit 6c71ddd80e6b · 2026-07-16T20:59:48.000-07:00
Audit follow-up to the runCatching cancellation fix in #1921. `runCatching` (and a broad `catch (Exception/Throwable/RuntimeException/IllegalStateException)`) swallow kotlinx `CancellationException` — it's a plain `IllegalStateException` subtype — so a coroutine cancelled mid-suspend-call had its cancellation converted into a `Result.failure`/`success(null)`/empty value instead of propagating. That breaks structured concurrency: cancelled work keeps running and callers see a cancellation as an ordinary error/empty state. Swept the codebase; narrow catches (IOException, JSONException, SecurityException, IllegalArgumentException, …) don't catch cancellation and were left alone. Fixed every site where a swallowing construct wrapped a real suspend call in a cancellable coroutine by rethrowing CancellationException before swallowing — matching the existing idiom in RouteMapController/SingleFlight: - ObaApiProvider.call/callOrNull — the core wrapper behind ~every OBA network data source (highest leverage). Both layers needed the guard: callers wrap this in their own runCatching, so fixing only one layer re-swallows one level up. - SurveyDataSource.studies/submit, WeatherRepository.currentForecast, RouteSearchRepository.search — plain suspend funs returning the swallowed value. - BikeStationsRepository — outer runCatching plus an inner `catch (Exception)` that retried a second network fetch after cancellation (issued new work on the other URL structure after the coroutine was already cancelled). - TripInfoRepository.requestAlarm — a cancelled reminder save surfaced to the UI as TripInfoEvent.SaveFailed. - RouteInfoRepository, StopSearchRepository, SearchResultsRepository, MyListRepository — mitigated today by an enclosing withContext/coroutineScope prompt-cancellation guarantee, but fragile; guarded at the source. RouteInfo's recoverCatching had additionally disguised a cancellation as a "route not found" error string. Added SurveyDataSourceCancellationTest (JVM-testable; its cancellation path rethrows before touching android.util.Log). The Android-typed repos aren't directly unit-testable in this module (no Robolectric/mocking), same constraint SingleFlightTest documents. Not changed: ReminderClient (never-cancelled fire-and-forget scope), RegionsClient (non-suspend runBlocking bridges), MyListViewModel's Flow.catch (cancellation- transparent), and the many narrow catches. FocusedTripRepository's route-stops swallow is fixed separately in #1921 (getOrThrow). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/api/data/SurveyDataSource.kt b/onebusaway-android/src/main/java/org/onebusaway/android/api/data/SurveyDataSource.kt
@@ -17,6 +17,7 @@ package org.onebusaway.android.api.data
 
 import android.util.Log
 import javax.inject.Inject
+import kotlinx.coroutines.CancellationException
 import org.onebusaway.android.api.contract.StudyResponse
 import org.onebusaway.android.api.contract.SurveyWebService
 import org.onebusaway.android.models.Survey
@@ -53,7 +54,10 @@ class DefaultSurveyDataSource @Inject constructor(
 
     override suspend fun studies(url: String, userId: String?): Result<List<Survey>> = runCatching {
         service.getStudy(url, userId).toSurveys()
-    }.onFailure { Log.e(TAG, "studies failed", it) }
+    }.onFailure {
+        if (it is CancellationException) throw it
+        Log.e(TAG, "studies failed", it)
+    }
 
     override suspend fun submit(
         url: String,
@@ -73,7 +77,10 @@ class DefaultSurveyDataSource @Inject constructor(
             stopLongitude = stopLongitude,
             responses = responses,
         ).let { SurveySubmitResult(it.surveyResponse?.id) }
-    }.onFailure { Log.e(TAG, "submit failed", it) }
+    }.onFailure {
+        if (it is CancellationException) throw it
+        Log.e(TAG, "submit failed", it)
+    }
 
     private companion object {
         const val TAG = "SurveyDataSource"
diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/api/net/ObaApiProvider.kt b/onebusaway-android/src/main/java/org/onebusaway/android/api/net/ObaApiProvider.kt
@@ -19,6 +19,7 @@ import android.net.Uri
 import java.io.IOException
 import javax.inject.Inject
 import javax.inject.Singleton
+import kotlinx.coroutines.CancellationException
 import kotlinx.serialization.json.Json
 import okhttp3.MediaType.Companion.toMediaType
 import okhttp3.OkHttpClient
@@ -52,7 +53,9 @@ class ObaApiProvider @Inject constructor(
      */
     suspend fun <T> call(block: suspend (ObaWebService) -> T): Result<T> {
         val service = service() ?: return Result.failure(noEndpoint())
-        return runCatching { block(service) }
+        // runCatching catches everything, so a cancelled coroutine's CancellationException would become
+        // a Result.failure and stop propagating; rethrow it so cancellation still unwinds normally.
+        return runCatching { block(service) }.onFailure { if (it is CancellationException) throw it }
     }
 
     /**
@@ -61,7 +64,7 @@ class ObaApiProvider @Inject constructor(
      */
     suspend fun <T> callOrNull(block: suspend (ObaWebService) -> T): Result<T?> {
         val service = service() ?: return Result.success(null)
-        return runCatching { block(service) }
+        return runCatching { block(service) }.onFailure { if (it is CancellationException) throw it }
     }
 
     /**
diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/map/bike/BikeStationsRepository.kt b/onebusaway-android/src/main/java/org/onebusaway/android/map/bike/BikeStationsRepository.kt
@@ -17,6 +17,7 @@ package org.onebusaway.android.map.bike
 
 import javax.inject.Inject
 import android.location.Location
+import kotlinx.coroutines.CancellationException
 import org.onebusaway.android.R
 import org.onebusaway.android.api.adapters.toBikeStations
 import org.onebusaway.android.api.contract.BikeWebService
@@ -54,12 +55,15 @@ class DefaultBikeStationsRepository @Inject constructor(
         val useOld = prefs.getBoolean(R.string.preference_key_otp_api_url_version, false)
         try {
             fetch(base, useOld, southWest, northEast)
+        } catch (e: CancellationException) {
+            // Don't treat a cancelled fetch as a "wrong URL structure" and retry it on the other path.
+            throw e
         } catch (e: Exception) {
             if (useOld) throw e
             fetch(base, useOldUrlStructure = true, southWest, northEast)
                 .also { prefs.setBoolean(R.string.preference_key_otp_api_url_version, true) }
         }
-    }
+    }.onFailure { if (it is CancellationException) throw it }
 
     private suspend fun fetch(
         base: String,
diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/home/weather/WeatherRepository.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/home/weather/WeatherRepository.kt
@@ -19,6 +19,7 @@ import android.content.Context
 import dagger.hilt.android.qualifiers.ApplicationContext
 import java.io.IOException
 import javax.inject.Inject
+import kotlinx.coroutines.CancellationException
 import org.onebusaway.android.R
 import org.onebusaway.android.api.contract.WeatherWebService
 import org.onebusaway.android.region.RegionRepository
@@ -56,5 +57,5 @@ class DefaultWeatherRepository @Inject constructor(
         val forecast = weatherService.getWeather(url).current_forecast
             ?: throw IOException("No weather forecast for region $regionId")
         WeatherData(forecast.icon ?: "", forecast.temperature, forecast.summary)
-    }
+    }.onFailure { if (it is CancellationException) throw it }
 }
diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/mylists/MyListRepository.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/mylists/MyListRepository.kt
@@ -20,6 +20,7 @@ import android.text.format.DateUtils
 import androidx.annotation.ArrayRes
 import androidx.annotation.ColorRes
 import androidx.annotation.StringRes
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.async
@@ -389,7 +390,10 @@ private suspend fun fetchStopBadges(context: Context, stopId: String): List<Arri
         convertArrivals(context, snapshot.arrivals, ServerTime(snapshot.currentTime), false)
             .take(MAX_ARRIVALS_PER_STOP)
             .map { it.toBadge(context) }
-    }.getOrDefault(emptyList())
+    }.getOrElse {
+        if (it is CancellationException) throw it
+        emptyList()
+    }
 
 private fun ArrivalInfo.toBadge(context: Context): ArrivalBadge {
     val etaText = if (eta <= 0) {
diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/routeinfo/RouteInfoRepository.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/routeinfo/RouteInfoRepository.kt
@@ -23,6 +23,7 @@ import android.util.Log
 import dagger.hilt.android.qualifiers.ApplicationContext
 import java.io.IOException
 import javax.inject.Inject
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.async
 import kotlinx.coroutines.coroutineScope
@@ -75,6 +76,9 @@ class DefaultRouteInfoRepository @Inject constructor(
                 registerRouteUsage(route)
                 toRouteInfo(route, directions)
             }
+                // Let a cancelled load unwind instead of logging it and disguising it (below) as a
+                // "route not found" error string.
+                .onFailure { if (it is CancellationException) throw it }
                 .onFailure { Log.e(TAG, "loadRouteInfo($routeId) failed", it) }
                 // The VM renders Result.failure's message, so surface a user-facing route error
                 // string. Preserve the OBA code when we have one (e.g. 404 -> "route not found");
diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/search/RouteSearchRepository.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/search/RouteSearchRepository.kt
@@ -19,6 +19,7 @@ import org.onebusaway.android.api.data.LocationSearchDataSource
 
 import android.util.Log
 import java.io.IOException
+import kotlinx.coroutines.CancellationException
 import org.onebusaway.android.location.SearchCenter
 import org.onebusaway.android.models.ObaRoute
 import org.onebusaway.android.util.routeDisplayNames
@@ -73,7 +74,12 @@ class DefaultRouteSearchRepository(
             }
         }
         routes.map(::toResult)
-    }.onFailure { Log.e(TAG, "route search failed", it) }
+    }.onFailure {
+        // transformLatest cancels the in-flight search on each keystroke; let that cancellation
+        // propagate instead of reporting it to the UI as a search Error.
+        if (it is CancellationException) throw it
+        Log.e(TAG, "route search failed", it)
+    }
 
     private fun toResult(route: ObaRoute): RouteSearchResult {
         val names = routeDisplayNames(route.shortName, route.longName, route.description)
diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/search/StopSearchRepository.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/search/StopSearchRepository.kt
@@ -20,6 +20,7 @@ import org.onebusaway.android.api.data.LocationSearchDataSource
 import android.content.Context
 import android.util.Log
 import java.io.IOException
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.withContext
 import org.onebusaway.android.app.di.DatabaseEntryPoint
@@ -80,7 +81,10 @@ class DefaultStopSearchRepository(
                 db.importGate().awaitReady()
                 val userInfo = db.stopDao().userInfoMap().toStopUserInfoMap()
                 stops.map { it.toStopSearchResult(userInfo[it.id]) }
-            }.onFailure { Log.e(TAG, "stop search failed", it) }
+            }.onFailure {
+                if (it is CancellationException) throw it
+                Log.e(TAG, "stop search failed", it)
+            }
         }
 
     private companion object {
diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/searchresults/SearchResultsRepository.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/searchresults/SearchResultsRepository.kt
@@ -21,6 +21,7 @@ import org.onebusaway.android.api.data.RoutesNearResult
 import javax.inject.Inject
 import android.location.Location
 import java.io.IOException
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.async
 import kotlinx.coroutines.coroutineScope
 import org.onebusaway.android.database.oba.ImportGate
@@ -57,8 +58,14 @@ class DefaultSearchResultsRepository @Inject constructor(
             ?: return@coroutineScope Result.failure(IOException("No search location available"))
 
         // Each search resolves a non-OK code / transport failure to Result.failure (requireData).
-        val routes = async { runCatching { searchRoutes(query, center) } }
-        val stops = async { runCatching { searchStops(query, center) } }
+        // Keep a cancelled search from being coalesced into Result.failure here; rethrowing lets it
+        // propagate through await() and cancel the sibling search too.
+        val routes = async {
+            runCatching { searchRoutes(query, center) }.onFailure { if (it is CancellationException) throw it }
+        }
+        val stops = async {
+            runCatching { searchStops(query, center) }.onFailure { if (it is CancellationException) throw it }
+        }
         val routeResult = routes.await()
         val stopResult = stops.await()
 
@@ -73,7 +80,11 @@ class DefaultSearchResultsRepository @Inject constructor(
         // The favourite/custom-name enrichment is best-effort: a DB hiccup here must not fail a search
         // that already has route/stop results, so treat it as a soft miss (empty map) like the lookups.
         importGate.awaitReady()
-        val userInfo = runCatching { stopDao.userInfoMap().toStopUserInfoMap() }.getOrDefault(emptyMap())
+        val userInfo = runCatching { stopDao.userInfoMap().toStopUserInfoMap() }
+            .getOrElse {
+                if (it is CancellationException) throw it
+                emptyMap()
+            }
         val items = buildList {
             routeResult.getOrNull()?.let { result ->
                 result.routes.forEach { add(toRoute(it, result.agencyNames)) }
diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripinfo/TripInfoRepository.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripinfo/TripInfoRepository.kt
@@ -19,6 +19,7 @@ import android.content.Context
 import android.text.format.DateUtils
 import dagger.hilt.android.qualifiers.ApplicationContext
 import javax.inject.Inject
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.withContext
 import org.onebusaway.android.R
@@ -270,7 +271,11 @@ class DefaultTripInfoRepository @Inject constructor(
                 secondsBefore = reminderSeconds,
                 vehicleId = data.vehicleId,
             ).url
-        }.getOrNull()
+        }.getOrElse {
+            // Don't let a cancelled save be reported to the UI as a save failure; propagate cancellation.
+            if (it is CancellationException) throw it
+            null
+        }
     }
 
     companion object {
diff --git a/onebusaway-android/src/test/java/org/onebusaway/android/api/data/SurveyDataSourceCancellationTest.kt b/onebusaway-android/src/test/java/org/onebusaway/android/api/data/SurveyDataSourceCancellationTest.kt
@@ -0,0 +1,75 @@
+/*
+ * Copyright (C) 2026 Open Transit Software Foundation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.onebusaway.android.api.data
+
+import kotlinx.coroutines.CancellationException
+import kotlinx.coroutines.ExperimentalCoroutinesApi
+import kotlinx.coroutines.test.runTest
+import org.junit.Assert.assertTrue
+import org.junit.Assert.fail
+import org.junit.Test
+import org.onebusaway.android.api.contract.SurveyWebService
+
+/**
+ * A cancelled survey fetch must let its [CancellationException] propagate rather than swallow it into a
+ * `Result.failure`. `runCatching` catches everything (including cancellation), so the data source
+ * rethrows the cancellation before it can be logged or reported as an ordinary failure — otherwise a
+ * cancelled coroutine would keep running and structured concurrency would break. This mirrors
+ * `SingleFlightTest`'s "cancelled block propagates" coverage; the same rethrow guard is applied across
+ * the api/repository layer (ObaApiProvider, the search repositories, etc.), which are Android-typed and
+ * so not directly JVM-testable here.
+ */
+@OptIn(ExperimentalCoroutinesApi::class)
+class SurveyDataSourceCancellationTest {
+
+    /** A web service whose calls behave as if the coroutine was cancelled mid-request. */
+    private val cancellingService = object : SurveyWebService {
+        override suspend fun getStudy(url: String, userId: String?) =
+            throw CancellationException("cancelled")
+
+        override suspend fun submitSurvey(
+            url: String,
+            userIdentifier: String?,
+            surveyId: Int,
+            stopIdentifier: String?,
+            stopLatitude: Double,
+            stopLongitude: Double,
+            responses: String,
+        ) = throw CancellationException("cancelled")
+    }
+
+    private val dataSource = DefaultSurveyDataSource(cancellingService)
+
+    @Test
+    fun `a cancelled study fetch propagates instead of becoming a Result failure`() = runTest {
+        try {
+            dataSource.studies("url", userId = null)
+            fail("expected CancellationException to propagate")
+        } catch (e: CancellationException) {
+            assertTrue(e.message == "cancelled")
+        }
+    }
+
+    @Test
+    fun `a cancelled submit propagates instead of becoming a Result failure`() = runTest {
+        try {
+            dataSource.submit("url", null, 1, null, 0.0, 0.0, "[]")
+            fail("expected CancellationException to propagate")
+        } catch (e: CancellationException) {
+            assertTrue(e.message == "cancelled")
+        }
+    }
+}
