From df929f65ec2960d630f8e580ad190bcd96d2e529 Mon Sep 17 00:00:00 2001 From: Bryan Chan Date: Mon, 15 Jun 2026 21:40:49 -0400 Subject: [PATCH 1/3] ADFA-4328: tolerate channel close in KeyedDebouncingAction Sentry APPDEVFORALL-14C. Cancel job before closing channel, use receiveCatching, add a CoroutineExceptionHandler to the LSP scope. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../androidide/utils/KeyedDebouncingAction.kt | 57 ++++++++++++------- .../kotlin/compiler/CompilationEnvironment.kt | 11 +++- 2 files changed, 45 insertions(+), 23 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/utils/KeyedDebouncingAction.kt b/common/src/main/java/com/itsaky/androidide/utils/KeyedDebouncingAction.kt index 637c531239..10beee9ac2 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/KeyedDebouncingAction.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/KeyedDebouncingAction.kt @@ -2,11 +2,13 @@ package com.itsaky.androidide.utils import com.itsaky.androidide.progress.ICancelChecker import com.itsaky.androidide.tasks.JobCancelChecker +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.channels.ClosedReceiveChannelException import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.ensureActive import kotlinx.coroutines.isActive @@ -30,8 +32,11 @@ class KeyedDebouncingAction( val job: Job, ) { fun cancel() { - channel.close() + // Cancel the job FIRST, then close the channel. Closing the channel first + // wakes a parked receive() with a ClosedReceiveChannelException before the + // job is cancelled, which can crash a worker that has no exception handling. job.cancel() + channel.close() } } @@ -55,33 +60,41 @@ class KeyedDebouncingAction( val channel = Channel(Channel.CONFLATED) val job = scope.launch(actionContext) { while (isActive) { - var latestKey = channel.receive() - var debouncing = true - while (debouncing) { - debouncing = select { - onTimeout(debounceDuration) { false } - channel.onReceive { newKey -> - latestKey = newKey - true + try { + var latestKey = channel.receive() + var debouncing = true + while (debouncing) { + debouncing = select { + onTimeout(debounceDuration) { false } + channel.onReceive { newKey -> + latestKey = newKey + true + } } } - } - ensureActive() - val actionJob = launch { - val cancelChecker = JobCancelChecker(currentCoroutineContext()[Job]) - action(latestKey, cancelChecker) - } + ensureActive() + val actionJob = launch { + val cancelChecker = JobCancelChecker(currentCoroutineContext()[Job]) + action(latestKey, cancelChecker) + } - select { - actionJob.onJoin {} - channel.onReceive { newerKey -> - actionJob.cancel() - channel.trySend(newerKey) + select { + actionJob.onJoin {} + channel.onReceive { newerKey -> + actionJob.cancel() + channel.trySend(newerKey) + } } - } - actionJob.join() + actionJob.join() + } catch (e: ClosedReceiveChannelException) { + // The channel was closed (entry cancelled). Stop the worker cleanly + // instead of letting the exception propagate to an uncaught handler. + break + } catch (e: CancellationException) { + throw e + } } } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt index 92c01e5d73..88eb7b3a94 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt @@ -17,6 +17,8 @@ import com.itsaky.androidide.projects.FileManager import com.itsaky.androidide.projects.api.Workspace import com.itsaky.androidide.utils.KeyedDebouncingAction import io.sentry.Sentry +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineExceptionHandler import kotlinx.coroutines.CoroutineName import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -66,7 +68,14 @@ internal class CompilationEnvironment( languageVersion: LanguageVersion = DEFAULT_LANGUAGE_VERSION, enableParserEventSystem: Boolean = true, val coroutineScope: CoroutineScope = CoroutineScope( - SupervisorJob() + CoroutineName("CompilationEnv[$name]") + SupervisorJob() + CoroutineName("CompilationEnv[$name]") + + CoroutineExceptionHandler { _, t -> + // Defense in depth: swallow (but log) non-cancellation failures from the + // debounce worker so a ClosedReceiveChannelException can never crash the app. + if (t !is CancellationException) { + logger.warn("Uncaught exception in compilation environment coroutine", t) + } + } ), ) : AbstractCompilationEnvironment( name = name, From df9039ea7e514f5bc5970ecbb06ae5bd700723d1 Mon Sep 17 00:00:00 2001 From: Bryan Chan Date: Wed, 17 Jun 2026 05:07:59 -0700 Subject: [PATCH 2/3] ADFA-4328: failing-first repro test for KeyedDebouncingAction channel-close crash Adds a :common unit test that parks a debounce worker on channel.receive(), cancels the entry, and asserts no ClosedReceiveChannelException escapes to the scope CoroutineExceptionHandler. Red on the pre-fix baseline (cancel() closed the channel before cancelling the job), green with the fix. Co-Authored-By: Claude Opus 4.8 (1M context) --- common/build.gradle.kts | 2 + .../utils/KeyedDebouncingActionCancelTest.kt | 65 +++++++++++++++++++ 2 files changed, 67 insertions(+) create mode 100644 common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionCancelTest.kt diff --git a/common/build.gradle.kts b/common/build.gradle.kts index ed723738b4..86f6715fee 100755 --- a/common/build.gradle.kts +++ b/common/build.gradle.kts @@ -43,6 +43,8 @@ dependencies { implementation(libs.monitor) testImplementation(projects.testing.common) + testImplementation(libs.tests.kotlinx.coroutines) + testImplementation(libs.tests.google.truth) androidTestImplementation(projects.testing.android) // brotli4j diff --git a/common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionCancelTest.kt b/common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionCancelTest.kt new file mode 100644 index 0000000000..9f4e090001 --- /dev/null +++ b/common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionCancelTest.kt @@ -0,0 +1,65 @@ +package com.itsaky.androidide.utils + +import com.google.common.truth.Truth.assertThat +import kotlinx.coroutines.CoroutineExceptionHandler +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.channels.ClosedReceiveChannelException +import kotlinx.coroutines.delay +import kotlinx.coroutines.runBlocking +import org.junit.Test +import java.util.concurrent.atomic.AtomicReference +import kotlin.coroutines.CoroutineContext +import kotlin.time.Duration.Companion.milliseconds + +/** + * Repro for ADFA-4328: cancelling a [KeyedDebouncingAction] entry whose worker is + * parked on `channel.receive()` must NOT let a [ClosedReceiveChannelException] + * escape to the scope's uncaught-exception handler. + * + * On the pre-fix baseline, `ActionEntry.cancel()` did `channel.close()` BEFORE + * `job.cancel()`. Closing the channel wakes the parked `receive()` with a + * [ClosedReceiveChannelException] (NOT a CancellationException), which propagates + * uncaught to the [CoroutineExceptionHandler] -> the Sentry crash this ticket fixes. + * + * The fix swaps the order (job.cancel() first) AND wraps the worker loop in a + * try/catch that swallows ClosedReceiveChannelException, so no uncaught exception fires. + */ +class KeyedDebouncingActionCancelTest { + + @Test + fun `cancelling a parked worker does not leak a ClosedReceiveChannelException`() = runBlocking { + val uncaught = AtomicReference(null) + // A plain Job (not Supervisor of the worker) + a handler that records anything + // that escapes the debounce worker coroutine. + val handler = CoroutineExceptionHandler { _, t -> uncaught.set(t) } + val scope = CoroutineScope(SupervisorJob() + handler) + + val ctx: CoroutineContext = scope.coroutineContext + + val debouncer = KeyedDebouncingAction( + scope = scope, + debounceDuration = 50.milliseconds, + actionContext = ctx, + action = { _, _ -> /* never invoked: we cancel while parked on receive */ }, + ) + + // schedule() creates the entry + launches the worker. With a CONFLATED channel and + // no further sends, the worker debounces the single key, runs the (empty) action, + // then loops back and parks on channel.receive() waiting for the next key. + debouncer.schedule("k") + + // Give the worker time to: receive "k", run the empty action, loop, and PARK on + // the next channel.receive(). 200ms >> 50ms debounce window. + delay(200) + + // Cancel the entry while the worker is parked on receive(). + debouncer.cancelPending("k") + + // Let any uncaught exception propagate to the handler. + delay(200) + + val leaked = uncaught.get() + assertThat(leaked).isNull() + } +} From 5a39efa1d991280ed9b686e1cfb06f3bf816f98d Mon Sep 17 00:00:00 2001 From: Bryan Chan Date: Fri, 19 Jun 2026 04:48:31 -0700 Subject: [PATCH 3/3] ADFA-4328: add KDoc for docstring coverage (CodeRabbit) --- .../com/itsaky/androidide/utils/KeyedDebouncingAction.kt | 5 +++++ .../androidide/utils/KeyedDebouncingActionCancelTest.kt | 1 + 2 files changed, 6 insertions(+) diff --git a/common/src/main/java/com/itsaky/androidide/utils/KeyedDebouncingAction.kt b/common/src/main/java/com/itsaky/androidide/utils/KeyedDebouncingAction.kt index 10beee9ac2..987808fe7b 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/KeyedDebouncingAction.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/KeyedDebouncingAction.kt @@ -31,6 +31,7 @@ class KeyedDebouncingAction( val channel: Channel, val job: Job, ) { + /** Cancels this entry's worker job and closes its channel, in that order. */ fun cancel() { // Cancel the job FIRST, then close the channel. Closing the channel first // wakes a parked receive() with a ClosedReceiveChannelException before the @@ -55,6 +56,10 @@ class KeyedDebouncingAction( entry.channel.trySend(key) } + /** + * Creates a new [ActionEntry]: a CONFLATED channel plus a worker coroutine that debounces + * incoming keys and runs [action] for the latest one, stopping cleanly when the channel is closed. + */ @OptIn(ExperimentalCoroutinesApi::class) private fun createEntry(): ActionEntry { val channel = Channel(Channel.CONFLATED) diff --git a/common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionCancelTest.kt b/common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionCancelTest.kt index 9f4e090001..2576d46490 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionCancelTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionCancelTest.kt @@ -27,6 +27,7 @@ import kotlin.time.Duration.Companion.milliseconds */ class KeyedDebouncingActionCancelTest { + /** Cancelling an entry whose worker is parked on receive() must not surface an uncaught exception. */ @Test fun `cancelling a parked worker does not leak a ClosedReceiveChannelException`() = runBlocking { val uncaught = AtomicReference(null)