ADFA-4387: add global analysis lock for Kotlin analysis

[itsaky-adfa]

See ADFA-4387 for more details.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bd6caff3-8693-4ecc-af95-979a065342bc

📥 Commits

Reviewing files that changed from the base of the PR and between 7bca4d7 and e2f9c8e.

📒 Files selected for processing (1)

• lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt

🚧 Files skipped from review as they are similar to previous changes (1)

• lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt

---

📝 Walkthrough

• Added a global Kotlin analysis lock and a withAnalysisLock helper to serialize all Analysis API entry points, with documentation warning about reentrant use and avoiding nested LSP write locks.

• Updated dangling-file analysis and completions to use the new serialized analysis flow, reducing lifetime/context corruption during concurrent completion requests.

• Added regression tests that stress concurrent analyzeMaybeDangling calls and verify overlapping analyses are serialized without lifetime exceptions.

• Changed unresolved-reference diagnostics to store plain unresolved-reference data instead of carrying the full diagnostic object into later phases, and updated add-import handling to use that extracted reference.

• Adjusted file-modification handling to acquire read/write locks in a safer order around Kotlin source modification notifications.

• Risk: the new analysis lock may become a contention point and could reduce concurrency/performance under heavy Kotlin LSP traffic.

• Risk: code inside withAnalysisLock must not call the LSP write lock, or it may introduce read→write deadlocks.

• Best-practice improvement: diagnostic extra data is now plain/primitive data, which better avoids lifetime leaks from retaining analysis-bound objects.

Walkthrough

Adds serialized Kotlin Analysis API access, updates completions and diagnostics to use the new analysis path, moves modification handling under the write lock, and adds regression tests for concurrent analysis.

Changes

Kotlin analysis concurrency and diagnostic flow

Layer / File(s)	Summary
Analysis lock infrastructure lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt	Adds ReentrantLock-based analysis serialization and the withAnalysisLock helper with usage constraints.
Serialized dangling analysis and completions lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt, lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt	Runs analyzeMaybeDangling under the analysis lock and switches completion analysis from analyzeCopy to analyzeMaybeDangling.
Primitive diagnostic data propagation lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt, lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt	Stores unresolved-reference text in KotlinDiagnosticExtra and updates add-import handling to use that reference value.
Write-locked modification handling lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt	Moves file/module resolution under a read lock and source modification handling under project.write.
Concurrency regression tests lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt	Adds concurrency tests for overlapping analyzeMaybeDangling calls and serialized execution.

Sequence Diagram(s)

sequenceDiagram
  participant withAnalysisLock
  participant KotlinCompletions
  participant KotlinDiagnosticProvider
  participant AddImportAction
  participant CompilationEnvironment

  KotlinCompletions->>withAnalysisLock: analyzeMaybeDangling(completionKtFile)
  withAnalysisLock-->>KotlinCompletions: completion context
  KotlinDiagnosticProvider-->>AddImportAction: KotlinDiagnosticExtra(unresolvedReference, env)
  AddImportAction->>AddImportAction: findSymbolBySimpleName(reference)
  CompilationEnvironment->>CompilationEnvironment: project.read / project.write

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• appdevforall/CodeOnTheGo#1112: Both PRs modify lsp/kotlin/.../diagnostic/KotlinDiagnosticProvider.kt, with this PR changing the shape of the diagnostic extra data.
• appdevforall/CodeOnTheGo#1202: Both PRs involve AddImportAction and the unresolved-reference data it consumes.
• appdevforall/CodeOnTheGo#1214: Both PRs touch CompilationEnvironment.notifyElementModifiedForPath and its lock ordering.

Suggested reviewers

• dara-abijo-adfa
• jatezzz

Poem

🐇 I hop where analyses softly queue,
One lock at a time keeps troubles few.
Tiny references now travel light,
And imports land in the proper light.
Read and write take turns, neat and clear,
While bunny tests cheer from ear to ear.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: adding a global lock for Kotlin analysis.
Description check	✅ Passed	The description links to the related ticket, so it is relevant to the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/ADFA-4387

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt`:
- Around line 150-153: Move the withAnalysisLock block outside and after the
env.project.read section to avoid deadlock risks from nested lock acquisition.
Since completionKtFile is already created in the earlier project.read block
(lines 137-145) and its content is stable, remove the second env.project.read
block entirely. Keep only the withAnalysisLock call wrapping the analysis work
that follows, ensuring the analysis lock is acquired after the project read lock
is released rather than held concurrently.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 491ec53b-f447-412a-86a2-ec1c9d81839e

📥 Commits

Reviewing files that changed from the base of the PR and between d73ec0b and d8c32bd.

📒 Files selected for processing (2)

• lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt
• lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt

[hal-eisen-adfa]

Automated high-effort review (code-review, recall-biased). The global analysisLock is a reasonable fix for the reported crash — verifier-confirmed root cause is concurrent analyze from indexing + diagnostics running under the shared project.read (the RW lock allows multiple readers), so serializing analyze closes that gap.

However, two confirmed, still-open instances of the same KaInaccessibleLifetimeOwnerAccessException class are not fixed by this PR. They're in files this PR doesn't touch, so I couldn't attach them inline — flagging here:

---

🟥 1. Escaped lifetime-owner: diagnostics → AddImportAction (CONFIRMED)
KotlinDiagnosticProvider.kt:~83 stores the live KaDiagnosticWithPsi (a KaLifetimeOwner) into DiagnosticItem.extra (KotlinDiagnosticExtra(diagnostic, env)) instead of extracting primitives inside the analyze block. Later AddImportAction.prepare() (AddImportAction.kt:56) and execAction() (:80) read diagnostic.reference outside any analyze block. prepare() runs whenever the code-actions menu is built for a Kotlin file with an unresolved-reference diagnostic (action is registered via KotlinCodeActionsMenu → KotlinLanguageServer). A lock can't keep an escaped token valid → same exception, from the quick-fix path. Fix: extract the reference string/FqName (plain data) inside the analyze block; don't store Ka* lifetime owners in extra.

🟥 2. handleElementModification outside the write lock (CONFIRMED)
CompilationEnvironment.kt:218-221 (notifyElementModifiedForPath, used by onFileCreated/onFileRemoved) calls KaSourceModificationService.handleElementModification(...) before the project.write { block opens at line 226. That session mutation is serialized by neither the RW lock nor the new analysisLock, so it can race a concurrent analyze (which holds only project.read) → the same lifetime/session corruption. (onFileContentChanged at :283 correctly keeps it inside project.write.) Fix: move the call inside project.write {} (or take analysisLock around it).

---

If the goal is to eliminate this exception class (not just the indexing-vs-diagnostics race), #1 and #2 should be addressed too. The inline comments below cover responsiveness/altitude/cleanup on the changed lines.

[coderabbitai]

🧹 Nitpick comments (1)

lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt (1)

15-31: ⚡ Quick win

Consider adding a test for lock reentrancy.

The KDoc in KtFileExts.kt notes that analysisLock is a ReentrantLock specifically so "an (indirect) nested analysis on the same thread cannot deadlock." While these tests validate mutual exclusion and lifetime exception prevention, they don't verify that nested or reentrant analysis calls work correctly.

If the lock were later changed to a non-reentrant implementation, these tests would still pass but nested analysis scenarios would deadlock.

Example test structure

`@Test`
fun `nested analyzeMaybeDangling calls do not deadlock`(): Unit = runBlocking {
  val file = createSourceFile("Nested.kt", "class Outer { class Inner }")
  
  env.project.read {
    analyzeMaybeDangling(file) {
      // Outer analysis
      file.declarations.first().symbol
      
      // Nested analysis on same thread
      analyzeMaybeDangling(file) {
        file.declarations.first().symbol
      }
    }
  }
  // If lock is non-reentrant, this would deadlock
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt`
around lines 15 - 31, The AnalysisSerializationTest class currently validates
mutual exclusion and lifetime exception prevention but does not test that nested
or reentrant analyzeMaybeDangling calls work correctly without deadlocking. Add
a new test method to the AnalysisSerializationTest class that verifies
reentrancy by calling analyzeMaybeDangling from within another
analyzeMaybeDangling block on the same thread, ensuring the ReentrantLock
behavior is preserved. This test should complete without deadlock and
demonstrate that the analysisLock reentrant implementation allows nested
analysis calls to proceed safely.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt`:
- Around line 15-31: The AnalysisSerializationTest class currently validates
mutual exclusion and lifetime exception prevention but does not test that nested
or reentrant analyzeMaybeDangling calls work correctly without deadlocking. Add
a new test method to the AnalysisSerializationTest class that verifies
reentrancy by calling analyzeMaybeDangling from within another
analyzeMaybeDangling block on the same thread, ensuring the ReentrantLock
behavior is preserved. This test should complete without deadlock and
demonstrate that the analysisLock reentrant implementation allows nested
analysis calls to proceed safely.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: eb359812-c8e8-4308-87b4-6ec25f69befd

📥 Commits

Reviewing files that changed from the base of the PR and between d8c32bd and dc471d3.

📒 Files selected for processing (1)

• lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt`:
- Around line 213-219: The PSI and module resolution calls in the code block
(involving psiManager.findFile, structureProvider.getModule, and
findModuleForSourceId) are executing without read lock protection, creating a
race condition vulnerability since concurrent analyze operations hold read
locks. Wrap the entire code block from structureProvider.getInstance through the
module assignment in a project.read block, following the same pattern used in
the loadKtFile method which correctly protects its psiManager.findFile call with
project.read.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0ac09d2a-2484-41fe-af4b-6bcd7daa70e1

📥 Commits

Reviewing files that changed from the base of the PR and between dc471d3 and 90e5471.

📒 Files selected for processing (5)

• lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt
• lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt
• lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt
• lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt
• lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt

🚧 Files skipped from review as they are similar to previous changes (1)

• lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt