fix(kotlin): stop public extensions from swallowing coroutine cancellation#572
fix(kotlin): stop public extensions from swallowing coroutine cancellation#572ayaangazali wants to merge 1 commit into
Conversation
…ation Four public extension sites caught Throwable around suspending or JNI-driving work, which also traps kotlinx CancellationException and JVM Errors: - loadModel (RunAnywhereModelLifecycle.kt): the ensureServicesReady() guard ate the caller's cancellation, so a cancelled coroutine went on to run the heavy native load anyway. - transcribeStream (RunAnywhereSTT.kt): collector cancellation was converted into a synthetic "STT stream failed" final partial instead of propagating as a cancel. - synthesizeStream (RunAnywhereTTS.kt): normal cancellation was logged as a spurious "synthesizeStream errored" warning; flow teardown is unchanged since close() stays in the finally block. - downloadModel cleanup (RunAnywhereDownload.kt): no cancellation bug here (the block runs under NonCancellable and the original exception keeps propagating after the finally), but the best-effort native cancel now narrows to Exception so JVM Errors are not logged-and- ignored. Pattern matches the reviewed fix in RunanywhereAI#563: rethrow CancellationException first, then handle Exception. On the JVM CancellationException extends Exception, so the explicit rethrow clause is required, not just a Throwable-to-Exception narrowing. Gates: :compileDebugKotlin (no local natives), :ktlintCheck, :testDebugUnitTest, :detekt all green. Signed-off-by: ayaangazali <ayaangazali.work@gmail.com>
📝 WalkthroughWalkthroughKotlin SDK exception handling now preserves coroutine cancellation in model loading, STT streaming, and TTS streaming, while native download cancellation narrows its catch block from ChangesCoroutine cancellation handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdk/runanywhere-kotlin/src/main/kotlin/com/runanywhere/sdk/public/extensions/TTS/RunAnywhereTTS.kt (1)
151-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow exception handling to avoid swallowing JVM errors.
Consider replacing
catch (t: Throwable)withcatch (t: Exception)for the nativestop()call, aligning with the native download cancellation changes in this PR. Based on learnings, you should avoid catchingThrowable/Errorto ensure that critical JVMErrorsubclasses (likeOutOfMemoryError) are allowed to propagate.♻️ Proposed refactor
- } catch (t: Throwable) { + } catch (t: Exception) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/runanywhere-kotlin/src/main/kotlin/com/runanywhere/sdk/public/extensions/TTS/RunAnywhereTTS.kt` at line 151, Update the exception handler around the native TTS stop call in RunAnywhereTTS to catch Exception instead of Throwable, allowing critical JVM Error subclasses to propagate while preserving handling of ordinary operation failures.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@sdk/runanywhere-kotlin/src/main/kotlin/com/runanywhere/sdk/public/extensions/TTS/RunAnywhereTTS.kt`:
- Line 151: Update the exception handler around the native TTS stop call in
RunAnywhereTTS to catch Exception instead of Throwable, allowing critical JVM
Error subclasses to propagate while preserving handling of ordinary operation
failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fdd15c9-0e93-44a1-a24a-d9488b8b0524
📒 Files selected for processing (4)
sdk/runanywhere-kotlin/src/main/kotlin/com/runanywhere/sdk/public/extensions/Models/RunAnywhereModelLifecycle.ktsdk/runanywhere-kotlin/src/main/kotlin/com/runanywhere/sdk/public/extensions/STT/RunAnywhereSTT.ktsdk/runanywhere-kotlin/src/main/kotlin/com/runanywhere/sdk/public/extensions/Storage/RunAnywhereDownload.ktsdk/runanywhere-kotlin/src/main/kotlin/com/runanywhere/sdk/public/extensions/TTS/RunAnywhereTTS.kt
What
Follow-up to the pattern from #563: four public extension sites catch
Throwable, which also trapskotlinx.coroutines.CancellationException(and JVMErrors). This breaks Kotlin's cooperative cancellation contract, a cancelled coroutine should stop, not keep going.Sites
RunAnywhereModelLifecycle.kt:51): theensureServicesReady()guard ate the caller's cancellation, sotask.cancel()during init still ran the heavy native load afterwards. Now rethrows cancellation and swallows onlyException, same as the reviewed#563registry fix.RunAnywhereSTT.kt:112): collector cancellation got converted into a synthetic"STT stream failed"final partial. Now cancellation propagates; real errors still produce the failure partial and close the flow.RunAnywhereTTS.kt:131): normal cancellation logged a spurious"synthesizeStream errored"warning. Now rethrows; teardown is unchanged becauseclose()lives in thefinally.RunAnywhereDownload.kt:149): being honest, this one is NOT a cancellation bug. The block runs underNonCancellableand the originalCancellationExceptionkeeps propagating after thefinallyeither way. I only narrowedThrowabletoExceptionso the best-effort native cancel doesn't log-and-ignore JVMErrors. Happy to drop this hunk if you'd rather keep the change minimal.Why the two-clause pattern
On the JVM
CancellationExceptionextendsException(viajava.util.concurrent.CancellationException), so just narrowingThrowabletoExceptionwould still swallow it. The explicitcatch (e: CancellationException) { throw e }clause first is required. Import used iskotlinx.coroutines.CancellationException, matching the existing usage inNativeUnaryRequest.kt:8andToolCallingOrchestrator.kt:31.Testing
From a clean worktree of main:
:compileDebugKotlin(no local natives),:ktlintCheck,:testDebugUnitTest,:detektall pass.Small note for transparency: I'm a freshman learning by contributing here. I found these by systematically comparing the Kotlin extensions against the Swift ones after you merged my earlier registry fix, and I bounced my reasoning off Claude Code before writing anything. If any of these catches were intentionally broad, tell me and I'll adjust or revert those hunks. Thanks for the quick reviews so far, it genuinely made my week :)
Summary by CodeRabbit