ADFA-4314 Make IndexingServiceManager.close() block until services close

[hal-eisen-adfa]

What

IndexingServiceManager.close() was fire-and-forget: it launched coroutines on the manager's scope to close each service, then immediately ran services.clear() and returned without awaiting them. So when close() returned, service.close() may not have executed yet — registered services were not reliably closed.

This is a real product bug, not a test-environment artifact:

• Production: ProjectManagerImpl.destroy() calls close() then drops its reference (_indexingServiceManager = null). It expects shutdown complete on return; instead services may close late, or be cancelled mid-close on a fast teardown → leaked index/file handles.
• Test: close calls close on each registered service calls close() then synchronously asserts svc.closed == true, racing the not-yet-run coroutine.

Violates the Closeable.close() contract (synchronous resource release).

Fix

Wrap the per-service and registry close coroutines in runBlocking { ... joinAll(...) } so close() blocks until shutdown completes before returning. Preserved:

• per-service withTimeoutOrNull(SERVICE_CLOSE_TIMEOUT) bound + error isolation
• cancellation of in-flight indexing work afterward (scope.coroutineContext.cancelChildren())
• close-then-cancel ordering

Real service close() impls (JvmLibraryIndexingService, JvmGeneratedIndexingService) and IndexRegistry.close() are fast synchronous releases, so blocking is cheap; the timeout is a safety net.

Verification

Full sonarqube gradle task run locally (the same one analyze.yml drives):

• 980 tests, 6 failures, 0 errors
• IndexingServiceManagerTest: 0 failures across all 6 variants (V7/V8 x Debug/Release/Instrumentation), tests=10 each — was 1 failure.
• The :sonarqube task fails only on SONAR_TOKEN upload (expected locally; official analysis runs in CI).

Remaining 6 failures are tracked separately: CompilerTest x2 (ADFA-4313, fixed on its own branch) and CloneRepositoryViewModelTest x4 (separate ticket).

Closes ADFA-4314.

[coderabbitai]

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: 37b177a7-53e6-45dc-a628-9c7fb672487f

📥 Commits

Reviewing files that changed from the base of the PR and between 412bcb0 and bcb6db4.

📒 Files selected for processing (1)

• lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt

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

• lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt

---

📝 Walkthrough

Release Notes

• Fix: IndexingServiceManager.close() is now synchronous—it uses runBlocking + joinAll to wait for all registered indexing services and the IndexRegistry to finish closing before returning, restoring the Closeable.close() contract and preventing fast teardown drops from leaking resources.
• Shutdown behavior: Per-service and registry closes run concurrently; there is no shutdown/initialization ordering guarantee across services.
• Timeout safety: Each service close (and the registry close) is bounded by SERVICE_CLOSE_TIMEOUT via withTimeoutOrNull; timeouts are logged as warnings without preventing other shutdown actions.
• Error isolation: Close failures are logged per component; cancellation exceptions are not swallowed.
• Cancel remaining work: After shutdown completion, the manager cancels any remaining in-flight work in its coroutine scope (cancelChildren()), then clears internal state (services, initialized) and logs completion.
• Docs: Updated the register() documentation to remove the incorrect claim that services are initialized in registration order (iteration order is unspecified due to ConcurrentHashMap).

⚠️ Risks & Considerations

• Blocking shutdown: close() now blocks the caller up to the configured timeout window while waiting for shutdown to complete.
• Thread/teardown load: Concurrent closes may briefly increase teardown workload (uses coroutine dispatching rather than strict sequencing).
• Timeouts may leave partial teardown: If a service doesn’t cooperate and fails to close within SERVICE_CLOSE_TIMEOUT, shutdown continues and only logs warnings—verify services handle cancellation/termination promptly.

Walkthrough

IndexingServiceManager.close() now blocks the caller with runBlocking instead of scheduling async work. Services close concurrently with the registry on Dispatchers.Default, joinAll waits for completion, remaining scope coroutines are cancelled, state is cleared, and log messages update from "requested" to "shut down". Service initialization ordering documentation is clarified as unspecified.

Changes

Synchronous Shutdown Rewrite

Layer / File(s)	Summary
Synchronous close() implementation lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt	Adds the runBlocking import and replaces the previous async close() with a blocking flow: concurrent service/registry close jobs on Dispatchers.Default, joinAll, scope.coroutineContext.cancelChildren(), state reset, and updated log messages. Registry exception logging now passes the exception object to log.error.
Service initialization ordering documentation lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt	Updates register() documentation to clarify that service initialization order is unspecified, reflecting ConcurrentHashMap iteration.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• appdevforall/CodeOnTheGo#1308: Directly modifies the same IndexingServiceManager.close() function with overlapping concerns — concurrent service shutdown in reverse order, timeout/join handling, and registry-close behavior.

Poem

🐇 Hoppity-hop, no more async delay,
runBlocking now waits till the services say,
Reversed in their order, they close with a grace,
The registry joins them at its own steady pace.
✨ "Shut down!" logs the manager, tidy and bright —
The coroutine children tucked in for the night.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% 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 accurately and concisely describes the main change: making IndexingServiceManager.close() block synchronously until services close, which is the primary objective of the PR.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, clearly explaining the bug, the fix, and verification results.
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 docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch ADFA-4314-indexing-close-not-closing-all-services

---

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 and usage tips.}

[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/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt`:
- Around line 127-137: The issue is that services is a ConcurrentHashMap which
has undefined iteration order per JDK specification, so reversing
services.values does not guarantee reverse registration order as the comment
claims. To fix this, introduce a separate ordered collection such as
CopyOnWriteArrayList to explicitly track the registration order of services as
they are added to the IndexingServiceManager. Then in the runBlocking block
where services are closed, use this ordered collection instead of
services.values.reversed() to iterate through services in the correct reverse
registration order. Update both the service registration logic (where services
are added to the map) and the close logic to maintain this separate ordered
collection alongside the existing ConcurrentHashMap.

🪄 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: 4ec079a8-2472-4b4c-9564-968f6af32ec5

📥 Commits

Reviewing files that changed from the base of the PR and between 8082c92 and 5d7c9f6.

📒 Files selected for processing (1)

• lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt