ADFA-4174: prioritize code completion requests over over analyze requests#1439
Open
itsaky-adfa wants to merge 11 commits into
Open
ADFA-4174: prioritize code completion requests over over analyze requests#1439itsaky-adfa wants to merge 11 commits into
itsaky-adfa wants to merge 11 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.
![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
📝 Walkthrough
WalkthroughThis PR introduces a process-global, priority-aware, preemptive ChangesAnalysis scheduling and cancellation infrastructure
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant EditorCompletionWindow
participant ProgressManager
participant AnalysisScheduler
participant KotlinCompletions
EditorCompletionWindow->>EditorCompletionWindow: requireCompletion() debounce
EditorCompletionWindow->>ProgressManager: register(completionThread, checker)
EditorCompletionWindow->>KotlinCompletions: doComplete(params)
KotlinCompletions->>AnalysisScheduler: acquire(COMPLETION, checker)
AnalysisScheduler-->>KotlinCompletions: preempt lower-priority holder if needed
KotlinCompletions-->>EditorCompletionWindow: CompletionResult / AnalysisPreemptedException
sequenceDiagram
participant IndexWorker
participant SourceFileIndexer
participant AnalysisScheduler
IndexWorker->>SourceFileIndexer: indexSourceFile(checker)
SourceFileIndexer->>AnalysisScheduler: analyzeMaybeDangling(INDEXING, checker)
AnalysisScheduler-->>SourceFileIndexer: AnalysisPreemptedException
SourceFileIndexer-->>IndexWorker: propagate exception
IndexWorker->>IndexWorker: re-queue IndexCommand via submitCommand
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
Comment on lines
+76
to
+107
| 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<Any?>(1) | ||
| try { | ||
| // 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) | ||
| // 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() | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Move cancelChecker.invokeOnCancel { ... } inside the try/finally that releases the lock.
AnalysisScheduler.acquire(...) (Line 76-80) and cancelChecker.invokeOnCancel { ... } (Line 84-87) both run before the try { ... } finally { AnalysisScheduler.release() } block starts. If invokeOnCancel throws for any reason after the lock has already been granted, AnalysisScheduler.release() is never called — and since AnalysisScheduler is a process-global singleton lock, this permanently deadlocks all subsequent Kotlin analysis (completion, diagnostics, indexing) until the process restarts.
The current ScheduledCancelChecker.invokeOnCancel/Default.invokeOnCancel implementations don't appear to throw in practice, so the risk is currently low, but the blast radius (total analysis lockup) is severe enough to warrant defensive placement inside the lock-guarded region.
🔒 Proposed fix
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 {
+ // 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()
+ }
val holder = arrayOfNulls<Any?>(1)
try {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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<Any?>(1) | |
| try { | |
| // 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) | |
| // 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() | |
| } | |
| 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() | |
| } | |
| try { | |
| // 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() | |
| } | |
| val holder = arrayOfNulls<Any?>(1) | |
| try { | |
| // 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) | |
| // 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() | |
| } |
🤖 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/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt`
around lines 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.
Comment on lines
73
to
81
| @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() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
TOCTOU race in abortIfCancelled().
The read of threads[thisThread] and the removal happen in two separate synchronized(threads) blocks. Between them, another thread could register() a new checker for thisThread (e.g. a subsequent completion request reusing the same worker thread), and this method would then erroneously remove that new, unrelated registration instead of the stale one it actually observed as cancelled.
Combine the check and the removal into a single synchronized block to make the whole operation atomic.
🔒 Proposed fix
`@JvmName`("internalAbortIfCancelled")
private fun abortIfCancelled() {
val thisThread = Thread.currentThread()
- val checker = synchronized(threads) { threads[thisThread] }
- if (checker != null && checker.isCancelled()) {
- synchronized(threads) { threads.remove(thisThread) }
- throw CancellationException()
+ synchronized(threads) {
+ val checker = threads[thisThread]
+ if (checker != null && checker.isCancelled()) {
+ threads.remove(thisThread)
+ throw CancellationException()
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @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() | |
| } | |
| } | |
| `@JvmName`("internalAbortIfCancelled") | |
| private fun abortIfCancelled() { | |
| val thisThread = Thread.currentThread() | |
| synchronized(threads) { | |
| val checker = threads[thisThread] | |
| if (checker != null && checker.isCancelled()) { | |
| threads.remove(thisThread) | |
| throw CancellationException() | |
| } | |
| } | |
| } |
🤖 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 `@shared/src/main/java/com/itsaky/androidide/progress/ProgressManager.kt`
around lines 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.
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
2 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.