From d8c32bd068bbec0ebe9c33cb4ba0c567154cb393 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Fri, 19 Jun 2026 22:28:53 +0530 Subject: [PATCH 1/9] fix: add global analysis lock for Kotlin analysis Signed-off-by: Akash Yadav --- .../lsp/kotlin/compiler/modules/KtFileExts.kt | 38 ++++++++-- .../kotlin/completion/KotlinCompletions.kt | 69 ++++++++++--------- 2 files changed, 68 insertions(+), 39 deletions(-) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt index bdcc53abf5..5e530badcb 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt @@ -11,14 +11,40 @@ import org.jetbrains.kotlin.psi.KtElement import org.jetbrains.kotlin.psi.KtFile import org.jetbrains.kotlin.psi.UserDataProperty import java.nio.file.Path +import java.util.concurrent.locks.ReentrantLock +import kotlin.concurrent.withLock private val KT_LSP_COMPLETION_BACKING_FILE = Key("KT_LSP_COMPLETION_BACKING_FILE") var KtFile.backingFilePath by UserDataProperty(KT_LSP_COMPLETION_BACKING_FILE) -internal inline fun analyzeMaybeDangling(useSiteElement: KtElement, crossinline action: KaSession.() -> R): R { - if (useSiteElement is KtFile && useSiteElement.isDangling && useSiteElement.copyOrigin != null) { - return analyzeCopy(useSiteElement, KaDanglingFileResolutionMode.PREFER_SELF, action) - } +/** + * Serializes all Kotlin Analysis API access (`analyze` / `analyzeCopy`). + * + * The Analysis API tracks its `analyze` lifetime context in a per-thread stack and is not safe to + * drive concurrently from multiple background threads without the platform read-action coordination + * that this LSP replaces with a custom [com.itsaky.androidide.lsp.kotlin.compiler.read] lock. + * Indexing, diagnostics and completion all run analysis on `Dispatchers.Default` and frequently + * target the same edited file, so overlapping `analyze` calls corrupted the lifetime/session + * lifecycle and surfaced as + * `KaInaccessibleLifetimeOwnerAccessException: ... Called outside an \`analyze\` context.` + * + * Holding this lock around every analysis entry point makes analyses mutually exclusive. It is a + * [ReentrantLock] so an (indirect) nested analysis on the same thread cannot deadlock. + */ +private val analysisLock = ReentrantLock() + +/** + * Runs [action] while holding the shared [analysisLock]. **All** Analysis API access must go through + * this helper (or [analyzeMaybeDangling], which already does); never call `analyze` / `analyzeCopy` + * directly, or the serialization guarantee is lost. + */ +internal inline fun withAnalysisLock(action: () -> R): R = analysisLock.withLock(action) - return analyze(useSiteElement, action) -} +internal inline fun analyzeMaybeDangling(useSiteElement: KtElement, crossinline action: KaSession.() -> R): R = + withAnalysisLock { + if (useSiteElement is KtFile && useSiteElement.isDangling && useSiteElement.copyOrigin != null) { + analyzeCopy(useSiteElement, KaDanglingFileResolutionMode.PREFER_SELF, action) + } else { + analyze(useSiteElement, action) + } + } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt index a558208e94..1058ecc330 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt @@ -3,6 +3,7 @@ package com.itsaky.androidide.lsp.kotlin.completion import com.itsaky.androidide.lookup.Lookup import com.itsaky.androidide.lsp.api.describeSnippet import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment +import com.itsaky.androidide.lsp.kotlin.compiler.modules.withAnalysisLock import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.AnalysisContext import com.itsaky.androidide.lsp.kotlin.utils.ContextKeywords @@ -149,41 +150,43 @@ internal fun doComplete(params: CompletionParams): CompletionResult { env.project.read { abortIfCancelled() - analyzeCopy( - useSiteElement = completionKtFile, - resolutionMode = KaDanglingFileResolutionMode.PREFER_SELF, - ) { - val ctx = - resolveAnalysisContext( - env = env, - file = params.file, - ktFile = completionKtFile, - offset = completionOffset, - partial = partial - ) - - if (ctx == null) { - logger.error( - "Unable to determine context at offset {} in file {}", - completionOffset, - params.file - ) - return@analyzeCopy CompletionResult.EMPTY - } - - abortIfCancelled() - context(ctx) { - val items = mutableListOf() - val completionContext = determineCompletionContext(ctx.psiElement) - when (completionContext) { - CompletionContext.Scope -> - collectScopeCompletions(to = items) - - CompletionContext.Member -> - collectMemberCompletions(to = items) + withAnalysisLock { + analyzeCopy( + useSiteElement = completionKtFile, + resolutionMode = KaDanglingFileResolutionMode.PREFER_SELF, + ) { + val ctx = + resolveAnalysisContext( + env = env, + file = params.file, + ktFile = completionKtFile, + offset = completionOffset, + partial = partial + ) + + if (ctx == null) { + logger.error( + "Unable to determine context at offset {} in file {}", + completionOffset, + params.file + ) + return@analyzeCopy CompletionResult.EMPTY } - CompletionResult(items) + abortIfCancelled() + context(ctx) { + val items = mutableListOf() + val completionContext = determineCompletionContext(ctx.psiElement) + when (completionContext) { + CompletionContext.Scope -> + collectScopeCompletions(to = items) + + CompletionContext.Member -> + collectMemberCompletions(to = items) + } + + CompletionResult(items) + } } } } From dc471d32a2557b68ca5626ff132f8d3bf513416b Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Fri, 19 Jun 2026 23:59:02 +0530 Subject: [PATCH 2/9] tests: add test case Signed-off-by: Akash Yadav --- .../modules/AnalysisSerializationTest.kt | 115 ++++++++++++++++++ 1 file changed, 115 insertions(+) create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt new file mode 100644 index 0000000000..e2ffcd966c --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt @@ -0,0 +1,115 @@ +package com.itsaky.androidide.lsp.kotlin.compiler.modules + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.lsp.kotlin.compiler.read +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking +import org.junit.Test +import java.util.Collections +import java.util.concurrent.atomic.AtomicInteger +import kotlin.math.max + +/** + * Regression tests for the `KaInaccessibleLifetimeOwnerAccessException: ... Called outside an + * `analyze` context.` reported in Sentry (APPDEVFORALL-VR / 7454434587). + * + * Root cause: indexing, diagnostics and completion all drove the stock Kotlin Analysis API + * concurrently from `Dispatchers.Default` threads (the modified-file indexer is debounced into + * independent coroutines), frequently against the same file. The Analysis API tracks its `analyze` + * lifetime context in a per-thread stack and is not safe to run concurrently without platform + * read-action coordination (which this LSP replaces with a shared read lock that does not serialize + * analysis). Overlapping `analyze` calls corrupted the lifetime/session lifecycle. + * + * Fix: [analyzeMaybeDangling] / [withAnalysisLock] hold a process-wide reentrant lock so analyses + * are mutually exclusive. + * + * Both tests fail before the fix (either by throwing the exception or by observing overlapping + * analyses) and pass after it. + */ +class AnalysisSerializationTest : KtLspTest() { + + @Test + fun `concurrent analyzeMaybeDangling never throws lifetime exception`(): Unit = runBlocking { + val files = (0 until 8).map { i -> + createSourceFile( + "Concurrent$i.kt", + """ + class Klass$i { + fun member$i(p: Int): Int = p + $i + val prop$i: String = "v$i" + } + + fun topLevel$i() = $i + """.trimIndent() + ) + } + + val errors = Collections.synchronizedList(mutableListOf()) + + // Many short, overlapping analyses on a high-parallelism dispatcher to reproduce the race. + coroutineScope { + repeat(240) { iter -> + launch(Dispatchers.IO) { + val file = files[iter % files.size] + try { + env.project.read { + analyzeMaybeDangling(file) { + // Touching declaration symbols is what triggered the lifetime check. + file.declarations.forEach { dcl -> + dcl.symbol + } + } + } + } catch (t: Throwable) { + errors.add(t) + } + } + } + } + + assertThat(errors).isEmpty() + } + + @Test + fun `analyzeMaybeDangling serializes overlapping analyses`(): Unit = runBlocking { + val files = (0 until 8).map { i -> + createSourceFile("Serialized$i.kt", "class S$i { fun f$i() = $i }") + } + + val inFlight = AtomicInteger(0) + val maxObserved = AtomicInteger(0) + val errors = Collections.synchronizedList(mutableListOf()) + + coroutineScope { + repeat(64) { iter -> + launch(Dispatchers.IO) { + val file = files[iter % files.size] + try { + env.project.read { + analyzeMaybeDangling(file) { + val concurrent = inFlight.incrementAndGet() + maxObserved.updateAndGet { max(it, concurrent) } + try { + file.declarations.forEach { it.symbol } + // Widen the window so any real overlap is observed. + Thread.sleep(2) + } finally { + inFlight.decrementAndGet() + } + } + } + } catch (t: Throwable) { + errors.add(t) + } + } + } + } + + assertThat(errors).isEmpty() + // The shared analysis lock must prevent two analyses from running at once. + assertThat(maxObserved.get()).isEqualTo(1) + } +} From 206c0c146dca4ad2bfd38310b6f639054fedf843 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 23 Jun 2026 22:20:07 +0000 Subject: [PATCH 3/9] fix: address review findings (lifetime owner escape, write-lock race, completion cleanup) Apply the actionable items from Hal's review on PR #1428: - Diagnostics: extract the unresolved-reference name inside the analyze block instead of storing the live KaDiagnosticWithPsi (a KaLifetimeOwner) in DiagnosticItem.extra. AddImportAction now reads the pre-extracted string, preventing KaInaccessibleLifetimeOwnerAccessException from the quick-fix path. - CompilationEnvironment.notifyElementModifiedForPath: run handleElementModification inside project.write so the session mutation can't race a concurrent analyze (mirrors onFileContentChanged). - KotlinCompletions: collapse manual withAnalysisLock + analyzeCopy into analyzeMaybeDangling, removing the only in-prod direct analyzeCopy call. - KtFileExts: document that code under withAnalysisLock must not call project.write (non-upgradeable RW lock footgun). --- .../lsp/kotlin/actions/AddImportAction.kt | 12 ++-- .../kotlin/compiler/CompilationEnvironment.kt | 14 ++-- .../lsp/kotlin/compiler/modules/KtFileExts.kt | 6 ++ .../kotlin/completion/KotlinCompletions.kt | 69 +++++++++---------- .../diagnostic/KotlinDiagnosticProvider.kt | 16 ++++- 5 files changed, 64 insertions(+), 53 deletions(-) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt index 7d948477f7..3ca3e33683 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt @@ -18,7 +18,6 @@ import com.itsaky.androidide.lsp.models.DocumentChange import com.itsaky.androidide.lsp.models.TextEdit import com.itsaky.androidide.resources.R import org.appdevforall.codeonthego.indexing.jvm.JvmSymbol -import org.jetbrains.kotlin.analysis.api.fir.diagnostics.KaFirDiagnostic import org.slf4j.LoggerFactory class AddImportAction : BaseKotlinCodeAction() { @@ -46,14 +45,13 @@ class AddImportAction : BaseKotlinCodeAction() { return } - val diagnostic = extra.diagnostic as? KaFirDiagnostic.UnresolvedReference? - if (diagnostic == null) { + val reference = extra.unresolvedReference + if (reference == null) { markInvisible() return } val env = extra.compilationEnv - val reference = diagnostic.reference val hasImportableSymbols = env.ktSymbolIndex .findSymbolBySimpleName(reference, limit = 0) .any { it.kind.isClassifier } @@ -65,10 +63,10 @@ class AddImportAction : BaseKotlinCodeAction() { } override suspend fun execAction(data: ActionData): Map> { - val (diagnostic, env) = data.require().extra as? KotlinDiagnosticExtra + val (reference, env) = data.require().extra as? KotlinDiagnosticExtra ?: return emptyMap() - diagnostic as KaFirDiagnostic.UnresolvedReference + if (reference == null) return emptyMap() val file = data.requireFile() val nioPath = file.toPath() @@ -77,7 +75,7 @@ class AddImportAction : BaseKotlinCodeAction() { ?: return emptyMap() return env.ktSymbolIndex - .findSymbolBySimpleName(diagnostic.reference, limit = 0) + .findSymbolBySimpleName(reference, limit = 0) .filter { it.kind.isClassifier } .associateWith { symbol -> insertImport(ktFile, symbol.fqName) } } 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..fb3f2bd225 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 @@ -208,22 +208,24 @@ internal class CompilationEnvironment( @OptIn(KaImplementationDetail::class) private inline fun notifyElementModifiedForPath( path: Path, - typeProvider: (KtFile) -> KaElementModificationType, + crossinline typeProvider: (KtFile) -> KaElementModificationType, ) { val structureProvider = ProjectStructureProvider.getInstance(project) val ktFile = path.toVirtualFileOrNull()?.let { psiManager.findFile(it) as? KtFile } - if (ktFile != null) { - KaSourceModificationService.getInstance(project) - .handleElementModification(ktFile, typeProvider(ktFile)) - } - val module = (ktFile?.let { structureProvider.getModule(it, null) } ?: structureProvider.findModuleForSourceId(path.pathString)) as? AbstractKtModule project.write { + // Must run under the write lock so the session mutation can't race a concurrent + // `analyze` (which only holds the read lock); see onFileContentChanged. + if (ktFile != null) { + KaSourceModificationService.getInstance(project) + .handleElementModification(ktFile, typeProvider(ktFile)) + } + if (module != null) { module.invalidateSearchScope() project.publishModificationEvent( diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt index 5e530badcb..db20d93e91 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt @@ -30,6 +30,12 @@ var KtFile.backingFilePath by UserDataProperty(KT_LSP_COMPLETION_BACKING_FILE) * * Holding this lock around every analysis entry point makes analyses mutually exclusive. It is a * [ReentrantLock] so an (indirect) nested analysis on the same thread cannot deadlock. + * + * **Footgun:** analysis runs under the *read* (shared) side of the global + * [com.itsaky.androidide.lsp.kotlin.compiler.read] lock, and that `ReentrantReadWriteLock` is + * non-upgradeable. Code running inside [withAnalysisLock] / an `analyze` block must therefore never + * call [com.itsaky.androidide.lsp.kotlin.compiler.write] — upgrading read → write on the same thread + * deadlocks. */ private val analysisLock = ReentrantLock() diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt index 1058ecc330..d7e012f2ff 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt @@ -3,7 +3,7 @@ package com.itsaky.androidide.lsp.kotlin.completion import com.itsaky.androidide.lookup.Lookup import com.itsaky.androidide.lsp.api.describeSnippet import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment -import com.itsaky.androidide.lsp.kotlin.compiler.modules.withAnalysisLock +import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.AnalysisContext import com.itsaky.androidide.lsp.kotlin.utils.ContextKeywords @@ -31,8 +31,6 @@ import org.jetbrains.kotlin.analysis.api.KaContextParameterApi import org.jetbrains.kotlin.analysis.api.KaExperimentalApi import org.jetbrains.kotlin.analysis.api.KaIdeApi import org.jetbrains.kotlin.analysis.api.KaSession -import org.jetbrains.kotlin.analysis.api.analyzeCopy -import org.jetbrains.kotlin.analysis.api.projectStructure.KaDanglingFileResolutionMode import org.jetbrains.kotlin.analysis.api.renderer.types.KaTypeRenderer import org.jetbrains.kotlin.analysis.api.renderer.types.impl.KaTypeRendererForSource import org.jetbrains.kotlin.analysis.api.symbols.KaCallableSymbol @@ -150,43 +148,38 @@ internal fun doComplete(params: CompletionParams): CompletionResult { env.project.read { abortIfCancelled() - withAnalysisLock { - analyzeCopy( - useSiteElement = completionKtFile, - resolutionMode = KaDanglingFileResolutionMode.PREFER_SELF, - ) { - val ctx = - resolveAnalysisContext( - env = env, - file = params.file, - ktFile = completionKtFile, - offset = completionOffset, - partial = partial - ) - - if (ctx == null) { - logger.error( - "Unable to determine context at offset {} in file {}", - completionOffset, - params.file - ) - return@analyzeCopy CompletionResult.EMPTY - } - - abortIfCancelled() - context(ctx) { - val items = mutableListOf() - val completionContext = determineCompletionContext(ctx.psiElement) - when (completionContext) { - CompletionContext.Scope -> - collectScopeCompletions(to = items) - - CompletionContext.Member -> - collectMemberCompletions(to = items) - } + analyzeMaybeDangling(completionKtFile) { + val ctx = + resolveAnalysisContext( + env = env, + file = params.file, + ktFile = completionKtFile, + offset = completionOffset, + partial = partial + ) + + if (ctx == null) { + logger.error( + "Unable to determine context at offset {} in file {}", + completionOffset, + params.file + ) + return@analyzeMaybeDangling CompletionResult.EMPTY + } - CompletionResult(items) + abortIfCancelled() + context(ctx) { + val items = mutableListOf() + val completionContext = determineCompletionContext(ctx.psiElement) + when (completionContext) { + CompletionContext.Scope -> + collectScopeCompletions(to = items) + + CompletionContext.Member -> + collectMemberCompletions(to = items) } + + CompletionResult(items) } } } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt index e529d029df..f9304a9230 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt @@ -13,6 +13,7 @@ import org.jetbrains.kotlin.analysis.api.KaExperimentalApi import org.jetbrains.kotlin.analysis.api.components.KaDiagnosticCheckerFilter import org.jetbrains.kotlin.analysis.api.diagnostics.KaDiagnosticWithPsi import org.jetbrains.kotlin.analysis.api.diagnostics.KaSeverity +import org.jetbrains.kotlin.analysis.api.fir.diagnostics.KaFirDiagnostic import org.jetbrains.kotlin.com.intellij.openapi.util.TextRange import org.jetbrains.kotlin.com.intellij.psi.PsiErrorElement import org.jetbrains.kotlin.com.intellij.psi.PsiFile @@ -23,7 +24,14 @@ import java.nio.file.Path private val logger = LoggerFactory.getLogger("KotlinDiagnosticProvider") internal data class KotlinDiagnosticExtra( - val diagnostic: KaDiagnosticWithPsi<*>, + /** + * The unresolved-reference name extracted from an [KaFirDiagnostic.UnresolvedReference] + * diagnostic, or `null` for any other diagnostic. This is plain data extracted *inside* the + * `analyze` block on purpose: storing the [KaDiagnosticWithPsi] (a `KaLifetimeOwner`) here and + * reading its members later from a code action would access it outside an `analyze` context and + * crash with `KaInaccessibleLifetimeOwnerAccessException`. + */ + val unresolvedReference: String?, val compilationEnv: CompilationEnvironment, ) @@ -79,8 +87,12 @@ private fun doAnalyze(file: Path, cancelChecker: ICancelChecker): DiagnosticResu ktFile.collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) .forEach { diagnostic -> cancelChecker.abortIfCancelled() + // Extract plain data while still inside the analyze context; never let + // the KaLifetimeOwner diagnostic escape (see KotlinDiagnosticExtra). + val unresolvedReference = + (diagnostic as? KaFirDiagnostic.UnresolvedReference)?.reference add(diagnostic.toDiagnosticItem().apply { - extra = KotlinDiagnosticExtra(diagnostic, env) + extra = KotlinDiagnosticExtra(unresolvedReference, env) }) } } From f281d0e80644d42d4f2e6496babd2b10e1764491 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 23 Jun 2026 22:45:44 +0000 Subject: [PATCH 4/9] feat(ADFA-4174): priority-aware preemptive analysis scheduler Replace the FIFO analysis lock with AnalysisScheduler, a process-global, priority-aware, preemptive, reentrant lock that serializes all Kotlin Analysis API access while letting interactive work win. Priority order: Completion > Diagnostics > Indexing. - A higher-priority request preempts a strictly lower-priority in-progress analysis. Preemption is cooperative (the Analysis API can't be interrupted mid-analyze): the holder's ScheduledCancelChecker is flagged and the running analysis bails at its next abortIfCancelled() checkpoint with AnalysisPreemptedException. - A lower-priority request waits while a higher-priority one holds. - Preempted diagnostics/indexing work is auto-rescheduled so it still completes: diagnostics re-schedules via the fileAnalyzer; indexing re-queues the command. - Indexing is now actively preemptible (was ICancelChecker.NOOP). Wiring: - KotlinCompletions: COMPLETION priority, request-scoped checker from Lookup. - KotlinDiagnosticProvider: DIAGNOSTICS priority; CompilationEnvironment's fileAnalyzer catches AnalysisPreemptedException to re-schedule. - IndexWorker/SourceFileIndexer: INDEXING priority; re-queue on preemption. Tests: extend AnalysisSerializationTest with reentrancy, higher-preempts-lower, and lower-waits-for-higher cases (all 5 tests pass). --- .../kotlin/compiler/CompilationEnvironment.kt | 14 +- .../lsp/kotlin/compiler/index/IndexWorker.kt | 36 ++-- .../compiler/index/SourceFileIndexer.kt | 18 +- .../compiler/modules/AnalysisScheduler.kt | 159 ++++++++++++++++++ .../lsp/kotlin/compiler/modules/KtFileExts.kt | 41 +++-- .../kotlin/completion/KotlinCompletions.kt | 10 +- .../diagnostic/KotlinDiagnosticProvider.kt | 14 +- .../modules/AnalysisSerializationTest.kt | 110 +++++++++++- 8 files changed, 359 insertions(+), 43 deletions(-) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt 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 fb3f2bd225..a9a0eb89d0 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 @@ -3,6 +3,7 @@ package com.itsaky.androidide.lsp.kotlin.compiler import com.itsaky.androidide.lsp.api.ILanguageClient import com.itsaky.androidide.lsp.kotlin.compiler.index.KtSymbolIndex import com.itsaky.androidide.lsp.kotlin.compiler.modules.AbstractKtModule +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPreemptedException import com.itsaky.androidide.lsp.kotlin.compiler.modules.KtModule import com.itsaky.androidide.lsp.kotlin.compiler.modules.asFlatSequence import com.itsaky.androidide.lsp.kotlin.compiler.modules.backingFilePath @@ -166,9 +167,16 @@ internal class CompilationEnvironment( scope = coroutineScope, debounceDuration = DEFAULT_FILE_MOD_EVENT_DEBOUNCE_DURATION, ) { path, cancelChecker -> - val result = collectDiagnosticsFor(path, cancelChecker) - withContext(Dispatchers.Main.immediate) { - languageClient?.publishDiagnostics(result) + try { + val result = collectDiagnosticsFor(path, cancelChecker) + withContext(Dispatchers.Main.immediate) { + languageClient?.publishDiagnostics(result) + } + } catch (e: AnalysisPreemptedException) { + // A higher-priority analysis (completion) preempted this diagnostics run. + // Re-schedule so diagnostics still run once the higher-priority work finishes. + logger.debug("diagnostics for {} preempted; rescheduling", path) + fileAnalyzer.schedule(path) } } } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.kt index e27bd84f94..556912f4c9 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.kt @@ -1,6 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.compiler.index import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPreemptedException import com.itsaky.androidide.lsp.kotlin.compiler.modules.backingFilePath import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.progress.ICancelChecker @@ -8,6 +9,7 @@ import com.itsaky.androidide.utils.KeyedDebouncingAction import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.isActive +import kotlinx.coroutines.launch import org.appdevforall.codeonthego.indexing.jvm.JvmSymbolIndex import org.appdevforall.codeonthego.indexing.jvm.KtFileMetadata import org.appdevforall.codeonthego.indexing.jvm.KtFileMetadataIndex @@ -54,8 +56,14 @@ internal class IndexWorker( debounceDuration = CompilationEnvironment.DEFAULT_FILE_MOD_EVENT_DEBOUNCE_DURATION ) { (path, ktFile), cancelChecker -> logger.debug("Indexing modified file: {}", path) - indexSourceFile(project, ktFile, fileIndex, sourceIndex, cancelChecker) - sourceIndexCount++ + try { + indexSourceFile(project, ktFile, fileIndex, sourceIndex, cancelChecker) + sourceIndexCount++ + } catch (e: AnalysisPreemptedException) { + // Preempted by higher-priority analysis; re-queue so the edit still gets indexed. + logger.debug("Indexing of modified file {} preempted; re-queueing", path) + scope.launch { submitCommand(IndexCommand.IndexModifiedFile(ktFile)) } + } } while (isActive) { @@ -82,15 +90,23 @@ internal class IndexWorker( continue } - indexSourceFile( - project = project, - ktFile = ktFile, - fileIndex = fileIndex, - symbolsIndex = sourceIndex, - cancelChecker = ICancelChecker.NOOP - ) + try { + indexSourceFile( + project = project, + ktFile = ktFile, + fileIndex = fileIndex, + symbolsIndex = sourceIndex, + // A real (cancellable) checker so the scheduler can preempt this pass + // in favour of completion/diagnostics. + cancelChecker = ICancelChecker.Default() + ) - sourceIndexCount++ + sourceIndexCount++ + } catch (e: AnalysisPreemptedException) { + // Preempted by higher-priority analysis; re-queue so the file still gets indexed. + logger.debug("Indexing of {} preempted; re-queueing", cmd.vf.path) + scope.launch { submitCommand(cmd) } + } } is IndexCommand.IndexModifiedFile -> { diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.kt index 6d38dee820..11b73381e1 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.kt @@ -1,5 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.compiler.index +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.modules.backingFilePath import com.itsaky.androidide.lsp.kotlin.compiler.read @@ -74,9 +76,15 @@ internal suspend fun indexSourceFile( symbolsIndex: JvmSymbolIndex, cancelChecker: ICancelChecker, ) { + // Indexing runs at the lowest (INDEXING) priority: it yields to both completion and diagnostics. + // Wrapping the checker lets the scheduler preempt an in-progress index pass; the preemption + // surfaces as AnalysisPreemptedException at the abortIfCancelled() checkpoints below, which the + // IndexWorker catches to re-queue the file. + val checker = cancelChecker as? ScheduledCancelChecker ?: ScheduledCancelChecker(cancelChecker) + val newFile = ktFile.toMetadata(project, isIndexed = true) val existingFile = fileIndex.get(newFile.filePath) - cancelChecker.abortIfCancelled() + checker.abortIfCancelled() if (KtFileMetadata.shouldBeSkipped(existingFile, newFile) && existingFile?.isIndexed == true) { return @@ -85,18 +93,18 @@ internal suspend fun indexSourceFile( // Remove stale symbols written during the previous indexing pass. if (existingFile?.isIndexed == true) { symbolsIndex.removeBySource(newFile.filePath) - cancelChecker.abortIfCancelled() + checker.abortIfCancelled() } val symbols = project.read { val list = mutableListOf() - analyzeMaybeDangling(ktFile) { + analyzeMaybeDangling(ktFile, AnalysisPriority.INDEXING, checker) { val session = this ktFile.accept(object : KtTreeVisitorVoid() { override fun visitDeclaration(dcl: KtDeclaration) { - cancelChecker.abortIfCancelled() + checker.abortIfCancelled() val symbol = with(session) { analyzeDeclaration(newFile.filePath, dcl) } - cancelChecker.abortIfCancelled() + checker.abortIfCancelled() symbol?.let { list.add(it) } super.visitDeclaration(dcl) } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt new file mode 100644 index 0000000000..42b7ba9d96 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt @@ -0,0 +1,159 @@ +package com.itsaky.androidide.lsp.kotlin.compiler.modules + +import com.itsaky.androidide.progress.ICancelChecker +import java.util.concurrent.CancellationException +import java.util.concurrent.locks.ReentrantLock +import kotlin.concurrent.withLock + +/** + * Priority of an Analysis API request. Higher [ordinal] wins: a request can preempt any strictly + * lower-priority analysis that is currently running, and is served before any lower-priority request + * that is merely waiting. + * + * Order: [INDEXING] < [DIAGNOSTICS] < [COMPLETION] — interactive completion beats background + * diagnostics, which beats bulk indexing. + */ +internal enum class AnalysisPriority { + INDEXING, + DIAGNOSTICS, + COMPLETION, +} + +/** + * Thrown at an `abortIfCancelled()` checkpoint when the running analysis has been preempted by a + * higher-priority request (see [AnalysisScheduler]). It is a [CancellationException] so it unwinds + * cleanly through the existing cancellation-aware `catch` blocks; callers that want the preempted work + * to run later catch this specific type and re-schedule it. + */ +internal class AnalysisPreemptedException : + CancellationException("analysis preempted by a higher-priority request") + +/** + * An [ICancelChecker] that adds a cooperative *preemption* signal on top of an existing [delegate] + * checker. The Analysis API cannot be interrupted mid-`analyze` (it runs with a no-op + * `ProgressManager`), so [AnalysisScheduler] flags preemption here and the running analysis notices it + * at its next [abortIfCancelled] checkpoint. + * + * Preemption is distinct from ordinary cancellation: [abortIfCancelled] throws + * [AnalysisPreemptedException] (so the source can re-schedule the work) while still honouring the + * delegate's own cancellation (e.g. a superseding edit or a closed file). + */ +internal class ScheduledCancelChecker( + private val delegate: ICancelChecker, +) : ICancelChecker { + + @Volatile + private var preempted = false + + /** Marks this analysis as preempted; the next [abortIfCancelled] will throw. */ + fun preempt() { + preempted = true + } + + override fun cancel() { + delegate.cancel() + } + + override fun isCancelled(): Boolean = preempted || delegate.isCancelled() + + override fun abortIfCancelled() { + if (preempted) { + throw AnalysisPreemptedException() + } + delegate.abortIfCancelled() + } +} + +/** + * A process-global, priority-aware, preemptive lock that serializes all Kotlin Analysis API access. + * + * It replaces the plain FIFO lock that previously guarded `analyze` / `analyzeCopy`. Semantics: + * - only one analysis runs at a time (the Analysis API is not safe to drive concurrently); + * - a higher-priority requester **preempts** a strictly lower-priority holder by invoking its + * `onPreempt` callback once (cooperative — the holder bails at its next `abortIfCancelled()`); + * - when the lock frees, the highest-priority waiter acquires it next; + * - it is **reentrant**: a nested analysis on the same thread re-enters without deadlocking. + * + * Access it through [withAnalysisLock] / [analyzeMaybeDangling] rather than directly. + */ +internal object AnalysisScheduler { + + private val mutex = ReentrantLock() + private val available = mutex.newCondition() + + private var holderThread: Thread? = null + private var holderPriority: AnalysisPriority? = null + private var holderReentry = 0 + private var holderPreempted = false + private var holderPreempt: (() -> Unit)? = null + + /** Number of threads currently waiting to acquire, per priority. */ + private val waiting = IntArray(AnalysisPriority.entries.size) + + /** + * Acquire the analysis lock at the given [priority]. Blocks until the current thread may run. If a + * strictly lower-priority analysis is in progress, [onPreempt] of *that* holder is invoked so it + * yields; [onPreempt] passed here is stored and used if this acquisition is later preempted. + */ + fun acquire(priority: AnalysisPriority, onPreempt: () -> Unit) { + mutex.withLock { + val me = Thread.currentThread() + if (holderThread === me) { + // Reentrant: nested analysis on the same thread shares the outer hold. + holderReentry++ + return + } + + waiting[priority.ordinal]++ + try { + while (true) { + val hp = holderPriority + if (holderThread != null && hp != null && + hp.ordinal < priority.ordinal && !holderPreempted + ) { + // Signal the lower-priority holder to bail (once). + holderPreempted = true + holderPreempt?.invoke() + } + + if (holderThread == null && !higherPriorityWaiting(priority)) { + break + } + available.await() + } + } finally { + waiting[priority.ordinal]-- + } + + holderThread = me + holderPriority = priority + holderPreempt = onPreempt + holderPreempted = false + holderReentry = 1 + } + } + + /** Release a hold acquired via [acquire]. Wakes waiters when the outermost hold is released. */ + fun release() { + mutex.withLock { + if (holderThread !== Thread.currentThread()) { + return + } + if (--holderReentry > 0) { + return + } + holderThread = null + holderPriority = null + holderPreempt = null + holderPreempted = false + available.signalAll() + } + } + + private fun higherPriorityWaiting(priority: AnalysisPriority): Boolean { + for (i in priority.ordinal + 1 until waiting.size) { + if (waiting[i] > 0) return true + } + return false + } +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt index db20d93e91..260e99eb0e 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt @@ -11,14 +11,12 @@ import org.jetbrains.kotlin.psi.KtElement import org.jetbrains.kotlin.psi.KtFile import org.jetbrains.kotlin.psi.UserDataProperty import java.nio.file.Path -import java.util.concurrent.locks.ReentrantLock -import kotlin.concurrent.withLock private val KT_LSP_COMPLETION_BACKING_FILE = Key("KT_LSP_COMPLETION_BACKING_FILE") var KtFile.backingFilePath by UserDataProperty(KT_LSP_COMPLETION_BACKING_FILE) /** - * Serializes all Kotlin Analysis API access (`analyze` / `analyzeCopy`). + * Runs [action] while holding the global analysis lock at the given [priority]. * * The Analysis API tracks its `analyze` lifetime context in a per-thread stack and is not safe to * drive concurrently from multiple background threads without the platform read-action coordination @@ -28,8 +26,12 @@ var KtFile.backingFilePath by UserDataProperty(KT_LSP_COMPLETION_BACKING_FILE) * lifecycle and surfaced as * `KaInaccessibleLifetimeOwnerAccessException: ... Called outside an \`analyze\` context.` * - * Holding this lock around every analysis entry point makes analyses mutually exclusive. It is a - * [ReentrantLock] so an (indirect) nested analysis on the same thread cannot deadlock. + * Serialization is handled by [AnalysisScheduler], which is priority-aware and preemptive: a + * higher-priority request preempts a strictly lower-priority one (cooperatively, via [cancelChecker]). + * The scheduler is reentrant, so an (indirect) nested analysis on the same thread cannot deadlock. + * + * **All** Analysis API access must go through this helper (or [analyzeMaybeDangling], which already + * does); never call `analyze` / `analyzeCopy` directly, or the serialization guarantee is lost. * * **Footgun:** analysis runs under the *read* (shared) side of the global * [com.itsaky.androidide.lsp.kotlin.compiler.read] lock, and that `ReentrantReadWriteLock` is @@ -37,17 +39,26 @@ var KtFile.backingFilePath by UserDataProperty(KT_LSP_COMPLETION_BACKING_FILE) * call [com.itsaky.androidide.lsp.kotlin.compiler.write] — upgrading read → write on the same thread * deadlocks. */ -private val analysisLock = ReentrantLock() - -/** - * Runs [action] while holding the shared [analysisLock]. **All** Analysis API access must go through - * this helper (or [analyzeMaybeDangling], which already does); never call `analyze` / `analyzeCopy` - * directly, or the serialization guarantee is lost. - */ -internal inline fun withAnalysisLock(action: () -> R): R = analysisLock.withLock(action) +internal inline fun withAnalysisLock( + priority: AnalysisPriority, + cancelChecker: ScheduledCancelChecker, + action: () -> R, +): R { + AnalysisScheduler.acquire(priority, cancelChecker::preempt) + try { + return action() + } finally { + AnalysisScheduler.release() + } +} -internal inline fun analyzeMaybeDangling(useSiteElement: KtElement, crossinline action: KaSession.() -> R): R = - withAnalysisLock { +internal inline fun analyzeMaybeDangling( + useSiteElement: KtElement, + priority: AnalysisPriority, + cancelChecker: ScheduledCancelChecker, + crossinline action: KaSession.() -> R, +): R = + withAnalysisLock(priority, cancelChecker) { if (useSiteElement is KtFile && useSiteElement.isDangling && useSiteElement.copyOrigin != null) { analyzeCopy(useSiteElement, KaDanglingFileResolutionMode.PREFER_SELF, action) } else { diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt index d7e012f2ff..c8617c4732 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt @@ -3,6 +3,8 @@ package com.itsaky.androidide.lsp.kotlin.completion import com.itsaky.androidide.lookup.Lookup import com.itsaky.androidide.lsp.api.describeSnippet import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.AnalysisContext @@ -144,11 +146,17 @@ internal fun doComplete(params: CompletionParams): CompletionResult { abortIfCancelled() + // Completion is the highest-priority analysis: it preempts in-progress diagnostics/indexing and + // is never itself preempted. The cancel checker is the editor's request-scoped one (from Lookup). + val cancelChecker = ScheduledCancelChecker( + Lookup.getDefault().lookup(ICancelChecker::class.java) ?: ICancelChecker.NOOP + ) + return try { env.project.read { abortIfCancelled() - analyzeMaybeDangling(completionKtFile) { + analyzeMaybeDangling(completionKtFile, AnalysisPriority.COMPLETION, cancelChecker) { val ctx = resolveAnalysisContext( env = env, diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt index f9304a9230..2a14070ed7 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt @@ -1,6 +1,8 @@ package com.itsaky.androidide.lsp.kotlin.diagnostic import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.toRange @@ -64,11 +66,17 @@ private fun doAnalyze(file: Path, cancelChecker: ICancelChecker): DiagnosticResu return DiagnosticResult.NO_UPDATE } + // Diagnostics run at DIAGNOSTICS priority: they yield to completion but preempt indexing. The + // wrapped checker turns a scheduler preemption into an AnalysisPreemptedException, which + // CompilationEnvironment's fileAnalyzer catches to re-schedule this run after the higher-priority + // work finishes. + val checker = ScheduledCancelChecker(cancelChecker) + val diagnostics = env.project.read { buildList { PsiTreeUtil.collectElementsOfType(ktFile, PsiErrorElement::class.java) .forEach { errorElement -> - cancelChecker.abortIfCancelled() + checker.abortIfCancelled() add( diagnosticItem( file = ktFile, @@ -83,10 +91,10 @@ private fun doAnalyze(file: Path, cancelChecker: ICancelChecker): DiagnosticResu // The analysis API uses a no-op implementation of // Intellij's ProgressManager for cancellations, so the following // isn't really cancellable at the moment - analyzeMaybeDangling(ktFile) { + analyzeMaybeDangling(ktFile, AnalysisPriority.DIAGNOSTICS, checker) { ktFile.collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) .forEach { diagnostic -> - cancelChecker.abortIfCancelled() + checker.abortIfCancelled() // Extract plain data while still inside the analyze context; never let // the KaLifetimeOwner diagnostic escape (see KotlinDiagnosticExtra). val unresolvedReference = diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt index e2ffcd966c..0cea853185 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt @@ -3,12 +3,16 @@ package com.itsaky.androidide.lsp.kotlin.compiler.modules import com.google.common.truth.Truth.assertThat import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.progress.ICancelChecker import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import org.junit.Test import java.util.Collections +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicInteger import kotlin.math.max @@ -23,11 +27,12 @@ import kotlin.math.max * read-action coordination (which this LSP replaces with a shared read lock that does not serialize * analysis). Overlapping `analyze` calls corrupted the lifetime/session lifecycle. * - * Fix: [analyzeMaybeDangling] / [withAnalysisLock] hold a process-wide reentrant lock so analyses - * are mutually exclusive. + * Fix: [analyzeMaybeDangling] / [withAnalysisLock] route through [AnalysisScheduler], a process-wide + * priority-aware, preemptive, reentrant lock so analyses are mutually exclusive. * - * Both tests fail before the fix (either by throwing the exception or by observing overlapping - * analyses) and pass after it. + * The first two tests cover serialization (they fail before the fix, either by throwing the exception + * or by observing overlapping analyses). The remaining tests cover the scheduler's priority, + * preemption, and reentrancy behaviour. */ class AnalysisSerializationTest : KtLspTest() { @@ -56,7 +61,11 @@ class AnalysisSerializationTest : KtLspTest() { val file = files[iter % files.size] try { env.project.read { - analyzeMaybeDangling(file) { + analyzeMaybeDangling( + file, + AnalysisPriority.DIAGNOSTICS, + ScheduledCancelChecker(ICancelChecker.NOOP), + ) { // Touching declaration symbols is what triggered the lifetime check. file.declarations.forEach { dcl -> dcl.symbol @@ -89,7 +98,11 @@ class AnalysisSerializationTest : KtLspTest() { val file = files[iter % files.size] try { env.project.read { - analyzeMaybeDangling(file) { + analyzeMaybeDangling( + file, + AnalysisPriority.DIAGNOSTICS, + ScheduledCancelChecker(ICancelChecker.NOOP), + ) { val concurrent = inFlight.incrementAndGet() maxObserved.updateAndGet { max(it, concurrent) } try { @@ -112,4 +125,89 @@ class AnalysisSerializationTest : KtLspTest() { // The shared analysis lock must prevent two analyses from running at once. assertThat(maxObserved.get()).isEqualTo(1) } + + @Test(timeout = 10_000) + fun `reentrant withAnalysisLock on the same thread does not deadlock`() { + var innerRan = false + withAnalysisLock(AnalysisPriority.DIAGNOSTICS, ScheduledCancelChecker(ICancelChecker.NOOP)) { + withAnalysisLock(AnalysisPriority.COMPLETION, ScheduledCancelChecker(ICancelChecker.NOOP)) { + innerRan = true + } + } + assertThat(innerRan).isTrue() + } + + @Test(timeout = 10_000) + fun `higher priority request preempts a lower priority holder`() { + val holderChecker = ScheduledCancelChecker(ICancelChecker.NOOP) + val holding = CountDownLatch(1) + val preempted = AtomicBoolean(false) + val higherRan = AtomicBoolean(false) + + // Low-priority (indexing) holder runs a long, cooperatively-cancellable analysis. + val lower = Thread { + try { + withAnalysisLock(AnalysisPriority.INDEXING, holderChecker) { + holding.countDown() + repeat(2_000) { + holderChecker.abortIfCancelled() + Thread.sleep(5) + } + } + } catch (e: AnalysisPreemptedException) { + preempted.set(true) + } + } + lower.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + // A completion request must preempt the in-progress indexing. + val higher = Thread { + withAnalysisLock(AnalysisPriority.COMPLETION, ScheduledCancelChecker(ICancelChecker.NOOP)) { + higherRan.set(true) + } + } + higher.start() + higher.join(5_000) + lower.join(5_000) + + assertThat(preempted.get()).isTrue() + assertThat(higherRan.get()).isTrue() + } + + @Test(timeout = 10_000) + fun `lower priority request waits while a higher priority holder runs`() { + val holding = CountDownLatch(1) + val release = CountDownLatch(1) + val lowerEntered = AtomicBoolean(false) + + // High-priority (completion) holder holds the lock until released. + val higher = Thread { + withAnalysisLock(AnalysisPriority.COMPLETION, ScheduledCancelChecker(ICancelChecker.NOOP)) { + holding.countDown() + release.await() + } + } + higher.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + // A diagnostics request is strictly lower priority: it must not preempt completion. + val lower = Thread { + withAnalysisLock(AnalysisPriority.DIAGNOSTICS, ScheduledCancelChecker(ICancelChecker.NOOP)) { + lowerEntered.set(true) + } + } + lower.start() + + // Give the lower-priority request time to (incorrectly) barge in. + Thread.sleep(300) + val enteredWhileHeld = lowerEntered.get() + + release.countDown() + higher.join(5_000) + lower.join(5_000) + + assertThat(enteredWhileHeld).isFalse() + assertThat(lowerEntered.get()).isTrue() + } } From c9349b0ce850942369971f87a79f5c8e2d8a9cd8 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Thu, 2 Jul 2026 11:31:07 +0000 Subject: [PATCH 5/9] fix: integrate cancel checker with ProgressManager --- .../compiler/modules/AnalysisScheduler.kt | 7 +-- .../modules/CancelCheckerProgressIndicator.kt | 37 ++++++++++++++++ .../lsp/kotlin/compiler/modules/KtFileExts.kt | 38 ++++++++++++++-- .../diagnostic/KotlinDiagnosticProvider.kt | 7 ++- .../modules/AnalysisSerializationTest.kt | 44 +++++++++++++++++++ 5 files changed, 123 insertions(+), 10 deletions(-) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/CancelCheckerProgressIndicator.kt diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt index 42b7ba9d96..219a1ebc9b 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt @@ -30,9 +30,10 @@ internal class AnalysisPreemptedException : /** * An [ICancelChecker] that adds a cooperative *preemption* signal on top of an existing [delegate] - * checker. The Analysis API cannot be interrupted mid-`analyze` (it runs with a no-op - * `ProgressManager`), so [AnalysisScheduler] flags preemption here and the running analysis notices it - * at its next [abortIfCancelled] checkpoint. + * checker. [AnalysisScheduler] flags preemption here; the running analysis observes it both at its + * LSP-level [abortIfCancelled] checkpoints and, because [withAnalysisLock] installs a + * [CancelCheckerProgressIndicator] bridging [isCancelled] to the compiler's `ProgressManager`, + * mid-`analyze` at the compiler's own internal cancellation checks. * * Preemption is distinct from ordinary cancellation: [abortIfCancelled] throws * [AnalysisPreemptedException] (so the source can re-schedule the work) while still honouring the diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/CancelCheckerProgressIndicator.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/CancelCheckerProgressIndicator.kt new file mode 100644 index 0000000000..7c47d33034 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/CancelCheckerProgressIndicator.kt @@ -0,0 +1,37 @@ +package com.itsaky.androidide.lsp.kotlin.compiler.modules + +import com.itsaky.androidide.progress.ICancelChecker +import org.jetbrains.kotlin.com.intellij.openapi.progress.ProcessCanceledException +import org.jetbrains.kotlin.com.intellij.openapi.progress.util.AbstractProgressIndicatorBase + +/** + * A [com.intellij.openapi.progress.ProgressIndicator] whose cancellation state is driven by an + * [ICancelChecker]. Installing it as the analysis thread's indicator (via + * [withAnalysisLock]) is what makes the Kotlin Analysis API actually interruptible *mid*-`analyze`. + * + * The embeddable analysis API ships the full IntelliJ `CoreProgressManager`, whose + * `ProgressManager.checkCanceled()` (called densely throughout FIR resolution) re-fetches the + * current thread's indicator and rethrows its [checkCanceled]. By bridging that to [checker], a + * preemption or ordinary cancellation aborts the running analysis at the compiler's next internal + * checkpoint instead of only at the coarse LSP-level [ICancelChecker.abortIfCancelled] checks. + * + * This extends [AbstractProgressIndicatorBase] rather than `EmptyProgressIndicator` on purpose: + * the base is a *non-standard* indicator, so `CoreProgressManager` runs a background task that + * polls [checkCanceled] every ~10ms. That poll flips the manager's internal "should check + * cancelled" flag to active once [checker] reports cancellation, which is what arms the in-`analyze` + * checks. [cancel] (invoked synchronously when this analysis is preempted) flips the same flag + * immediately, so preemption does not have to wait for the poll. + */ +internal class CancelCheckerProgressIndicator( + private val checker: ICancelChecker, +) : AbstractProgressIndicatorBase() { + + override fun isCanceled(): Boolean = super.isCanceled() || checker.isCancelled() + + override fun checkCanceled() { + if (checker.isCancelled()) { + throw ProcessCanceledException() + } + super.checkCanceled() + } +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt index 260e99eb0e..f9ca1d8720 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt @@ -6,15 +6,20 @@ import org.jetbrains.kotlin.analysis.api.analyzeCopy import org.jetbrains.kotlin.analysis.api.projectStructure.KaDanglingFileResolutionMode import org.jetbrains.kotlin.analysis.api.projectStructure.copyOrigin import org.jetbrains.kotlin.analysis.api.projectStructure.isDangling +import org.jetbrains.kotlin.com.intellij.openapi.progress.ProcessCanceledException +import org.jetbrains.kotlin.com.intellij.openapi.progress.ProgressManager import org.jetbrains.kotlin.com.intellij.openapi.util.Key import org.jetbrains.kotlin.psi.KtElement import org.jetbrains.kotlin.psi.KtFile import org.jetbrains.kotlin.psi.UserDataProperty +import org.slf4j.LoggerFactory import java.nio.file.Path private val KT_LSP_COMPLETION_BACKING_FILE = Key("KT_LSP_COMPLETION_BACKING_FILE") var KtFile.backingFilePath by UserDataProperty(KT_LSP_COMPLETION_BACKING_FILE) +private val logger = LoggerFactory.getLogger("KtFileExts") + /** * Runs [action] while holding the global analysis lock at the given [priority]. * @@ -33,6 +38,15 @@ var KtFile.backingFilePath by UserDataProperty(KT_LSP_COMPLETION_BACKING_FILE) * **All** Analysis API access must go through this helper (or [analyzeMaybeDangling], which already * does); never call `analyze` / `analyzeCopy` directly, or the serialization guarantee is lost. * + * **Cancellation.** The [action] runs under a [CancelCheckerProgressIndicator] installed as the + * thread's IntelliJ progress indicator, so the compiler's own `ProgressManager.checkCanceled()` + * calls (dense throughout FIR resolution) abort the analysis *mid*-`analyze` once [cancelChecker] + * reports preemption or cancellation — not just at the coarse LSP [ScheduledCancelChecker.abortIfCancelled] + * checkpoints between work units. The compiler signals this by throwing [ProcessCanceledException]; + * we translate it back into the typed exception the callers already handle + * ([AnalysisPreemptedException] when preempted, else the delegate's `CancellationException`) so + * preempted work is rescheduled rather than silently dropped. + * * **Footgun:** analysis runs under the *read* (shared) side of the global * [com.itsaky.androidide.lsp.kotlin.compiler.read] lock, and that `ReentrantReadWriteLock` is * non-upgradeable. Code running inside [withAnalysisLock] / an `analyze` block must therefore never @@ -42,11 +56,29 @@ var KtFile.backingFilePath by UserDataProperty(KT_LSP_COMPLETION_BACKING_FILE) internal inline fun withAnalysisLock( priority: AnalysisPriority, cancelChecker: ScheduledCancelChecker, - action: () -> R, + crossinline action: () -> R, ): R { - AnalysisScheduler.acquire(priority, cancelChecker::preempt) + val indicator = CancelCheckerProgressIndicator(cancelChecker) + // When this analysis is preempted, also cancel the indicator so the compiler's in-`analyze` + // cancellation checks fire immediately instead of waiting for the manager's background poll. + AnalysisScheduler.acquire(priority) { + cancelChecker.preempt() + indicator.cancel() + } try { - return action() + val holder = arrayOfNulls(1) + try { + ProgressManager.getInstance() + .executeProcessUnderProgress({ holder[0] = action() }, indicator) + } catch (e: ProcessCanceledException) { + logger.debug("process cancelled: prio={}", priority) + // Re-derive the semantically-correct exception callers expect (re-throws + // AnalysisPreemptedException when preempted, or the delegate's CancellationException). + cancelChecker.abortIfCancelled() + throw e + } + @Suppress("UNCHECKED_CAST") + return holder[0] as R } finally { AnalysisScheduler.release() } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt index 2a14070ed7..c60bece3fe 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt @@ -87,10 +87,9 @@ private fun doAnalyze(file: Path, cancelChecker: ICancelChecker): DiagnosticResu ) } - // This should be canceled as well - // The analysis API uses a no-op implementation of - // Intellij's ProgressManager for cancellations, so the following - // isn't really cancellable at the moment + // analyzeMaybeDangling installs a CancelCheckerProgressIndicator, so this analysis is + // cancellable mid-`analyze`: it aborts at the compiler's internal checkCanceled() once + // `checker` reports preemption/cancellation (in addition to the abortIfCancelled() below). analyzeMaybeDangling(ktFile, AnalysisPriority.DIAGNOSTICS, checker) { ktFile.collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) .forEach { diagnostic -> diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt index 0cea853185..7b5ef23b05 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt @@ -4,6 +4,7 @@ import com.google.common.truth.Truth.assertThat import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest import com.itsaky.androidide.progress.ICancelChecker +import org.jetbrains.kotlin.com.intellij.openapi.progress.ProgressManager import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.launch @@ -175,6 +176,49 @@ class AnalysisSerializationTest : KtLspTest() { assertThat(higherRan.get()).isTrue() } + @Test(timeout = 10_000) + fun `analysis is interrupted mid-analyze at the compiler's ProgressManager checkpoint`() { + // The Kotlin Analysis API calls ProgressManager.checkCanceled() densely during FIR + // resolution, but never the LSP-level ICancelChecker.abortIfCancelled(). This body mimics + // that: it only polls ProgressManager.checkCanceled(). Before withAnalysisLock installed a + // CancelCheckerProgressIndicator, that call was inert (no indicator => the manager's + // check-cancelled behaviour stayed disabled) and the work ran to completion regardless of + // preemption. It must now be interruptible. + val holderChecker = ScheduledCancelChecker(ICancelChecker.NOOP) + val holding = CountDownLatch(1) + val preempted = AtomicBoolean(false) + val ranToCompletion = AtomicBoolean(false) + + val lower = Thread { + try { + withAnalysisLock(AnalysisPriority.INDEXING, holderChecker) { + holding.countDown() + repeat(2_000) { + // Compiler-level checkpoint only — no abortIfCancelled() here. + ProgressManager.checkCanceled() + Thread.sleep(5) + } + ranToCompletion.set(true) + } + } catch (e: AnalysisPreemptedException) { + preempted.set(true) + } + } + lower.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + // A completion request preempts the in-progress (indexing) analysis. + val higher = Thread { + withAnalysisLock(AnalysisPriority.COMPLETION, ScheduledCancelChecker(ICancelChecker.NOOP)) {} + } + higher.start() + higher.join(5_000) + lower.join(5_000) + + assertThat(preempted.get()).isTrue() + assertThat(ranToCompletion.get()).isFalse() + } + @Test(timeout = 10_000) fun `lower priority request waits while a higher priority holder runs`() { val holding = CountDownLatch(1) From 959e0f0b7ea9fde00abe1b778431312c52f492e7 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Thu, 2 Jul 2026 18:41:43 +0530 Subject: [PATCH 6/9] fix: allow same-priority pre-emption for code completion requests Signed-off-by: Akash Yadav --- .claude/.gitignore | 12 +++ build.gradle.kts | 6 ++ .../compiler/modules/AnalysisScheduler.kt | 33 +++++--- .../kotlin/completion/KotlinCompletions.kt | 6 +- .../modules/AnalysisSerializationTest.kt | 75 +++++++++++++++++++ 5 files changed, 121 insertions(+), 11 deletions(-) create mode 100644 .claude/.gitignore diff --git a/.claude/.gitignore b/.claude/.gitignore new file mode 100644 index 0000000000..6d88c79ccb --- /dev/null +++ b/.claude/.gitignore @@ -0,0 +1,12 @@ +# Local, machine-specific settings (not shared) +settings.local.json + +# Claude Code worktrees +worktrees/ + +# Session/runtime artifacts +projects/ +todos/ +shell-snapshots/ +statsig/ +.credentials.json diff --git a/build.gradle.kts b/build.gradle.kts index 142e2c4ca4..8bb596384a 100755 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -97,11 +97,17 @@ subprojects { // setAccessible on java.lang.Class fields. // - java.base/java.io, java.util: needed by Robolectric/Gradle worker // reflection in the same test JVM. + // - java.base/java.util.concurrent: the embedded IntelliJ scheduler + // (BoundedTaskExecutor.info -> AppDelayQueue "Periodic tasks thread") + // reflectively reads FutureTask.callable. Without this the periodic + // thread dies, disabling CoreProgressManager's cancellation poll that + // makes the Kotlin Analysis API interruptible mid-`analyze`. jvmArgs( "--add-opens=java.base/java.lang=ALL-UNNAMED", "--add-opens=java.base/java.lang.reflect=ALL-UNNAMED", "--add-opens=java.base/java.io=ALL-UNNAMED", "--add-opens=java.base/java.util=ALL-UNNAMED", + "--add-opens=java.base/java.util.concurrent=ALL-UNNAMED", "--add-opens=jdk.unsupported/sun.misc=ALL-UNNAMED", ) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt index 219a1ebc9b..aaaad78169 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt @@ -12,11 +12,20 @@ import kotlin.concurrent.withLock * * Order: [INDEXING] < [DIAGNOSTICS] < [COMPLETION] — interactive completion beats background * diagnostics, which beats bulk indexing. + * + * [supersedesSamePriority] additionally lets a *newer* request preempt an in-flight request of the + * **same** priority. This is enabled only for [COMPLETION]: when the user types fast, several + * completion requests fire in a row and the in-flight one is computing results for a now-stale cursor + * position, so the newer request cancels it and the fresh position is analysed immediately. A + * superseded completion is simply *discarded* — nothing reschedules it (see + * `KotlinCompletions.codeComplete`). It is intentionally off for [DIAGNOSTICS] and [INDEXING], whose + * preempted work is re-queued rather than dropped; same-priority preemption there would livelock, as + * two contenders would endlessly re-queue and re-preempt each other. */ -internal enum class AnalysisPriority { - INDEXING, - DIAGNOSTICS, - COMPLETION, +internal enum class AnalysisPriority(val supersedesSamePriority: Boolean) { + INDEXING(supersedesSamePriority = false), + DIAGNOSTICS(supersedesSamePriority = false), + COMPLETION(supersedesSamePriority = true), } /** @@ -72,6 +81,9 @@ internal class ScheduledCancelChecker( * - only one analysis runs at a time (the Analysis API is not safe to drive concurrently); * - a higher-priority requester **preempts** a strictly lower-priority holder by invoking its * `onPreempt` callback once (cooperative — the holder bails at its next `abortIfCancelled()`); + * - a newer requester of the **same** priority likewise preempts the holder when that priority is + * [AnalysisPriority.supersedesSamePriority] (completion only — its superseded work is discarded, not + * rescheduled); * - when the lock frees, the highest-priority waiter acquires it next; * - it is **reentrant**: a nested analysis on the same thread re-enters without deadlocking. * @@ -93,8 +105,9 @@ internal object AnalysisScheduler { /** * Acquire the analysis lock at the given [priority]. Blocks until the current thread may run. If a - * strictly lower-priority analysis is in progress, [onPreempt] of *that* holder is invoked so it - * yields; [onPreempt] passed here is stored and used if this acquisition is later preempted. + * preemptable analysis is in progress — strictly lower priority, or the same priority when that + * priority is [AnalysisPriority.supersedesSamePriority] — [onPreempt] of *that* holder is invoked so + * it yields; [onPreempt] passed here is stored and used if this acquisition is later preempted. */ fun acquire(priority: AnalysisPriority, onPreempt: () -> Unit) { mutex.withLock { @@ -109,10 +122,12 @@ internal object AnalysisScheduler { try { while (true) { val hp = holderPriority - if (holderThread != null && hp != null && - hp.ordinal < priority.ordinal && !holderPreempted + if (holderThread != null && hp != null && !holderPreempted && + (hp.ordinal < priority.ordinal || + (hp == priority && priority.supersedesSamePriority)) ) { - // Signal the lower-priority holder to bail (once). + // Signal the holder to bail (once): either it is strictly lower priority, or a + // newer same-priority request supersedes it (completion only). holderPreempted = true holderPreempt?.invoke() } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt index c8617c4732..0370632f71 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt @@ -146,8 +146,10 @@ internal fun doComplete(params: CompletionParams): CompletionResult { abortIfCancelled() - // Completion is the highest-priority analysis: it preempts in-progress diagnostics/indexing and - // is never itself preempted. The cancel checker is the editor's request-scoped one (from Lookup). + // Completion is the highest-priority analysis: it preempts in-progress diagnostics/indexing and is + // never preempted by lower-priority analysis. It can, however, be superseded by a *newer* completion + // request (the user typing on) — that in-flight completion is then cancelled and simply discarded. + // The cancel checker is the editor's request-scoped one (from Lookup). val cancelChecker = ScheduledCancelChecker( Lookup.getDefault().lookup(ICancelChecker::class.java) ?: ICancelChecker.NOOP ) diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt index 7b5ef23b05..fe80666dda 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt @@ -254,4 +254,79 @@ class AnalysisSerializationTest : KtLspTest() { assertThat(enteredWhileHeld).isFalse() assertThat(lowerEntered.get()).isTrue() } + + @Test(timeout = 10_000) + fun `same priority completion supersedes an in-flight completion`() { + val holderChecker = ScheduledCancelChecker(ICancelChecker.NOOP) + val holding = CountDownLatch(1) + val preempted = AtomicBoolean(false) + val newerRan = AtomicBoolean(false) + + // An in-flight completion runs a long, cooperatively-cancellable analysis. + val older = Thread { + try { + withAnalysisLock(AnalysisPriority.COMPLETION, holderChecker) { + holding.countDown() + repeat(2_000) { + holderChecker.abortIfCancelled() + Thread.sleep(5) + } + } + } catch (e: AnalysisPreemptedException) { + preempted.set(true) + } + } + older.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + // A newer completion request (user typed on) must supersede the in-flight one. + val newer = Thread { + withAnalysisLock(AnalysisPriority.COMPLETION, ScheduledCancelChecker(ICancelChecker.NOOP)) { + newerRan.set(true) + } + } + newer.start() + newer.join(5_000) + older.join(5_000) + + assertThat(preempted.get()).isTrue() + assertThat(newerRan.get()).isTrue() + } + + @Test(timeout = 10_000) + fun `same priority diagnostics does not preempt an in-flight diagnostics`() { + val holding = CountDownLatch(1) + val release = CountDownLatch(1) + val secondEntered = AtomicBoolean(false) + + // A diagnostics holder holds the lock until released. + val first = Thread { + withAnalysisLock(AnalysisPriority.DIAGNOSTICS, ScheduledCancelChecker(ICancelChecker.NOOP)) { + holding.countDown() + release.await() + } + } + first.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + // A second diagnostics request is the same priority but must NOT supersede the holder + // (only completion supersedes same-priority work); it waits until the holder releases. + val second = Thread { + withAnalysisLock(AnalysisPriority.DIAGNOSTICS, ScheduledCancelChecker(ICancelChecker.NOOP)) { + secondEntered.set(true) + } + } + second.start() + + // Give the second request time to (incorrectly) barge in. + Thread.sleep(300) + val enteredWhileHeld = secondEntered.get() + + release.countDown() + first.join(5_000) + second.join(5_000) + + assertThat(enteredWhileHeld).isFalse() + assertThat(secondEntered.get()).isTrue() + } } From b17aabdbf0b51c15af89384a84645f03bc43390a Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Fri, 3 Jul 2026 15:03:56 +0000 Subject: [PATCH 7/9] feat: add push cancellation and thread registration to shared cancel checker ICancelChecker.invokeOnCancel(listener) pushes on cancel (fire-once, immediate if already cancelled, no-op on NOOP). ProgressManager gains register/unregister so cancel(thread) flips the request's own checker. Adds ICancelCheckerTest and ProgressManagerTest. --- .../androidide/progress/ICancelChecker.kt | 40 +++++++- .../androidide/progress/ProgressManager.kt | 35 +++++-- .../androidide/progress/ICancelCheckerTest.kt | 98 +++++++++++++++++++ .../progress/ProgressManagerTest.kt | 58 +++++++++++ 4 files changed, 222 insertions(+), 9 deletions(-) create mode 100644 shared/src/test/java/com/itsaky/androidide/progress/ICancelCheckerTest.kt create mode 100644 shared/src/test/java/com/itsaky/androidide/progress/ProgressManagerTest.kt diff --git a/shared/src/main/java/com/itsaky/androidide/progress/ICancelChecker.kt b/shared/src/main/java/com/itsaky/androidide/progress/ICancelChecker.kt index 059824966f..ce0bcb1974 100644 --- a/shared/src/main/java/com/itsaky/androidide/progress/ICancelChecker.kt +++ b/shared/src/main/java/com/itsaky/androidide/progress/ICancelChecker.kt @@ -18,6 +18,7 @@ package com.itsaky.androidide.progress import java.util.concurrent.CancellationException +import java.util.concurrent.CopyOnWriteArrayList import java.util.concurrent.atomic.AtomicBoolean /** @@ -45,12 +46,31 @@ interface ICancelChecker { @Throws(CancellationException::class) fun abortIfCancelled() + /** + * Register [listener] to run when this process is cancelled — a *push* notification, so a consumer + * can react to cancellation immediately instead of polling [isCancelled]. If already cancelled, + * [listener] runs synchronously now. [listener] runs at most once. + * + * The default implementation only fires when already cancelled; an implementation that can transition + * to cancelled after registration (e.g. [Default]) overrides this to fire on the transition. + */ + fun invokeOnCancel(listener: () -> Unit) { + if (isCancelled()) { + listener() + } + } + open class Default(cancelled: Boolean = false) : ICancelChecker { private val cancelled = AtomicBoolean(cancelled) + private val onCancelListeners = CopyOnWriteArrayList<() -> Unit>() override fun cancel() { - cancelled.set(true) + // Fire listeners once, on the false -> true transition only. + if (cancelled.compareAndSet(false, true)) { + onCancelListeners.forEach { it() } + onCancelListeners.clear() + } } override fun isCancelled(): Boolean { @@ -62,6 +82,19 @@ interface ICancelChecker { throw CancellationException() } } + + override fun invokeOnCancel(listener: () -> Unit) { + if (isCancelled()) { + listener() + return + } + onCancelListeners.add(listener) + // Guard the race where cancel() ran between the check above and the add: if we now observe + // cancellation, run the listener ourselves (removing it so cancel() can't also run it). + if (isCancelled() && onCancelListeners.remove(listener)) { + listener() + } + } } companion object { @@ -70,7 +103,10 @@ interface ICancelChecker { * A no-op cancel checker. The task is never cancelled. */ @JvmField - val NOOP = Default(false) + val NOOP = object : Default(false) { + // Never transitions to cancelled, so retaining listeners would only leak them. + override fun invokeOnCancel(listener: () -> Unit) = Unit + } /** * An already cancelled cancel checker. diff --git a/shared/src/main/java/com/itsaky/androidide/progress/ProgressManager.kt b/shared/src/main/java/com/itsaky/androidide/progress/ProgressManager.kt index b977b9ac33..def9416f1e 100644 --- a/shared/src/main/java/com/itsaky/androidide/progress/ProgressManager.kt +++ b/shared/src/main/java/com/itsaky/androidide/progress/ProgressManager.kt @@ -40,21 +40,42 @@ class ProgressManager private constructor() { } } + /** + * Associate an existing [checker] with [thread]. A subsequent [cancel] of [thread] then flips + * *this* checker (rather than a throwaway [Default]), so a caller that also polls [checker] + * observes the cancellation. Used by the editor to make a completion's cancel checker cancellable + * via the thread it runs on. Pair with [unregister]. + */ + fun register(thread: Thread, checker: ICancelChecker) { + synchronized(threads) { + threads[thread] = checker + } + } + + /** Remove any checker previously associated with [thread] via [register]. */ + fun unregister(thread: Thread) { + synchronized(threads) { + threads.remove(thread) + } + } + fun cancel(thread: Thread) { - var checker = threads[thread] - if (checker == null) { - checker = Default() + synchronized(threads) { + var checker = threads[thread] + if (checker == null) { + checker = Default() + threads[thread] = checker + } + checker.cancel() } - checker.cancel() - threads[thread] = checker } @JvmName("internalAbortIfCancelled") private fun abortIfCancelled() { val thisThread = Thread.currentThread() - val checker = threads[thisThread] + val checker = synchronized(threads) { threads[thisThread] } if (checker != null && checker.isCancelled()) { - threads.remove(thisThread) + synchronized(threads) { threads.remove(thisThread) } throw CancellationException() } } diff --git a/shared/src/test/java/com/itsaky/androidide/progress/ICancelCheckerTest.kt b/shared/src/test/java/com/itsaky/androidide/progress/ICancelCheckerTest.kt new file mode 100644 index 0000000000..571fec354b --- /dev/null +++ b/shared/src/test/java/com/itsaky/androidide/progress/ICancelCheckerTest.kt @@ -0,0 +1,98 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.progress + +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import java.util.concurrent.atomic.AtomicInteger + +/** + * Tests for the push-based [ICancelChecker.invokeOnCancel] added for ADFA-4174, which lets the Kotlin + * LSP abort an in-flight `analyze` the moment cancellation happens instead of polling. + */ +class ICancelCheckerTest { + + @Test + fun `invokeOnCancel fires when cancel is called`() { + val checker = ICancelChecker.Default() + val fired = AtomicInteger(0) + + checker.invokeOnCancel { fired.incrementAndGet() } + assertThat(fired.get()).isEqualTo(0) + + checker.cancel() + assertThat(fired.get()).isEqualTo(1) + } + + @Test + fun `invokeOnCancel fires immediately when already cancelled`() { + val checker = ICancelChecker.Default(cancelled = true) + val fired = AtomicInteger(0) + + checker.invokeOnCancel { fired.incrementAndGet() } + + assertThat(fired.get()).isEqualTo(1) + } + + @Test + fun `invokeOnCancel fires at most once across repeated cancel calls`() { + val checker = ICancelChecker.Default() + val fired = AtomicInteger(0) + + checker.invokeOnCancel { fired.incrementAndGet() } + checker.cancel() + checker.cancel() + checker.cancel() + + assertThat(fired.get()).isEqualTo(1) + } + + @Test + fun `multiple listeners all fire on cancel`() { + val checker = ICancelChecker.Default() + val fired = AtomicInteger(0) + + checker.invokeOnCancel { fired.incrementAndGet() } + checker.invokeOnCancel { fired.incrementAndGet() } + + checker.cancel() + + assertThat(fired.get()).isEqualTo(2) + } + + @Test + fun `NOOP invokeOnCancel is a no-op`() { + val fired = AtomicInteger(0) + + // NOOP is a shared singleton that is never cancelled; registering must be a no-op so listeners + // (which may capture large objects) do not accumulate on it forever. We deliberately do NOT call + // NOOP.cancel() — flipping the shared singleton would corrupt every other user of it. + ICancelChecker.NOOP.invokeOnCancel { fired.incrementAndGet() } + + assertThat(fired.get()).isEqualTo(0) + } + + @Test + fun `CANCELLED fires immediately`() { + val fired = AtomicInteger(0) + + ICancelChecker.CANCELLED.invokeOnCancel { fired.incrementAndGet() } + + assertThat(fired.get()).isEqualTo(1) + } +} diff --git a/shared/src/test/java/com/itsaky/androidide/progress/ProgressManagerTest.kt b/shared/src/test/java/com/itsaky/androidide/progress/ProgressManagerTest.kt new file mode 100644 index 0000000000..41edc171c7 --- /dev/null +++ b/shared/src/test/java/com/itsaky/androidide/progress/ProgressManagerTest.kt @@ -0,0 +1,58 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.progress + +import com.google.common.truth.Truth.assertThat +import org.junit.Test + +class ProgressManagerTest { + + @Test + fun `cancel flips a registered checker`() { + // Regression for ADFA-4174: cancel(thread) must act on the *registered* checker so a caller that + // polls that same checker (the completion cancel checker driving mid-analyze abort) observes the + // cancellation — before, cancel() always stored a throwaway Default and the registered checker + // never became cancelled. + val checker = ICancelChecker.Default() + val thread = Thread.currentThread() + + ProgressManager.instance.register(thread, checker) + try { + assertThat(checker.isCancelled()).isFalse() + + ProgressManager.instance.cancel(thread) + + assertThat(checker.isCancelled()).isTrue() + } finally { + ProgressManager.instance.unregister(thread) + } + } + + @Test + fun `unregister detaches the checker so cancel no longer affects it`() { + val checker = ICancelChecker.Default() + val thread = Thread.currentThread() + + ProgressManager.instance.register(thread, checker) + ProgressManager.instance.unregister(thread) + + ProgressManager.instance.cancel(thread) + + assertThat(checker.isCancelled()).isFalse() + } +} From fdb4e5ee4446f34dd9a2416859017fc7f5f082af Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Fri, 3 Jul 2026 15:04:13 +0000 Subject: [PATCH 8/9] fix: abort in-flight Kotlin analysis promptly on completion cancellation withAnalysisLock installs a cancellable Job and registers an invokeOnCancel listener so both scheduler preemption and ordinary editor cancellation abort mid-analyze via a single push (no polling). AnalysisScheduler.acquire is cancellation-aware while waiting so superseded requests bail instead of parking. KotlinCompletions uses params.cancelChecker (race-free) and classifies all cancellation exception types uniformly. Adds AnalysisSerializationTest coverage. --- .../compiler/modules/AnalysisScheduler.kt | 37 +++++- .../modules/AnalysisThreadContext.java | 37 ++++++ .../lsp/kotlin/compiler/modules/KtFileExts.kt | 51 +++++--- .../kotlin/completion/KotlinCompletions.kt | 81 ++++++++++--- .../modules/AnalysisSerializationTest.kt | 109 ++++++++++++++++++ 5 files changed, 286 insertions(+), 29 deletions(-) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisThreadContext.java diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt index aaaad78169..73c3c26a63 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt @@ -2,6 +2,8 @@ package com.itsaky.androidide.lsp.kotlin.compiler.modules import com.itsaky.androidide.progress.ICancelChecker import java.util.concurrent.CancellationException +import java.util.concurrent.CopyOnWriteArrayList +import java.util.concurrent.TimeUnit import java.util.concurrent.locks.ReentrantLock import kotlin.concurrent.withLock @@ -55,9 +57,14 @@ internal class ScheduledCancelChecker( @Volatile private var preempted = false + private val onCancelListeners = CopyOnWriteArrayList<() -> Unit>() + /** Marks this analysis as preempted; the next [abortIfCancelled] will throw. */ fun preempt() { preempted = true + // Push: preemption is a cancellation too, so notify [invokeOnCancel] listeners immediately. + onCancelListeners.forEach { it() } + onCancelListeners.clear() } override fun cancel() { @@ -72,6 +79,18 @@ internal class ScheduledCancelChecker( } delegate.abortIfCancelled() } + + override fun invokeOnCancel(listener: () -> Unit) { + // Fire on either scheduler preemption (stored locally, run by preempt()) or the delegate's own + // cancellation (forwarded so the editor's CompletionCancelChecker.cancel() pushes it). The + // listener body is idempotent, so firing via both paths is harmless. + onCancelListeners.add(listener) + delegate.invokeOnCancel(listener) + // Guard the race where preempt() ran between add and now. + if (preempted && onCancelListeners.remove(listener)) { + listener() + } + } } /** @@ -91,6 +110,9 @@ internal class ScheduledCancelChecker( */ internal object AnalysisScheduler { + /** Upper bound on how long a queued requester waits before re-checking its cancellation. */ + private const val WAIT_POLL_MILLIS = 25L + private val mutex = ReentrantLock() private val available = mutex.newCondition() @@ -108,8 +130,14 @@ internal object AnalysisScheduler { * preemptable analysis is in progress — strictly lower priority, or the same priority when that * priority is [AnalysisPriority.supersedesSamePriority] — [onPreempt] of *that* holder is invoked so * it yields; [onPreempt] passed here is stored and used if this acquisition is later preempted. + * + * [cancelChecker] is *this* requester's checker: while waiting for the lock, the wait is re-checked on + * a short timer, and if the requester has been cancelled — e.g. the editor superseded + * this completion — [acquire] throws instead of parking until the lock frees. This is what stops + * superseded completions from piling up holding heavy state (KtFile copies, symbol lists) while they + * wait, which on-device saturated the heap and triggered multi-second GC stalls. */ - fun acquire(priority: AnalysisPriority, onPreempt: () -> Unit) { + fun acquire(priority: AnalysisPriority, cancelChecker: ICancelChecker, onPreempt: () -> Unit) { mutex.withLock { val me = Thread.currentThread() if (holderThread === me) { @@ -121,6 +149,9 @@ internal object AnalysisScheduler { waiting[priority.ordinal]++ try { while (true) { + // Bail out (rather than keep waiting) if this requester was cancelled while queued. + cancelChecker.abortIfCancelled() + val hp = holderPriority if (holderThread != null && hp != null && !holderPreempted && (hp.ordinal < priority.ordinal || @@ -135,7 +166,9 @@ internal object AnalysisScheduler { if (holderThread == null && !higherPriorityWaiting(priority)) { break } - available.await() + // Timed wait so a cancellation that arrives without a lock-state change (no signal) is + // still observed within WAIT_POLL_MILLIS, bounding how long a superseded waiter parks. + available.await(WAIT_POLL_MILLIS, TimeUnit.MILLISECONDS) } } finally { waiting[priority.ordinal]-- diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisThreadContext.java b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisThreadContext.java new file mode 100644 index 0000000000..35aa3dde9c --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisThreadContext.java @@ -0,0 +1,37 @@ +package com.itsaky.androidide.lsp.kotlin.compiler.modules; + +import kotlin.coroutines.CoroutineContext; +import kotlinx.coroutines.Job; +import org.jetbrains.kotlin.com.intellij.concurrency.ThreadContext; +import org.jetbrains.kotlin.com.intellij.openapi.application.AccessToken; + +/** + * Bridge to the embeddable IntelliJ {@link ThreadContext} coroutine-context API. + * + *

{@code currentThreadContext}/{@code installThreadContext} live in a Kotlin file facade whose + * metadata the Kotlin compiler cannot resolve against from this module (they surface as + * "unresolved reference" from Kotlin). At the bytecode level, though, they are plain + * {@code public static} methods, so Java can call them directly. + * + *

This is used by {@code withAnalysisLock} to install a cancellable {@link Job} into the analysis + * thread's context: the embeddable {@code CoreProgressManager.checkCanceled()} routes through + * {@code Cancellation.checkCancelled()}, which throws as soon as that Job is cancelled, aborting the + * running analysis mid-{@code analyze}. + */ +public final class AnalysisThreadContext { + + private AnalysisThreadContext() { + } + + /** + * Installs {@code job} into the current thread's IntelliJ coroutine context (preserving any + * context already present) and returns a token that restores the previous context when closed. + * + *

Public because it is referenced from the {@code internal inline} {@code withAnalysisLock}; + * an inline function may only reference declarations at least as accessible as itself. + */ + public static AccessToken installJob(Job job) { + CoroutineContext context = ThreadContext.currentThreadContext().plus(job); + return ThreadContext.installThreadContext(context, true); + } +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt index f9ca1d8720..58813a34f0 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt @@ -6,6 +6,7 @@ import org.jetbrains.kotlin.analysis.api.analyzeCopy import org.jetbrains.kotlin.analysis.api.projectStructure.KaDanglingFileResolutionMode import org.jetbrains.kotlin.analysis.api.projectStructure.copyOrigin import org.jetbrains.kotlin.analysis.api.projectStructure.isDangling +import kotlinx.coroutines.Job import org.jetbrains.kotlin.com.intellij.openapi.progress.ProcessCanceledException import org.jetbrains.kotlin.com.intellij.openapi.progress.ProgressManager import org.jetbrains.kotlin.com.intellij.openapi.util.Key @@ -38,14 +39,16 @@ private val logger = LoggerFactory.getLogger("KtFileExts") * **All** Analysis API access must go through this helper (or [analyzeMaybeDangling], which already * does); never call `analyze` / `analyzeCopy` directly, or the serialization guarantee is lost. * - * **Cancellation.** The [action] runs under a [CancelCheckerProgressIndicator] installed as the - * thread's IntelliJ progress indicator, so the compiler's own `ProgressManager.checkCanceled()` - * calls (dense throughout FIR resolution) abort the analysis *mid*-`analyze` once [cancelChecker] - * reports preemption or cancellation — not just at the coarse LSP [ScheduledCancelChecker.abortIfCancelled] - * checkpoints between work units. The compiler signals this by throwing [ProcessCanceledException]; - * we translate it back into the typed exception the callers already handle - * ([AnalysisPreemptedException] when preempted, else the delegate's `CancellationException`) so - * preempted work is rescheduled rather than silently dropped. + * **Cancellation.** The [action] runs with a [kotlinx.coroutines.Job] installed in the thread's + * IntelliJ coroutine context. The compiler's own `ProgressManager.checkCanceled()` calls (dense + * throughout FIR resolution) route through `Cancellation.checkCancelled()`, which throws as soon as + * that Job is cancelled — aborting the analysis *mid*-`analyze`, not just at the coarse LSP + * [ScheduledCancelChecker.abortIfCancelled] checkpoints between work units. (A + * [CancelCheckerProgressIndicator] is also installed as a fallback, but its poll-driven arming does + * not run in this embeddable environment; see the body.) The compiler signals cancellation by + * throwing [ProcessCanceledException]; we translate it back into the typed exception the callers + * already handle ([AnalysisPreemptedException] when preempted, else the delegate's + * `CancellationException`) so preempted work is rescheduled rather than silently dropped. * * **Footgun:** analysis runs under the *read* (shared) side of the global * [com.itsaky.androidide.lsp.kotlin.compiler.read] lock, and that `ReentrantReadWriteLock` is @@ -59,19 +62,39 @@ internal inline fun withAnalysisLock( crossinline action: () -> R, ): R { val indicator = CancelCheckerProgressIndicator(cancelChecker) - // When this analysis is preempted, also cancel the indicator so the compiler's in-`analyze` - // cancellation checks fire immediately instead of waiting for the manager's background poll. - AnalysisScheduler.acquire(priority) { + + // Cancelling [job] is what actually aborts the running analysis *mid*-`analyze`: the embeddable + // `CoreProgressManager.checkCanceled()` (called densely throughout FIR resolution) invokes + // `Cancellation.checkCancelled()` *unconditionally* — before any indicator/`CheckCanceledBehavior` + // gating — and that throws a `CeProcessCanceledException` (a `ProcessCanceledException`) the moment + // the [Job] installed in this thread's context is cancelled. + // + // The [indicator] is kept only as a fallback for environments whose `CoreProgressManager` runs its + // ~10ms non-standard-indicator poll; that poll does not run in this (Android, embeddable) one. + val job = Job() + + AnalysisScheduler.acquire(priority, cancelChecker) { + // Preemption is signalled by flipping the checker; the invokeOnCancel listener below turns that + // (and ordinary editor cancellation) into the actual mid-`analyze` abort. cancelChecker.preempt() + } + // Single push path for *both* preemption and ordinary editor cancellation: [ScheduledCancelChecker] + // fires this on preempt() and on its delegate's cancel(), so the running analyze aborts immediately + // — no polling, and unaffected by GC pauses that would stall a poll thread. + cancelChecker.invokeOnCancel { indicator.cancel() + job.cancel() } try { val holder = arrayOfNulls(1) try { - ProgressManager.getInstance() - .executeProcessUnderProgress({ holder[0] = action() }, indicator) + // Install the Job for the duration of the analysis and restore the previous context after. + AnalysisThreadContext.installJob(job).use { + ProgressManager.getInstance() + .executeProcessUnderProgress({ holder[0] = action() }, indicator) + } } catch (e: ProcessCanceledException) { - logger.debug("process cancelled: prio={}", priority) + logger.debug("process cancelled: prio={}", priority) // Re-derive the semantically-correct exception callers expect (re-throws // AnalysisPreemptedException when preempted, or the delegate's CancellationException). cancelChecker.abortIfCancelled() diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt index 0370632f71..7b760991ce 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt @@ -3,6 +3,7 @@ package com.itsaky.androidide.lsp.kotlin.completion import com.itsaky.androidide.lookup.Lookup import com.itsaky.androidide.lsp.api.describeSnippet import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPreemptedException import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling @@ -23,7 +24,9 @@ import com.itsaky.androidide.preferences.utils.indentationString import com.itsaky.androidide.progress.ICancelChecker import com.itsaky.androidide.progress.ProgressManager import com.itsaky.androidide.projects.FileManager +import io.github.rosemoe.sora.lang.completion.CompletionCancelledException import kotlinx.coroutines.CancellationException +import org.jetbrains.kotlin.com.intellij.openapi.progress.ProcessCanceledException import org.appdevforall.codeonthego.indexing.jvm.JvmClassInfo import org.appdevforall.codeonthego.indexing.jvm.JvmFunctionInfo import org.appdevforall.codeonthego.indexing.jvm.JvmSymbol @@ -77,13 +80,49 @@ private const val KT_COMPLETION_PLACEHOLDER = "KT_COMPLETION_PLACEHOLDER" private val logger = LoggerFactory.getLogger("KotlinCompletions") +/** Max unimported symbols pulled from each index for scope completion (see [collectUnimportedSymbols]). */ +private const val UNIMPORTED_SYMBOL_LIMIT = 100 + +/** + * The [ScheduledCancelChecker] for the completion request running on this thread, set for the + * duration of [doComplete]. The LSP-level [abortIfCancelled] checkpoints consult it so they observe + * scheduler *preemption* (a newer completion superseding this one, or this one yielding to a + * higher-priority request) — not just the editor's own request cancellation. Without this, those + * checkpoints only saw the raw delegate from [Lookup] and every preemption was misreported as an + * ordinary cancellation. + */ +private val currentCancelChecker = ThreadLocal() + private fun abortIfCancelled() { ProgressManager.abortIfCancelled() - Lookup.getDefault() - .lookup(ICancelChecker::class.java) - ?.abortIfCancelled() + val checker = currentCancelChecker.get() + if (checker != null) { + checker.abortIfCancelled() + } else { + Lookup.getDefault() + .lookup(ICancelChecker::class.java) + ?.abortIfCancelled() + } } +/** + * A cancelled completion surfaces as one of several exception types depending on *where* it was + * observed, and none of them is an error: + * - [CancellationException]/[AnalysisPreemptedException] — a coarse [abortIfCancelled] checkpoint; + * - [ProcessCanceledException] — the compiler's mid-`analyze` cancellation (our `job` was cancelled); + * - [CompletionCancelledException] — the editor's sora publisher was cancelled (surfaced via the + * [com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker] delegate); + * - [InterruptedException] — the sora `CompletionThread` was interrupted mid-resolution. + * + * All of them mean "this completion was superseded/cancelled"; treat them uniformly so mid-`analyze` + * cancellations are reported cleanly instead of logged as spurious errors with a stack trace. + */ +private fun Throwable.isCancellation(): Boolean = + this is CancellationException || + this is InterruptedException || + this is ProcessCanceledException || + this is CompletionCancelledException + /** * Provide code completion for the given completion parameters. * @@ -96,8 +135,9 @@ internal fun codeComplete(params: CompletionParams): CompletionResult { return try { doComplete(params) } catch (error: Throwable) { - if (error is CancellationException || error is InterruptedException) { - logger.info("completion cancelled") + if (error.isCancellation()) { + val isPreempted = error is AnalysisPreemptedException + logger.info("completion cancelled (preempted={})", isPreempted) if (error is InterruptedException) { Thread.interrupted() } @@ -149,10 +189,17 @@ internal fun doComplete(params: CompletionParams): CompletionResult { // Completion is the highest-priority analysis: it preempts in-progress diagnostics/indexing and is // never preempted by lower-priority analysis. It can, however, be superseded by a *newer* completion // request (the user typing on) — that in-flight completion is then cancelled and simply discarded. - // The cancel checker is the editor's request-scoped one (from Lookup). - val cancelChecker = ScheduledCancelChecker( - Lookup.getDefault().lookup(ICancelChecker::class.java) ?: ICancelChecker.NOOP - ) + // + // Use the request-scoped checker carried on [params] rather than the global Lookup: Lookup holds a + // single ICancelChecker updated per request, so with concurrent completion threads an older request + // could read a newer request's checker and never observe its own cancellation. [params.cancelChecker] + // is the editor's own CompletionCancelChecker for *this* request. Fall back to Lookup only if a + // caller supplied the NOOP checker (e.g. tests that don't set one). + val delegate = params.cancelChecker.takeUnless { it === ICancelChecker.NOOP } + ?: Lookup.getDefault().lookup(ICancelChecker::class.java) + ?: ICancelChecker.NOOP + val cancelChecker = ScheduledCancelChecker(delegate) + currentCancelChecker.set(cancelChecker) return try { env.project.read { @@ -194,12 +241,17 @@ internal fun doComplete(params: CompletionParams): CompletionResult { } } } catch (e: Throwable) { - if (e is CancellationException) { + // Let cancellation (incl. mid-`analyze` ProcessCanceledException / sora + // CompletionCancelledException / InterruptedException) propagate to codeComplete's uniform + // handler rather than logging it as an error. + if (e.isCancellation()) { throw e } logger.warn("An error occurred while computing completions for {}", params.file, e) return CompletionResult.EMPTY + } finally { + currentCancelChecker.remove() } } @@ -362,13 +414,16 @@ private fun KaSession.collectUnimportedSymbols( buildUnimportedSymbolItem(symbol)?.let { to += it } } - env.libraryIndex?.findByPrefix(ctx.partial, limit = 0) + // Bounded: limit = 0 means "unlimited", which pulled every matching symbol from all three + // indexes on every keystroke — a major allocation/GC source. A capped result set is more than + // enough for a completion popup and keeps the per-request footprint small. + env.libraryIndex?.findByPrefix(ctx.partial, limit = UNIMPORTED_SYMBOL_LIMIT) ?.forEach(::addCompletionItem) - env.sourceIndex?.findByPrefix(ctx.partial, limit = 0) + env.sourceIndex?.findByPrefix(ctx.partial, limit = UNIMPORTED_SYMBOL_LIMIT) ?.forEach(::addCompletionItem) - env.generatedIndex?.findByPrefix(ctx.partial, limit = 0) + env.generatedIndex?.findByPrefix(ctx.partial, limit = UNIMPORTED_SYMBOL_LIMIT) ?.forEach(::addCompletionItem) } diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt index fe80666dda..29156f98d2 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt @@ -11,10 +11,12 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import org.junit.Test import java.util.Collections +import java.util.concurrent.CancellationException import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicInteger +import java.util.concurrent.atomic.AtomicReference import kotlin.math.max /** @@ -219,6 +221,113 @@ class AnalysisSerializationTest : KtLspTest() { assertThat(ranToCompletion.get()).isFalse() } + @Test(timeout = 10_000) + fun `ordinary cancellation aborts an in-flight analysis mid-analyze`() { + // Regression for ADFA-4174. Unlike preemption (a competing higher/same-priority request), an + // *ordinary* cancellation — the editor cancelling because the user typed on / moved the cursor + // / dismissed the popup — flips the request's ICancelChecker. Via ScheduledCancelChecker's + // invokeOnCancel push, cancelling the delegate must abort the compiler's mid-`analyze` FIR + // resolution promptly, rather than letting it run to completion (observed on-device as ~900ms + // stalls and piled-up completion threads). + val delegate = ICancelChecker.Default() + val file = createSourceFile("OrdinaryCancel.kt", "class C { fun f(): Int = 1 }") + val holding = CountDownLatch(1) + val ranToCompletion = AtomicBoolean(false) + val caught = AtomicReference(null) + + // Run inside a real analyze under the read lock: this also guards against a read/write-lock + // upgrade deadlock regression (withAnalysisLock runs under the shared read lock). + val worker = Thread { + try { + env.project.read { + analyzeMaybeDangling( + file, + AnalysisPriority.COMPLETION, + ScheduledCancelChecker(delegate), + ) { + holding.countDown() + repeat(2_000) { + // Compiler-level checkpoint only — mirrors FIR resolution, which never calls + // the LSP-level abortIfCancelled(). + ProgressManager.checkCanceled() + Thread.sleep(5) + } + ranToCompletion.set(true) + } + } + } catch (t: Throwable) { + caught.set(t) + } + } + worker.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + val startNs = System.nanoTime() + // Simulates EditorCompletionWindow.cancelCompletion() -> ProgressManager.cancel(thread) -> + // CompletionCancelChecker.cancel(). + delegate.cancel() + worker.join(2_000) + val elapsedMs = (System.nanoTime() - startNs) / 1_000_000 + + assertThat(worker.isAlive).isFalse() + assertThat(ranToCompletion.get()).isFalse() + // Ordinary cancellation, not preemption: the delegate's CancellationException, not + // AnalysisPreemptedException. + assertThat(caught.get()).isInstanceOf(CancellationException::class.java) + assertThat(caught.get()).isNotInstanceOf(AnalysisPreemptedException::class.java) + assertThat(elapsedMs).isLessThan(500) + } + + @Test(timeout = 10_000) + fun `a waiting requester bails when cancelled instead of waiting for the lock`() { + // Regression for ADFA-4174: a superseded completion that is queued behind another analysis must + // abort as soon as it is cancelled, rather than parking (holding heavy state) until the lock + // frees. On-device, parked-until-release completions piled up and saturated the heap. + val holding = CountDownLatch(1) + val release = CountDownLatch(1) + val waiterDelegate = ICancelChecker.Default() + val waiterThrew = AtomicReference(null) + val waiterEntered = AtomicBoolean(false) + + // A completion holder keeps the lock until released. + val holder = Thread { + withAnalysisLock(AnalysisPriority.COMPLETION, ScheduledCancelChecker(ICancelChecker.NOOP)) { + holding.countDown() + release.await() + } + } + holder.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + // A lower-priority (diagnostics) requester must wait behind the completion holder (it does not + // preempt). It is cancelled while waiting and must bail from acquire() promptly. + val waiter = Thread { + try { + withAnalysisLock(AnalysisPriority.DIAGNOSTICS, ScheduledCancelChecker(waiterDelegate)) { + waiterEntered.set(true) + } + } catch (t: Throwable) { + waiterThrew.set(t) + } + } + waiter.start() + + // Let it enter the wait loop, then cancel it. + Thread.sleep(200) + waiterDelegate.cancel() + + // The waiter must finish (bail) while the holder still holds the lock. + waiter.join(2_000) + val bailedWhileHeld = !waiter.isAlive + + release.countDown() + holder.join(5_000) + + assertThat(bailedWhileHeld).isTrue() + assertThat(waiterEntered.get()).isFalse() + assertThat(waiterThrew.get()).isInstanceOf(CancellationException::class.java) + } + @Test(timeout = 10_000) fun `lower priority request waits while a higher priority holder runs`() { val holding = CountDownLatch(1) From 9b3de6afcd2a63887742b1168320bc785714921c Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Fri, 3 Jul 2026 15:04:25 +0000 Subject: [PATCH 9/9] fix: coalesce Kotlin completion requests to a single in-flight analysis EditorCompletionWindow debounces keystrokes so at most one completion analysis runs at a time for the latest cursor position, preventing the CompletionThread/allocation pile-up. IDELanguage registers the request's CompletionCancelChecker on its thread so the editor's cancel routes to it and pushes the mid-analyze abort. --- .../androidide/editor/language/IDELanguage.kt | 8 +++ .../editor/ui/EditorCompletionWindow.kt | 57 ++++++++++++++++--- 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/editor/src/main/java/com/itsaky/androidide/editor/language/IDELanguage.kt b/editor/src/main/java/com/itsaky/androidide/editor/language/IDELanguage.kt index ddca102e71..f074ec747a 100644 --- a/editor/src/main/java/com/itsaky/androidide/editor/language/IDELanguage.kt +++ b/editor/src/main/java/com/itsaky/androidide/editor/language/IDELanguage.kt @@ -25,6 +25,7 @@ import com.itsaky.androidide.lsp.debug.model.BreakpointDefinition import com.itsaky.androidide.lsp.debug.model.BreakpointRequest import com.itsaky.androidide.preferences.internal.EditorPreferences import com.itsaky.androidide.progress.ICancelChecker +import com.itsaky.androidide.progress.ProgressManager import io.github.rosemoe.sora.lang.Language import io.github.rosemoe.sora.lang.completion.CompletionCancelledException import io.github.rosemoe.sora.lang.completion.CompletionPublisher @@ -67,11 +68,18 @@ abstract class IDELanguage : Language { publisher: CompletionPublisher, extraArguments: Bundle ) { + val completionThread = Thread.currentThread() try { val cancelChecker = CompletionCancelChecker(publisher) Lookup.getDefault().update(ICancelChecker::class.java, cancelChecker) + // Bind the checker to this completion thread. EditorCompletionWindow.cancelCompletion() + // cancels the (old) thread via ProgressManager.cancel(thread); routing that to *this* + // checker's cancel() lets the LSP's invokeOnCancel listener abort the running analysis + // mid-`analyze` immediately, instead of only at coarse checkpoints. + ProgressManager.instance.register(completionThread, cancelChecker) doComplete(content, position, publisher, cancelChecker, extraArguments) } finally { + ProgressManager.instance.unregister(completionThread) Lookup.getDefault().unregister( ICancelChecker::class.java ) diff --git a/editor/src/main/java/com/itsaky/androidide/editor/ui/EditorCompletionWindow.kt b/editor/src/main/java/com/itsaky/androidide/editor/ui/EditorCompletionWindow.kt index 8561b79de3..ad5e4f5709 100644 --- a/editor/src/main/java/com/itsaky/androidide/editor/ui/EditorCompletionWindow.kt +++ b/editor/src/main/java/com/itsaky/androidide/editor/ui/EditorCompletionWindow.kt @@ -41,9 +41,22 @@ class EditorCompletionWindow(val editor: IDEEditor) : EditorAutoCompletion(edito private var listView: ListView? = null private val items: MutableList = mutableListOf() + /** + * A scheduled-but-not-yet-started completion request, kept so a newer keystroke can cancel it. + * See [requireCompletion]. + */ + private var pendingCompletion: Runnable? = null + companion object { private val log = LoggerFactory.getLogger(EditorCompletionWindow::class.java) + + /** + * Quiet period used to coalesce a burst of keystrokes into a single completion request. Rapid + * typing (re)schedules the start; only after the user pauses this long does one analysis run, + * for the latest cursor position. Keeps at most one completion in flight. + */ + private const val COMPLETION_DEBOUNCE_MS = 80L } init { @@ -119,29 +132,57 @@ class EditorCompletionWindow(val editor: IDEEditor) : EditorAutoCompletion(edito } override fun cancelCompletion() { + // Drop any request that was scheduled but hasn't started yet. + pendingCompletion?.let { editor.handler.removeCallbacks(it) } + pendingCompletion = null if (completionThread != null) { ProgressManager.instance.cancel(completionThread) } super.cancelCompletion() } - override fun requireCompletion() { + /** + * Whether a completion may be shown for the current editor state. Hides the window (matching the + * prior inline behaviour) when the cursor is selected or completion is otherwise not applicable. + */ + private fun canStartCompletion(): Boolean { if (cancelShowUp || !isEnabled || !editor.isAttachedToWindow) { - return + return false } - - val text = editor.text - if (text.cursor.isSelected || checkNoCompletion()) { + if (editor.text.cursor.isSelected || checkNoCompletion()) { hide() - return + return false } + return true + } - if (System.nanoTime() - requestTime < editor.props.cancelCompletionNs) { - requestTime = System.nanoTime() + override fun requireCompletion() { + if (!canStartCompletion()) { return } + // Coalesce a burst of keystrokes into a single completion. Cancel the in-flight completion and + // any pending (not-yet-started) one, then (re)schedule one start after a short quiet period. + // This guarantees at most one completion analysis in flight and that only the latest cursor + // position is computed — preventing the CompletionThread/allocation pile-up that saturated the + // heap and froze the editor during fast typing. cancelCompletion() clears any pending request, + // so we always schedule exactly one. cancelCompletion() + + val request = Runnable { startCompletion() } + pendingCompletion = request + editor.handler.postDelayed(request, COMPLETION_DEBOUNCE_MS) + } + + /** Starts a single completion for the current cursor position. Runs on the UI thread. */ + private fun startCompletion() { + pendingCompletion = null + + // The editor state may have changed during the debounce delay; re-check the guards. + if (!canStartCompletion()) { + return + } + requestTime = System.nanoTime() currentSelection = -1