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 bdcc53abf5..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 @@ -11,14 +11,46 @@ 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. + * + * **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() + +/** + * 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..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,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.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 @@ -30,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 @@ -149,10 +148,7 @@ internal fun doComplete(params: CompletionParams): CompletionResult { env.project.read { abortIfCancelled() - analyzeCopy( - useSiteElement = completionKtFile, - resolutionMode = KaDanglingFileResolutionMode.PREFER_SELF, - ) { + analyzeMaybeDangling(completionKtFile) { val ctx = resolveAnalysisContext( env = env, @@ -168,7 +164,7 @@ internal fun doComplete(params: CompletionParams): CompletionResult { completionOffset, params.file ) - return@analyzeCopy CompletionResult.EMPTY + return@analyzeMaybeDangling CompletionResult.EMPTY } abortIfCancelled() 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) }) } } 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) + } +}