ADFA-4174: prioritize code completion requests over analyze requests#1439
Open
itsaky-adfa wants to merge 22 commits into
Open
ADFA-4174: prioritize code completion requests over analyze requests#1439itsaky-adfa wants to merge 22 commits into
itsaky-adfa wants to merge 22 commits into
Conversation
Signed-off-by: Akash Yadav <itsaky01@gmail.com>
Signed-off-by: Akash Yadav <itsaky01@gmail.com>
… 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).
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).
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
…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.
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.
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.
itsaky-adfa
marked this pull request as ready for review
July 4, 2026 03:40
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
Author
|
@claude review |
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, comment @claude review on this pull request to trigger a review.
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 Walkthrough
WalkthroughThis PR adds priority-aware, preemptive Kotlin analysis scheduling, propagates cancellation through indexing, diagnostics, completion, and signature help, debounces editor completion requests, updates progress APIs, adds regression tests, and adjusts test and repository configuration. ChangesAnalysis scheduling and cancellation core
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant EditorCompletionWindow
participant IDELanguage
participant ProgressManager
participant KotlinCompletions
participant AnalysisScheduler
EditorCompletionWindow->>EditorCompletionWindow: debounce completion request
IDELanguage->>ProgressManager: register completion checker
IDELanguage->>KotlinCompletions: doComplete(params)
KotlinCompletions->>AnalysisScheduler: acquire INTERACTIVE priority
AnalysisScheduler-->>KotlinCompletions: complete or preempt prior analysis
IDELanguage->>ProgressManager: unregister completion thread
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt (1)
160-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: silence detekt
SwallowedExceptionon the intentional preemption catches.Catching
AnalysisPreemptedExceptiononly to flip a flag is intentional here, but detekt flags the swallowed exception at Lines 160, 205, and 384. If detekt runs as a build gate, either reference the exception (e.g. rename to_) or add a brief justifying comment to keep it quiet without behavior change.♻️ Example
- } catch (e: AnalysisPreemptedException) { + } catch (_: AnalysisPreemptedException) { preempted.set(true) }Also applies to: 205-207, 384-386
🤖 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 160 - 162, The intentional AnalysisPreemptedException catches in AnalysisSerializationTest are being flagged by detekt as swallowed exceptions; update the affected catch blocks in the test methods to make the intent explicit, either by referencing the exception variable (for example with an underscore-style naming convention if supported) or by adding a brief explanatory comment right where the catch occurs. Keep the behavior the same in the relevant catch sites around the AnalysisPreemptedException handling in AnalysisSerializationTest, especially the preemption flag logic.Source: Linters/SAST tools
🤖 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/modules/KtFileExts.kt`:
- Around line 76-107: The cancellation hook setup around
AnalysisScheduler.acquire and AnalysisScheduler.release is outside the
lock-guarded try/finally, so a failure in cancelChecker.invokeOnCancel can
bypass release and deadlock Kotlin analysis globally. Move the invokeOnCancel
registration into the same try block that already wraps
AnalysisThreadContext.installJob and
ProgressManager.executeProcessUnderProgress, keeping AnalysisScheduler.release
in the finally so the lock is always released. Use the existing symbols
AnalysisScheduler.acquire, cancelChecker.invokeOnCancel, and
AnalysisScheduler.release to place the fix correctly.
In `@shared/src/main/java/com/itsaky/androidide/progress/ProgressManager.kt`:
- Around line 73-81: The race in `ProgressManager.abortIfCancelled()` comes from
checking `threads[thisThread]` and removing it in separate synchronized
sections, which can delete a newer registration for the same thread. Update
`abortIfCancelled()` so the lookup, cancellation check, and conditional
`threads.remove(thisThread)` happen atomically within one
`synchronized(threads)` block, using the current `checker` only if it is still
the same entry you observed.
---
Nitpick comments:
In
`@lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt`:
- Around line 160-162: The intentional AnalysisPreemptedException catches in
AnalysisSerializationTest are being flagged by detekt as swallowed exceptions;
update the affected catch blocks in the test methods to make the intent
explicit, either by referencing the exception variable (for example with an
underscore-style naming convention if supported) or by adding a brief
explanatory comment right where the catch occurs. Keep the behavior the same in
the relevant catch sites around the AnalysisPreemptedException handling in
AnalysisSerializationTest, especially the preemption flag logic.
🪄 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: 8af68e26-ff1a-4dd5-ae48-25d3b719ee53
📒 Files selected for processing (18)
.claude/.gitignorebuild.gradle.ktseditor/src/main/java/com/itsaky/androidide/editor/language/IDELanguage.kteditor/src/main/java/com/itsaky/androidide/editor/ui/EditorCompletionWindow.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisThreadContext.javalsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/CancelCheckerProgressIndicator.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.ktshared/src/main/java/com/itsaky/androidide/progress/ICancelChecker.ktshared/src/main/java/com/itsaky/androidide/progress/ProgressManager.ktshared/src/test/java/com/itsaky/androidide/progress/ICancelCheckerTest.ktshared/src/test/java/com/itsaky/androidide/progress/ProgressManagerTest.kt
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Contributor
There was a problem hiding this comment.
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/signaturehelp/KotlinSignatureHelp.kt`:
- Around line 124-134: Change the catch clause surrounding the signature-help
computation in KotlinSignatureHelp to catch Exception instead of Throwable.
Preserve the existing isAnalysisCancellation handling, logging, and
SignatureHelp.empty() fallback for exceptions while allowing fatal Error
subtypes to propagate.
🪄 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: 509590d0-07d1-4591-89b9-46dcb53eaeb1
📒 Files selected for processing (6)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/signaturehelp/KotlinSignatureHelp.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/KtLspTest.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.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/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt
dara-abijo-adfa
requested changes
Jul 14, 2026
dara-abijo-adfa
left a comment
dara-abijo-adfa
left a comment
Contributor
There was a problem hiding this comment.
There's a duplicate word in the PR title.
I also added two comments.
itsaky-adfa
changed the title
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
dara-abijo-adfa
approved these changes
Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See ADFA-4174 for more details.