Skip to content

Commit 199ad11

Browse files
bmanderclaude
bmander
and
claude
authored
Enforce cancellation-safe runCatching via wrapper + lint check (#1931)
Follow-up to #1928 (the audit): make the "runCatching swallows CancellationException in coroutine code" bug class impossible to reintroduce, deterministically, in CI. Two parts: 1. `runCatchingCancellable` (org.onebusaway.android.util) — the sanctioned wrapper. Behaviour-identical to kotlin.runCatching but rethrows CancellationException, so a cancelled coroutine propagates instead of being captured into Result.failure. `inline`, mirroring runCatching, so `block` may call suspend functions and the guard is free. 2. `SwallowedCancellation` lint check (:lint-rules) — fails the build on a bare `runCatching` whose nearest enclosing named function is `suspend` (matching detekt's SuspendFunSwallowedCancellation; suspend detected via the Continuation param on the light method). Lambdas are transparent, so `async { runCatching { … } }` in a suspend function is caught; a runCatching in a coroutine-builder lambda inside a non-suspend function is out of scope (guard those by hand). With one sanctioned wrapper the rule is a simple symbol ban — no fragile "does this lambda suspend" analysis and no guard-detection. Migrated every flagged site onto the wrapper: the #1928 inline-guard sites (ObaApiProvider, SurveyDataSource, WeatherRepository, RouteSearchRepository, StopSearchRepository, SearchResultsRepository, RouteInfoRepository, TripInfoRepository, MyListRepository, BikeStationsRepository) plus four pure-but-in-suspend-fn sites the rule newly surfaced (GeocodeRepository, TripResultsRepository x2, TripPlanRepository.plan). Net -24 lines — the wrapper reads cleaner than the inline guards. BikeStations keeps its inner `catch (CancellationException) { throw e }` (the rule covers runCatching, not try/catch). Renamed TimeDomainIssueRegistry -> OneBusAwayIssueRegistry (it's no longer time-only) and updated the manifest attribute. Added detector unit tests, lint-rules/README, and a CLAUDE.md section. Verified: lint-rules tests green; app compiles clean under -PwarningsAsErrors; a full app lint run flagged exactly one SwallowedCancellation (a temporary probe, since removed) and zero real sites — confirming registry discovery, the rule firing, and complete migration. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 193365b commit 199ad11

21 files changed

Lines changed: 366 additions & 82 deletions

‎CLAUDE.md‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,24 @@ follow that pattern when adding server-domain time math.
218218
mints into `WallTime`/`ElapsedTime` (domain made explicit) or carries an inline `@Suppress` with a
219219
one-line rationale (there is no lint baseline — these live at the site). See `lint-rules/README.md`.
220220

221+
## Coroutines: never swallow CancellationException (#1908 / #1921)
222+
223+
`kotlin.runCatching` catches **every** `Throwable`, and a broad `catch (Exception | Throwable |
224+
RuntimeException | IllegalStateException)` catches `CancellationException` too — it's a plain
225+
`java.util.concurrent.CancellationException`, i.e. an `IllegalStateException` subtype. So in coroutine code
226+
either one silently converts a cancelled coroutine's `CancellationException` into a `Result.failure` /
227+
caught value: the cancelled work keeps running and callers read the cancellation as an ordinary
228+
error/empty state (structured concurrency breaks). It passes compilation and review and only misbehaves
229+
under real cancellation.
230+
231+
- **Use `runCatchingCancellable`** (`org.onebusaway.android.util`) instead of bare `runCatching` in any
232+
`suspend`/coroutine code. It's behaviour-identical but rethrows `CancellationException`. The custom lint
233+
check `SwallowedCancellation` (module `:lint-rules`, no baseline) fails the build on a bare `runCatching`
234+
in a `suspend` function, so this is the enforced path.
235+
- For a broad `try/catch` in coroutine code, put a leading `catch (e: CancellationException) { throw e }`
236+
before the broad catch (the lint check covers `runCatching` only). Narrow catches (`IOException`, …)
237+
don't catch cancellation and are fine.
238+
221239
## No unsanctioned heuristics
222240

223241
Do **not** introduce heuristics — magic thresholds, magnitude guesses, or "good enough" inference of

‎lint-rules/README.md‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,29 @@ is a property of the *endpoint* — the two same-named `StopTime` (seconds) and
8181
millis) DTOs are the proof — so only the endpoint-aware adapter can mint correctly, and reading the raw
8282
field elsewhere, even to pass it straight on, is the escape.
8383

84+
### `SwallowedCancellation` (warning)
85+
Fires when `kotlin.runCatching` is called inside a `suspend` function. `runCatching` catches every
86+
`Throwable`, so in a coroutine it also swallows the `CancellationException` a cancelled coroutine throws
87+
from a suspend call — converting cancellation into an ordinary `Result.failure`, so the cancelled work
88+
keeps running and callers read the cancellation as a normal error/empty state (structured concurrency
89+
breaks). `CancellationException` is a plain `IllegalStateException` subtype, so nothing at the type level
90+
catches this; it passes compilation and review and only misbehaves under real cancellation (screen closed,
91+
search superseded by a keystroke, poll tick outrun). This is the #1908 / #1921 bug class.
92+
93+
Fix by using **`runCatchingCancellable`** (`org.onebusaway.android.util`) — behaviour-identical to
94+
`runCatching` but it rethrows `CancellationException` (a real failure still resolves to `Result.failure`).
95+
The wrapper is the single sanctioned path, so the check degrades to a symbol ban: any bare `runCatching`
96+
in a suspend function is flagged. For a broad `try/catch (Exception)` in a suspend function, add a leading
97+
`catch (e: CancellationException) { throw e }` — this check covers `runCatching` only.
98+
99+
Scope is the **nearest enclosing named function** being `suspend` (matching detekt's
100+
`SuspendFunSwallowedCancellation`), detected via the `Continuation` parameter on its light method — so
101+
`async { runCatching { … } }` inside a suspend function is caught (lambdas are transparent), while a
102+
`runCatching` in a coroutine-builder lambda inside a *non-suspend* function is deliberately out of scope
103+
(the boundary isn't visible without heuristics; guard those by hand). A `runCatching` around purely
104+
non-suspending work in a suspend function is still flagged — the wrapper is a strict superset, so it's
105+
never wrong, and the rule stays a simple ban rather than a fragile "does this lambda suspend" analysis.
106+
84107
## Lifecycle
85108

86109
Each check names its reason to exist and its reason to die. Regenerate the baseline after landing a
@@ -92,6 +115,7 @@ check, then only ever shrink it.
92115
| `UnwrappedClockValue` | foreign clock namespace, coming to rest | never (can't forbid `Long`) |
93116
| `PrematureUnwrap` | the elimination door of the typed region | a real module boundary makes `.epochMs` cross-module |
94117
| `WireTimeEscape` | adapter-only consumption of wire DTOs | `:api` becomes a Gradle module with `internal` DTOs |
118+
| `SwallowedCancellation` | `runCatching` swallowing cancellation in suspend code | Kotlin/lint ships an equivalent built-in check |
95119

96120
**Enrollment sweep — nothing left to enroll.** The plan anticipated adding the legacy Jackson
97121
`io/elements` getters (`ObaArrivalInfo#getScheduledArrivalTime`, …) to `CLOCK_SOURCES`. That surface has
@@ -105,7 +129,7 @@ CLAUDE.md. The checks are a latch discipline, not a suggestion.
105129
## Status: enforced
106130

107131
Wired into the app via `lintChecks(project(":lint-rules"))` in `onebusaway-android/build.gradle.kts`, so a
108-
**new** violation of any of the four issues fails the build under the strict `-PwarningsAsErrors` gate
132+
**new** violation of any of the five issues fails the build under the strict `-PwarningsAsErrors` gate
109133
(verified). There is **no lint baseline** — the app is kept clean under the full catalog, so every
110134
sanctioned pre-existing site is handled *at the site*, not grandfathered in a file:
111135

@@ -123,8 +147,11 @@ sanctioned pre-existing site is handled *at the site*, not grandfathered in a fi
123147
visible at the site, not hidden in this file. Keep it empty.
124148
- **0** `WireTimeEscape` — the migration left no wire-field read in app logic; the check starts empty and
125149
exists to keep it that way.
150+
- **0** `SwallowedCancellation` — every bare `runCatching` in a suspend function was migrated to
151+
`runCatchingCancellable` (#1908 / #1921 audit); the check starts empty and keeps new coroutine code on
152+
the sanctioned wrapper.
126153

127-
Keep all four checks at zero: mint each new site (or inline-`@Suppress` a genuinely-sanctioned one with a
154+
Keep all five checks at zero: mint each new site (or inline-`@Suppress` a genuinely-sanctioned one with a
128155
rationale) — don't reintroduce a baseline.
129156

130157
## Develop

‎lint-rules/build.gradle.kts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,6 @@ dependencies {
3535
// Lint discovers the registry through this manifest attribute when the jar is loaded via lintChecks.
3636
tasks.jar {
3737
manifest {
38-
attributes(mapOf("Lint-Registry-v2" to "org.onebusaway.lint.TimeDomainIssueRegistry"))
38+
attributes(mapOf("Lint-Registry-v2" to "org.onebusaway.lint.OneBusAwayIssueRegistry"))
3939
}
4040
}

lint-rules/src/main/java/org/onebusaway/lint/TimeDomainIssueRegistry.kt renamed to lint-rules/src/main/java/org/onebusaway/lint/OneBusAwayIssueRegistry.kt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,15 @@ import com.android.tools.lint.detector.api.Issue
2424
* Registry that publishes onebusaway-android's custom lint checks. Discovered via the
2525
* `Lint-Registry-v2` manifest attribute set in this module's `build.gradle`.
2626
*/
27-
class TimeDomainIssueRegistry : IssueRegistry() {
27+
class OneBusAwayIssueRegistry : IssueRegistry() {
2828

2929
override val issues: List<Issue> =
3030
listOf(
3131
RawTimeDetector.ISSUE,
3232
RawTimeDetector.ISSUE_VALUE,
3333
PrematureUnwrapDetector.ISSUE,
3434
WireTimeEscapeDetector.ISSUE,
35+
SwallowedCancellationDetector.ISSUE,
3536
)
3637

3738
override val api: Int = CURRENT_API

‎lint-rules/src/main/java/org/onebusaway/lint/SwallowedCancellationDetector.kt‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/*
2+
* Copyright (C) 2026 Open Transit Software Foundation
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package org.onebusaway.lint
17+
18+
import com.android.tools.lint.detector.api.Category
19+
import com.android.tools.lint.detector.api.Detector
20+
import com.android.tools.lint.detector.api.Implementation
21+
import com.android.tools.lint.detector.api.Issue
22+
import com.android.tools.lint.detector.api.JavaContext
23+
import com.android.tools.lint.detector.api.Scope
24+
import com.android.tools.lint.detector.api.Severity
25+
import com.android.tools.lint.detector.api.SourceCodeScanner
26+
import com.intellij.psi.PsiMethod
27+
import org.jetbrains.uast.UCallExpression
28+
import org.jetbrains.uast.UMethod
29+
import org.jetbrains.uast.getParentOfType
30+
31+
/**
32+
* Gives the app a phobia of bare `runCatching` in coroutine code. `kotlin.runCatching` catches every
33+
* `Throwable`, so inside a `suspend` function it also swallows the `CancellationException` a cancelled
34+
* coroutine throws from a suspend call — turning cancellation into an ordinary `Result.failure`. The
35+
* cancelled coroutine then keeps running and its callers read the cancellation as a normal error/empty
36+
* state; structured concurrency breaks. That is the #1908/#1921 bug class, and it's invisible on the
37+
* happy path and in review.
38+
*
39+
* `CancellationException` is a plain `IllegalStateException` subtype, so there is no type-level guard —
40+
* the compiler can't tell a swallow from a legitimate catch. So this detector keys on the shape: a
41+
* `runCatching` whose **nearest enclosing named function is `suspend`** (the coroutine boundary lint can
42+
* see deterministically). The sanctioned replacement is `runCatchingCancellable` (in
43+
* `org.onebusaway.android.util`), which is behaviour-identical but rethrows `CancellationException`.
44+
*
45+
* Scope is the enclosing `suspend` function, matching detekt's `SuspendFunSwallowedCancellation`. A
46+
* `runCatching` in a coroutine-builder lambda inside a *non-suspend* function (e.g. `scope.launch { … }`)
47+
* is deliberately out of scope — the enclosing named function isn't `suspend`, so lint can't see the
48+
* boundary without heuristics; guard those by hand.
49+
*/
50+
class SwallowedCancellationDetector : Detector(), SourceCodeScanner {
51+
52+
override fun getApplicableMethodNames(): List<String> = listOf("runCatching")
53+
54+
override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: PsiMethod) {
55+
// Both the receiverless `runCatching` and the `T.runCatching` extension live in kotlin.ResultKt;
56+
// confirm the owner so a same-named function elsewhere doesn't match.
57+
if (method.containingClass?.qualifiedName != KOTLIN_RESULT_KT) return
58+
// Only inside a suspend function — that's where a CancellationException can be thrown and where
59+
// swallowing it breaks cancellation. Lambdas are skipped, so `async { runCatching { … } }` in a
60+
// suspend function is still attributed to that suspend function.
61+
val enclosing = node.getParentOfType<UMethod>() ?: return
62+
if (!enclosing.isSuspendFunction()) return
63+
context.report(
64+
ISSUE,
65+
node,
66+
context.getLocation(node),
67+
"`runCatching` in a `suspend` function also catches `CancellationException`, converting a " +
68+
"cancelled coroutine into a `Result.failure` (the cancellation stops propagating and the " +
69+
"work keeps running). Use `runCatchingCancellable` " +
70+
"(org.onebusaway.android.util), which rethrows cancellation and is otherwise identical.",
71+
)
72+
}
73+
74+
/**
75+
* True when this function carries the `suspend` modifier. A Kotlin `suspend fun` compiles with a
76+
* trailing `kotlin.coroutines.Continuation` parameter on its light (`javaPsi`) method, which is how
77+
* lint sees suspension without depending on Kotlin compiler PSI.
78+
*/
79+
private fun UMethod.isSuspendFunction(): Boolean =
80+
javaPsi.parameterList.parameters.lastOrNull()
81+
?.type?.canonicalText?.startsWith(CONTINUATION_FQN) == true
82+
83+
companion object {
84+
private const val KOTLIN_RESULT_KT = "kotlin.ResultKt"
85+
private const val CONTINUATION_FQN = "kotlin.coroutines.Continuation"
86+
87+
@JvmField
88+
val ISSUE: Issue = Issue.create(
89+
id = "SwallowedCancellation",
90+
briefDescription = "runCatching in a suspend function swallows CancellationException",
91+
explanation = """
92+
`kotlin.runCatching` catches every `Throwable`. Inside a `suspend` function that includes \
93+
the `CancellationException` a cancelled coroutine throws from a suspend call, so the \
94+
cancellation is captured into the returned `Result` instead of propagating. The cancelled \
95+
coroutine keeps running, structured concurrency breaks, and callers see the cancellation \
96+
as an ordinary failure/empty result. `CancellationException` is a plain \
97+
`IllegalStateException` subtype, so nothing at the type level catches this — it passes \
98+
compilation and review, and only misbehaves under real cancellation (screen closed, \
99+
search superseded, poll tick outrun). This is the #1908 / #1921 bug class.
100+
101+
Replace it with `runCatchingCancellable` from `org.onebusaway.android.util`: it is \
102+
behaviour-identical to `runCatching` but rethrows `CancellationException` (a real failure \
103+
still resolves to `Result.failure`). For a broad `try/catch (Exception)` in a suspend \
104+
function, add a leading `catch (e: CancellationException) { throw e }` instead — this \
105+
check covers `runCatching` only.
106+
107+
If a `suspend` function genuinely must capture cancellation (extremely rare), suppress \
108+
this id at the site with a one-line rationale and a tracking issue.
109+
""",
110+
category = Category.CORRECTNESS,
111+
priority = 6,
112+
severity = Severity.WARNING,
113+
implementation = Implementation(
114+
SwallowedCancellationDetector::class.java,
115+
Scope.JAVA_FILE_SCOPE,
116+
),
117+
)
118+
}
119+
}

‎lint-rules/src/test/java/org/onebusaway/lint/SwallowedCancellationDetectorTest.kt‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
/*
2+
* Copyright (C) 2026 Open Transit Software Foundation
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package org.onebusaway.lint
17+
18+
import com.android.tools.lint.checks.infrastructure.TestFiles.kotlin
19+
import com.android.tools.lint.checks.infrastructure.TestLintTask.lint
20+
import org.junit.Test
21+
22+
class SwallowedCancellationDetectorTest {
23+
24+
private fun lintKotlin(vararg sources: String) =
25+
lint()
26+
.files(*sources.map { kotlin(it).indented() }.toTypedArray())
27+
.issues(SwallowedCancellationDetector.ISSUE)
28+
.allowMissingSdk()
29+
.run()
30+
31+
/** The motivating case: bare `runCatching` directly in a suspend function body. */
32+
@Test
33+
fun flagsRunCatchingInSuspendFunction() {
34+
lintKotlin(
35+
"""
36+
package test
37+
suspend fun load(): Result<Int> = runCatching { fetch() }
38+
suspend fun fetch(): Int = 1
39+
""",
40+
).expectWarningCount(1)
41+
}
42+
43+
/** Nested in an `async { … }` lambda, but the enclosing named function is still suspend — flagged. */
44+
@Test
45+
fun flagsRunCatchingInLambdaInsideSuspendFunction() {
46+
lintKotlin(
47+
"""
48+
package test
49+
suspend fun load(): Result<Int> {
50+
lateinit var r: Result<Int>
51+
run { r = runCatching { fetch() } }
52+
return r
53+
}
54+
suspend fun fetch(): Int = 1
55+
""",
56+
).expectWarningCount(1)
57+
}
58+
59+
/** The sanctioned wrapper must not be flagged (different symbol). */
60+
@Test
61+
fun doesNotFlagRunCatchingCancellable() {
62+
lintKotlin(
63+
"""
64+
package test
65+
fun <T> runCatchingCancellable(block: () -> T): Result<T> = runCatching(block)
66+
suspend fun load(): Result<Int> = runCatchingCancellable { fetch() }
67+
suspend fun fetch(): Int = 1
68+
""",
69+
).expectClean()
70+
}
71+
72+
/** `runCatching` in a plain (non-suspend) function is fine — no coroutine, no cancellation. */
73+
@Test
74+
fun doesNotFlagRunCatchingInNonSuspendFunction() {
75+
lintKotlin(
76+
"""
77+
package test
78+
fun parse(s: String): Result<Int> = runCatching { s.toInt() }
79+
""",
80+
).expectClean()
81+
}
82+
83+
/** The wrapper's own body: `runCatching` inside a non-suspend inline fun is not flagged. */
84+
@Test
85+
fun doesNotFlagInsideNonSuspendInlineWrapper() {
86+
lintKotlin(
87+
"""
88+
package test
89+
inline fun <T> guard(block: () -> T): Result<T> =
90+
runCatching(block).onFailure { }
91+
""",
92+
).expectClean()
93+
}
94+
95+
/** A same-named `runCatching` that is not kotlin.runCatching must not be flagged. */
96+
@Test
97+
fun doesNotFlagUnrelatedRunCatchingFunction() {
98+
lintKotlin(
99+
"""
100+
package test
101+
class Helper { fun runCatching(block: () -> Int): Int = block() }
102+
suspend fun load(h: Helper): Int = h.runCatching { 1 }
103+
""",
104+
).expectClean()
105+
}
106+
}

‎onebusaway-android/src/main/java/org/onebusaway/android/api/data/StopsForRouteRepository.kt‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ package org.onebusaway.android.api.data
1818
import android.location.Location
1919
import javax.inject.Inject
2020
import javax.inject.Singleton
21-
import kotlinx.coroutines.CancellationException
2221
import kotlinx.coroutines.CoroutineScope
2322
import kotlinx.coroutines.Dispatchers
2423
import kotlinx.coroutines.SupervisorJob
@@ -38,6 +37,7 @@ import org.onebusaway.android.models.RouteMapStop
3837
import org.onebusaway.android.models.RouteStopGroup
3938
import org.onebusaway.android.util.PolylineDecoder
4039
import org.onebusaway.android.util.SingleFlight
40+
import org.onebusaway.android.util.runCatchingCancellable
4141
import java.io.IOException
4242

4343
/**
@@ -111,10 +111,9 @@ class DefaultStopsForRouteRepository internal constructor(
111111
val fetched = entry(routeId).getOrElse { return Result.failure(it) }
112112
?: return Result.success(null) // no endpoint — nothing to show
113113
// Decoding the route's shape polylines is the one bit of non-trivial CPU work in this layer.
114-
// runCatching swallows everything, so rethrow a CancellationException (raised by withContext if
115-
// the caller is cancelled mid-decode) rather than reporting the abandoned work as a failure.
116-
return runCatching { withContext(Dispatchers.Default) { fetched.toRouteMapData(routeId) } }
117-
.onFailure { if (it is CancellationException) throw it }
114+
// runCatchingCancellable rethrows a CancellationException (raised by withContext if the caller is
115+
// cancelled mid-decode) rather than reporting the abandoned work as a failure.
116+
return runCatchingCancellable { withContext(Dispatchers.Default) { fetched.toRouteMapData(routeId) } }
118117
}
119118

120119
/**

0 commit comments

Comments
 (0)