Skip to content

ADFA-4010: close indexing services on background thread#1308

Open
itsaky-adfa wants to merge 3 commits into
stagefrom
fix/ADFA-4010
Open

ADFA-4010: close indexing services on background thread#1308
itsaky-adfa wants to merge 3 commits into
stagefrom
fix/ADFA-4010

Conversation

@itsaky-adfa
Copy link
Copy Markdown
Contributor

@itsaky-adfa itsaky-adfa commented May 14, 2026

See ADFA-4010 for more details.

Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
@itsaky-adfa itsaky-adfa requested a review from a team May 14, 2026 13:06
@itsaky-adfa itsaky-adfa self-assigned this May 14, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 14, 2026

Review Change Stack

Warning

Rate limit exceeded

@itsaky-adfa has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 28 minutes and 49 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d080c15a-e747-42d5-96ef-629c2bede8f0

📥 Commits

Reviewing files that changed from the base of the PR and between 386b467 and 7b25a0d.

📒 Files selected for processing (1)
  • lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt
📝 Walkthrough

Walkthrough

IndexingServiceManager's shutdown behavior is enhanced to close all registered services concurrently with per-service timeouts, await their completion, then cancel remaining in-flight work. Coroutine helpers and a 10-second timeout constant are added. ProjectManagerImpl's import is updated to reference the modified manager.

Changes

Graceful service shutdown with timeouts

Layer / File(s) Summary
Coroutine imports and shutdown timeout constant
lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt
Imports coroutine join/timeout helpers (joinAll, withTimeout, withTimeoutOrNull). Adds companion constant SERVICE_CLOSE_TIMEOUT set to 10 seconds.
Concurrent graceful service close with per-service timeout
lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt
close() method launches all service close operations concurrently in reverse registration order, wrapping each service.close() with withTimeoutOrNull to enforce the timeout. Awaits all close jobs via joinAll with error logging, then cancels remaining in-flight work via cancelChildren().
Manager import update in ProjectManagerImpl
subprojects/projects/src/main/java/com/itsaky/androidide/projects/ProjectManagerImpl.kt
Removes IndexingService import and adds IndexingServiceManager import to reflect the modified manager class.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Services now close with grace and care,
Timeouts in place, a watchful stare,
Concurrent shutdowns, no rush or waste,
Each farewell bounded, at proper pace!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and specifically describes the main change: closing indexing services on background thread, which aligns with the IndexingServiceManager updates implementing coroutine-based timeout/shutdown behavior.
Description check ✅ Passed The description provides a reference to the ADFA-4010 issue ticket with a link, which is sufficient context even if minimal. It relates to the changeset by referencing the issue that motivated these changes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/ADFA-4010

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

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt (1)

143-157: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Finish async close work before clearing manager state.

Lines 153-157 run immediately after the jobs are scheduled, not after they finish. That lets service.close() race registry.close() and makes the final shutdown log fire too early.

Suggested fix
 override fun close() {
 	log.info("Shutting down indexing services")
+	initialized = false
 
@@
 	scope.launch {
 		runCatching { joinAll(*cancellationJobs.toTypedArray()) }
 			.onFailure { err ->
 				log.error("Failed to close indexing services", err)
 			}
 
 		// Cancel in-flight work
 		scope.coroutineContext.cancelChildren()
+
+		services.clear()
+		registry.close()
+		log.info("Indexing services shut down")
 	}
-
-	services.clear()
-	registry.close()
-	initialized = false
-
-	log.info("Indexing services shut down")
 }
🤖 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/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt`
around lines 143 - 157, The shutdown schedules a coroutine with scope.launch but
then immediately clears state (services.clear, registry.close,
initialized=false) and logs shutdown before the async work finishes; change the
logic to wait for the launched coroutine to complete before mutating
state—either by making the surrounding function suspend and using runCatching {
joinAll(*cancellationJobs.toTypedArray());
scope.coroutineContext.cancelChildren() } directly, or by capturing the Job
returned from scope.launch (the launch call around joinAll) and calling
job.join() (or awaiting it in a runBlocking) before calling services.clear,
registry.close, setting initialized = false and logging; ensure the join/await
happens on the same coroutine scope so service.close()/registry.close() cannot
race with state clearing.
🤖 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/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt`:
- Around line 130-136: The catch block inside the
withTimeoutOrNull(SERVICE_CLOSE_TIMEOUT) around service.close() is catching
CancellationException and masking timeout cancellations; update the exception
handling in IndexingServiceManager (the withTimeoutOrNull block that calls
service.close() and logs via log.debug/log.error) to rethrow
CancellationException (or specifically catch CancellationException first and
throw it) and only handle other exceptions (e.g., Exception) for logging, so a
timeout cancellation propagates as intended to the timeout path.

---

Outside diff comments:
In
`@lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt`:
- Around line 143-157: The shutdown schedules a coroutine with scope.launch but
then immediately clears state (services.clear, registry.close,
initialized=false) and logs shutdown before the async work finishes; change the
logic to wait for the launched coroutine to complete before mutating
state—either by making the surrounding function suspend and using runCatching {
joinAll(*cancellationJobs.toTypedArray());
scope.coroutineContext.cancelChildren() } directly, or by capturing the Job
returned from scope.launch (the launch call around joinAll) and calling
job.join() (or awaiting it in a runBlocking) before calling services.clear,
registry.close, setting initialized = false and logging; ensure the join/await
happens on the same coroutine scope so service.close()/registry.close() cannot
race with state clearing.
🪄 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: cf20a36b-6c35-4403-84c7-b4b729134cf7

📥 Commits

Reviewing files that changed from the base of the PR and between b85bbc2 and 386b467.

📒 Files selected for processing (2)
  • lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt
  • subprojects/projects/src/main/java/com/itsaky/androidide/projects/ProjectManagerImpl.kt
💤 Files with no reviewable changes (1)
  • subprojects/projects/src/main/java/com/itsaky/androidide/projects/ProjectManagerImpl.kt

Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Copy link
Copy Markdown
Collaborator

@jatezzz jatezzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at coderabbit comment.

Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants