From 5d7c9f68824cb5d733171e55dbdd7bdc72540983 Mon Sep 17 00:00:00 2001 From: Hal Eisen Date: Wed, 17 Jun 2026 11:03:36 -0400 Subject: [PATCH 1/3] ADFA-4314 Make IndexingServiceManager.close() block until services close close() launched fire-and-forget coroutines on the manager scope to close each service, then returned immediately after clearing the services map. So when close() returned, service.close() may not have run yet -- the registered services were not reliably closed. This violated the Closeable contract that ProjectManagerImpl.destroy() relies on (it drops its reference right after close()), and made the 'close calls close on each registered service' test race the not-yet-run coroutine. Wrap the per-service and registry close coroutines in runBlocking { joinAll (...) } so close() blocks until shutdown completes. Per-service withTimeoutOrNull(SERVICE_CLOSE_TIMEOUT) protection and error isolation are preserved; in-flight indexing work is still cancelled afterward. Surfaced by the nightly Jacoco/SonarQube analyze.yml run after ADFA-4306. --- .../service/IndexingServiceManager.kt | 68 ++++++++++--------- 1 file changed, 37 insertions(+), 31 deletions(-) diff --git a/lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt b/lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt index 61f6af2d31..1441ae6566 100644 --- a/lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt +++ b/lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt @@ -7,6 +7,7 @@ import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.cancelChildren import kotlinx.coroutines.joinAll import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking import kotlinx.coroutines.withTimeoutOrNull import org.slf4j.LoggerFactory import java.io.Closeable @@ -123,51 +124,56 @@ class IndexingServiceManager( override fun close() { log.info("Shutting down indexing services") + // Close services (reverse registration order) and the registry, and + // block until they finish. Callers (e.g. ProjectManagerImpl.destroy()) + // rely on shutdown being complete when close() returns -- the Closeable + // contract -- before they drop their reference to the manager. + // + // Each close runs concurrently on Dispatchers.Default and is bounded by + // SERVICE_CLOSE_TIMEOUT so a cooperatively-cancellable service cannot + // stall teardown indefinitely. Failures are isolated per service. + runBlocking { + val serviceJobs = services.values.reversed().map { service -> + launch(Dispatchers.Default) { + withTimeoutOrNull(SERVICE_CLOSE_TIMEOUT) { + try { + service.close() + log.debug("Closed service: {}", service.id) + } catch (e: Exception) { + if (e is CancellationException) throw e + log.error("Failed to close service: {}", service.id, e) + } + } ?: log.warn( + "Indexing service {} failed to close within timeout period: {}ms", + service.id, SERVICE_CLOSE_TIMEOUT.inWholeMilliseconds, + ) + } + } - // Close services in reverse registration order - val cancellationJobs = services.values.reversed().map { service -> - scope.launch(Dispatchers.Default) { + val closeRegistryJob = launch(Dispatchers.Default) { withTimeoutOrNull(SERVICE_CLOSE_TIMEOUT) { try { - service.close() - log.debug("Closed service: {}", service.id) + registry.close() } catch (e: Exception) { if (e is CancellationException) throw e - log.error("Failed to close service: {}", service.id, e) + log.error("Failed to close index registry", e) } - } ?: run { - log.warn("Indexing service {} failed to close within timeout period: {}ms", service.id, SERVICE_CLOSE_TIMEOUT.inWholeMilliseconds) - } + } ?: log.warn( + "Index registry failed to close within timeout: {}ms", + SERVICE_CLOSE_TIMEOUT.inWholeMilliseconds, + ) } - } - val closeRegistryJob = scope.launch(Dispatchers.Default) { - withTimeoutOrNull(SERVICE_CLOSE_TIMEOUT) { - try { - registry.close() - } catch (e: Exception) { - if (e is CancellationException) throw e - log.error("Failed to close index registry") - } - } ?: run { - log.warn("Index registry failed to close within timeout: {}ms", SERVICE_CLOSE_TIMEOUT.inWholeMilliseconds) - } + joinAll(*serviceJobs.toTypedArray(), closeRegistryJob) } - scope.launch { - runCatching { joinAll(closeRegistryJob, *cancellationJobs.toTypedArray()) } - .onFailure { err -> - log.error("Failed to close indexing services", err) - } - - // Cancel in-flight work - scope.coroutineContext.cancelChildren() - } + // Cancel any in-flight indexing work still running on the manager scope. + scope.coroutineContext.cancelChildren() services.clear() initialized = false - log.info("Indexing services shut down requested") + log.info("Indexing services shut down") } private suspend fun initializeServices() { From 412bcb0a0f5596fbe099545f0f0541482577acd4 Mon Sep 17 00:00:00 2001 From: Hal Eisen Date: Wed, 17 Jun 2026 11:34:44 -0400 Subject: [PATCH 2/3] ADFA-4314 Drop misleading reverse-order claim in close() services is a ConcurrentHashMap (undefined iteration order), so services.values.reversed() never yielded reverse registration order as the comment claimed. Order is moot anyway: services close concurrently (one launch{} each, awaited by joinAll), so launch order has no effect on close behavior. Remove the no-op .reversed() and correct the comment. Addresses CodeRabbit review feedback on #1406. --- .../indexing/service/IndexingServiceManager.kt | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt b/lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt index 1441ae6566..f613762e17 100644 --- a/lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt +++ b/lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt @@ -124,16 +124,17 @@ class IndexingServiceManager( override fun close() { log.info("Shutting down indexing services") - // Close services (reverse registration order) and the registry, and - // block until they finish. Callers (e.g. ProjectManagerImpl.destroy()) - // rely on shutdown being complete when close() returns -- the Closeable - // contract -- before they drop their reference to the manager. + // Close all services and the registry, and block until they finish. + // Callers (e.g. ProjectManagerImpl.destroy()) rely on shutdown being + // complete when close() returns -- the Closeable contract -- before they + // drop their reference to the manager. // - // Each close runs concurrently on Dispatchers.Default and is bounded by - // SERVICE_CLOSE_TIMEOUT so a cooperatively-cancellable service cannot - // stall teardown indefinitely. Failures are isolated per service. + // Services are closed concurrently on Dispatchers.Default (so no ordering + // is implied), and each close is bounded by SERVICE_CLOSE_TIMEOUT so a + // cooperatively-cancellable service cannot stall teardown indefinitely. + // Failures are isolated per service. runBlocking { - val serviceJobs = services.values.reversed().map { service -> + val serviceJobs = services.values.map { service -> launch(Dispatchers.Default) { withTimeoutOrNull(SERVICE_CLOSE_TIMEOUT) { try { From bcb6db4398528be11abddeac244b3f7a51591853 Mon Sep 17 00:00:00 2001 From: Hal Eisen Date: Wed, 17 Jun 2026 12:09:07 -0400 Subject: [PATCH 3/3] ADFA-4314 Correct register() docstring: init order is unspecified services is a ConcurrentHashMap, so initializeServices() runs in an unspecified order -- the previous 'initialized in registration order' claim was false. Document the actual contract (no cross-service init-order dependency) rather than add ordering machinery nothing currently needs (YAGNI). --- .../codeonthego/indexing/service/IndexingServiceManager.kt | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt b/lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt index f613762e17..d9166cd1c8 100644 --- a/lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt +++ b/lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/service/IndexingServiceManager.kt @@ -42,8 +42,10 @@ class IndexingServiceManager( /** * Register an [IndexingService]. * - * Must be called before [onProjectSynced]. Services are initialized - * in registration order. + * Must be called before [onProjectSynced]. Services are initialized in an + * unspecified order (they are held in a [ConcurrentHashMap]), so a service's + * [IndexingService.initialize] must not depend on another service having + * been initialized first. * * @throws IllegalStateException if called after initialization. */